Refactor get_expected_withdrawals
#4766
Conversation
|
I think this type of change that improves the readability of the specs are very good. |
Splitting the code in more functions make them more self-explanatory |
|
LGTM! |
| def is_eligible_for_partial_withdrawals(validator: Validator, balance: Gwei) -> bool: | ||
| """ | ||
| Check if ``validator`` can process a pending partial withdrawal. | ||
| """ | ||
| has_sufficient_effective_balance = validator.effective_balance >= MIN_ACTIVATION_BALANCE | ||
| has_excess_balance = balance > MIN_ACTIVATION_BALANCE | ||
| return ( | ||
| validator.exit_epoch == FAR_FUTURE_EPOCH | ||
| and has_sufficient_effective_balance | ||
| and has_excess_balance | ||
| ) |
There was a problem hiding this comment.
In a follow up PR, I would like to rename:
is_partially_withdrawable_validator->has_withdrawable_excess_balanceis_eligible_for_partial_withdrawals->is_partially_withdrawable_validator
The "partially withdrawable" function is very misleading now.
|
@jihoonsong Thanks for the review! All good suggestions, I'll fix these on Monday 👍 |
specs/capella/beacon-chain.md
Outdated
| epoch = get_current_epoch(state) | ||
| withdrawal_index = state.next_withdrawal_index | ||
| validator_index = state.next_withdrawal_validator_index | ||
| def get_balance_minus_withdrawals( |
There was a problem hiding this comment.
This function name sounds odd. It only explains the calculation part but doesn't explain what this returned value is for. Maybe get_withdrawable_balance() or get_available_balance_to_withdraw()?
There was a problem hiding this comment.
Hey @ensi321. Yeah I agree the name is odd. I'm not sure that your two suggestions are accurate though. They don't return the "withdrawable" or "available" amount; given a validator with 32.50 ETH and 0.30 ETH of withdrawals, I would expect that to return 0.20 ETH. Instead, they return the validator's balance after the given withdrawals; 32.20 ETH. Maybe get_balance_after_withdrawals() would be more clear? I'm open to other suggestions too.
There was a problem hiding this comment.
Another option is get_remaining_balance(), though it feels a little ambiguous.
There was a problem hiding this comment.
Hmm.. isn't it a balance at the point after previous withdrawals and before applying further withdrawals? Then, it can viewed as a temporal withdrawable balance. Maybe get_current_withdrawable_balance() is more straightforward at the cost of a bit verbosity? What do others think?
There was a problem hiding this comment.
@jihoonsong get_current_withdrawable_balance still has the same issue 😢
I think calling it "withdrawable" is misleading.
There was a problem hiding this comment.
Ah, not the whole amount is withdrawable.
There was a problem hiding this comment.
Yes. I've renamed the function to get_balance_after_withdrawals. I think this is marginally better than the original. Still open to other suggestions.
jihoonsong
left a comment
jihoonsong
left a comment
There was a problem hiding this comment.
The refactor of processed sweep count looks neat. It will affect on #4765's update_next_withdrawal_indices so we probably want to merge this PR first.
I've left some comments but please excuse that I wore the bikeshedder's hat 😅
jtraglia
force-pushed
the
refactor-get-expected-withdrawals
branch
from
1b8a0d4 to
92c34c7
Compare
|
Rebased to pull in changes from master (need to resolve some conflicts). @jihoonsong I've fixed 3/4 of your comments. How does it look now? (Also, I'm a fan of nit picky comments, keep em coming!) |
jihoonsong
left a comment
jihoonsong
left a comment
There was a problem hiding this comment.
Looks good! Looking forward to #4765 and builder sweep one. Great work!
In #4766, we added `processed_validators_sweep_count` to the return but forgot to update the return type.
7 participants
The
get_expected_withdrawalsis a long, complex function.This PR splits
get_expected_withdrawalsinto smaller functions:get_sweep_withdrawalsget_pending_partial_withdrawalsget_builder_withdrawalsAssociated with:
process_withdrawals#4765Note: I decided to include
prior_withdrawalsin all functions so that we don't need create a modified function which makes such a simple change. I didn't include many comments; we can look into making that better. Open to suggestions on other simplifications.