Skip to content
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

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Sep 4, 2024

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.

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.
@fjl fjl requested a review from eth-bot as a code owner September 4, 2024 11:05
@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels Sep 4, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Sep 4, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added the a-review Waiting on author to review label Sep 4, 2024
@eth-bot eth-bot changed the title EIP-6110: change request to flat encoding Update EIP-6110: change request to flat encoding Sep 4, 2024
@lightclient
Copy link
Member

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
Copy link
Contributor

@mkalinin mkalinin Sep 4, 2024

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

Copy link
Member

@lightclient lightclient Sep 4, 2024

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.

Copy link
Contributor Author

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.

@mkalinin
Copy link
Contributor

mkalinin commented Sep 4, 2024

we can avoid defining the structure of these requests in the execution layer client implementation.

I believe the structure would still need to be required to serve Engine API requests, wouldn’t it?

@fjl
Copy link
Contributor Author

fjl commented Sep 4, 2024

The corresponding engine API update is here: ethereum/execution-apis#577

@Magken91
Copy link

CNN someone tell how do this so I can use funds to pay things

@lightclient
Copy link
Member

I believe the structure would still need to be required to serve Engine API requests, wouldn’t it?

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:
Copy link
Contributor

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

@eth-bot eth-bot enabled auto-merge (squash) October 1, 2024 09:42
Copy link
Collaborator

@eth-bot eth-bot left a 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...

@eth-bot eth-bot merged commit b48903e into ethereum:master Oct 1, 2024
12 checks passed
return DEPOSIT_REQUEST_TYPE + rlp([
pubkey, withdrawal_credentials, amount, signature, index
])
return DEPOSIT_REQUEST_TYPE + pubkey + withdrawal_credentials + amount + signature + index
Copy link
Contributor

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.

Copy link
Contributor

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-update Modifies an existing proposal s-review This EIP is in Review t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants