fix(x/staking): dust share #18841
fix(x/staking): dust share #18841
Conversation
WalkthroughWalkthroughThe code changes aim to address the persistence of "dust shares" on validators after delegation removal. By implementing a tolerance-based condition to fully unbond small delegation shares, the update ensures that residual dust shares are effectively cleared, enhancing user experience and minimizing unnecessary gas fees. 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 (
|
There was a problem hiding this comment.
@JoowonYun can you please provide a changelog entry, along with a unit test or update an existing unit test case?
Of course. I'll add it. |
| @@ -1201,7 +1201,12 @@ func (k Keeper) ValidateUnbondAmount( | |||
| // due to rounding, however we don't want to truncate the shares or take the | |||
| // minimum because we want to allow for the full withdraw of shares from a | |||
| // delegation. | |||
| if shares.GT(delShares) { | |||
| tolerance, err := validator.SharesFromTokens(math.OneInt()) | |||
CHANGELOG.md
Outdated
| @@ -89,6 +89,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| * (simulation) [#17911](https://github.com/cosmos/cosmos-sdk/pull/17911) Fix all problems with executing command `make test-sim-custom-genesis-fast` for simulation test. | |||
| * (simulation) [#18196](https://github.com/cosmos/cosmos-sdk/pull/18196) Fix the problem of `validator set is empty after InitGenesis` in simulation test. | |||
| * (baseapp) [#18551](https://github.com/cosmos/cosmos-sdk/pull/18551) Fix SelectTxForProposal the calculation method of tx bytes size is inconsistent with CometBFT | |||
| * (x/staking) [#18841](https://github.com/cosmos/cosmos-sdk/pull/18841) Fix delegation when undelegate dust share | |||
There was a problem hiding this comment.
this deserves a more thorough explanation. "implement a minimum shares per delegation" or something
CHANGELOG.md
Outdated
| @@ -193,6 +194,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
|
|
|||
| * (x/upgrade) [#16244](https://github.com/cosmos/cosmos-sdk/pull/16244) Upgrade module no longer stores the app version but gets and sets the app version stored in the `ParamStore` of baseapp. | |||
| * (x/staking) [#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. | |||
| * (x/staking) [#18841](https://github.com/cosmos/cosmos-sdk/pull/18841) The existing dust share delegation must be removed. | |||
JoowonYun
force-pushed
the
fix/dust-share
branch
from
9bf1414 to
08570a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
x/staking/keeper/delegation.go
Outdated
| // Depending on the share, amount can be smaller than unit amount(1stake). | ||
| // All shares smaller than the minimum share are unboned. | ||
| tolerance, err := validator.SharesFromTokens(math.OneInt()) | ||
| if err != nil { | ||
| return shares, err | ||
| } | ||
|
|
||
| if delShares.Sub(shares).LT(tolerance) { | ||
| shares = delShares | ||
| } |
There was a problem hiding this comment.
The logic introduced to handle dust shares during unbonding appears to be correct. It checks if the remaining shares after unbonding are less than the tolerance (which is the smallest unit of shares that can be unbonded) and if so, it sets the shares to be unbonded to the total delegation shares. This ensures that no dust shares remain.
However, the comment on line 1201 could be improved for clarity. It currently reads "All shares smaller than the minimum share are unboned." which might be a typo and could be confusing. It should clearly state that any remaining shares smaller than the minimum share unit will be completely unbonded to avoid dust shares.
- // All shares smaller than the minimum share are unboned.
+ // If the remaining shares after unbonding are smaller than the minimum share unit,
+ // they are considered dust and will be completely unbonded to avoid leaving dust shares.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.
| // Depending on the share, amount can be smaller than unit amount(1stake). | |
| // All shares smaller than the minimum share are unboned. | |
| tolerance, err := validator.SharesFromTokens(math.OneInt()) | |
| if err != nil { | |
| return shares, err | |
| } | |
| if delShares.Sub(shares).LT(tolerance) { | |
| shares = delShares | |
| } | |
| // Depending on the share, amount can be smaller than unit amount(1stake). | |
| // If the remaining shares after unbonding are smaller than the minimum share unit, | |
| // they are considered dust and will be completely unbonded to avoid leaving dust shares. | |
| tolerance, err := validator.SharesFromTokens(math.OneInt()) | |
| if err != nil { | |
| return shares, err | |
| } | |
| if delShares.Sub(shares).LT(tolerance) { | |
| shares = delShares | |
| } |
JoowonYun
force-pushed
the
fix/dust-share
branch
from
08570a0 to
b23d00f
Compare
Co-authored-by: atheeshp <[email protected]>
|
LGTM. @facundomedica mind taking a quick look please? |
|
I have a one question. For this purpose, I am curious about the policy on whether it is correct to up the ConsensusVersion of the staking module and create a migration function, or whether it through the upgrade handler like above. |
|
If you want a migration to happen in a module, like the one you linked, you need to bump the consensus version yes. Otherwise, the migration won't be triggered. Alternatively, you can manually call this migration in your app's upgrade handler. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yml
Files selected for processing (1)
- CHANGELOG.md (3 hunks)
Additional comments: 2
CHANGELOG.md (2)
- 100-100: The API Breaking Changes section mentions a significant change in the
x/consensusmodule creation process. This entry is clear and specifies the PR for reference. It's important to ensure that all downstream dependencies and documentation are updated to reflect this change.Verification successful
The search results indicate that the
x/consensusmodule and its changes are mentioned in several places within the project, including theCHANGELOG.md,README.md,UPGRADING.md, and a specificREADME.mdwithin thex/consensusdirectory itself. These references include details about the changes made to thex/consensusmodule, such as the introduction of a new module to handle Tendermint consensus parameters, changes in method signatures (e.g.,NewKeepernow takes aKVStoreService), and guidance for migrating existing parameters from the deprecatedx/paramsto the newx/consensusmodule.Given this context, it appears that the documentation and upgrade guides have been updated to reflect the significant changes in the
x/consensusmodule creation process. The presence of detailed entries inUPGRADING.mdandREADME.mdfiles, specifically addressing thex/consensusmodule, suggests that efforts have been made to ensure downstream dependencies and documentation are aligned with these changes.Based on the information gathered, it seems reasonable to conclude that the necessary documentation and examples have been updated to account for the changes in the
x/consensusmodule creation process.* 2797-2797: The link to the previous versions' CHANGELOG is a helpful resource for users looking to understand the historical context of changes. Ensure this link is kept up to date with the correct branch or tag to maintain accessibility.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for documentation or examples that might need updating due to the consensus module creation change. rg --type md 'x/consensus' --exec cat {}Length of output: 451
Script:
#!/bin/bash # Corrected search for documentation or examples mentioning 'x/consensus' in markdown files. rg --type md 'x/consensus'Length of output: 1985
Verification successful
The verification process has successfully confirmed that the linked
CHANGELOG.mdfor previous versions in the cosmos-sdk repository is accessible and returns aHTTP/2 200status code, indicating that the link points to a valid resource. Therefore, the link is correctly maintained and accessible, aligning with the review comment's suggestion to ensure the link is kept up to date for user accessibility.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the linked CHANGELOG.md for previous versions is accessible and points to the correct branch or tag. curl -sI https://github.com/cosmos/cosmos-sdk/blob/c17c3caab86a1426a1eef4541e8203f5f54a1a54/CHANGELOG.md | grep HTTP/2 200Length of output: 161
Script:
#!/bin/bash # Verify that the linked CHANGELOG.md for previous versions is accessible and points to the correct branch or tag. curl -sI https://github.com/cosmos/cosmos-sdk/blob/c17c3caab86a1426a1eef4541e8203f5f54a1a54/CHANGELOG.md | grep "HTTP/2 200"Length of output: 138
CHANGELOG.md
Outdated
| @@ -94,6 +94,10 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
| * (crypto) [#19371](https://github.com/cosmos/cosmos-sdk/pull/19371) Avoid cli redundant log in stdout, log to stderr instead. | |||
| * (client) [#19393](https://github.com/cosmos/cosmos-sdk/pull/19393/) Add `ReadDefaultValuesFromDefaultClientConfig` to populate the default values from the default client config in client.Context without creating a app folder. | |||
|
|
|||
| ### State Machine Breaking | |||
|
|
|||
| * (x/staking) [#18841](https://github.com/cosmos/cosmos-sdk/pull/18841) In a undelegation or redelegation if the shares being un/re-delegated left correspond to less than 1 token (in base denom) the entire delegation gets removed. | |||
There was a problem hiding this comment.
The description of the change under the State Machine Breaking section for the x/staking module is clear and directly references the PR addressing the issue. However, it would be beneficial to briefly explain the impact on existing delegations and the rationale behind removing entire delegations for clarity to the reader.
Consider adding a brief explanation of the impact on users and the rationale behind this change for better clarity.
| @@ -1234,7 +1238,7 @@ | |||
| * (x/group) [#13214](https://github.com/cosmos/cosmos-sdk/pull/13214) Add `withdraw-proposal` command to group module's CLI transaction commands. | |||
| * (x/auth) [#13048](https://github.com/cosmos/cosmos-sdk/pull/13048) Add handling of AccountNumberStoreKeyPrefix to the simulation decoder. | |||
| * (simapp) [#13107](https://github.com/cosmos/cosmos-sdk/pull/13107) Call `SetIAVLCacheSize` with the configured value in simapp. | |||
| * [#13301](https://github.com/cosmos/cosmos-sdk/pull/13301) Keep the balance query endpoint compatible with legacy blocks | |||
| * [#13301](https://github.com/cosmos/cosmos-sdk/pull/13301) Keep the balance query endpoint compatible with legacy blocks | |||
There was a problem hiding this comment.
The addition regarding the balance query endpoint compatibility with legacy blocks is a valuable update for users relying on historical data. Ensure that this change is well-documented in the relevant API documentation to avoid confusion.
Recommend updating the API documentation to clearly reflect this compatibility change.
CHANGELOG.md
Outdated
| * (crypto) [#19371](https://github.com/cosmos/cosmos-sdk/pull/19371) Avoid cli redundant log in stdout, log to stderr instead. | ||
| * (client) [#19393](https://github.com/cosmos/cosmos-sdk/pull/19393/) Add `ReadDefaultValuesFromDefaultClientConfig` to populate the default values from the default client config in client.Context without creating a app folder. | ||
|
|
||
| ### State Machine Breaking | ||
|
|
||
| * (x/staking) [#18841](https://github.com/cosmos/cosmos-sdk/pull/18841) In a undelegation or redelegation if the shares being un/re-delegated left correspond to less than 1 token (in base denom) the entire delegation gets removed. | ||
|
|
||
| ### API Breaking Changes | ||
|
|
||
| * (x/consensus) [#19488](https://github.com/cosmos/cosmos-sdk/pull/19488) Consensus module creation takes `appmodule.Environment` instead of individual services. |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [13321-13321]
The introduction of a flag to disable fast node migration and usage is an important feature that could affect deployment and operational strategies. It would be helpful to provide more context or a link to documentation on when and why to use this flag.
Consider adding more context or a link to documentation for the flag to disable fast node migration and usage, helping users understand its purpose and application scenarios.
Description
Closes: #13086
Amount less than 1stake remains in the share, delegation is removed.
PS. If you agree with this policy, I will add test code and migration details.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
CHANGELOG.mdwith details on significant changes, including: