implement process_withdrawals #11634
Conversation
| } | ||
| for i, withdrawal := range withdrawals { | ||
| if withdrawal.WithdrawalIndex != expected[i].WithdrawalIndex { | ||
| return nil, errors.New("invalid withdrawal index") |
There was a problem hiding this comment.
dont feel strongly about this, but perhaps we can turn these errors into variables. It might be useful later...
var ErrInvalidWithdrawalIndex = errors.New("invalid withdrawal index")
| if withdrawal.ValidatorIndex != expected[i].ValidatorIndex { | ||
| return nil, errors.New("invalid validator index") | ||
| } | ||
| if bytesutil.ToBytes20(withdrawal.ExecutionAddress) != bytesutil.ToBytes20(expected[i].ExecutionAddress) { |
There was a problem hiding this comment.
would it be better to use the isHexAddress function or any of the common functions from geth for address / also wondering if there's any benefit to doing !bytes.compare instead
beacon-chain/state/interfaces.go
Outdated
| @@ -65,6 +65,7 @@ type ReadOnlyBeaconState interface { | |||
| LatestExecutionPayloadHeader() (interfaces.ExecutionData, error) | |||
| LastWithdrawalValidatorIndex() (types.ValidatorIndex, error) | |||
| ExpectedWithdrawals() ([]*enginev1.Withdrawal, error) | |||
| NextWithdrawalIndex() (uint64, error) | |||
There was a problem hiding this comment.
That's the third getter related to withdrawals. How about we extract them to a type ReadOnlyWithdrawals interface?
| } | ||
| if len(withdrawals) > 0 { | ||
| if err := st.SetNextWithdrawalIndex(withdrawals[len(withdrawals)-1].WithdrawalIndex + 1); err != nil { | ||
| return nil, errors.Wrap(err, "could not set withdrawal index") |
There was a problem hiding this comment.
| return nil, errors.Wrap(err, "could not set withdrawal index") | |
| return nil, errors.Wrap(err, "could not set next withdrawal index") |
| }, | ||
| { | ||
| Args: args{ | ||
| Name: "success more than max fully withdrawals", |
There was a problem hiding this comment.
We don't have more than max withdrawals here
There was a problem hiding this comment.
we do, the fully withdrawable indices are more than 16
| }, | ||
| { | ||
| Args: args{ | ||
| Name: "success more than max partially withdrawals", |
There was a problem hiding this comment.
We don't have more than max withdrawals here
| }, | ||
| { | ||
| Args: args{ | ||
| Name: "failure invalid withdrawal amount", |
There was a problem hiding this comment.
This test is the same as "failure invalid validator index"
There was a problem hiding this comment.
no it's not, the third withdrawal is a partial one
| }, | ||
| { | ||
| Args: args{ | ||
| Name: "failure validator not withdrawable", |
There was a problem hiding this comment.
| Name: "failure validator not withdrawable", | |
| Name: "failure validator not partially withdrawable", |
Implement process withdrawal logic, as part of block operations.