-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Update EIP-6110: Spec parse_deposit_data
#9460
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
Changes from 1 commit
232872a
8e67988
9b687a3
4f912ac
3c96c46
9bef0b9
eb150ed
01e54fc
194f66e
43cf243
b1e69fd
31641e8
6b0eb53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,11 +76,51 @@ Beginning with the `FORK_BLOCK`, each deposit accumulated in the block **MUST** | |
in the order they appear in the logs. To illustrate: | ||
|
||
```python | ||
def parse_deposit_data(deposit_event_data) -> bytes[]: | ||
""" | ||
Parses deposit data from DepositContract.DepositEvent data | ||
""" | ||
pass | ||
def parse_deposit_data(deposit_event_data): | ||
""" | ||
Parses deposit data from DepositContract.DepositEvent data. | ||
""" | ||
|
||
# TODO: verify byte length of deposit_event_data? Should be 576. Should this be part of the spec? (Yes?) | ||
|
||
""" | ||
The deposit event is encoded as: | ||
event DepositEvent( | ||
bytes pubkey, | ||
bytes withdrawal_credentials, | ||
bytes amount, | ||
bytes signature, | ||
bytes index | ||
); | ||
""" | ||
|
||
# Start offset of data reading skips over the first 5 32-byte values | ||
# which represents the dynamic size of the encoded bytes event data | ||
# The size itself is encoded in a 32-byte, so skip over this as well | ||
# to arrive at the first actual data offset to be read (pubkey) | ||
currentOffset = 32 * 5 + 32 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have PUBKEY_OFFSET, WITHDRAWAL_CREDENTIALS_OFFSET, etc instead of these computations? and then: pubkey = deposit_event_data[PUBKEY_OFFSET:PUBKEY_OFFSET + 48]
withdrawal_credentials = deposit_event_data[WITHDRAWAL_CREDENTIALS_OFFSET: WITHDRAWAL_CREDENTIALS_OFFSET + 32]
... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have added! Please verify if my offsets and sizes are correct 😄 👍 |
||
|
||
# Read the pubkey data | ||
pubkey = deposit_event_data[currentOffset:currentOffset + 48] | ||
# The ABI data is encoded in 32-byte chunks, so the 64 bytes are reserved for the data | ||
# The next 32 bytes are reserved for the size of the next item (withdrawals) so skip over this as well | ||
currentOffset += 48 + 16 + 32 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using an ABI isn't particularly important; what is important is the data fields are length prefixed byte arrays. The fixed offsets are a quirk of the current contracts. Properly parsing (using length prefix) means it would still work if a contract on a different chain output the same data but trimmed leading zeros; however using fixed offsets would break (if the argument is not to use ABI because "it might change"; changing would first break the offsets) (edited) What if the deposit contract was written in viper or some other language that decided to encode packed? Is valid by the event log specification, would fail to be parsed. It's also both literally encoding ABI and also ignoring it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, good point regarding the ABI, I should not have added this as comment at all. Have removed :) The goal this PR wants to solve is that
Note that the current EIP spec has this in it:
This is how the EIP-7685 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have updated the OP of this PR also regarding this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this EIP is encoding that anything decoding using the specified ABI should fail if junky data was passed but the offset would "pass" which seems sloppy? Since the data would be wrong for anyone parsing the event upstream (and would the data even be correct in that circumstance?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not entirely sure if I got this comment right but I think this applies here as well: #9460 (comment) |
||
|
||
withdrawal_credentials = deposit_event_data[currentOffset:currentOffset + 32] | ||
# This data fits in one 32-byte chunk. Also skip over the size 32-bytes of the next item (amount) | ||
currentOffset += 32 + 32 | ||
|
||
amount = deposit_event_data[currentOffset:currentOffset + 8] | ||
# This data fits in a 32 byte chunk. Also skip over the size 32-bytes of the next item (signature) | ||
currentOffset += 8 + 24 + 32 | ||
|
||
signature = deposit_event_data[currentOffset:currentOffset + 96] | ||
# This data fits in three 32 byte chunks. Also skip over the size 32-bytes of the next item (index) | ||
currentOffset += 96 + 32 | ||
|
||
index = deposit_event_data[currentOffset:currentOffset + 8] | ||
|
||
return [pubkey, withdrawal_credentials, amount, signature, index] | ||
|
||
def event_data_to_deposit_request(deposit_event_data) -> bytes: | ||
deposit_data = parse_deposit_data(deposit_event_data) | ||
|
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.
What is the failure mode for this check? Maybe we should do it here?
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 have added this also! 4f912ac 😄 👍