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

feat(genutil): allow manually setting the consensus key type in genesis. #19971

Merged
merged 11 commits into from Apr 30, 2024
Merged

feat(genutil): allow manually setting the consensus key type in genesis. #19971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
977a2cb
bet
itsdevbear Apr 6, 2024
61fb733
update comment
itsdevbear Apr 6, 2024
80c5125
Merge branch 'main' into overrideval
itsdevbear Apr 13, 2024
8cf122d
changelog
itsdevbear Apr 13, 2024
0505162
fix
itsdevbear Apr 13, 2024
ed8613b
fix
itsdevbear Apr 13, 2024
4b7892d
Merge branch 'main' into overrideval
itsdevbear Apr 15, 2024
965b6ea
Merge branch 'main' into overrideval
tac0turtle Apr 16, 2024
b7afe7a
linting
tac0turtle Apr 30, 2024
0b4e5a7
Merge branch 'main' into overrideval
tac0turtle Apr 30, 2024
3aff845
Merge branch 'main' into overrideval
tac0turtle Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -58,6 +58,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
* (x/consensus) [#19483](https://github.com/cosmos/cosmos-sdk/pull/19483) Add consensus messages registration to consensus module.
* (types) [#19759](https://github.com/cosmos/cosmos-sdk/pull/19759) Align SignerExtractionAdapter in PriorityNonceMempool Remove.
* (client) [#19870](https://github.com/cosmos/cosmos-sdk/pull/19870) Add new query command `wait-tx`. Alias `event-query-tx-for` to `wait-tx` for backward compatibility.
* (genutil) [#19971](https://github.com/cosmos/cosmos-sdk/pull/19971) Allow manually setting the consensus key type in genesis
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry clearly communicates the addition of the new feature allowing manual setting of the consensus key type during genesis. However, consider adding more detail about the impact or benefits of this change to provide better context for users.

- * (genutil) [#19971](https://github.com/cosmos/cosmos-sdk/pull/19971) Allow manually setting the consensus key type in genesis
+ * (genutil) [#19971](https://github.com/cosmos/cosmos-sdk/pull/19971) Allow manually setting the consensus key type in genesis, enhancing flexibility and control over cryptographic parameters during network initialization.
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
* (genutil) [#19971](https://github.com/cosmos/cosmos-sdk/pull/19971) Allow manually setting the consensus key type in genesis
* (genutil) [#19971](https://github.com/cosmos/cosmos-sdk/pull/19971) Allow manually setting the consensus key type in genesis, enhancing flexibility and control over cryptographic parameters during network initialization.


### Improvements

Expand Down
12 changes: 12 additions & 0 deletions x/genutil/client/cli/init.go
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@ import (
"path/filepath"

cfg "github.com/cometbft/cometbft/config"
cmttypes "github.com/cometbft/cometbft/types"
"github.com/cosmos/go-bip39"
"github.com/spf13/cobra"

Expand All @@ -35,6 +36,9 @@ const (

// FlagDefaultBondDenom defines the default denom to use in the genesis file.
FlagDefaultBondDenom = "default-denom"

// FlagConsensusKeyAlgo defines the algorithm to use for the consensus signing key.
FlagConsensusKeyAlgo = "consensus-key-algo"
)

type printInfo struct {
Expand Down Expand Up @@ -158,7 +162,14 @@ func InitCmd(mm *module.Manager) *cobra.Command {
appGenesis.InitialHeight = initHeight
appGenesis.Consensus = &types.ConsensusGenesis{
Validators: nil,
Params: cmttypes.DefaultConsensusParams(),
}

consensusKey, err := cmd.Flags().GetString(FlagConsensusKeyAlgo)
if err != nil {
return errorsmod.Wrap(err, "Failed to get consensus key algo")
}
appGenesis.Consensus.Params.Validator.PubKeyTypes = []string{consensusKey}
Comment on lines +168 to +172
Copy link
Contributor

@coderabbitai coderabbitai bot Apr 13, 2024 •
edited

Choose a reason for hiding this comment

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

Proper error handling is implemented for the GetString method when retrieving the FlagConsensusKeyAlgo. However, as previously suggested in the PR comments, consider adding a validation check for the consensusKey to ensure it contains one of the expected values (e.g., "ed25519" or "secp256k1"). This will help prevent configuration errors that might be hard to debug later.

+ if consensusKey != "ed25519" && consensusKey != "secp256k1" {
+     return errorsmod.Wrapf(errors.New("invalid consensus key algorithm"), "Invalid consensus key algo: %s", consensusKey)
+ }
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
consensusKey, err := cmd.Flags().GetString(FlagConsensusKeyAlgo)
if err != nil {
return errorsmod.Wrap(err, "Failed to get consensus key algo")
}
appGenesis.Consensus.Params.Validator.PubKeyTypes = []string{consensusKey}
consensusKey, err := cmd.Flags().GetString(FlagConsensusKeyAlgo)
if err != nil {
return errorsmod.Wrap(err, "Failed to get consensus key algo")
}
if consensusKey != "ed25519" && consensusKey != "secp256k1" {
return errorsmod.Wrapf(errors.New("invalid consensus key algorithm"), "Invalid consensus key algo: %s", consensusKey)
}
appGenesis.Consensus.Params.Validator.PubKeyTypes = []string{consensusKey}

Copy link
Contributor

Choose a reason for hiding this comment

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

the consensus key is validated in the ExportGenesisFile step. It would return params.Validator.PubKeyTypes[%d], %s, is an unknown pubkey type for unsupported types. It is ok for me to rely on this and not fail fast

Copy link
Contributor

Choose a reason for hiding this comment

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

@alpe, thank you for clarifying the validation process for the consensus key in the ExportGenesisFile step. It makes sense to rely on this built-in validation to handle any unsupported types. I appreciate your insight and will remember this for future reviews.

🐰

New Learnings Added 
User: alpe"
URL: https://github.com/cosmos/cosmos-sdk/pull/19971

Timestamp: 2024-04-16T10:27:16.225Z
Learning: The consensus key is validated in the `ExportGenesisFile` step of the `genutil` module in the Cosmos SDK, which checks for unsupported types and returns an error if the key is unknown.

Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.


if err = genutil.ExportGenesisFile(appGenesis, genFile); err != nil {
return errorsmod.Wrap(err, "Failed to export genesis file")
Expand All @@ -176,6 +187,7 @@ func InitCmd(mm *module.Manager) *cobra.Command {
cmd.Flags().String(flags.FlagChainID, "", "genesis file chain-id, if left blank will be randomly created")
cmd.Flags().String(FlagDefaultBondDenom, "", "genesis file default denomination, if left blank default value is 'stake'")
cmd.Flags().Int64(flags.FlagInitHeight, 1, "specify the initial block height at genesis")
cmd.Flags().String(FlagConsensusKeyAlgo, "ed25519", "algorithm to use for the consensus key")

return cmd
}