-
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: Validator Power Dec-> Int #2958
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2958 +/- ##
==========================================
- Coverage 54.82% 54.8% -0.03%
==========================================
Files 133 133
Lines 9552 9545 -7
==========================================
- Hits 5237 5231 -6
+ Misses 3994 3993 -1
Partials 321 321 |
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.
Hmm, as far as I can see this doesn't address the key point in #2513, which is dealing with the case when power (as a big.Int
) exceeds int64
bounds. Is it intended to?
Remaining task before merge:
|
All the tests seem to fail, is there a known issue or an unknown issue? |
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.
Left two minor comments, but otherwise the changes LGTM.
@cwgoes I made a small oopsie - all fixed |
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.
LGTM ☕️
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.
Tested ACK - although @rigelrozanski did you get to make the docs updates you wanted to?
Also needs a rebase since the changes for the gaia-9002 fix PR. |
@rigelrozanski If you have a chance to rebase this I think we can merge it - pending my question about docs updates. |
K yeah I will rebase soon probably this weekend - may add a doc section describing the nuances involved with distribution shares - however no trimmings are actually burned anymore, just shifted to the last delegator within a validator. |
@rigelrozanski any updates here? Looks like this is ready to be merged pending a rebase? |
ready to be merged! so long as no CI fails unexpectedly here |
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.
IMO we need better justification for the defensive mins / maxes - I'd rather figure out what specific sort of rounding or truncation is leading to their requirement, then fix that.
Ref #2513
Closes #2327
docs/
)PENDING.md
with issue #Files changed
in the github PR explorerFor Admin Use: