Add new get_min_activation_balance helper function#5412
Conversation
jihoonsong
left a comment
jihoonsong
left a comment
There was a problem hiding this comment.
Good change. It seems there are some missing changes and I've had my agent to make some suggestion. Please take a look to see if these make sense.
| max_effective_balance = spec.MAX_EFFECTIVE_BALANCE_ELECTRA | ||
| else: | ||
| max_effective_balance = spec.MAX_EFFECTIVE_BALANCE | ||
| max_effective_balance = get_min_activation_balance(spec) |
There was a problem hiding this comment.
I think the previous code delivers the intention better, which is setting the max effective balance.
| @@ -114,7 +112,9 @@ def test_initialize_beacon_state_some_small_balances(spec): | |||
| # only main deposits participate to the active balance | |||
| # NOTE: they are pre-ELECTRA deposits with BLS_WITHDRAWAL_PREFIX, | |||
| # so `MAX_EFFECTIVE_BALANCE` is used | |||
There was a problem hiding this comment.
Just a nit, a comment here became stale.
| signed=True, | ||
| ) | ||
|
|
||
| # Submit last pubkey deposit as MAX_EFFECTIVE_BALANCE - MIN_DEPOSIT_AMOUNT |
There was a problem hiding this comment.
This one is stale, too. Another option is not changing these tests as they're run in Phase0 only. But I'd vote for just updating comments.
There was a problem hiding this comment.
Hmm yes, I shouldn't have touched anything that only executed in phase0. Let me look more into this.
|
Thank you for the review! I will look into fixing these soon. Should have something ready by Monday. |
2 participants
Technically,
MAX_EFFECTIVE_BALANCEshould not be used post-Electra. But there are many instances where it is used, so I thought I'd make a new helper function which replaces them. That way, we can markMAX_EFFECTIVE_BALANCEas removed in Electra. This will be handled in a different PR.