refactor(x/staking): use sdk validator updates #19788
Conversation
WalkthroughWalkthroughThe update focuses on transitioning the staking module in the Cosmos SDK to use 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 (
|
x/staking/keeper/keeper.go
Outdated
| @@ -41,6 +42,49 @@ func HistoricalInfoCodec(cdc codec.BinaryCodec) collcodec.ValueCodec[types.Histo | |||
| }) | |||
| } | |||
|
|
|||
| type moduleValidatorUpdatesCodec struct { | |||
There was a problem hiding this comment.
@testinginprod, can we generalize that in collection? Like a JSONValueCodec that always does the following?
There was a problem hiding this comment.
It isn't needed here, but I'll create one now in collections
x/staking/keeper/keeper.go
Outdated
| @@ -84,7 +128,7 @@ type Keeper struct { | |||
| // LastTotalPower value: LastTotalPower | |||
| LastTotalPower collections.Item[math.Int] | |||
| // ValidatorUpdates value: ValidatorUpdates | |||
| ValidatorUpdates collections.Item[types.ValidatorUpdates] | |||
| ValidatorUpdates collections.Item[types.ModuleValidatorUpdates] | |||
There was a problem hiding this comment.
does anyone use this value? seems we only set it
There was a problem hiding this comment.
That's a good point 👍🏾 Let me ask, we don't use it ourselves indeed.
![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: 4
Configuration used: .coderabbit.yml
Files selected for processing (11)
- core/appmodule/module.go (1 hunks)
- tests/integration/staking/keeper/genesis_test.go (2 hunks)
- x/staking/CHANGELOG.md (1 hunks)
- x/staking/keeper/abci.go (2 hunks)
- x/staking/keeper/genesis.go (2 hunks)
- x/staking/keeper/keeper.go (4 hunks)
- x/staking/keeper/val_state_change.go (7 hunks)
- x/staking/keeper/validator_test.go (2 hunks)
- x/staking/module.go (2 hunks)
- x/staking/types/validator.go (3 hunks)
- x/staking/types/validator_test.go (1 hunks)
Additional comments: 19
x/staking/keeper/abci.go (2)
- 7-7: The addition of the
"cosmossdk.io/core/appmodule"import is necessary for the updated functionality in theEndBlockerfunction. This change aligns with the PR's objective to standardize validator updates handling.- 21-21: The update of the
EndBlockerfunction's return type to[]appmodule.ValidatorUpdateis in line with the PR's goal of improving modularity and standardization. Ensure that theBlockValidatorUpdatesfunction, which is called withinEndBlocker, also returns this updated type.core/appmodule/module.go (1)
- 32-34: The addition of the
ValidatorUpdatetype as an alias toappmodule.ValidatorUpdateis a clear and effective way to standardize the handling of validator updates across different modules. This change supports the PR's objectives well.x/staking/module.go (2)
- 148-148: The update of the
InitGenesisfunction's return type to[]appmodule.ValidatorUpdatealigns with the PR's objectives of improving modularity and standardization. Ensure that the implementation ofInitGenesisis compatible with this updated return type.- 176-176: The update of the
EndBlockfunction's return type to[]appmodule.ValidatorUpdateis consistent with the PR's goal of standardizing validator updates handling. Ensure that theEndBlockerfunction, which is called withinEndBlock, also returns this updated type.tests/integration/staking/keeper/genesis_test.go (1)
- 10-10: The addition of the
"cosmossdk.io/core/appmodule"import in the test file is necessary to support the updatedappmodule.ValidatorUpdatetype. This change is consistent with the PR's objectives.x/staking/CHANGELOG.md (2)
- 46-93: The changelog entries under "API Breaking Changes" detail the use of collections for various data structures and the removal of specific functions and keys. These changes are significant for developers and should be clearly communicated. Ensure that the descriptions are accurate and provide enough context for developers to understand the impact of these changes.
- 99-102: The "State Breaking changes" section mentions significant changes that could affect the state and operation of the Cosmos SDK. It's crucial to ensure that these entries are clear and provide sufficient detail for users to understand the implications of the changes. Consider adding more context or examples if necessary to clarify the impact of these changes.
x/staking/keeper/genesis.go (2)
- 20-20: The update to the
InitGenesisfunction to use[]appmodule.ValidatorUpdateas the return type aligns with the PR's objective to standardize how validator updates are handled. This change is crucial for ensuring consistency across different modules. Ensure that all references to the old return type have been updated accordingly.- 178-178: The introduction of
moduleValidatorUpdatesas a slice ofappmodule.ValidatorUpdateis a direct consequence of the updated return type of theInitGenesisfunction. This change is necessary for accumulating validator updates to be returned by the function. Verify that the logic for populating this slice correctly reflects the intended validator updates, especially in the context of exported and non-exported data.x/staking/types/validator_test.go (1)
- 60-65: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Please ensure that all removed tests have their functionality covered by new or existing tests, especially since
ABCIValidatorUpdateandABCIValidatorUpdateZerofunctions have been removed. This is crucial for maintaining the integrity of the test suite.x/staking/types/validator.go (2)
- 13-13: The addition of
"cosmossdk.io/core/appmodule"is necessary for the newModuleValidatorUpdateandModuleValidatorUpdateZerofunctions. This aligns with the PR's goal of integrating more closely with the Cosmos SDK's core functionalities.- 259-288: The new
ModuleValidatorUpdateandModuleValidatorUpdateZerofunctions are well-implemented replacements for the removedABCIValidatorUpdateandABCIValidatorUpdateZero. However, it's crucial to ensure that these changes are reflected wherever validator updates are handled across the SDK to maintain consistency and avoid potential integration issues.x/staking/keeper/keeper.go (2)
- 45-82: The introduction of
moduleValidatorUpdatesCodecwith JSON encoding and decoding methods forModuleValidatorUpdatesis a significant change. Ensure that JSON is the preferred format for these operations, considering performance and compatibility with the rest of the Cosmos SDK. JSON parsing can be more CPU-intensive compared to binary formats like Protobuf, which might already be used elsewhere in the SDK.- 131-131: The update to
ValidatorUpdatesto usetypes.ModuleValidatorUpdatesaligns with the PR's objective to standardize validator updates handling. However, ensure that all references and usages ofValidatorUpdatesthroughout the staking module and any other dependent modules have been updated accordingly to prevent type mismatches.Verification successful
The references to
types.ValidatorUpdatesin bothx/staking/keeper/keeper.goandsimapp/sim_test.goare related to keys or identifiers, not direct type usages that would require updating totypes.ModuleValidatorUpdates. These findings indicate that the initial concern about ensuring all references and usages ofValidatorUpdateshave been updated accordingly to prevent type mismatches does not apply to these cases. Therefore, the review comment is verified as the changes align with the PR's objective without any issues found in the context provided.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old `types.ValidatorUpdates` usage that might have been missed. rg --type go "types.ValidatorUpdates"Length of output: 276
Script:
#!/bin/bash # Examine the context of `types.ValidatorUpdates` usage in `x/staking/keeper/keeper.go` rg --type go -C 5 "types.ValidatorUpdates" x/staking/keeper/keeper.go # Examine the context of `types.ValidatorUpdates` usage in `simapp/sim_test.go` rg --type go -C 5 "types.ValidatorUpdates" simapp/sim_test.goLength of output: 1607
x/staking/keeper/val_state_change.go (2)
- 25-25: The function
BlockValidatorUpdatesnow returns[]appmodule.ValidatorUpdateinstead of the previous type. This change is consistent with the PR's objective to standardize validator updates handling. Ensure that all callers of this function are updated to handle the new return type correctly.- 215-215: The use of
appmodule.ValidatorUpdatein various parts of the code, including key rotations and setting validator updates, is noted. Ensure that the transition from the old type to the newappmodule.ValidatorUpdateis seamless and does not affect the logic for updating validator sets, especially during consensus key rotations.Also applies to: 250-250, 292-292, 331-331
Verification successful
Given the initial search did not yield results, it's necessary to broaden our approach to better understand how
appmodule.ValidatorUpdateis handled, especially in contexts that might not explicitly mention "consensus key rotation". Let's perform a more general search forValidatorUpdatereferences in the Go files to uncover its usage patterns.
The broadened search results confirm the extensive use of
appmodule.ValidatorUpdatethroughout the staking module, including in the context of consensus key rotations. The specific implementation and usage patterns, such as inx/staking/types/validator.goandx/staking/keeper/val_state_change.go, demonstrate a well-integrated approach to handling validator updates. This includes the generation of validator updates during consensus public key rotations, aligning with the review comment's emphasis on ensuring seamless transitions and updates. Therefore, the handling ofappmodule.ValidatorUpdateduring consensus key rotations and other validator update scenarios appears to be correctly implemented and integrated into the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the handling of `appmodule.ValidatorUpdate` during consensus key rotations. rg --type go "ValidatorUpdate" | grep "consensus key rotation"Length of output: 62
Script:
#!/bin/bash # Broaden the search for `ValidatorUpdate` to understand its usage in the codebase. rg --type go "ValidatorUpdate"Length of output: 30648
x/staking/keeper/validator_test.go (2)
- 9-9: The addition of the import
cosmossdk.io/core/appmoduleis necessary for the updated functionality in this test suite. This change aligns with the PR's objective to standardize validator updates handling.- 20-20: The change in the return type of
applyValidatorSetUpdatesfrommodule.ValidatorUpdatetoappmodule.ValidatorUpdateis crucial for aligning with the new standard for handling validator updates. Please ensure that all usages of this function within the test suite have been updated to handle the new return type correctly.Verification successful
The review comment is accurate regarding the change in the return type of
applyValidatorSetUpdatesfrommodule.ValidatorUpdatetoappmodule.ValidatorUpdatewithin thex/staking/keeper/validator_test.gofile and its direct usages. It's important to ensure that all usages of this function within the test suite have been updated to handle the new return type correctly. Additionally, it's noted that there is another function with the same name but a different return type (module.ValidatorUpdate) used in the integration tests, which is a separate context and does not contradict the review comment but is worth acknowledging for a comprehensive understanding of the codebase changes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `applyValidatorSetUpdates` is correctly used with its new return type throughout the test suite. rg "applyValidatorSetUpdates" --type goLength of output: 5482
| require.Equal(t, d, d3) | ||
| } | ||
|
|
||
| func TestABCIValidatorUpdate(t *testing.T) { | ||
| validator := newValidator(t, valAddr1, pk1) | ||
| abciVal := validator.ABCIValidatorUpdate(sdk.DefaultPowerReduction) | ||
| pk, err := validator.TmConsPublicKey() | ||
| require.NoError(t, err) | ||
| require.Equal(t, pk, abciVal.PubKey) | ||
| require.Equal(t, validator.BondedTokens().Int64(), abciVal.Power) | ||
| } | ||
|
|
||
| func TestABCIValidatorUpdateZero(t *testing.T) { | ||
| validator := newValidator(t, valAddr1, pk1) | ||
| abciVal := validator.ABCIValidatorUpdateZero() | ||
| pk, err := validator.TmConsPublicKey() | ||
| require.NoError(t, err) | ||
| require.Equal(t, pk, abciVal.PubKey) | ||
| require.Equal(t, int64(0), abciVal.Power) | ||
| } | ||
|
|
||
| func TestShareTokens(t *testing.T) { | ||
| validator := mkValidator(100, math.LegacyNewDec(100)) | ||
| assert.True(math.LegacyDecEq(t, math.LegacyNewDec(50), validator.TokensFromShares(math.LegacyNewDec(50)))) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Given the significant changes in the handling of validator updates, it's important to add tests that specifically verify the new ModuleValidatorUpdate and ModuleValidatorUpdateZero functionalities to ensure they behave as expected within the staking module.
Would you like assistance in drafting these tests?
| // ModuleValidatorUpdate returns a appmodule.ValidatorUpdate from a staking validator type | ||
| // with the full validator power. | ||
| // It replaces the previous ABCIValidatorUpdate function. | ||
| func (v Validator) ModuleValidatorUpdate(r math.Int) appmodule.ValidatorUpdate { |
There was a problem hiding this comment.
The use of panic in ModuleValidatorUpdate and ModuleValidatorUpdateZero for handling errors from ConsPubKey() is a concern. Consider returning an error from these functions instead of panicking to allow for more graceful error handling in the calling code.
- func (v Validator) ModuleValidatorUpdate(r math.Int) appmodule.ValidatorUpdate {
+ func (v Validator) ModuleValidatorUpdate(r math.Int) (appmodule.ValidatorUpdate, error) {
- func (v Validator) ModuleValidatorUpdateZero() appmodule.ValidatorUpdate {
+ func (v Validator) ModuleValidatorUpdateZero() (appmodule.ValidatorUpdate, error) {
- panic(err)
+ return appmodule.ValidatorUpdate{}, errCommittable 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 (v Validator) ModuleValidatorUpdate(r math.Int) appmodule.ValidatorUpdate { | |
| func (v Validator) ModuleValidatorUpdate(r math.Int) (appmodule.ValidatorUpdate, error) { |
| @@ -128,8 +128,7 @@ func TestInitGenesis(t *testing.T) { | |||
| assert.Assert(t, found) | |||
| assert.Equal(t, types.Bonded, resVal.Status) | |||
|
|
|||
| abcivals := make([]abci.ValidatorUpdate, len(vals)) | |||
| validatorUpdates := make([]module.ValidatorUpdate, len(abcivals)) | |||
| validatorUpdates := make([]appmodule.ValidatorUpdate, len(vals)) | |||
There was a problem hiding this comment.
The update of references from abci.ValidatorUpdate to appmodule.ValidatorUpdate in the test file aligns with the PR's goal of standardizing the handling of validator updates. Consider adding more comprehensive tests to ensure that all scenarios involving validator updates are thoroughly covered.
Would you like me to help with adding more comprehensive tests to cover these changes?
| @@ -138,7 +136,7 @@ func (k Keeper) BlockValidatorUpdates(ctx context.Context) ([]module.ValidatorUp | |||
| // CONTRACT: Only validators with non-zero power or zero-power that were bonded | |||
| // at the previous block height or were removed from the validator set entirely | |||
| // are returned to CometBFT. | |||
| func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx context.Context) ([]module.ValidatorUpdate, error) { | |||
| func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx context.Context) ([]appmodule.ValidatorUpdate, error) { | |||
There was a problem hiding this comment.
The function ApplyAndReturnValidatorSetUpdates also reflects the change to []appmodule.ValidatorUpdate. Given the critical role of this function in the staking logic, thoroughly test these changes to ensure that the validator updates are correctly applied and returned, especially in edge cases such as validator power changes and key rotations.
Consider adding more unit tests covering various scenarios of validator set updates to ensure the robustness of these changes.
x/staking/CHANGELOG.md
Outdated
| * remove `Keeper`: `SetLastTotalPower`, `GetLastTotalPower` | ||
| * [#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`. | ||
|
|
||
| ### State Breaking changes | ||
|
|
||
| * [#18841](https://github.com/cosmos/cosmos-sdk/pull/18841) In a undelegation or redelegation if the shares being left delegated correspond to less than 1 token (in base denom) the entire delegation gets removed. | ||
| * [#18142](https://github.com/cosmos/cosmos-sdk/pull/18142) Introduce `key_rotation_fee` param to calculate fees while rotating the keys | ||
| * [#17655](https://github.com/cosmos/cosmos-sdk/pull/17655) `HistoricalInfo` was replaced with `HistoricalRecord`, it removes the validator set and comet header and only keep what is needed for IBC. | ||
| * [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic. No newline at end of file | ||
| * [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic. |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [46-103]
The changelog provides a comprehensive overview of the significant changes made to the staking module. However, there are a few areas that could benefit from additional clarity or correction:
-
Ensure consistency in the presentation of changes, particularly in the "API Breaking Changes" section. Each entry should clearly describe the change and its impact on the module or SDK users.
-
For removed functions and types, consider providing brief guidance on the migration path or alternatives that developers should use. This can help ease the transition for those affected by the breaking changes.
-
In the "State Breaking Changes" section, the description of changes could be enhanced by explaining the rationale behind the changes and their expected impact on the staking module's behavior or state.
-
Verify the accuracy of all linked PRs and ensure they correctly correspond to the described changes. This helps readers to explore the changes in more detail if needed.
-
Check for typographical errors and grammatical issues throughout the document to maintain professionalism and readability.
Improving these areas will enhance the changelog's utility as a resource for developers and users of the Cosmos SDK.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yml
Files selected for processing (13)
- api/cosmos/staking/v1beta1/staking.pulsar.go (2 hunks)
- x/staking/CHANGELOG.md (2 hunks)
- x/staking/README.md (2 hunks)
- x/staking/keeper/keeper.go (2 hunks)
- x/staking/keeper/migrations.go (2 hunks)
- x/staking/keeper/val_state_change.go (8 hunks)
- x/staking/migrations/v5/store.go (2 hunks)
- x/staking/migrations/v6/keys.go (1 hunks)
- x/staking/migrations/v6/store.go (1 hunks)
- x/staking/module.go (3 hunks)
- x/staking/proto/cosmos/staking/v1beta1/staking.proto (1 hunks)
- x/staking/types/keys.go (2 hunks)
- x/staking/types/validator.go (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/staking/keeper/val_state_change.go
- x/staking/module.go
- x/staking/types/validator.go
Additional comments: 23
x/staking/migrations/v6/keys.go (1)
- 5-5: The introduction of
ValidatorUpdatesKeywith a prefix value of97is noted. It's good practice to add a comment explaining the purpose of this key or the type of data it will be associated with in the store for future reference and clarity.x/staking/migrations/v6/store.go (1)
- 11-15: The
MigrateStorefunction correctly performs the intended migration step by deleting theValidatorUpdatesKey. The implementation is straightforward and follows best practices for store migrations.x/staking/keeper/migrations.go (1)
- 45-49: The addition of the
Migrate5to6function is well-implemented, following the established patterns for migration functions within the Cosmos SDK. It correctly utilizes thev6.MigrateStorefor the migration logic.x/staking/migrations/v5/store.go (2)
- 16-16: Simplifying the
migrateDelegationsByValidatorIndexfunction by removing unused parameters improves clarity and maintainability. Good refactor.- 34-34: The update to the
MigrateStorefunction to callmigrateDelegationsByValidatorIndexwith its new signature is correctly implemented. The rest of the migration logic remains clear and focused.x/staking/CHANGELOG.md (1)
- 88-102: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [46-102]
The updates to the changelog provide a comprehensive overview of the significant changes made to the staking module, including new features, improvements, bug fixes, and breaking changes. It's important to maintain clarity and accuracy in these entries to ensure they serve as a useful resource for developers and users. The categorization and referencing of pull requests are well done.
x/staking/types/keys.go (3)
- 60-61: The introduction of
HistoricalInfoKeyand the modification ofParamsKeyare consistent with the Cosmos SDK's practices for defining store keys. It's good practice to add comments explaining the purpose of these keys for future reference.- 73-76: The reserved key prefix with a value of
97is noted. While its purpose is not immediately clear, reserving keys for future use is a common practice. Adding a comment to explain its intended use or reservation would improve clarity.- 77-77: The introduction of the
UnbondingTypeenum provides a clear way to distinguish between different types of unbonding operations. This is a good practice for enhancing code readability and maintainability.x/staking/keeper/keeper.go (9)
- 83-88: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Please ensure the package documentation comment is present at the top of the file to provide a high-level overview of its purpose and functionalities. This is a common practice in Go for better code readability and maintainability.
- 83-88: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [17-17]
The use of
gogotypes "github.com/cosmos/gogoproto/types"is noted. Ensure that the specific types from this package are necessary and correctly used throughout the file, especially considering the move towards more standardized Protobuf types in newer Cosmos SDK versions.
- 83-88: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [29-29]
The introduction of
appmoduleandaddresscodecfromcosmossdk.io/corealigns with the PR's objective to integrate more closely with Cosmos SDK's core functionalities. Ensure that these imports replace previous mechanisms effectively and that there are no remnants of the old implementation that could cause confusion or errors.
- 83-88: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [33-33]
The removal of
ValidatorUpdatesfrom theKeeperstruct is a significant change. Ensure that all functionalities relying onValidatorUpdateshave been appropriately updated or refactored to work without it. This change should also be reflected in any relevant documentation or comments within the code.Verification successful
The search results indicate that
ValidatorUpdatesis still referenced and utilized within thex/stakingmodule, particularly in protobuf definitions, keeper functionalities, and migrations. This suggests that whileValidatorUpdatesmay have been removed from theKeeperstruct, its concept and utility persist in various forms across the module. The presence of operational code and migrations handlingValidatorUpdatesimplies that the necessary updates or refactors following its removal from theKeeperstruct have been considered and implemented. Therefore, the review comment's concern appears to have been addressed through the module's current handling ofValidatorUpdates.* 83-88: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [35-35]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any code segments that might still be referencing `ValidatorUpdates` within the staking module. rg --type go 'ValidatorUpdates' x/staking/Length of output: 3537
The
HistoricalInfoCodecfunction demonstrates good use of thecollcodec.ValueCodecinterface for custom serialization logic. Ensure that the deprecation ofHistoricalInfoand the transition toHistoricalRecordare well-documented and communicated to module users, as this could impact downstream applications.
- 83-88: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [35-35]
The comment
//nolint: staticcheck // HistoricalInfo is deprecateddirectly addresses the use of deprecated types. While this is a valid approach to suppress linting errors, it's crucial to plan for the removal of deprecated types and ensure that all references toHistoricalInfoare eventually replaced withHistoricalRecordor other updated types.
- 83-88: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [35-35]
The transition from
HistoricalInfotoHistoricalRecordwithin theHistoricalInfoCodecfunction aligns with the PR's objectives of improving modularity and organization. Ensure that this change is consistently applied across the module and that any external modules or applications depending on the staking module are updated accordingly.
- 83-88: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [35-35]
The explicit error handling in
HistoricalInfoCodecis commendable. It ensures that any issues during the unmarshalling process are caught and handled appropriately. This practice should be consistently applied throughout the codebase to maintain robustness and reliability.
- 83-88: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [35-35]
The use of
types.HistoricalRecordas the return type inHistoricalInfoCodecis a good practice, as it encapsulates the relevant data in a more structured and type-safe manner. This approach enhances code readability and maintainability.x/staking/proto/cosmos/staking/v1beta1/staking.proto (1)
- 409-410: Marking
ValidatorUpdatesas deprecated is a significant change. It's important to communicate this change effectively to developers and users of the Cosmos SDK to ensure a smooth transition. Consider adding more detailed deprecation notices in the documentation or release notes.x/staking/README.md (2)
- 23-28: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-7]
The update to the table of contents aligns with the removal of the
ValidatorUpdatessection as mentioned. The changes are clear and maintain the document's structure.
- 23-28: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [9-79]
The updates to the
LastTotalPowerdescription and the detailed explanation ofUnbondingIDare well-articulated, enhancing the clarity of the documentation. The technical terms used are accurate, and there are no spelling or grammatical errors in the context of this technical documentation.api/cosmos/staking/v1beta1/staking.pulsar.go (2)
- 15898-15899: The
ValidatorUpdatestype is marked as deprecated, which is good for indicating that it should no longer be used. However, it would be beneficial to provide guidance on what should be used instead or any other recommended actions for developers. This could be in the form of a comment suggesting an alternative type or a reference to documentation for further reading.- 16414-16478: It appears this hunk includes changes to protobuf serialization data, likely as a result of modifications to protobuf definitions. While reviewing the binary data directly isn't practical, it's important to ensure that these changes are consistent with the intended modifications to the protobuf definitions. If the protobuf definitions were updated as part of this PR, ensure that all necessary regeneration steps have been followed and that the changes accurately reflect the updated definitions.
| HistoricalInfo collections.Map[uint64, types.HistoricalRecord] | ||
| // LastTotalPower value: LastTotalPower | ||
| LastTotalPower collections.Item[math.Int] | ||
| // ValidatorUpdates value: ValidatorUpdates | ||
| ValidatorUpdates collections.Item[types.ValidatorUpdates] | ||
| // DelegationsByValidator key: valAddr+delAddr | value: none used (index key for delegations by validator index) | ||
| DelegationsByValidator collections.Map[collections.Pair[sdk.ValAddress, sdk.AccAddress], []byte] | ||
| UnbondingID collections.Sequence |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [35-35]
The implementation of HistoricalInfoCodec for backward compatibility is noted. However, ensure that there's a clear migration path for users of the staking module to move away from deprecated types like HistoricalInfo. This might involve providing scripts, documentation, or other forms of support to facilitate the transition.
Would you like assistance in creating a migration guide or script for users to transition away from HistoricalInfo?
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [35-35]
The use of custom serialization logic in HistoricalInfoCodec is a critical change. It's important to thoroughly test this logic to prevent any data corruption or loss during the serialization and deserialization processes. Consider adding unit tests specifically targeting edge cases and error handling scenarios.
Do you need help with writing these unit tests?
| option deprecated = true; | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
The script successfully found a reference to ValidatorUpdates within a .proto file in the repository. This indicates that there are still references to ValidatorUpdates in the codebase, which aligns with the need to ensure all references to this deprecated message are updated as mentioned in the review comment. Therefore, it's important to address these references and provide appropriate migration documentation for users of the staking module.
Analysis chain
The deprecation of the ValidatorUpdates message aligns with the PR's objectives of streamlining the handling of validator updates. Ensure that all references to ValidatorUpdates in the codebase are updated to use the new mechanism, and consider providing migration documentation for external users of the staking module.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to `ValidatorUpdates` in the staking module and related modules.
rg --type proto 'ValidatorUpdates' proto/cosmos/staking/
Length of output: 86
Script:
#!/bin/bash
# Corrected search for any remaining references to `ValidatorUpdates` in `.proto` files within the staking module and related modules.
rg 'ValidatorUpdates' proto/cosmos/staking/*.proto
Length of output: 121
Script:
#!/bin/bash
# Use fd to find .proto files within the staking module and rg to search for ValidatorUpdates within those files.
fd '.proto$' proto/cosmos/staking/ --exec rg 'ValidatorUpdates'
Length of output: 172
Script:
#!/bin/bash
# Search the entire repository for .proto files related to the staking module and check for references to ValidatorUpdates.
fd '.proto$' --exec rg 'ValidatorUpdates'
Length of output: 138
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- simapp/sim_test.go (1 hunks)
Path instructions used (1)
simapp/sim_test.go (2)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request
Additional comments (1)
simapp/sim_test.go (1)
- 197-197: The removal of
stakingtypes.ValidatorUpdatesKeyfrom theskipPrefixesmap forstakingtypes.StoreKeyaligns with the PR's objective of refactoring and improving the staking module. This change suggests thatValidatorUpdatesKeyis no longer relevant or necessary in the context of theTestAppImportExportfunction, which likely reflects broader changes in how validator updates are handled within the module.Ensure that this removal does not inadvertently affect the logic of the
TestAppImportExporttest, particularly in the comparison of stores section. It's important to verify that the test still accurately reflects the intended state and behavior of the application after the refactoring.
Description
Follow-up of #19754
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
ModuleValidatorUpdateto optimize validator updates processing, replacingABCIValidatorUpdate.