-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(protocol): document skipping withdrawalsRoot check in circuits and client #13796
Conversation
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 code changes in this Pull Request look good overall. The only potential issue is that the withdrawalsRoot
is set to a constant 0x1
instead of the kecceck hash of the L1-to-L2 Ether deposits. It might be better to use the actual hash value here.
Additionally, there are a few typos in repeating variables such as processed_deposits_data
and processed_deposits
. It would be better to use consistent variable names throughout the code.
Overall, this looks like a good Pull Request and I recommend merging it.
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.
Feedback:
- The first patch changes the text from “The kecceck hash of the L1-to-L2 Ether deposits” to “0x1”. This should be clarified as it is not clear why this change is being made.
- The second patch adds a constant variable for “1” and “0”. This should be clarified as it is not clear why these variables are being added.
- There are typos in the repeating variables “h_withdrawals_root” and “processed_deposits_data” in the first patch and “zero” and “one” in the second patch. They should be corrected to ensure consistency.
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.
Overall, this pull request looks good. Here are some points to consider:
- Ensure that the
withdrawalsRoot
field is set to the kecceck hash of the L1-to-L2 Ether deposits instead of0x1
. - Check that all repeating variables have the same name (e.g.
processed_deposits
andprocessed_deposits_data
). - Make sure that all constants are defined in the same place (e.g.
zero
,one
,empty_string
,empty_list
).
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.
Overall, the code changes in this pull request appear to be well-structured and consistent. However, there are a few potential issues that need to be addressed.
- There is a typo in the variable
h_withdrawals_root
on line 168, where it should be0x1
instead of1
. - On line 210, the variable
m_deposits_root
is not defined anywhere in the patch. - On line 233, the variable
tx_list
is repeated twice. - On line 349, the constant
one("1\n(one"):::constant
should have an extra closing parentheses. - The constant
processed_deposits("onchain deposits data"):::constant
on line 355 is repeated twice. - The constant
empty_string("' '\n(empty bytes)"):::constant
on line 360 should have a single quote instead of double quotes.
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.
Overall, this pull request looks like it makes some useful changes to the document. Here are some comments and suggestions:
- The commit messages could be more descriptive.
- It appears that the variable
l2_treasury
is used multiple times in the code, but is only declared once. Consider declaring it in each patch to ensure consistency. - In Patch 2, the constant
one
is declared twice - consider removing one of these declarations. - In Patch 5, consider adding a comment to explain why
l2_treasury
is being declared again.
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.
Overall, this pull request looks good. Here are some of my comments:
- The commit messages could be more descriptive and provide more context.
- There are a few typos in the repeating variables such as 'kececk' instead of 'keccak'.
- It is not clear why one is used as the withdrawalsRoot in the Global Variable section.
- The empty_list and empty_string constants could be better named to provide more context.
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
e4e0dd4
to
8bf62c7
Compare
h_difficulty(difficulty) | ||
h_extra_data(extraData) | ||
h_nonce(nonce) | ||
h_logs_bloom("logsBloom = []") |
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.
not sure the motivation of make header.logsBloom
to equal to []
?
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.
Is it correct to say: "logsBloom
must be a bytes32[8]
with all zeros"?
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.
It seems like the common representation is bytes[256]
where I look, but yes, 256 0
bytes.
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
See #13795