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.

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

refactor(genutil): Use sdk types genesis validator #21678

Open
wants to merge 47 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
fddadd3
move use sdk types genesis validator
sontrinh16 Sep 11, 2024
41c5520
debug
sontrinh16 Sep 13, 2024
219213b
debug
sontrinh16 Sep 13, 2024
794ecbc
debug
sontrinh16 Sep 13, 2024
252f50f
Merge branch 'main' into son/use_sdk_genesis_validator
sontrinh16 Sep 16, 2024
0033996
fix conflict
sontrinh16 Sep 16, 2024
ed34222
debug
sontrinh16 Sep 17, 2024
17a42a3
assert nil for empty val set
sontrinh16 Sep 17, 2024
6bb526b
minor
sontrinh16 Sep 17, 2024
f9aa48b
fix test
sontrinh16 Sep 17, 2024
389b7b8
lint
sontrinh16 Sep 17, 2024
5775c9f
fix e2e test
sontrinh16 Sep 17, 2024
85a753e
Merge branch 'main' into son/use_sdk_genesis_validator
sontrinh16 Sep 17, 2024
5fe570c
add migration and genesis validator in genutil v2
sontrinh16 Sep 18, 2024
5b31f47
Merge branch 'son/use_sdk_genesis_validator' of https://github.com/co…
sontrinh16 Sep 18, 2024
dd2a634
cleanup
sontrinh16 Sep 18, 2024
fba3f54
lint
sontrinh16 Sep 18, 2024
05fe687
more lint
sontrinh16 Sep 18, 2024
e4f1972
resolve conflict
sontrinh16 Sep 19, 2024
80a92ae
minor
sontrinh16 Sep 19, 2024
fb41419
Merge branch 'main' into son/use_sdk_genesis_validator
sontrinh16 Sep 23, 2024
d0f9f1c
make work with migrate cmd
sontrinh16 Sep 23, 2024
7a7c42a
Merge branch 'son/use_sdk_genesis_validator' of https://github.com/co…
sontrinh16 Sep 23, 2024
ddf3149
remove migrator
sontrinh16 Sep 23, 2024
7c0ce57
rename
sontrinh16 Sep 23, 2024
641164f
remove duplicated code
sontrinh16 Sep 23, 2024
bbb3b01
Update x/genutil/migration/v052/migrate_test.go
sontrinh16 Sep 23, 2024
5d02a84
Update x/genutil/migration/v052/migrate.go
sontrinh16 Sep 23, 2024
e9e27f6
address comment
sontrinh16 Sep 23, 2024
cebc09d
Merge branch 'son/use_sdk_genesis_validator' of https://github.com/co…
sontrinh16 Sep 23, 2024
a0d41c9
populate simapp v2 export
sontrinh16 Sep 23, 2024
7075a3d
lint
sontrinh16 Sep 23, 2024
d1f59d2
minor
sontrinh16 Sep 24, 2024
528606f
Merge branch 'main' into son/use_sdk_genesis_validator
sontrinh16 Sep 24, 2024
b5abb09
address comment
sontrinh16 Sep 24, 2024
29156ed
remove encoding
sontrinh16 Sep 26, 2024
b1924fd
Merge branch 'son/use_sdk_genesis_validator' of https://github.com/co…
sontrinh16 Sep 26, 2024
bb2d7f1
fix test
sontrinh16 Sep 30, 2024
8937cb7
fix conflict
sontrinh16 Sep 30, 2024
b4014f3
fix test
sontrinh16 Sep 30, 2024
bae84ba
minor
sontrinh16 Sep 30, 2024
e1a6b0c
Merge branch 'main' into son/use_sdk_genesis_validator
sontrinh16 Sep 30, 2024
76ab968
address comments
sontrinh16 Sep 30, 2024
33e0432
Merge branch 'son/use_sdk_genesis_validator' of https://github.com/co…
sontrinh16 Sep 30, 2024
6a81119
Merge branch 'main' into son/use_sdk_genesis_validator
sontrinh16 Sep 30, 2024
496627d
Merge branch 'main' into son/use_sdk_genesis_validator
kocubinski Oct 1, 2024
e7600cd
Merge branch 'main' into son/use_sdk_genesis_validator
sontrinh16 Oct 3, 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
53 changes: 53 additions & 0 deletions crypto/codec/pubkey.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package codec

