Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor!(staking): remove comet crypto as a dep #20295

Merged
merged 4 commits into from
May 7, 2024
Merged

refactor!(staking): remove comet crypto as a dep #20295

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b0e4a6
remove some auxillary comet types in staking
tac0turtle May 6, 2024
b6d23d9
fix tests and add a changelog entry in staking
tac0turtle May 7, 2024
7784a8d
Merge branch 'main' into marko/staking_cleanup
tac0turtle May 7, 2024
ba88f50
remove extra methods & add section for the removal
tac0turtle May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions baseapp/abci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"github.com/cosmos/cosmos-sdk/baseapp"
baseapptestutil "github.com/cosmos/cosmos-sdk/baseapp/testutil"
"github.com/cosmos/cosmos-sdk/baseapp/testutil/mock"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
"github.com/cosmos/cosmos-sdk/testutil"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -1799,8 +1800,11 @@ func TestABCI_PrepareProposal_VoteExtensions(t *testing.T) {
},
}

pk, err := cryptocodec.FromCmtProtoPublicKey(tmPk)
require.NoError(t, err)

consAddr := sdk.ConsAddress(addr.String())
valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), consAddr.Bytes()).Return(tmPk, nil)
valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), consAddr.Bytes()).Return(pk, nil)

// set up baseapp
prepareOpt := func(bapp *baseapp.BaseApp) {
Expand Down Expand Up @@ -1828,7 +1832,7 @@ func TestABCI_PrepareProposal_VoteExtensions(t *testing.T) {

suite := NewBaseAppSuite(t, prepareOpt)

_, err := suite.baseApp.InitChain(&abci.InitChainRequest{
_, err = suite.baseApp.InitChain(&abci.InitChainRequest{
InitialHeight: 1,
ConsensusParams: &cmtproto.ConsensusParams{
Feature: &cmtproto.FeatureParams{
Expand Down Expand Up @@ -2092,7 +2096,10 @@ func TestBaseApp_VoteExtensions(t *testing.T) {
Secp256K1: pubKey.Bytes(),
},
}
valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), val).Return(tmPk, nil)

pk, err := cryptocodec.FromCmtProtoPublicKey(tmPk)
require.NoError(t, err)
valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), val).Return(pk, nil)
}

baseappOpts := func(app *baseapp.BaseApp) {
Expand Down
12 changes: 9 additions & 3 deletions baseapp/abci_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (

"github.com/cockroachdb/errors"
abci "github.com/cometbft/cometbft/abci/types"
cmtprotocrypto "github.com/cometbft/cometbft/api/cometbft/crypto/v1"
cmtproto "github.com/cometbft/cometbft/api/cometbft/types/v1"
cryptoenc "github.com/cometbft/cometbft/crypto/encoding"
cmttypes "github.com/cometbft/cometbft/types"
Expand All @@ -17,6 +16,8 @@ import (

"cosmossdk.io/core/comet"

cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/mempool"
)
Expand All @@ -26,7 +27,7 @@ type (
// extension signatures. Typically, this will be implemented by the x/staking
// module, which has knowledge of the CometBFT public key.
ValidatorStore interface {
GetPubKeyByConsAddr(context.Context, sdk.ConsAddress) (cmtprotocrypto.PublicKey, error)
GetPubKeyByConsAddr(context.Context, sdk.ConsAddress) (cryptotypes.PubKey, error)
}

// GasTx defines the contract that a transaction with a gas limit must implement.
Expand Down Expand Up @@ -110,7 +111,12 @@ func ValidateVoteExtensions(
return fmt.Errorf("failed to get validator %X public key: %w", valConsAddr, err)
}

cmtPubKey, err := cryptoenc.PubKeyFromProto(pubKeyProto)
cmtpk, err := cryptocodec.ToCmtProtoPublicKey(pubKeyProto)
if err != nil {
return fmt.Errorf("failed to convert validator %X public key: %w", valConsAddr, err)
}

cmtPubKey, err := cryptoenc.PubKeyFromProto(cmtpk)
if err != nil {
return fmt.Errorf("failed to convert validator %X public key: %w", valConsAddr, err)
}
Expand Down
9 changes: 6 additions & 3 deletions baseapp/abci_utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/cosmos/cosmos-sdk/baseapp/testutil/mock"
"github.com/cosmos/cosmos-sdk/client"
codectestutil "github.com/cosmos/cosmos-sdk/codec/testutil"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -93,9 +94,11 @@ func NewABCIUtilsTestSuite(t *testing.T) *ABCIUtilsTestSuite {
s.valStore = valStore

// set up mock
s.valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), s.vals[0].consAddr.Bytes()).Return(s.vals[0].tmPk, nil).AnyTimes()
s.valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), s.vals[1].consAddr.Bytes()).Return(s.vals[1].tmPk, nil).AnyTimes()
s.valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), s.vals[2].consAddr.Bytes()).Return(s.vals[2].tmPk, nil).AnyTimes()
for _, val := range s.vals {
pk, err := cryptocodec.FromCmtProtoPublicKey(val.tmPk)
require.NoError(t, err)
valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), val.consAddr.Bytes()).Return(pk, nil).AnyTimes()
}

