refactor(x/staking)!: removing unbonding queue index #22795
Conversation
tac0turtle
requested review from
facundomedica,
testinginprod,
JulianToledano,
ziscky and
a team
as code owners
📝 Walkthrough📝 WalkthroughThe pull request introduces significant changes to the staking module, particularly in the management of unbonding and redelegation processes. Key modifications include the removal of ID parameters from various methods, which simplifies the handling of unbonding and redelegation entries. Additionally, checks related to unbonding statuses have been removed, altering the conditions under which entries are processed. Error handling improvements and test suite enhancements are also included, alongside the removal of deprecated methods and files, streamlining the overall functionality and clarity of the staking module. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
tac0turtle
force-pushed
the
marko/20715
branch
from
f6f14ca to
fe19c3c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🛑 Comments failed to post (2)
x/staking/keeper/validator.go (2)
550-550:
⚠️ Potential issueCorrect the argument type in
k.RemoveValidatorAt line 550,
k.RemoveValidatorexpects asdk.ValAddress, butval.GetOperator()returns astring. This causes a type mismatch error.Apply this diff to fix the argument type:
- k.RemoveValidator(ctx, val.GetOperator()) + valAddr, err := k.validatorAddressCodec.StringToBytes(val.GetOperator()) + if err != nil { + return err + } + err = k.RemoveValidator(ctx, sdk.ValAddress(valAddr)) + if err != nil { + return err + }📝 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.valAddr, err := k.validatorAddressCodec.StringToBytes(val.GetOperator()) if err != nil { return err } err = k.RemoveValidator(ctx, sdk.ValAddress(valAddr)) if err != nil { return err }🧰 Tools
🪛 GitHub Check: tests (03)
[failure] 550-550:
cannot use val.GetOperator() (value of type string) as "github.com/cosmos/cosmos-sdk/types".ValAddress value in argument to k.RemoveValidator
[failure] 550-550:
cannot use val.GetOperator() (value of type string) as "github.com/cosmos/cosmos-sdk/types".ValAddress value in argument to k.RemoveValidator
548-548:
⚠️ Potential issueFix assignment to handle multiple return values
At line 548,
k.UnbondingToUnbondedreturns two values, but only one is being assigned. This results in an assignment mismatch error.Apply this diff to correct the assignment:
-val = k.UnbondingToUnbonded(ctx, val) +val, err = k.UnbondingToUnbonded(ctx, val) +if err != nil { + return err +}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: tests (03)
[failure] 548-548:
assignment mismatch: 1 variable but k.UnbondingToUnbonded returns 2 values
[failure] 548-548:
assignment mismatch: 1 variable but k.UnbondingToUnbonded returns 2 values
tac0turtle
removed request for
a team,
ziscky,
JulianToledano,
facundomedica and
testinginprod
tac0turtle
force-pushed
the
marko/20715
branch
from
6ccc592 to
0837daa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
x/staking/keeper/msg_server_test.go (1)
942-942: Ensure test coverage reflects changes in unbonding logicWith the removal of the unbonding pausing logic, please verify that this test case adequately covers the updated behavior of
NewUnbondingDelegation. Consider adding additional test cases or assertions to ensure all aspects of the unbonding process are thoroughly tested without the unbonding ID parameter.x/staking/migrations/v6/store.go (1)
Line range hint
7-8: Update function comment to reflect all deleted keysThe comment for
MigrateStorestates that it deletes theValidatorUpdatesKeyfrom the store. However, the function now also deletesHistoricalInfoKey,UnbondingIDKey,UnbondingIndexKey, andUnbondingTypeKey. Please update the comment to accurately describe all the keys being deleted.Apply this diff to update the comment:
- // It deletes the ValidatorUpdatesKey from the store. + // It deletes the ValidatorUpdatesKey, HistoricalInfoKey, UnbondingIDKey, UnbondingIndexKey, and UnbondingTypeKey from the store.x/staking/CHANGELOG.md (2)
107-107: Typo: Correct 'there' to 'their'In the sentence "...no longer take an ID in there function signatures", 'there' should be 'their' to indicate possession.
Apply this diff to fix the typo:
- * [#22795](https://github.com/cosmos/cosmos-sdk/pull/22795) `NewUnbondingDelegationEntry`, `NewUnbondingDelegation`, `AddEntry`, `NewRedelegationEntry`, `NewRedelegation` and `NewRedelegationEntryResponse` no longer take an ID in there function signatures. + * [#22795](https://github.com/cosmos/cosmos-sdk/pull/22795) `NewUnbondingDelegationEntry`, `NewUnbondingDelegation`, `AddEntry`, `NewRedelegationEntry`, `NewRedelegation` and `NewRedelegationEntryResponse` no longer take an ID in their function signatures.
112-112: Correct article usage: Replace 'a' with 'an'In the sentence "In a undelegation or redelegation...", 'undelegation' starts with a vowel sound, so the article 'a' should be replaced with 'an'.
Apply this diff to fix the grammatical error:
- * [#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. + * [#18841](https://github.com/cosmos/cosmos-sdk/pull/18841) In an undelegation or redelegation if the shares being left delegated correspond to less than 1 token (in base denom) the entire delegation gets removed.🧰 Tools
🪛 LanguageTool
[misspelling] ~112-~112: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ub.com//pull/18841) In a undelegation or redelegation if the sha...(EN_A_VS_AN)
x/staking/types/delegation.go (1)
Remove UnbondingOnHoldRefCount field and related code
The
UnbondingOnHoldRefCountfield and its related code should be removed as it's part of the ICS unbonding pausing logic being deprecated. Evidence shows:
- Field is only initialized to 0 in constructors
- Only one business logic usage in
OnHold()method- Error constant
ErrUnbondingOnHoldRefCountNegativeis no longer needed- All other occurrences are in generated protobuf files
Locations needing cleanup:
x/staking/types/delegation.go: Remove field fromUnbondingDelegationEntryandRedelegationEntrystructs and their constructorsx/staking/types/validator.go: Remove field fromValidatorstructx/staking/types/delegation.go: RemoveOnHold()methodx/staking/types/errors.go: RemoveErrUnbondingOnHoldRefCountNegativeerror constant- Update protobuf definitions to remove the field
🔗 Analysis chain
Line range hint
69-108: Consider removing UnbondingOnHoldRefCount fieldSince the ICS unbonding pausing logic is being removed, the
UnbondingOnHoldRefCountfield inUnbondingDelegationEntryappears to be unused now.Let's verify if this field is still used elsewhere:
Let me verify if this field is still actively used in the codebase beyond just the generated files:
Let me check if there are any methods that use this field for business logic:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of UnbondingOnHoldRefCount rg "UnbondingOnHoldRefCount" --type goLength of output: 8607
Script:
#!/bin/bash # Search for UnbondingOnHoldRefCount excluding generated files rg "UnbondingOnHoldRefCount" --type go -g '!*.pb.go' -g '!*.pulsar.go'Length of output: 471
Script:
#!/bin/bash # Search for methods or functions that use UnbondingOnHoldRefCount ast-grep --pattern 'func $$$UnbondingOnHoldRefCount$$$'Length of output: 57
Script:
#!/bin/bash # Search for any function or method that uses UnbondingOnHoldRefCount in its body rg "UnbondingOnHoldRefCount" --type go -g '!*.pb.go' -g '!*.pulsar.go' -C 5Length of output: 4186
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/staking/types/staking.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (26)
api/cosmos/staking/v1beta1/staking.pulsar.go(4 hunks)simapp/sim_test.go(0 hunks)tests/integration/staking/keeper/deterministic_test.go(4 hunks)tests/integration/staking/keeper/msg_server_test.go(0 hunks)tests/integration/staking/keeper/slash_test.go(6 hunks)tests/integration/staking/keeper/unbonding_test.go(0 hunks)x/staking/CHANGELOG.md(1 hunks)x/staking/keeper/delegation.go(4 hunks)x/staking/keeper/delegation_test.go(7 hunks)x/staking/keeper/grpc_query.go(0 hunks)x/staking/keeper/keeper.go(0 hunks)x/staking/keeper/msg_server_test.go(1 hunks)x/staking/keeper/slash.go(2 hunks)x/staking/keeper/unbonding.go(0 hunks)x/staking/keeper/unbonding_test.go(0 hunks)x/staking/keeper/val_state_change.go(0 hunks)x/staking/keeper/validator.go(1 hunks)x/staking/migrations/v6/keys.go(1 hunks)x/staking/migrations/v6/store.go(1 hunks)x/staking/proto/cosmos/staking/v1beta1/staking.proto(1 hunks)x/staking/testutil/expected_keepers_mocks.go(0 hunks)x/staking/types/delegation.go(8 hunks)x/staking/types/delegation_test.go(4 hunks)x/staking/types/expected_keepers.go(0 hunks)x/staking/types/hooks.go(0 hunks)x/staking/types/keys.go(0 hunks)
🔥 Files not summarized due to errors (1)
- api/cosmos/staking/v1beta1/staking.pulsar.go: Error: Server error: no LLM provider could handle the message
💤 Files with no reviewable changes (12)
- x/staking/types/expected_keepers.go
- x/staking/keeper/val_state_change.go
- x/staking/types/hooks.go
- tests/integration/staking/keeper/unbonding_test.go
- x/staking/types/keys.go
- simapp/sim_test.go
- x/staking/testutil/expected_keepers_mocks.go
- x/staking/keeper/unbonding_test.go
- tests/integration/staking/keeper/msg_server_test.go
- x/staking/keeper/grpc_query.go
- x/staking/keeper/keeper.go
- x/staking/keeper/unbonding.go
🚧 Files skipped from review as they are similar to previous changes (3)
- x/staking/keeper/slash.go
- x/staking/keeper/validator.go
- x/staking/types/delegation_test.go
🧰 Additional context used
📓 Path-based instructions (10)
api/cosmos/staking/v1beta1/staking.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/staking/migrations/v6/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/staking/keeper/deterministic_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/staking/migrations/v6/keys.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/staking/keeper/msg_server_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/staking/keeper/delegation.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/staking/keeper/slash_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/staking/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
x/staking/keeper/delegation_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/staking/types/delegation.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 LanguageTool
x/staking/CHANGELOG.md
[misspelling] ~112-~112: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ub.com//pull/18841) In a undelegation or redelegation if the sha...
(EN_A_VS_AN)
🔇 Additional comments (23)
api/cosmos/staking/v1beta1/staking.pulsar.go (3)
15462-15463: LGTM: Proper deprecation notice added
The deprecation notice for the UnbondingId field is correctly formatted and aligns with the PR objectives of removing the unbonding queue index.
15517-15517: LGTM: Consistent deprecation of getter method
The GetUnbondingId method is properly marked as deprecated, maintaining consistency with the field deprecation while preserving backward compatibility.
Line range hint 16322-16543: Skip review of generated binary data
These are auto-generated protobuf binary data changes that reflect the deprecation modifications.
x/staking/proto/cosmos/staking/v1beta1/staking.proto (1)
262-262: Deprecation of unbonding_id aligns with PR objectives.
The deprecation of unbonding_id field is consistent with the removal of unbonding pausing logic.
✅ Verification successful
Deprecation of unbonding_id is correctly marked and aligns with codebase
The verification confirms that the unbonding_id field is being properly deprecated in the proto file. The search results show:
- The field is only used in generated code and test data files
- No new usages of the deprecated field are being introduced
- The deprecation is consistent with the existing codebase structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no new usages of the deprecated field
rg -l "unbonding_id" --type proto
Length of output: 63
Script:
#!/bin/bash
# Search for unbonding_id usage across all files
rg -l "unbonding_id"
Length of output: 323
Script:
#!/bin/bash
# Find proto files and search for unbonding_id
fd -e proto -x rg -l "unbonding_id" {}
Length of output: 95
Script:
#!/bin/bash
# Find all proto files to understand the proto file structure
fd -e proto
Length of output: 8633
Script:
#!/bin/bash
# Check the content of files that use unbonding_id to understand the context
rg "unbonding_id" -B 2 -A 2
Length of output: 58291
Script:
#!/bin/bash
# Check if there are any new usages or references to unbonding_id in recent changes
git diff HEAD~1 | rg "unbonding_id"
Length of output: 35
tests/integration/staking/keeper/deterministic_test.go (4)
555-555: Updated deterministic iteration count is correct.
The reduction in iteration count from 3719 to 3707 is consistent with the removal of unbonding queue index logic.
635-635: Updated deterministic iteration count is correct.
The reduction in iteration count from 1621 to 1615 aligns with the simplified unbonding structure.
758-758: Updated deterministic iteration count is correct.
The reduction in iteration count from 1302 to 1290 reflects the removal of unbonding-related fields.
878-878: Updated deterministic iteration count is correct.
The reduction in iteration count from 3926 to 3920 is consistent with the simplified redelegation structure.
tests/integration/staking/keeper/slash_test.go (4)
82-82: LGTM: Proper address codec initialization
The addition of address codecs ensures correct address encoding/decoding for validator and delegator addresses in the unbonding delegation test.
141-141: LGTM: Consistent address codec usage
The redelegation test correctly initializes address codecs for both validator and delegator addresses.
288-288: LGTM: Address codec initialization in unbonding test
The test properly initializes address codecs for the unbonding delegation with validator slashing scenario.
418-418: LGTM: Proper address handling in redelegation test
The redelegation test correctly handles address encoding through proper codec initialization.
x/staking/keeper/delegation.go (4)
515-518: Streamlined redelegation entry creation
The redelegation entry creation has been simplified while maintaining proper address codec handling.
967-967: LGTM: Simplified maturity check
The entry maturity check has been simplified by removing the OnHold status check, aligning with the removal of unbonding queue index.
1104-1104: LGTM: Consistent maturity handling
The redelegation entry maturity check follows the same simplified pattern as unbonding entries.
315-317: Simplified unbonding delegation entry handling
The code has been simplified by removing unbonding ID management, making the entry handling more straightforward.
✅ Verification successful
Let me generate additional verification scripts to check if there are any remaining references to unbonding IDs in the actual business logic code, excluding test files, proto definitions and generated code.
Let me search specifically for any remaining code that might be using the unbonding ID functionality in the actual implementation files.
Unbonding ID management has been safely removed
The verification confirms that the unbonding ID management has been safely removed from the codebase. The only remaining references are empty hook implementations in the slashing and distribution modules that are designed to be no-ops, indicating that the unbonding ID functionality has been properly deprecated and removed from the core staking logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that no code still relies on unbonding IDs
rg -A 3 "UnbondingId|unbondingId|unbonding_id"
Length of output: 67935
Script:
#!/bin/bash
# Search for unbonding ID references in business logic code, excluding tests, proto and generated files
rg -g '!{*.pb.go,*.pulsar.go,*_test.go,*.proto}' -A 3 "UnbondingId|unbondingId|unbonding_id"
Length of output: 38001
Script:
#!/bin/bash
# Search for unbonding ID related code in implementation files
rg -g '!{*.pb.go,*.pulsar.go,*_test.go,*.proto,*.yaml,*.json,*.md}' -A 3 "UnbondingId|unbondingId|unbonding_id|AfterUnbondingInitiated|PutUnbondingOnHold|UnbondingCanComplete"
Length of output: 594
x/staking/keeper/delegation_test.go (3)
345-346: LGTM: Consistent address codec initialization in tests
The tests properly initialize address codecs for both validator and delegator addresses across different test scenarios.
Also applies to: 1109-1110, 1150-1151
718-718: LGTM: Updated redelegation test cases
The redelegation test cases have been updated to reflect the simplified entry handling without unbonding IDs.
Also applies to: 748-748
1116-1116: LGTM: Proper entry merging verification
The tests verify that entries with the same creation height are correctly merged, and new entries are properly appended.
Also applies to: 1125-1125, 1181-1181
x/staking/types/delegation.go (4)
Line range hint 114-134: LGTM: Changes align with unbonding queue index removal
The modifications to AddEntry are consistent with the removal of ICS unbonding pausing logic while preserving the core functionality of managing delegation entries.
322-327: LGTM: Response type changes are consistent
The changes to NewRedelegationEntryResponse properly align with the removal of the unbonding queue index from the core types.
Line range hint 176-219: Remove unused ICS-related code
The following items appear to be remnants of the ICS unbonding pausing logic and should be removed:
UnbondingOnHoldRefCountfield inRedelegationEntryOnHoldmethod which checks this field
Let's verify if these are still used:
225-227: Consider harmonizing AddEntry behavior with UnbondingDelegation
Unlike UnbondingDelegation.AddEntry, this method doesn't check for existing entries with matching creationHeight and completionTime. Consider whether this difference in behavior is intentional.
Let's check if this behavioral difference is documented or tested:
| UnbondingTypeKey = collections.NewPrefix(57) // prefix for an index containing the type of unbonding operations | ||
| UnbondingIDKey = collections.NewPrefix(55) // key for the counter for the incrementing id for UnbondingOperations | ||
| UnbondingIndexKey = collections.NewPrefix(56) // prefix for an index for looking up unbonding operations by their IDs |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variables related to unbonding operations
The variables UnbondingTypeKey, UnbondingIDKey, and UnbondingIndexKey are defined but appear to be unused due to the removal of the unbonding pausing logic. Consider removing these variables to clean up the codebase.
Apply this diff to remove the unused variables:
- UnbondingTypeKey = collections.NewPrefix(57) // prefix for an index containing the type of unbonding operations
- UnbondingIDKey = collections.NewPrefix(55) // key for the counter for the incrementing id for UnbondingOperations
- UnbondingIndexKey = collections.NewPrefix(56) // prefix for an index for looking up unbonding operations by their IDs
julienrbrt
changed the title
julienrbrt
added
the
backport/v0.52.x
Description
Closes: #20715
This pr removes specific logic needed for interchain security that is not used any more.
i adopted some api breaking changes and removed the hook, but let me know if you think some of it is too much and i can revert
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
Release Notes
New Features
MinCommissionRatefor all validators throughMsgUpdateParams.Bug Fixes
Tests
Chores