fix(x/slashing): do not error when val has zero tokens #20977
Conversation
WalkthroughWalkthroughThis update introduces a new test to validate slashing and redelegation behavior in scenarios where a validator's tokens are completely exhausted. Additionally, it refines error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Keeper
participant Validator
participant Account
Tester->>Validator: Create Validator
Tester->>Account: Fund Account
Tester->>Validator: Delegate Tokens
Tester->>Validator: Commit Infraction
Tester->>Validator: Undelegate Tokens
Tester->>Keeper: Slash Validator
Keeper->>Validator: Reduce Tokens
Tester->>Tester: Assert Invariants
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 Configuration File (
|
|
@facundomedica your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- tests/integration/slashing/keeper/slash_redelegation_test.go (1 hunks)
- x/staking/keeper/slash.go (1 hunks)
Additional context used
Path-based instructions (2)
x/staking/keeper/slash.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/slashing/keeper/slash_redelegation_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"
Additional comments not posted (14)
x/staking/keeper/slash.go (1)
377-377: Ensure proper handling of zero tokens scenario.Ignoring the error from
SharesFromTokensTruncatedwhen tokens are zero is appropriate here to streamline control flow. However, ensure that this does not mask any other potential issues that could arise from different contexts.tests/integration/slashing/keeper/slash_redelegation_test.go (13)
369-389: Good setup for the test environment.The setup for various keepers and the application initialization is well-structured.
392-396: Ensure context and message server are correctly initialized.The initialization of the SDK context and the staking message server is correct. Ensure that the bond denomination retrieval does not fail in different environments.
398-407: Proper creation and funding of validators.The creation and funding of the source and destination validators are correctly implemented.
419-432: Correct delegation of user tokens to validators.The delegation of user tokens to the source and destination validators is correctly implemented.
437-443: Accurate recording of infraction height for DST validator.The recording of the infraction height for the destination validator is accurate.
448-451: Correct undelegation of user tokens from DST validator.The undelegation of user tokens from the destination validator is correctly implemented.
453-459: Accurate recording of infraction height for SRC validator.The recording of the infraction height for the source validator is accurate.
464-467: Proper redelegation of user tokens from SRC to DST validator.The redelegation of user tokens from the source validator to the destination validator is correctly implemented.
469-472: Correct undelegation of self-delegation from DST validator.The undelegation of self-delegation from the destination validator is correctly implemented.
477-479: Proper undelegation of user tokens from DST validator.The undelegation of user tokens from the destination validator is correctly implemented.
481-484: Correct assertion for zero tokens in DST validator.The assertion to check that the destination validator has zero tokens is correctly implemented.
486-491: Proper slashing of infractions for DST and SRC validators.The slashing of infractions for the destination and source validators is correctly implemented.
493-501: Correct assertion of invariants to ensure correctness.The assertions to ensure correctness of the slashing process are correctly implemented.
x/staking/keeper/slash.go
Outdated
|
|
||
| sharesToUnbond, _ := dstVal.SharesFromTokensTruncated(slashAmount) // ignore the error as it only errors when tokens == 0 |
There was a problem hiding this comment.
Seems like it would be safer to do the error check but ignore when sharesToUnbond.IsZero(). If that isn't the case, it could be some other error. This is more future proof
There was a problem hiding this comment.
Changed it to
sharesToUnbond, err := dstVal.SharesFromTokensTruncated(slashAmount) // ignore the error as it only errors when tokens == 0
if sharesToUnbond.IsZero() {
continue
} else if err != nil {
return math.ZeroInt(), err
}although in the future we should stop returning an error in SharesFromTokensTruncated, because we are only handling the error if we get a value != 0 and an error.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/staking/keeper/slash.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/staking/keeper/slash.go
facundomedica
added
the
backport/v0.52.x
(cherry picked from commit e8222c8)
…) (#21054) Co-authored-by: Facundo Medica <[email protected]>
Description
This bug was introduced in #20688, if a validator has no tokens left, then we should not error and just continue
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
Tests
Bug Fixes