// create context
s.ctx = sdk.Context{}.WithConsensusParams(cmtproto.ConsensusParams{
Expand Down
22 changes: 11 additions & 11 deletions baseapp/testutil/mock/mocks.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions types/staking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package types

import (
cmtprotocrypto "github.com/cometbft/cometbft/api/cometbft/crypto/v1"

"cosmossdk.io/math"

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
Expand Down Expand Up @@ -87,7 +85,6 @@ type ValidatorI interface {
IsUnbonding() bool // check if has status unbonding
GetOperator() string // operator address to receive/return validators coins
ConsPubKey() (cryptotypes.PubKey, error) // validation consensus pubkey (cryptotypes.PubKey)
TmConsPublicKey() (cmtprotocrypto.PublicKey, error) // validation consensus pubkey (CometBFT)
GetConsAddr() ([]byte, error) // validation consensus address
GetTokens() math.Int // validation tokens
GetBondedTokens() math.Int // validator bonded tokens
Expand Down
1 change: 1 addition & 0 deletions x/staking/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* remove `Keeper`: `SetLastTotalPower`, `GetLastTotalPower`
* [#17335](https://github.com/cosmos/cosmos-sdk/pull/17335) Remove usage of `"cosmossdk.io/x/staking/types".Infraction_*` in favour of `"cosmossdk.io/api/cosmos/staking/v1beta1".Infraction_` in order to remove dependency between modules on staking
* [#17655](https://github.com/cosmos/cosmos-sdk/pull/17655) `QueryHistoricalInfo` was adjusted to return `HistoricalRecord` and marked `Hist` as deprecated.
* [#20295](https://github.com/cosmos/cosmos-sdk/pull/20295) `GetValidatorByConsAddr` now returns the Cosmos SDK `cryptotypes.Pubkey` instead of `cometcrypto.Publickey`. The caller is responsible to translate the returned value to the expected type.

### State Breaking changes

Expand Down
9 changes: 5 additions & 4 deletions x/staking/keeper/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"fmt"
"time"

cmtprotocrypto "github.com/cometbft/cometbft/api/cometbft/crypto/v1"
gogotypes "github.com/cosmos/gogoproto/types"

"cosmossdk.io/collections"
Expand All @@ -17,6 +16,7 @@ import (
storetypes "cosmossdk.io/store/types"
"cosmossdk.io/x/staking/types"

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand Down Expand Up @@ -594,15 +594,16 @@ func (k Keeper) IsValidatorJailed(ctx context.Context, addr sdk.ConsAddress) (bo
}

// GetPubKeyByConsAddr returns the consensus public key by consensus address.
func (k Keeper) GetPubKeyByConsAddr(ctx context.Context, addr sdk.ConsAddress) (cmtprotocrypto.PublicKey, error) {
// Caller receives a Cosmos SDK Pubkey type and must cast it to a comet type
func (k Keeper) GetPubKeyByConsAddr(ctx context.Context, addr sdk.ConsAddress) (cryptotypes.PubKey, error) {
v, err := k.GetValidatorByConsAddr(ctx, addr)
if err != nil {
return cmtprotocrypto.PublicKey{}, err
return nil, err
}

pubkey, err := v.CmtConsPublicKey()
if err != nil {
return cmtprotocrypto.PublicKey{}, err
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

Several files in the codebase might be using an incorrect or undefined type for PubKey.

  • The mock implementations in expected_keepers_mocks.go and mocks.go reference types1.PubKey and types.PubKey, which do not appear to be aliases of cryptotypes.PubKey based on the script results.
  • Further investigation is needed to correct these references to ensure consistency across the codebase.
Analysis chain

Updated GetPubKeyByConsAddr to return cryptotypes.PubKey, aligning with the removal of Comet crypto dependencies. Ensure that all callers handle the new return type correctly.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `GetPubKeyByConsAddr` handle the new return type correctly.

# Test: Search for the function usage. Expect: Only occurrences of the new return type being handled.
rg --type go $'GetPubKeyByConsAddr'

Length of output: 2603

Script:

#!/bin/bash
# Description: Verify the type definitions of `types1.PubKey` and `types.PubKey` to confirm if they are aliases of `cryptotypes.PubKey`.

# Check the type definition of `types1.PubKey`
rg --type go "type PubKey" | grep "types1"

# Check the type definition of `types.PubKey`
rg --type go "type PubKey" | grep "types"

Length of output: 186

}

return pubkey, nil
Expand Down
5 changes: 2 additions & 3 deletions x/staking/testutil/expected_keepers_mocks.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 1 addition & 3 deletions x/staking/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package types
import (
"context"

cmtprotocrypto "github.com/cometbft/cometbft/api/cometbft/crypto/v1"

st "cosmossdk.io/api/cosmos/staking/v1beta1"
"cosmossdk.io/core/address"
"cosmossdk.io/math"
Expand Down Expand Up @@ -74,7 +72,7 @@ type ValidatorSet interface {

// GetPubKeyByConsAddr returns the consensus public key for a validator. Used in vote
// extension validation.
GetPubKeyByConsAddr(context.Context, sdk.ConsAddress) (cmtprotocrypto.PublicKey, error)
GetPubKeyByConsAddr(context.Context, sdk.ConsAddress) (cryptotypes.PubKey, error)
}

// DelegationSet expected properties for the set of all delegations for a particular (noalias)
Expand Down
20 changes: 3 additions & 17 deletions x/staking/types/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,13 @@ import (
"strings"
"time"

cmtprotocrypto "github.com/cometbft/cometbft/api/cometbft/crypto/v1"

"cosmossdk.io/core/address"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/errors"
"cosmossdk.io/math"

"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
Expand Down Expand Up @@ -478,24 +475,13 @@ func (v Validator) ConsPubKey() (cryptotypes.PubKey, error) {
}

// CmtConsPublicKey casts Validator.ConsensusPubkey to cmtprotocrypto.PubKey.
func (v Validator) CmtConsPublicKey() (cmtprotocrypto.PublicKey, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the methods be deprecated instead to avoid breaking changes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably, its always nicer to remove things so its easy for us

func (v Validator) CmtConsPublicKey() (cryptotypes.PubKey, error) {
pk, err := v.ConsPubKey()
if err != nil {
return cmtprotocrypto.PublicKey{}, err
}

tmPk, err := cryptocodec.ToCmtProtoPublicKey(pk)
if err != nil {
return cmtprotocrypto.PublicKey{}, err
return nil, err
}

return tmPk, nil
}

// Deprecated: use CmtConsPublicKey instead
// We do not delete this function as it is part of the ValidatorI interface
func (v Validator) TmConsPublicKey() (cmtprotocrypto.PublicKey, error) {
return v.CmtConsPublicKey()
return pk, nil
}

// GetConsAddr extracts Consensus key address
Expand Down
Loading