Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Generalized Proofs #346
Generalized Proofs #346
Changes from 9 commits
6b172db
ca2c983
b56976c
38dafc6
0e3f0f1
ceb5cb1
d290f04
59a328d
a6ac41a
7613bea
79d6950
258d95f
2096a24
87f6de2
a907f7e
7f45cac
e25381c
4ce6725
89db042
64f6858
673505f
eb718a2
fc2c45e
ab40644
fd6838a
7fc43a5
2d5216e
51721c0
b281a62
183dd32
1343c4d
24c4d8a
5407420
0de6c2c
2405226
cb9e679
75923d7
2e4b640
e869b1e
336e245
914ada4
f91a038
8ea334d
dcf8a04
340a819
83c5347
4e1663c
fd26c96
eb5c18e
e5054e1
53a0c4c
0720245
39ea24d
a14f62b
f7d2b8e
c533844
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 happens if a user proved a bunch of partial withdrawals and then did a zk proof on unproven withdrawals? They just don't see the funds?
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 I follow the question - if the try to "double prove" via merkle proofs then zk proofs, they would get nothing the second time aorund
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.
No i mean picture a person who does merkle proofs for some partial withdrawal. This necessarily increases their
sumOfPartialWithdrawalsClaimedGwei
Then we enable the proof service and they prove zk withdrawals for different, totally unrelated partial withdrawals. You're subtracting from the amount they get, no?
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.
oh I see, we're always gonna have "startTimestamp == mostRecentWithdrawalTimestamp" when we submit the request to bonsai. So if there is any claimed partial withdrawals, the starting timestamp provided is such that it counts that
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'd prefer we do an explicit
require
-type check, and put it where this method is called - rather than a modifier buried this deep in the stack traceThere 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.
(in
_verifyAndProcessWithdrawal
)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 there a reason for this? Just readability or is it something else?
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.
Yes, readability and style. We don't typically have modifiers on internal functions, and especially when one is so deep in the function call stack like this, it should really just be an explicit require check instead, because modifiers are easier to overlook. People are used to seeing them on external functions, not in places like this.
The reason I'd prefer it to be in
_verifyAndProcessWithdrawal
is so that it's more clear when reviewers are looking at that function that the entire branch can be turned off when the proof service is enabled. It's easier to grok than having to remember that there's a check buried in the function itself.