-
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
R4R: Sunny/tombstone spec #3103
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3103 +/- ##
===========================================
- Coverage 59.38% 59.32% -0.07%
===========================================
Files 131 131
Lines 9862 9862
===========================================
- Hits 5857 5851 -6
- Misses 3632 3638 +6
Partials 373 373 |
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.
Thanks for the spec - left comments - can we add more detail on what exactly a tombstone state means in terms of the store? e.g.
- Validator record left in store, but can never be bonded - essentially special kind of permanent "jail"
- Maximum slashing amount tracked in case multiple infractions are registered (or maybe this can be post-launch)
Also we probably want to change how we deal with unbonding delegations and redelegations a bit. Presently if we just start unbonding the validator delegators will have unbonding delegations, which can't be converted into redelegations (so they'll have to wait the full unbonding period to delegate their stake to a different validator). That seems suboptimal.
Co-Authored-By: sunnya97 <[email protected]>
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.
Tone of this document is a bit funny (ex. use of I) sounds more like ADR - content good however, few comments within
Co-Authored-By: sunnya97 <[email protected]>
Co-Authored-By: sunnya97 <[email protected]>
Co-Authored-By: sunnya97 <[email protected]>
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.
A few more comments, some first-pass comments still unaddressed I think.
Definitely in favor of the simplicity of implementation!
Bump. Anything anyone else can do to help here @sunnya97? |
I think this needs to get rebased as the tests are failing on this branch. |
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.
🍰
This looks ready to merge once the
@cwgoes and @alexanderbez can y'all approve here if the changes look good? |
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.
A few remaining minor comments.
Co-Authored-By: sunnya97 <[email protected]>
Co-Authored-By: sunnya97 <[email protected]>
Is this ready to go @alexanderbez ? |
updated + approved |
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: