-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(x/staking): dust share #18841
fix(x/staking): dust share #18841
Changes from 6 commits
cb0c593
b195a4b
b23d00f
ff6e240
b3213a3
5b229d5
82b9fa4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
||
### 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
|
@@ -1234,7 +1238,7 @@ replace github.com/confio/ics23/go => github.com/cosmos/cosmos-sdk/ics23/go v0.8 | |
* (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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
* [#13321](https://github.com/cosmos/cosmos-sdk/pull/13321) Add flag to disable fast node migration and usage. | ||
|
||
### Bug Fixes | ||
|
@@ -2790,4 +2794,4 @@ sure you are aware of any relevant breaking changes. | |
|
||
## Previous Releases | ||
|
||
[CHANGELOG of previous versions](https://github.com/cosmos/cosmos-sdk/blob/c17c3caab86a1426a1eef4541e8203f5f54a1a54/CHANGELOG.md#v0391---2020-08-11) (pre Stargate). | ||
[CHANGELOG of previous versions](https://github.com/cosmos/cosmos-sdk/blob/c17c3caab86a1426a1eef4541e8203f5f54a1a54/CHANGELOG.md#v0391---2020-08-11) (pre Stargate). |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1195,11 +1195,15 @@ func (k Keeper) ValidateUnbondAmount( | |
return shares, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "invalid shares amount") | ||
} | ||
|
||
// Cap the shares at the delegation's shares. Shares being greater could occur | ||
// 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) { | ||
// Depending on the share, amount can be smaller than unit amount(1stake). | ||
// If the remain amount after unbonding is smaller than the minimum share, | ||
// it's completely unbonded to avoid leaving dust shares. | ||
tolerance, err := validator.SharesFromTokens(math.OneInt()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update the comment above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if err != nil { | ||
return shares, err | ||
} | ||
|
||
if delShares.Sub(shares).LT(tolerance) { | ||
shares = delShares | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.