-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Update EIP-6110: change request to flat encoding #8856
Conversation
I am proposing to change the encoding of deposit requests to be a flat concatenation of the fields returned by the contract. The extra layer of RLP encoding is not necessary, and by removing it, we can avoid defining the structure of these requests in the execution layer client implementation.
✅ All reviewers have approved. |
Relies on #8854 |
EIPS/eip-6110.md
Outdated
deposit_data = parse_deposit_data(deposit_event_data) | ||
pubkey = Bytes48(deposit_data[0]) | ||
withdrawal_credentials = Bytes32(deposit_data[1]) | ||
amount = little_endian_to_uint64(deposit_data[2]) | ||
amount = deposit_data[2] # 4 bytes uint64 LE |
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.
Note that not making the endianness conversion here defers this responsibility to the Engine API handlers
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.
The idea is that this is already valid SSZ and that will be passed directly to CL.
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.
Yeah, AFAIK the number is already converted to LE by the contract.
I believe the structure would still need to be required to serve Engine API requests, wouldn’t it? |
The corresponding engine API update is here: ethereum/execution-apis#577 |
CNN someone tell how do this so I can use funds to pay things |
I think the idea from Felix's engine api PR is that the data will be "opaque" as far as the EL is concerned. |
@@ -94,17 +90,15 @@ def parse_deposit_data(deposit_event_data) -> bytes[]: | |||
def little_endian_to_uint64(data: bytes) -> uint64: |
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.
I think we can remove this helper now as it is no longer used
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.
All Reviewers Have Approved; Performing Automatic Merge...
return DEPOSIT_REQUEST_TYPE + rlp([ | ||
pubkey, withdrawal_credentials, amount, signature, index | ||
]) | ||
return DEPOSIT_REQUEST_TYPE + pubkey + withdrawal_credentials + amount + signature + index |
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.
Do we need to prepend the type? My understanding is that each element does not have the type, and the type is only used when calculating the commitment hash.
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.
Ah just realised this has changed in other PRs :)
I am proposing to change the encoding of deposit requests to be a flat concatenation of the fields returned by the contract. The extra layer of RLP encoding is not necessary, and by removing it, we can avoid defining the structure of these requests in the execution layer client implementation.