-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
R4R: Refactor validator deletion #2514
Conversation
8faa75f
to
193e53f
Compare
Codecov Report
@@ Coverage Diff @@
## develop #2514 +/- ##
==========================================
+ Coverage 57.87% 57.9% +0.02%
==========================================
Files 140 140
Lines 8119 8119
==========================================
+ Hits 4699 4701 +2
+ Misses 3123 3121 -2
Partials 297 297 |
193e53f
to
e8c7a2f
Compare
381be29
to
88f3b03
Compare
Can you make a follow-up issue to ensure the simulator will run this scenario? |
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.
Minor comments, otherwise LGTM.
docs/spec/staking/end_block.md
Outdated
if validator.DelegatorShares != 0 { | ||
RemoveValidator(unbondingValidator) | ||
} else { | ||
validator.Status = sdk.Bonded |
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.
Huh why would we set the status to bonded?
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.
Oh good catch, should be unbonded
x/stake/keeper/delegation.go
Outdated
if validator.DelegatorShares.IsZero() && validator.Status != sdk.Bonded { | ||
// if bonded, we must remove in EndBlocker instead | ||
if validator.DelegatorShares.IsZero() && validator.Status == sdk.Unbonded { | ||
// if not unbonded, we must remove in EndBlocker instead |
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.
"we must remove in EndBlocker or at the end of the unbonding queue instead"
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.
Those are the same thing. The unbonding queue is processed during the EndBlocker
x/stake/keeper/slash.go
Outdated
if validator.Tokens.IsZero() && validator.Status != sdk.Bonded { | ||
// if bonded, we must remove in ApplyAndReturnValidatorSetUpdates instead | ||
if validator.Tokens.IsZero() && validator.Status == sdk.Unbonded { | ||
// if not unbonded, we must remove in EndBlocker instead |
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.
Should we check DelegatorShares
instead of Tokens
here? Also I think the comment needs to be updated - "we must remove in EndBlocker or at the end of the unbonding queue instead".
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.
Yeah sure. They should have the same effect, but can I can change it to DelegatorShares.IsZero()
to remain consistent.
…os/cosmos-sdk into sunny/refactor-validator-deletion-2
closes #1673
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added entries in
PENDING.md
with issue #rereviewed
Files changed
in the github PR explorerFor Admin Use: