-
Notifications
You must be signed in to change notification settings - Fork 638
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
Put validator balances into a separate list in the state #1677
Conversation
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.
Looks good to me. Only a minor comment.
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.
Some post-merge cleanup that could be done.
@@ -350,11 +350,12 @@ def get_attestation_participants(state: 'BeaconState', | |||
# | |||
# Misc | |||
# | |||
def get_effective_balance(validator: 'ValidatorRecord', max_deposit: int) -> int: | |||
def get_effective_balance(validator_balances: Sequence[int], index: int, max_deposit: int) -> int: |
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.
It might be nice to change the variable name here to max_deposit_gwei
or max_deposit_in_gwei
so that it is explicit what the units are.
This fits into an architecture category that we've run into multiple times with addresses, hex/byte strings, integer/bytes representations of stack items. In each of these cases we constantly had trouble until we established a canonical representation and ensured that there were very explicit and clear boundaries where things were converted.
In this case, we have a value which is expressed in gwei
but which it appears we need to run computations on it in wei
(or maybe the ther way around). I think the current approach is going to prove problematic as it is very likely that at some point someone will chain together a few helper APIs and units are going to be mixed. I'd suggest converting all of these APIs to either operate on wei
or gwei
and establish a firm boundary where the conversions happen. Anything else is very likely to result in accidental bugs.
balance=5566, | ||
], | ||
validator_balances=[ | ||
100 |
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.
FYI, slightly more concise way to do this:
tuple(itertools.repeat(100, validator_registry_len))
(100,) * validator_registry_len
] | ||
], | ||
validator_balances=( | ||
max_deposit |
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.
Same: (max_deposit,) * 10
What was wrong?
Fixes #1647
How was it fixed?
Put
ValidatorRecord.balance: int
toBeaconState.validator_balances: Sequence[int]
.Cute Animal Picture