import (
"cosmossdk.io/errors"

cryptokeys "github.com/cosmos/cosmos-sdk/crypto/keys"
bls12_381 "github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381"
"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

func PubKeyToProto(pk cryptokeys.JSONPubkey) (cryptotypes.PubKey, error) {
switch pk.KeyType {
case ed25519.PubKeyName:
return &ed25519.PubKey{
Key: pk.Value,
}, nil
case secp256k1.PubKeyName:
return &secp256k1.PubKey{
Key: pk.Value,
}, nil
case bls12_381.PubKeyName:
return &bls12_381.PubKey{
Key: pk.Value,
}, nil
default:
return nil, errors.Wrapf(sdkerrors.ErrInvalidType, "cannot convert %v to proto public key", pk)
}
}

func PubKeyFromProto(pk cryptotypes.PubKey) (cryptokeys.JSONPubkey, error) {
switch pk := pk.(type) {
case *ed25519.PubKey:
return cryptokeys.JSONPubkey{
KeyType: ed25519.PubKeyName,
Value: pk.Bytes(),
}, nil
case *secp256k1.PubKey:
return cryptokeys.JSONPubkey{
KeyType: secp256k1.PubKeyName,
Value: pk.Bytes(),
}, nil
case *bls12_381.PubKey:
return cryptokeys.JSONPubkey{
KeyType: bls12_381.PubKeyName,
Value: pk.Bytes(),
}, nil
default:
return cryptokeys.JSONPubkey{}, errors.Wrapf(sdkerrors.ErrInvalidType, "cannot convert %v from proto public key", pk)
}
}
41 changes: 41 additions & 0 deletions crypto/keys/jsonkey.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package keys

import (
"github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381"
"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
"github.com/cosmos/cosmos-sdk/crypto/types"
)

// JSONPubKey defines a public key that are parse from JSON file.
// convert PubKey to JSONPubKey needs a in between step
type JSONPubkey struct {
KeyType string `json:"type"`
Value []byte `json:"value"`
}

func (pk JSONPubkey) Address() types.Address {
switch pk.KeyType {
case ed25519.PubKeyName:
ed25519 := ed25519.PubKey{
Key: pk.Value,
}
return ed25519.Address()
case secp256k1.PubKeyName:
secp256k1 := secp256k1.PubKey{
Key: pk.Value,
}
return secp256k1.Address()
case bls12_381.PubKeyName:
bls12_381 := bls12_381.PubKey{
Key: pk.Value,
}
return bls12_381.Address()
default:
return nil
}
}
Comment on lines +17 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding error handling for unknown key types.

The Address() method implementation looks good overall and follows the Uber Golang style guide. However, it might be beneficial to add error handling for unknown key types.

Consider updating the Address() method to return an error for unknown key types:

-func (pk JSONPubkey) Address() types.Address {
+func (pk JSONPubkey) Address() (types.Address, error) {
 	switch pk.KeyType {
 	case ed25519.PubKeyName:
 		ed25519 := ed25519.PubKey{
 			Key: pk.Value,
 		}
-		return ed25519.Address()
+		return ed25519.Address(), nil
 	case secp256k1.PubKeyName:
 		secp256k1 := secp256k1.PubKey{
 			Key: pk.Value,
 		}
-		return secp256k1.Address()
+		return secp256k1.Address(), nil
 	case bls12_381.PubKeyName:
 		bls12_381 := bls12_381.PubKey{
 			Key: pk.Value,
 		}
-		return bls12_381.Address()
+		return bls12_381.Address(), nil
 	default:
-		return nil
+		return nil, fmt.Errorf("unknown key type: %s", pk.KeyType)
 	}
 }

This change would allow callers to handle unknown key types more gracefully.

📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (pk JSONPubkey) Address() types.Address {
switch pk.KeyType {
case ed25519.PubKeyName:
ed25519 := ed25519.PubKey{
Key: pk.Value,
}
return ed25519.Address()
case secp256k1.PubKeyName:
secp256k1 := secp256k1.PubKey{
Key: pk.Value,
}
return secp256k1.Address()
case bls12_381.PubKeyName:
bls12_381 := bls12_381.PubKey{
Key: pk.Value,
}
return bls12_381.Address()
default:
return nil
}
}
func (pk JSONPubkey) Address() (types.Address, error) {
switch pk.KeyType {
case ed25519.PubKeyName:
ed25519 := ed25519.PubKey{
Key: pk.Value,
}
return ed25519.Address(), nil
case secp256k1.PubKeyName:
secp256k1 := secp256k1.PubKey{
Key: pk.Value,
}
return secp256k1.Address(), nil
case bls12_381.PubKeyName:
bls12_381 := bls12_381.PubKey{
Key: pk.Value,
}
return bls12_381.Address(), nil
default:
return nil, fmt.Errorf("unknown key type: %s", pk.KeyType)
}
}


func (pk JSONPubkey) Bytes() []byte {
return pk.Value
}
9 changes: 9 additions & 0 deletions runtime/v2/app.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package runtime

import (
"context"
"encoding/json"
"errors"
"slices"
Expand Down Expand Up @@ -123,3 +124,11 @@ func (a *App[T]) GetAppManager() *appmanager.AppManager[T] {
func (a *App[T]) GetGPRCMethodsToMessageMap() map[string]func() gogoproto.Message {
return a.GRPCMethodsToMessageMap
}

func (a *App[T]) Query(ctx context.Context, gasLimit, version uint64, req transaction.Msg) (transaction.Msg, error) {
Copy link
Member Author

@sontrinh16 sontrinh16 Sep 30, 2024

Choose a reason for hiding this comment

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

i couldn't find other way to get bonded validator directly because of executionContext. Dont know if i missed something here

Copy link
Member

Choose a reason for hiding this comment

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

We can use the staking keeper directly and remove this, like in export v1.

Copy link
Member

Choose a reason for hiding this comment

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

can't we pass the correct context from the calling func?

state, err := a.db.StateAt(version)
if err != nil {
return nil, err
}
return a.stf.Query(ctx, state, gasLimit, req)
}
4 changes: 2 additions & 2 deletions server/types/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (

cmtproto "github.com/cometbft/cometbft/api/cometbft/types/v1"
cmtcrypto "github.com/cometbft/cometbft/crypto"
cmttypes "github.com/cometbft/cometbft/types"
"github.com/cosmos/gogoproto/grpc"

"cosmossdk.io/core/server"
Expand All @@ -18,6 +17,7 @@ import (
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/server/api"
"github.com/cosmos/cosmos-sdk/server/config"
sdk "github.com/cosmos/cosmos-sdk/types"
)

type (
Expand Down Expand Up @@ -76,7 +76,7 @@ type (
// AppState is the application state as JSON.
AppState json.RawMessage
// Validators is the exported validator set.
Validators []cmttypes.GenesisValidator
Validators []sdk.GenesisValidator
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

Update remaining cmttypes.GenesisValidator usages for consistency

The verification process has identified several instances where cmttypes.GenesisValidator is still being used in the codebase. These occurrences need to be addressed to maintain consistency with the change to sdk.GenesisValidator in the ExportedApp struct:

  • x/genutil/types/genesis.go: Two instances of cmttypes.GenesisValidator usage
  • x/genutil/migration/migration.go: One instance in a struct field definition

Action items:

  • Update these instances to use sdk.GenesisValidator instead of cmttypes.GenesisValidator.
  • Carefully review the affected files to ensure proper migration and maintain functionality.
  • Test thoroughly after making these changes to verify that validator-related operations still work as expected.
Analysis chain

Verify compatibility with existing code.

The change from cmttypes.GenesisValidator to sdk.GenesisValidator aligns with the shift towards using Cosmos SDK types for the Validators structure. This modification is likely part of a larger refactor to integrate more closely with the Cosmos SDK.

However, it's important to ensure that existing code relying on the cmttypes.GenesisValidator type is updated to handle the new sdk.GenesisValidator type to maintain compatibility and functionality related to validator management.

Run the following script to identify instances of cmttypes.GenesisValidator usage in the codebase:

If the script returns instances of cmttypes.GenesisValidator usage outside of test files or comments, update the code to handle the new sdk.GenesisValidator type accordingly.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for usage of `cmttypes.GenesisValidator` in Go files.

# Test: Search for the type usage. Expect: Only occurrences in test files or comments.
rg --type go $'cmttypes\.GenesisValidator'

Length of output: 287

// Height is the app's latest block height.
Height int64
// ConsensusParams are the exported consensus params for ABCI.
Expand Down
19 changes: 1 addition & 18 deletions simapp/export.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,13 @@ import (
"fmt"

cmtproto "github.com/cometbft/cometbft/api/cometbft/types/v1"
cmttypes "github.com/cometbft/cometbft/types"

"cosmossdk.io/collections"
storetypes "cosmossdk.io/store/types"
slashingtypes "cosmossdk.io/x/slashing/types"
"cosmossdk.io/x/staking"
stakingtypes "cosmossdk.io/x/staking/types"

cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
servertypes "github.com/cosmos/cosmos-sdk/server/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)
Expand Down Expand Up @@ -43,25 +41,10 @@ func (app *SimApp) ExportAppStateAndValidators(forZeroHeight bool, jailAllowedAd
}

validators, err := staking.WriteValidators(ctx, app.StakingKeeper)
cmtValidators := []cmttypes.GenesisValidator{}
for _, val := range validators {
cmtPk, err := cryptocodec.ToCmtPubKeyInterface(val.PubKey)
if err != nil {
return servertypes.ExportedApp{}, err
}
cmtVal := cmttypes.GenesisValidator{
Address: val.Address.Bytes(),
PubKey: cmtPk,
Power: val.Power,
Name: val.Name,
}

cmtValidators = append(cmtValidators, cmtVal)
}

return servertypes.ExportedApp{
AppState: appState,
Validators: cmtValidators,
Validators: validators,
Height: height,
ConsensusParams: app.BaseApp.GetConsensusParams(ctx),
}, err
Expand Down
3 changes: 3 additions & 0 deletions simapp/v2/app_di.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
basedepinject "cosmossdk.io/x/accounts/defaults/base/depinject"
lockupdepinject "cosmossdk.io/x/accounts/defaults/lockup/depinject"
multisigdepinject "cosmossdk.io/x/accounts/defaults/multisig/depinject"
stakingkeeper "cosmossdk.io/x/staking/keeper"
upgradekeeper "cosmossdk.io/x/upgrade/keeper"

"github.com/cosmos/cosmos-sdk/client"
Expand All @@ -41,6 +42,7 @@ type SimApp[T transaction.Tx] struct {
// required keepers during wiring
// others keepers are all in the app
UpgradeKeeper *upgradekeeper.Keeper
StakingKeeper *stakingkeeper.Keeper
}

func init() {
Expand Down Expand Up @@ -168,6 +170,7 @@ func NewSimApp[T transaction.Tx](
&app.txConfig,
&app.interfaceRegistry,
&app.UpgradeKeeper,
&app.StakingKeeper,
); err != nil {
panic(err)
}
Expand Down
42 changes: 39 additions & 3 deletions simapp/v2/export.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@ package simapp

import (
"context"
"fmt"

stakingtypes "cosmossdk.io/x/staking/types"

cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
v2 "github.com/cosmos/cosmos-sdk/x/genutil/v2"
)

Expand All @@ -22,8 +27,39 @@ func (app *SimApp[T]) ExportAppStateAndValidators(jailAllowedAddrs []string) (v2
return v2.ExportedApp{}, err
}

// get the current bonded validators
resp, err := app.Query(ctx, 0, latestHeight, &stakingtypes.QueryValidatorsRequest{
Copy link
Member

Choose a reason for hiding this comment

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

Why not using the keeper directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmmm i tried but got panic: failed to get executionContext from context in simapp v2 test, seems like i have to use stf to set it in

Status: stakingtypes.BondStatusBonded,
})

vals, ok := resp.(*stakingtypes.QueryValidatorsResponse)
if !ok {
return v2.ExportedApp{}, fmt.Errorf("invalid response, expected QueryValidatorsResponse")
}

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the input argument to this func jailAllowedAddrs []string is not used anywhere, should there be filtering done somewhere in this func?

// convert to genesis validator
var genesisVals []sdk.GenesisValidator
for _, val := range vals.Validators {
pk, err := val.ConsPubKey()
if err != nil {
return v2.ExportedApp{}, err
}
jsonPk, err := cryptocodec.PubKeyFromProto(pk)
if err != nil {
return v2.ExportedApp{}, err
}

genesisVals = append(genesisVals, sdk.GenesisValidator{
Address: sdk.ConsAddress(pk.Address()).Bytes(),
PubKey: jsonPk,
Power: val.GetConsensusPower(app.StakingKeeper.PowerReduction(ctx)),
Name: val.Description.Moniker,
})
}

return v2.ExportedApp{
AppState: genesis,
Height: int64(latestHeight),
}, nil
AppState: genesis,
Height: int64(latestHeight),
Validators: genesisVals,
}, err
}
3 changes: 2 additions & 1 deletion tests/e2e/genutil/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/cosmos/cosmos-sdk/server/types"
simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims"
gentestutil "github.com/cosmos/cosmos-sdk/testutil/x/genutil"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/genutil"
genutilcli "github.com/cosmos/cosmos-sdk/x/genutil/client/cli"
genutiltypes "github.com/cosmos/cosmos-sdk/x/genutil/types"
Expand Down Expand Up @@ -184,7 +185,7 @@ func setupApp(t *testing.T, tempDir string) (*simapp.SimApp, context.Context, ge
ChainID: "theChainId",
AppState: stateBytes,
Consensus: &genutiltypes.ConsensusGenesis{
Validators: nil,
Validators: []sdk.GenesisValidator{},
},
}

Expand Down
10 changes: 0 additions & 10 deletions types/genesis.go

This file was deleted.

9 changes: 9 additions & 0 deletions types/staking.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package types
import (
"cosmossdk.io/math"

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

Expand Down Expand Up @@ -98,3 +99,11 @@ type ValidatorI interface {
SharesFromTokens(amt math.Int) (math.LegacyDec, error) // shares worth of delegator's bond
SharesFromTokensTruncated(amt math.Int) (math.LegacyDec, error) // truncated shares worth of delegator's bond
}

// GenesisValidator is an initial validator.
type GenesisValidator struct {
Address ConsAddress `json:"address"`
PubKey cryptokeys.JSONPubkey `json:"pub_key"`
Power int64 `json:"power"`
Name string `json:"name"`
}
19 changes: 16 additions & 3 deletions x/genutil/client/cli/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,17 @@ import (
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/version"
v052 "github.com/cosmos/cosmos-sdk/x/genutil/migration/v052"
"github.com/cosmos/cosmos-sdk/x/genutil/types"
)

const flagGenesisTime = "genesis-time"
const v52 = "v0.52"

// MigrationMap is a map of SDK versions to their respective genesis migration functions.
var MigrationMap = types.MigrationMap{}
var MigrationMap = types.MigrationMap{
v52: v052.Migrate,
}

// MigrateGenesisCmd returns a command to execute genesis state migration.
// Applications should pass their own migration map to this function.
Expand Down Expand Up @@ -56,7 +60,17 @@ func MigrateHandler(cmd *cobra.Command, args []string, migrations types.Migratio
}

importGenesis := args[1]
appGenesis, err := types.AppGenesisFromFile(importGenesis)
outputDocument, _ := cmd.Flags().GetString(flags.FlagOutputDocument)

// for v52 we need to migrate the consensus validator address from hex bytes to
// sdk consensus address.
var appGenesis *types.AppGenesis
var err error
if target == v52 {
appGenesis, err = v052.MigrateGenesisFile(importGenesis)
} else {
appGenesis, err = types.AppGenesisFromFile(importGenesis)
}
if err != nil {
return err
}
Expand Down Expand Up @@ -110,7 +124,6 @@ func MigrateHandler(cmd *cobra.Command, args []string, migrations types.Migratio
return fmt.Errorf("failed to marshal app genesis: %w", err)
}

outputDocument, _ := cmd.Flags().GetString(flags.FlagOutputDocument)
if outputDocument == "" {
cmd.Println(string(bz))
return nil
Expand Down
Loading
Loading