feat(genutil): allow manually setting the consensus key type in genesis. #19971
Conversation
WalkthroughThe recent update enhances the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
x/genutil/client/cli/init.go
Outdated
| 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} | ||
|
|
There was a problem hiding this comment.
Ensure proper error handling for the GetString method call. The current implementation correctly wraps the error, which is good practice. However, consider adding a specific check for the case where the consensusKey is not one of the expected values (e.g., not "ed25519" or "secp256k1"). This could prevent configuration errors that might be hard to debug later.
+ if consensusKey != "ed25519" && consensusKey != "secp256k1" {
+ return errorsmod.Wrapf(err, "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.
| 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(err, "Invalid consensus key algo: %s", consensusKey) | |
| + } | |
| appGenesis.Consensus.Params.Validator.PubKeyTypes = []string{consensusKey} | |
|
@julienrbrt added |
| @@ -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 | |||
There was a problem hiding this comment.
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.
| * (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. |
| 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} |
There was a problem hiding this comment.
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.
| 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} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
|
Can we get a |
Head branch was pushed to by a user without write access
|
@julienrbrt running lint-fix gives a ton of errors across various modules, but I think it should be good now?> |
|
Can you move the import manually then? The cometbft import should be in another group |
| 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} |
There was a problem hiding this comment.
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
x/genutil/client/cli/init.go
Outdated
| @@ -15,6 +15,7 @@ import ( | |||
| errorsmod "cosmossdk.io/errors" | |||
| "cosmossdk.io/math/unsafe" | |||
|
|
|||
| cmttypes "github.com/cometbft/cometbft/types" | |||
There was a problem hiding this comment.
Just move
cmttypes "github.com/cometbft/cometbft/types"
up to make the linter happy:
cfg "github.com/cometbft/cometbft/config"
cmttypes "github.com/cometbft/cometbft/types"
"github.com/cosmos/go-bip39"
|
Woops, forgot about this thanks Marko |
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
genutilmodule to allow manual setting of the consensus key type during genesis configuration.