refactor(genutil): Use sdk types genesis validator #21678
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
julienrbrt
added
the
backport/v0.52.x
There was a problem hiding this comment.
If you can re-add the new GenesisValdiator struct in genutil/v2, then it will solve this as well: #21634
i will added it in, just need to fix it to work with json unmarshal |
…smos/cosmos-sdk into son/use_sdk_genesis_validator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/staking/genesis.go (1)
Line range hint
16-34: Approved: Good refactoring to use SDK typesThe changes to the
WriteValidatorsfunction look good. Usingsdk.GenesisValidatorinstead of a custom type aligns well with the PR objective of refactoring to use SDK types. This change enhances compatibility with the Cosmos SDK and reduces the need for custom implementations.Consider adding a comment explaining the use of
sdk.ConsAddress(pk.Address()).Bytes()for setting theAddressfield, as it might not be immediately obvious to all developers why this conversion is necessary. For example:vals = append(vals, sdk.GenesisValidator{ // Convert the consensus public key address to bytes for SDK compatibility Address: sdk.ConsAddress(pk.Address()).Bytes(), PubKey: pk, Power: validator.GetConsensusPower(keeper.PowerReduction(ctx)), Name: validator.GetMoniker(), })This addition would improve code clarity and maintainability.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- crypto/types/types.go (1 hunks)
- simapp/v2/export.go (2 hunks)
- x/staking/genesis.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crypto/types/types.go
- simapp/v2/export.go
🧰 Additional context used
📓 Path-based instructions (1)
x/staking/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
github-prbot
assigned facundomedica, hieuvubk, kocubinski, raynaudoe, testinginprod and ziscky
|
@julienrbrt seems i can completely remove cmtjson maybe because of the consensus params, maybe i introduced sdk types for it on a seperated PR |
| @@ -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) { | |||
There was a problem hiding this comment.
i couldn't find other way to get bonded validator directly because of executionContext. Dont know if i missed something here
There was a problem hiding this comment.
We can use the staking keeper directly and remove this, like in export v1.
There was a problem hiding this comment.
can't we pass the correct context from the calling func?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
crypto/keys/jsonkey.go (1)
10-15: Minor improvement: Fix grammatical error in comment.The struct definition looks good and follows the Uber Golang style guide. However, there's a small grammatical error in the comment.
Consider updating the comment to fix the grammatical error:
-// JSONPubKey defines a public key that are parse from JSON file. +// JSONPubKey defines a public key that is parsed from a JSON file.simapp/v2/export.go (1)
30-58: LGTM: Validator query and processing logic is well-implemented.The new code block effectively queries for bonded validators and converts them to GenesisValidator format. Error handling is generally good, but there's room for improvement.
Consider wrapping errors with additional context for better debugging. For example:
if err != nil { return v2.ExportedApp{}, fmt.Errorf("failed to get consensus public key: %w", err) }This approach provides more context about where the error occurred, making debugging easier.
runtime/v2/app.go (1)
128-134: LGTM: New Query method looks good, with a minor suggestion.The new
Querymethod is well-implemented and integrates nicely with the existingstfstructure. It allows querying at specific blockchain heights, which is a valuable feature.Consider wrapping the error returned from
db.StateAtto provide more context:func (a *App[T]) Query(ctx context.Context, gasLimit, version uint64, req transaction.Msg) (transaction.Msg, error) { state, err := a.db.StateAt(version) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get state at version %d: %w", version, err) } return a.stf.Query(ctx, state, gasLimit, req) }This change would make debugging easier by providing more information about the context of the error.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (10)
- crypto/codec/pubkey.go (1 hunks)
- crypto/keys/jsonkey.go (1 hunks)
- runtime/v2/app.go (2 hunks)
- simapp/v2/app_di.go (3 hunks)
- simapp/v2/export.go (2 hunks)
- types/staking.go (2 hunks)
- x/genutil/migration/v052/migrate.go (1 hunks)
- x/genutil/migration/v052/migrate_test.go (1 hunks)
- x/genutil/types/genesis.go (6 hunks)
- x/staking/genesis.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- simapp/v2/app_di.go
- types/staking.go
- x/genutil/migration/v052/migrate.go
- x/genutil/migration/v052/migrate_test.go
- x/genutil/types/genesis.go
- x/staking/genesis.go
🧰 Additional context used
📓 Path-based instructions (4)
crypto/codec/pubkey.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.crypto/keys/jsonkey.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/export.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (7)
crypto/keys/jsonkey.go (2)
3-8: LGTM: Import section is well-organized.The import section follows the Uber Golang style guide. All imported packages are used in the code, and they are correctly grouped.
39-41: LGTM:Bytes()method is correct and concise.The
Bytes()method is implemented correctly and follows the Uber Golang style guide. It simply returns theValuefield, which is appropriate for its purpose.simapp/v2/export.go (4)
5-8: LGTM: New imports are appropriate and correctly placed.The added imports for "fmt" and "stakingtypes" are necessary for the new functionality and follow Go conventions.
9-10: LGTM: Crypto codec import is appropriate.The added import for "cryptocodec" is necessary for handling public keys and follows Go conventions.
61-64: LGTM: Return statement updated correctly.The
Validatorsfield is correctly added to theExportedAppstruct in the return statement, aligning with the updated function signature.
61-64: Verify ExportedApp struct definition in genutil/v2 package.To ensure consistency with the changes made here, we need to verify that the
ExportedAppstruct in thex/genutil/v2/types.gofile has been updated to include theValidatorsfield.Run the following script to verify the
ExportedAppstruct definition:✅ Verification successful
ExportedApp struct definition verified successfully.
The
ExportedAppstruct inx/genutil/v2/types.goincludes theValidatorsfield as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the ExportedApp struct definition in the genutil/v2 package # Test: Search for the ExportedApp struct definition rg --type go -A 10 'type ExportedApp struct' x/genutil/v2/Length of output: 477
runtime/v2/app.go (1)
4-4: LGTM: Import statement added correctly.The addition of the
contextpackage import is necessary for the newQuerymethod and follows the Uber Go Style Guide for import grouping.
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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) | |
| } | |
| } |
| @@ -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{ | |||
There was a problem hiding this comment.
Why not using the keeper directly?
There was a problem hiding this comment.
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
| @@ -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) { | |||
There was a problem hiding this comment.
We can use the staking keeper directly and remove this, like in export v1.
| if !ok { | ||
| return v2.ExportedApp{}, fmt.Errorf("invalid response, expected QueryValidatorsResponse") | ||
| } | ||
|
|
There was a problem hiding this comment.
It looks like the input argument to this func jailAllowedAddrs []string is not used anywhere, should there be filtering done somewhere in this func?
Description
follow up to: #21382
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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Refactor