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 all 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
2 changes: 2 additions & 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,8 @@ 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.
* Remove `CmtConsPublicKey()` and `TmConsPublicKey()` from `Validator` interface and as methods on the `Validator` struct.
Comment on lines +97 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure consistency in the naming of cryptographic types.

- * [#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. 
-    * Remove `CmtConsPublicKey()` and `TmConsPublicKey()` from `Validator` interface and as methods on the `Validator` struct.
+ * [#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. 
+    * Remove `CmtConsPublicKey()` and `TmConsPublicKey()` from `Validator` interface and as methods on the `Validator` struct.

Ensure that the type names cryptotypes.PubKey and cometcrypto.PublicKey are consistently capitalized as they appear in the code. This helps maintain clarity and accuracy in documentation.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* [#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.
* Remove `CmtConsPublicKey()` and `TmConsPublicKey()` from `Validator` interface and as methods on the `Validator` struct.
* [#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.
* Remove `CmtConsPublicKey()` and `TmConsPublicKey()` from `Validator` interface and as methods on the `Validator` struct.


### State Breaking changes

Expand Down
11 changes: 6 additions & 5 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()
pubkey, err := v.ConsPubKey()
if err != nil {
return cmtprotocrypto.PublicKey{}, err
return nil, err
}

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
24 changes: 0 additions & 24 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 @@ -477,27 +474,6 @@ func (v Validator) ConsPubKey() (cryptotypes.PubKey, error) {
return pk, nil
}

// 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

pk, err := v.ConsPubKey()
if err != nil {
return cmtprotocrypto.PublicKey{}, err
}

tmPk, err := cryptocodec.ToCmtProtoPublicKey(pk)
if err != nil {
return cmtprotocrypto.PublicKey{}, 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()
}

// GetConsAddr extracts Consensus key address
func (v Validator) GetConsAddr() ([]byte, error) {
pk, ok := v.ConsensusPubkey.GetCachedValue().(cryptotypes.PubKey)
Expand Down
Loading