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.

Sign up for GitHub

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

Jump to bottom

fix: override eth_getProof to enable withdrawals #97

Merged
merged 12 commits into from
Nov 26, 2024
Merged

fix: override eth_getProof to enable withdrawals #97

merged 12 commits into from
Nov 26, 2024

Conversation

joshieDo
Copy link
Contributor

@joshieDo joshieDo commented Nov 26, 2024 •
edited by onbjerg
Loading

closes #84 and closes #72

Overrides eth_getProof as to enable withdrawals

//! - `eth_getProof` will _ONLY_ return the storage proofs _WITHOUT_ an account proof _IF_ targeting
//!   the withdrawal contract. Otherwise, it fallbacks to default behaviour.

@joshieDo joshieDo added C-bug Something isn't working A-node labels Nov 26, 2024
@joshieDo joshieDo requested a review from rkrasiuk November 26, 2024 13:09
@joshieDo joshieDo changed the title feat: override eth_getProof to enable withdrawals fix: override eth_getProof to enable withdrawals Nov 26, 2024
mattsse
mattsse reviewed Nov 26, 2024
View reviewed changes
crates/node/src/rpc.rs

// If we are targeting the withdrawal contract, then we only need to provide the storage
// proofs for withdrawal.
if address == WITHDRAWAL_CONTRACT {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this something we want to bake into op-reth as well?

onbjerg
onbjerg reviewed Nov 26, 2024
View reviewed changes
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also like an e2e test, one for proving the withdrawal contract, one for proving something that isn't the withdrawal contract (to ensure the fallback works through refactors e.g.)

can take a look at our current e2e tests

crates/node/src/rpc.rs Outdated Show resolved Hide resolved
@joshieDo joshieDo marked this pull request as draft November 26, 2024 15:07
@joshieDo joshieDo marked this pull request as ready for review November 26, 2024 15:18
@joshieDo joshieDo mentioned this pull request Nov 26, 2024
Merged
onbjerg
onbjerg approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, great work bud

@onbjerg onbjerg merged commit b35e5a9 into main Nov 26, 2024
14 checks passed
@onbjerg onbjerg deleted the joshie/override branch November 26, 2024 18:04
onbjerg pushed a commit that referenced this pull request Nov 26, 2024
@joshieDo
chore: bump reth (#98)
4df8a14 
introduces
[StorageRootProvider::storage_multiproof](paradigmxyz/reth#12879)
that can be used on #97
@joshieDo joshieDo mentioned this pull request Nov 29, 2024
Merged
@joshieDo joshieDo mentioned this pull request Nov 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse left review comments

@onbjerg onbjerg onbjerg approved these changes

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

Assignees
No one assigned
Labels
A-node C-bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Swap on bridge issue Withdrawal proof cache
3 participants
@joshieDo @onbjerg @mattsse