refactor(x/staking,genutil)!: use validatorUpdates #19754
Conversation
WalkthroughWalkthroughThe update involves transitioning the Cosmos SDK to utilize Changes
Assessment against linked issues
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 (
|
chixiaowen
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yml
Commits
Files that changed from the base of the PR and between 5424b55 and 3fe4097b326d42c9a71b5e91af9bce890820e123.Files selected for processing (6)
- tests/integration/staking/keeper/genesis_test.go (3 hunks)
- x/genutil/genesis.go (2 hunks)
- x/genutil/module.go (1 hunks)
- x/staking/keeper/abci.go (2 hunks)
- x/staking/keeper/genesis.go (4 hunks)
- x/staking/module.go (2 hunks)
Additional comments: 8
x/genutil/genesis.go (1)
- 5-6: The import statements have been updated to include
fmtandmodulefromcosmos/cosmos-sdk/types, which aligns with the changes made in theInitGenesisfunction to usemodule.ValidatorUpdateinstead ofabci.ValidatorUpdate. This change is consistent with the PR's objective to streamline the process of generating validator updates.x/genutil/module.go (1)
- 81-81: The
InitGenesisfunction inx/genutil/module.gohas been simplified by directly returning the result ofInitGenesiswithout intermediate processing. This change is consistent with the PR's objective to streamline the process of generating validator updates by leveraging the newmodule.ValidatorUpdatetype directly within the modules. The simplification enhances the maintainability and readability of the code.x/staking/module.go (2)
- 151-151: The
InitGenesisfunction inx/staking/module.gohas been simplified by removing unnecessary code for handling different types of public keys, now directly returning results from the keeper functions. This change aligns with the PR's objective and improves code maintainability.- 177-177: The
EndBlockfunction has also been simplified in a similar manner toInitGenesis, directly returning results from the keeper functions without unnecessary intermediate processing. This simplification is consistent with the PR's objectives and enhances code readability and maintainability.tests/integration/staking/keeper/genesis_test.go (3)
- 5-5: The import statement for
github.com/cosmos/cosmos-sdk/types/modulehas been added, which is necessary for the updated tests to usemodule.ValidatorUpdateinstead ofabci.ValidatorUpdate. This change is consistent with the modifications made in thex/stakingandx/genutilmodules and is necessary for the tests to reflect these changes.- 132-149: The tests have been updated to use
module.ValidatorUpdateand to modify validator updates based on key types. This change ensures that the tests are aligned with the modifications made in thex/stakingandx/genutilmodules, thereby maintaining test accuracy and relevance.- 246-265: Similar updates have been made in another test within the same file, replacing
abcivalswithvalidatorUpdatesand usingmodule.ValidatorUpdate. These changes are consistent with the PR's objectives and ensure that the integration tests accurately reflect the updated logic in thex/stakingandx/genutilmodules.x/staking/keeper/genesis.go (1)
- 197-233: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [22-230]
The
InitGenesisfunction in thekeeperpackage of the staking module has been updated to return a slice ofmodule.ValidatorUpdateinstead ofabci.ValidatorUpdate. This change aligns with the PR's objective to directly returnValidatorUpdates, enhancing the efficiency and clarity of the Cosmos SDK's handling of validator updates. The function now handles CometBFT updates differently based on theExportedfield in theGenesisState, which is a thoughtful addition to accommodate different scenarios.Similar to previous files, the error handling for unexpected public key types could be enhanced by providing more context in the error message.
Consider enhancing the error message for unexpected validator public key types to include more context, such as the validator's address or name, to aid in debugging.
- return nil, fmt.Errorf("unexpected validator pubkey type: %T", v.PubKey) + return nil, fmt.Errorf("unexpected validator pubkey type: %T for validator with address: %s", v.PubKey, validatorAddress)Note:
validatorAddressshould be replaced with the actual variable holding the validator's address, which might require additional changes to include this information in the error context.
x/genutil/genesis.go
Outdated
| ) (validatorUpdates []module.ValidatorUpdate, err error) { | ||
| if len(genesisState.GenTxs) > 0 { | ||
| validators, err = DeliverGenTxs(ctx, genesisState.GenTxs, stakingKeeper, deliverTx, txEncodingConfig) | ||
| cometValidatorUpdates, err := DeliverGenTxs(ctx, genesisState.GenTxs, stakingKeeper, deliverTx, txEncodingConfig) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| validatorUpdates = make([]module.ValidatorUpdate, len(cometValidatorUpdates)) | ||
| for i, v := range cometValidatorUpdates { | ||
| if ed25519 := v.PubKey.GetEd25519(); len(ed25519) > 0 { | ||
| validatorUpdates[i] = module.ValidatorUpdate{ | ||
| PubKey: ed25519, | ||
| PubKeyType: "ed25519", | ||
| Power: v.Power, | ||
| } | ||
| } else if secp256k1 := v.PubKey.GetSecp256K1(); len(secp256k1) > 0 { | ||
| validatorUpdates[i] = module.ValidatorUpdate{ | ||
| PubKey: secp256k1, | ||
| PubKeyType: "secp256k1", | ||
| Power: v.Power, | ||
| } | ||
| } else { | ||
| return nil, fmt.Errorf("unexpected validator pubkey type: %T", v.PubKey) | ||
| } | ||
| } | ||
| return validatorUpdates, nil |
There was a problem hiding this comment.
The InitGenesis function has been updated to return validatorUpdates of type module.ValidatorUpdate instead of abci.ValidatorUpdate. This change is part of the PR's objective to directly return ValidatorUpdates in keeper functions, eliminating the need for intermediary conversion logic. The function now handles different types of public keys (ed25519 and secp256k1) and returns the appropriate ValidatorUpdate for each. This simplification enhances the maintainability and readability of the code.
However, there's a potential issue with the error handling for unexpected public key types. The error message uses %T to print the type of v.PubKey, which is correct, but it might be beneficial to include more context in the error message to aid in debugging, such as the validator's address or name if available.
Consider enhancing the error message in the case of an unexpected validator public key type to include more context, such as the validator's address or name, to aid in debugging.
- return nil, fmt.Errorf("unexpected validator pubkey type: %T", v.PubKey)
+ return nil, fmt.Errorf("unexpected validator pubkey type: %T for validator with address: %s", v.PubKey, validatorAddress)Note: validatorAddress should be replaced with the actual variable holding the validator's address, which might require additional changes to include this information in the error context.
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.
| ) (validatorUpdates []module.ValidatorUpdate, err error) { | |
| if len(genesisState.GenTxs) > 0 { | |
| validators, err = DeliverGenTxs(ctx, genesisState.GenTxs, stakingKeeper, deliverTx, txEncodingConfig) | |
| cometValidatorUpdates, err := DeliverGenTxs(ctx, genesisState.GenTxs, stakingKeeper, deliverTx, txEncodingConfig) | |
| if err != nil { | |
| return nil, err | |
| } | |
| validatorUpdates = make([]module.ValidatorUpdate, len(cometValidatorUpdates)) | |
| for i, v := range cometValidatorUpdates { | |
| if ed25519 := v.PubKey.GetEd25519(); len(ed25519) > 0 { | |
| validatorUpdates[i] = module.ValidatorUpdate{ | |
| PubKey: ed25519, | |
| PubKeyType: "ed25519", | |
| Power: v.Power, | |
| } | |
| } else if secp256k1 := v.PubKey.GetSecp256K1(); len(secp256k1) > 0 { | |
| validatorUpdates[i] = module.ValidatorUpdate{ | |
| PubKey: secp256k1, | |
| PubKeyType: "secp256k1", | |
| Power: v.Power, | |
| } | |
| } else { | |
| return nil, fmt.Errorf("unexpected validator pubkey type: %T", v.PubKey) | |
| } | |
| } | |
| return validatorUpdates, nil | |
| ) (validatorUpdates []module.ValidatorUpdate, err error) { | |
| if len(genesisState.GenTxs) > 0 { | |
| cometValidatorUpdates, err := DeliverGenTxs(ctx, genesisState.GenTxs, stakingKeeper, deliverTx, txEncodingConfig) | |
| if err != nil { | |
| return nil, err | |
| } | |
| validatorUpdates = make([]module.ValidatorUpdate, len(cometValidatorUpdates)) | |
| for i, v := range cometValidatorUpdates { | |
| if ed25519 := v.PubKey.GetEd25519(); len(ed25519) > 0 { | |
| validatorUpdates[i] = module.ValidatorUpdate{ | |
| PubKey: ed25519, | |
| PubKeyType: "ed25519", | |
| Power: v.Power, | |
| } | |
| } else if secp256k1 := v.PubKey.GetSecp256K1(); len(secp256k1) > 0 { | |
| validatorUpdates[i] = module.ValidatorUpdate{ | |
| PubKey: secp256k1, | |
| PubKeyType: "secp256k1", | |
| Power: v.Power, | |
| } | |
| } else { | |
| return nil, fmt.Errorf("unexpected validator pubkey type: %T for validator with address: %s", v.PubKey, validatorAddress) | |
| } | |
| } | |
| return validatorUpdates, nil |
x/staking/keeper/abci.go
Outdated
| func (k *Keeper) EndBlocker(ctx context.Context) ([]module.ValidatorUpdate, error) { | ||
| defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyEndBlocker) | ||
| return k.BlockValidatorUpdates(ctx) | ||
| cometValidatorUpdates, err := k.BlockValidatorUpdates(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| validatorUpdates := make([]module.ValidatorUpdate, len(cometValidatorUpdates)) | ||
| for i, v := range cometValidatorUpdates { | ||
| if ed25519 := v.PubKey.GetEd25519(); len(ed25519) > 0 { | ||
| validatorUpdates[i] = module.ValidatorUpdate{ | ||
| PubKey: ed25519, | ||
| PubKeyType: "ed25519", | ||
| Power: v.Power, | ||
| } | ||
| } else if secp256k1 := v.PubKey.GetSecp256K1(); len(secp256k1) > 0 { | ||
| validatorUpdates[i] = module.ValidatorUpdate{ | ||
| PubKey: secp256k1, | ||
| PubKeyType: "secp256k1", | ||
| Power: v.Power, | ||
| } | ||
| } else { | ||
| return nil, fmt.Errorf("unexpected validator pubkey type: %T", v.PubKey) | ||
| } | ||
| } | ||
| return validatorUpdates, nil |
There was a problem hiding this comment.
The EndBlocker function has been updated to return an array of module.ValidatorUpdate instead of abci.ValidatorUpdate, aligning with the PR's objective. The function now handles conversion of validator updates to the new type, which simplifies the process and eliminates the need for intermediary conversion logic. This change improves the maintainability and readability of the code.
Similar to the previous file, the error handling for unexpected public key types could be enhanced by providing more context in the error message.
Consider enhancing the error message for unexpected validator public key types to include more context, such as the validator's address or name, to aid in debugging.
- return nil, fmt.Errorf("unexpected validator pubkey type: %T", v.PubKey)
+ return nil, fmt.Errorf("unexpected validator pubkey type: %T for validator with address: %s", v.PubKey, validatorAddress)Note: validatorAddress should be replaced with the actual variable holding the validator's address, which might require additional changes to include this information in the error context.
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.
| func (k *Keeper) EndBlocker(ctx context.Context) ([]module.ValidatorUpdate, error) { | |
| defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyEndBlocker) | |
| return k.BlockValidatorUpdates(ctx) | |
| cometValidatorUpdates, err := k.BlockValidatorUpdates(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| validatorUpdates := make([]module.ValidatorUpdate, len(cometValidatorUpdates)) | |
| for i, v := range cometValidatorUpdates { | |
| if ed25519 := v.PubKey.GetEd25519(); len(ed25519) > 0 { | |
| validatorUpdates[i] = module.ValidatorUpdate{ | |
| PubKey: ed25519, | |
| PubKeyType: "ed25519", | |
| Power: v.Power, | |
| } | |
| } else if secp256k1 := v.PubKey.GetSecp256K1(); len(secp256k1) > 0 { | |
| validatorUpdates[i] = module.ValidatorUpdate{ | |
| PubKey: secp256k1, | |
| PubKeyType: "secp256k1", | |
| Power: v.Power, | |
| } | |
| } else { | |
| return nil, fmt.Errorf("unexpected validator pubkey type: %T", v.PubKey) | |
| } | |
| } | |
| return validatorUpdates, nil | |
| } else { | |
| return nil, fmt.Errorf("unexpected validator pubkey type: %T for validator with address: %s", v.PubKey, validatorAddress) | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Commits
Files that changed from the base of the PR and between 3fe4097b326d42c9a71b5e91af9bce890820e123 and ce30686e9eb67fe75473fa6a63547b7a2572cc35.Files selected for processing (4)
- tests/integration/staking/keeper/genesis_test.go (3 hunks)
- x/genutil/genesis.go (2 hunks)
- x/staking/keeper/abci.go (2 hunks)
- x/staking/keeper/genesis.go (3 hunks)
Files skipped from review as they are similar to previous changes (4)
- tests/integration/staking/keeper/genesis_test.go
- x/genutil/genesis.go
- x/staking/keeper/abci.go
- x/staking/keeper/genesis.go
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Commits
Files that changed from the base of the PR and between ce30686e9eb67fe75473fa6a63547b7a2572cc35 and b66ab4f7c628cd52eb4f26517b604b664a035c44.Files selected for processing (4)
- types/module/module.go (2 hunks)
- x/genutil/genesis.go (2 hunks)
- x/staking/keeper/abci.go (2 hunks)
- x/staking/keeper/genesis.go (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/genutil/genesis.go
- x/staking/keeper/abci.go
- x/staking/keeper/genesis.go
Additional comments: 1
types/module/module.go (1)
- 30-31: The addition of
tmed25519andtmsecp256k1imports is appropriate for handling cryptographic operations in the newParse2ModuleValidatorUpdatefunction.
types/module/module.go
Outdated
|
|
||
| // Parse2ModuleValidatorUpdate parses a slice of comet ValidatorUpdate to a slice of Module ValidatorUpdate | ||
| func Parse2ModuleValidatorUpdate(cometValidatorUpdates []abci.ValidatorUpdate) ([]ValidatorUpdate, error) { | ||
| validatorUpdates := make([]ValidatorUpdate, len(cometValidatorUpdates)) | ||
| for i, v := range cometValidatorUpdates { | ||
| if ed25519 := v.PubKey.GetEd25519(); len(ed25519) > 0 { | ||
| validatorUpdates[i] = ValidatorUpdate{ | ||
| PubKey: ed25519, | ||
| PubKeyType: tmed25519.KeyType, | ||
| Power: v.Power, | ||
| } | ||
| } else if secp256k1 := v.PubKey.GetSecp256K1(); len(secp256k1) > 0 { | ||
| validatorUpdates[i] = ValidatorUpdate{ | ||
| PubKey: secp256k1, | ||
| PubKeyType: tmsecp256k1.KeyType, | ||
| Power: v.Power, | ||
| } | ||
| } else { | ||
| return nil, fmt.Errorf("unexpected validator pubkey type: %T", v.PubKey) | ||
| } | ||
| } | ||
| return validatorUpdates, nil | ||
| } |
There was a problem hiding this comment.
The implementation of Parse2ModuleValidatorUpdate is correct and efficiently handles the conversion of validator updates from Comet to Module types. However, the error message in line 920 could be improved for clarity by including the actual type found.
- return nil, fmt.Errorf("unexpected validator pubkey type: %T", v.PubKey)
+ return nil, fmt.Errorf("unexpected validator pubkey type: %T, expected ed25519 or secp256k1", v.PubKey.Sum)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.
| // Parse2ModuleValidatorUpdate parses a slice of comet ValidatorUpdate to a slice of Module ValidatorUpdate | |
| func Parse2ModuleValidatorUpdate(cometValidatorUpdates []abci.ValidatorUpdate) ([]ValidatorUpdate, error) { | |
| validatorUpdates := make([]ValidatorUpdate, len(cometValidatorUpdates)) | |
| for i, v := range cometValidatorUpdates { | |
| if ed25519 := v.PubKey.GetEd25519(); len(ed25519) > 0 { | |
| validatorUpdates[i] = ValidatorUpdate{ | |
| PubKey: ed25519, | |
| PubKeyType: tmed25519.KeyType, | |
| Power: v.Power, | |
| } | |
| } else if secp256k1 := v.PubKey.GetSecp256K1(); len(secp256k1) > 0 { | |
| validatorUpdates[i] = ValidatorUpdate{ | |
| PubKey: secp256k1, | |
| PubKeyType: tmsecp256k1.KeyType, | |
| Power: v.Power, | |
| } | |
| } else { | |
| return nil, fmt.Errorf("unexpected validator pubkey type: %T", v.PubKey) | |
| } | |
| } | |
| return validatorUpdates, nil | |
| } | |
| // Parse2ModuleValidatorUpdate parses a slice of comet ValidatorUpdate to a slice of Module ValidatorUpdate | |
| func Parse2ModuleValidatorUpdate(cometValidatorUpdates []abci.ValidatorUpdate) ([]ValidatorUpdate, error) { | |
| validatorUpdates := make([]ValidatorUpdate, len(cometValidatorUpdates)) | |
| for i, v := range cometValidatorUpdates { | |
| if ed25519 := v.PubKey.GetEd25519(); len(ed25519) > 0 { | |
| validatorUpdates[i] = ValidatorUpdate{ | |
| PubKey: ed25519, | |
| PubKeyType: tmed25519.KeyType, | |
| Power: v.Power, | |
| } | |
| } else if secp256k1 := v.PubKey.GetSecp256K1(); len(secp256k1) > 0 { | |
| validatorUpdates[i] = ValidatorUpdate{ | |
| PubKey: secp256k1, | |
| PubKeyType: tmsecp256k1.KeyType, | |
| Power: v.Power, | |
| } | |
| } else { | |
| return nil, fmt.Errorf("unexpected validator pubkey type: %T, expected ed25519 or secp256k1", v.PubKey.Sum) | |
| } | |
| } | |
| return validatorUpdates, nil | |
| } |
chixiaowen
force-pushed
the
cxw/validator-updates
branch
from
b66ab4f to
a980312
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (7)
- tests/integration/staking/keeper/genesis_test.go (3 hunks)
- types/module/module.go (2 hunks)
- x/genutil/genesis.go (2 hunks)
- x/genutil/module.go (1 hunks)
- x/staking/keeper/abci.go (2 hunks)
- x/staking/keeper/genesis.go (3 hunks)
- x/staking/module.go (2 hunks)
Files skipped from review as they are similar to previous changes (7)
- tests/integration/staking/keeper/genesis_test.go
- types/module/module.go
- x/genutil/genesis.go
- x/genutil/module.go
- x/staking/keeper/abci.go
- x/staking/keeper/genesis.go
- x/staking/module.go
x/staking/keeper/genesis.go
Outdated
| @@ -195,18 +197,18 @@ func (k Keeper) InitGenesis(ctx context.Context, data *types.GenesisState) (res | |||
|
|
|||
| update := validator.ABCIValidatorUpdate(k.PowerReduction(ctx)) | |||
There was a problem hiding this comment.
i think we can remove this section of code as well. seems we are going to a abciValidatorupdate in order to go back to a different type.
There was a problem hiding this comment.
I do not konw about this, who can helps me ? can I just delete it directly?
There was a problem hiding this comment.
basically you can call this directly in the struct:
cosmos-sdk/x/staking/types/validator.go
Lines 262 to 270 in d54335e
There was a problem hiding this comment.
copying that code which is mentioned. we may be able to drop the comet dependency in staking for validatorupdate entirely
There was a problem hiding this comment.
ok, i will do it
There was a problem hiding this comment.
But this is another issue, can we open a separate issue for it? @julienrbrt @tac0turtle
There was a problem hiding this comment.
Imho, it's the same thing. Otherwise this PR only moves the logic a bit up which doesn't really have any advantage. We should use ValidatorUpdates as early as possible, and if it means not going via the abci struct we should do it.
There was a problem hiding this comment.
👌, I will do it soon
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (10)
- tests/integration/staking/keeper/validator_test.go (3 hunks)
- x/genutil/genesis.go (2 hunks)
- x/genutil/gentx.go (2 hunks)
- x/genutil/testutil/expected_keepers_mocks.go (5 hunks)
- x/genutil/types/expected_keepers.go (1 hunks)
- x/staking/keeper/abci.go (2 hunks)
- x/staking/keeper/genesis.go (3 hunks)
- x/staking/keeper/val_state_change.go (7 hunks)
- x/staking/keeper/validator_test.go (2 hunks)
- x/staking/types/validator.go (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/genutil/genesis.go
- x/staking/keeper/abci.go
- x/staking/keeper/genesis.go
Additional comments: 12
x/genutil/types/expected_keepers.go (1)
- 16-16: The change from
abci.ValidatorUpdatetomodule.ValidatorUpdatein theApplyAndReturnValidatorSetUpdatesmethod signature aligns with the PR's objectives to streamline validator updates handling. Ensure all implementations and usages of this interface are updated accordingly.x/genutil/gentx.go (2)
- 16-16: The addition of the
moduleimport is necessary for the updated return type ofDeliverGenTxsand aligns with the PR's objectives.- 96-96: The update to the return type of
DeliverGenTxsto[]module.ValidatorUpdateis in line with the PR's objectives. Ensure the function's implementation and its callers are correctly updated to handle this new return type.x/genutil/testutil/expected_keepers_mocks.go (2)
- 14-15: The addition of the
moduleimport is necessary for the updated mock function signature and aligns with the changes in the interface definition.- 43-46: The update to the
ApplyAndReturnValidatorSetUpdatesmock function to return[]module.ValidatorUpdatealigns with the interface changes. Ensure the mock's usage in tests is correctly updated to handle this new return type.x/staking/types/validator.go (2)
- 274-286: The addition of the
ModuleValidatorUpdatemethod aligns with the PR's objectives, simplifying the generation of validator updates by directly utilizing themodule.ValidatorUpdatetype. Ensure the method is correctly used wherever validator updates are generated.- 303-315: The addition of the
ModuleValidatorUpdateZeromethod is consistent with the PR's objectives, providing a straightforward way to generate validator updates with zero power using themodule.ValidatorUpdatetype. Ensure this method is used appropriately for the intended validator updates.x/staking/keeper/val_state_change.go (2)
- 27-27: The function
BlockValidatorUpdatesnow returns[]module.ValidatorUpdateinstead of[]abci.ValidatorUpdate. This change aligns with the PR's objective to usemodule.ValidatorUpdatedirectly, enhancing clarity and efficiency in handling validator updates.- 301-323: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [141-359]
In
ApplyAndReturnValidatorSetUpdates, the transition fromabci.ValidatorUpdatetomodule.ValidatorUpdateis evident. However, there are a few points to consider:
- The variable
updatesof type[]abci.ValidatorUpdateis still being used alongside the newmoduleValidatorUpdates. This dual handling could potentially lead to confusion and errors. It's important to ensure that the transition tomodule.ValidatorUpdateis consistent and that any remnants ofabci.ValidatorUpdateare intentional and well-justified.- The conversion process between
abci.ValidatorUpdateandmodule.ValidatorUpdateshould be scrutinized for efficiency and correctness. Ensure that all necessary fields are correctly mapped between these types.- The handling of
moduleValidatorUpdatesat the end of the function, where it's returned, aligns with the PR's goals. However, ensure that all callers of this function are updated to handle the new return type correctly.Consider simplifying the handling of validator updates by fully transitioning to
module.ValidatorUpdatewhere possible, reducing the complexity and potential for errors associated with managing two similar types.x/staking/keeper/validator_test.go (1)
- 20-20: The function
applyValidatorSetUpdatesnow correctly returns[]module.ValidatorUpdate, aligning with the changes made in the main code to usemodule.ValidatorUpdatedirectly. This ensures that the tests are consistent with the implementation and can accurately verify the behavior of the updated functions.tests/integration/staking/keeper/validator_test.go (2)
- 18-18: The import statement for
"github.com/cosmos/cosmos-sdk/types/module"has been added. This change aligns with the PR's objective to utilizemodule.ValidatorUpdateinstead ofabci.ValidatorUpdate, ensuring the tests are updated to reflect the new type usage.- 879-879: The function
applyValidatorSetUpdatesnow returns a slice ofmodule.ValidatorUpdateinstead ofabci.ValidatorUpdate. This change is consistent with the PR's goal to directly usemodule.ValidatorUpdateacross thex/stakingandx/gentutilmodules. It's crucial to ensure that all calls to this function within the test suite have been updated to handle the new return type correctly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- tests/integration/staking/keeper/validator_test.go (13 hunks)
- x/staking/keeper/val_state_change.go (8 hunks)
- x/staking/keeper/validator_test.go (6 hunks)
Files skipped from review as they are similar to previous changes (3)
- tests/integration/staking/keeper/validator_test.go
- x/staking/keeper/val_state_change.go
- x/staking/keeper/validator_test.go
| @@ -230,11 +243,24 @@ func TestInitGenesisLargeValidatorSet(t *testing.T) { | |||
| assert.NilError(t, err) | |||
|
|
|||
| abcivals := make([]abci.ValidatorUpdate, 100) | |||
| validatorUpdates := make([]module.ValidatorUpdate, len(abcivals)) | |||
| for i, val := range validators[:100] { | |||
| abcivals[i] = val.ABCIValidatorUpdate(f.stakingKeeper.PowerReduction(f.sdkCtx)) | |||
There was a problem hiding this comment.
Do we have to go via the abci struct in PowerReduction?
There was a problem hiding this comment.
done! use ModuleValidatorUpdate
| ) | ||
|
|
||
| // StakingKeeper defines the expected staking keeper (noalias) | ||
| type StakingKeeper interface { | ||
| ApplyAndReturnValidatorSetUpdates(context.Context) (updates []abci.ValidatorUpdate, err error) | ||
| ApplyAndReturnValidatorSetUpdates(context.Context) (updates []module.ValidatorUpdate, err error) |
There was a problem hiding this comment.
Can we get a changelog under x/staking for that?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (2)
- tests/integration/staking/keeper/genesis_test.go (3 hunks)
- x/staking/CHANGELOG.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/integration/staking/keeper/genesis_test.go
| @@ -89,6 +89,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| * [#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. | |||
| * [#19414](https://github.com/cosmos/cosmos-sdk/pull/19414) Staking module takes an environment variable in `NewStakingKeeper` instead of individual services. | |||
| * [#19754](https://github.com/cosmos/cosmos-sdk/pull/19754) update to use `[]module.ValidatorUpdate` as return for `ApplyAndReturnValidatorSetUpdates`. | |||
There was a problem hiding this comment.
The changelog entry for PR #19754 correctly summarizes the change made to the ApplyAndReturnValidatorSetUpdates function. However, the entry starts with a lowercase "update" which should be capitalized to maintain consistency with the rest of the document.
- * [#19754](https://github.com/cosmos/cosmos-sdk/pull/19754) update to use `[]module.ValidatorUpdate` as return for `ApplyAndReturnValidatorSetUpdates`.
+ * [#19754](https://github.com/cosmos/cosmos-sdk/pull/19754) Update to use `[]module.ValidatorUpdate` as return for `ApplyAndReturnValidatorSetUpdates`.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.
| * [#19754](https://github.com/cosmos/cosmos-sdk/pull/19754) update to use `[]module.ValidatorUpdate` as return for `ApplyAndReturnValidatorSetUpdates`. | |
| * [#19754](https://github.com/cosmos/cosmos-sdk/pull/19754) Update to use `[]module.ValidatorUpdate` as return for `ApplyAndReturnValidatorSetUpdates`. |
| func (v Validator) ModuleValidatorUpdate(r math.Int) module.ValidatorUpdate { | ||
| consPk, err := v.ConsPubKey() | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| func (v Validator) ModuleValidatorUpdateZero() module.ValidatorUpdate { | ||
| consPk, err := v.ConsPubKey() | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
Description
ref: #19736
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
module.ValidatorUpdateinstead ofabci.ValidatorUpdateacross various files, enhancing consistency and compatibility with newer standards.module.ValidatorUpdateinstances based on validator information, facilitating easier updates and management of validator states.