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

Implement EIP-1186 eth_getProof #1563

Open
wants to merge 13 commits into
base: nightly
Choose a base branch
from
Open

Implement EIP-1186 eth_getProof #1563

wants to merge 13 commits into from

Conversation

kpp
Copy link
Contributor

@kpp kpp commented Dec 5, 2024

@eyusufatik
Copy link
Member

can you base this on v0.5.5 release branch?

we can merge it to nightly later on.

@eyusufatik
Copy link
Member

note:

in ethereum accounts have storage root

so root -> account -> storage

you have 2 proofs that are actually continiuations on the state tree.

we use a different tree and accounts and storages live on different parts of this tree

that's why account storage root is not a thing in Citrea.

so we can return two different proofs, one for the AccountInfo and one for the storage queried. We'll need to explain in our docs how to verify proof from eth_getProof, but it's fine.

so leave storage root empty and put two proofs, one for account being read, one for the storage queried.

@orkunkilic
Copy link
Member

so we can return two different proofs, one for the AccountInfo and one for the storage queried. We'll need to explain in our docs how to verify proof from eth_getProof, but it's fine.

then I highly suggest renaming this endpoint into something else. Most people will query it and their program/library will panic.

@kpp
Copy link
Contributor Author

kpp commented Dec 6, 2024

can you base this on v0.5.5 release branch?

Yes, but I wanted to ported it into 0.5.5 later

@eyusufatik
Copy link
Member

so we can return two different proofs, one for the AccountInfo and one for the storage queried. We'll need to explain in our docs how to verify proof from eth_getProof, but it's fine.

then I highly suggest renaming this endpoint into something else. Most people will query it and their program/library will panic.

The response will have the same output format. Do you still think it’s a problem? We can have a verification code snippet in our docs for js

@orkunkilic
Copy link
Member

so we can return two different proofs, one for the AccountInfo and one for the storage queried. We'll need to explain in our docs how to verify proof from eth_getProof, but it's fine.

then I highly suggest renaming this endpoint into something else. Most people will query it and their program/library will panic.

The response will have the same output format. Do you still think it’s a problem? We can have a verification code snippet in our docs for js

If the output format doesn't blow up the caller client, then its ok. verification algorithm is not an issue as long as its documented (or even added as an endpoint?)

storage_proof.push(EIP1186StorageProof {
key: JsonStorageKey(key.into()),
value: value.unwrap_or_default(),
proof: vec![value_proof],
Copy link
Member

Choose a reason for hiding this comment

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

this feels weird? why a vec of single length?

I thought the storage proof was a vec of merkle nodes that should be hashed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because let proof: sov_state::storage::StorageProof<<<C as Spec>::Storage as Storage>::Proof> = working_set.get_with_proof(storage_key); - the real type is generic. I would need to hack into trait Proof to properly serialize it with rlp.

Do we need it?

Copy link
Member

Choose a reason for hiding this comment

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

we can force the rpc implementation to have NativeStorage = ProverStorage

so we get a concrete type for the Proof

then we can apply eth-like serialization to the proof

@eyusufatik
Copy link
Member

PR looks ok.

some testing we need:

  • regular old e2e tests with Rust verification of the proof
  • web3py test (bin/citrea/tests/evm/ethers_js)
  • ethersjs test (bin/citrea/tests/evm/web3_py)

@kpp kpp changed the title [WIP] Init eth_getProof Implement EIP-1186 eth_getProof Dec 16, 2024
@kpp kpp marked this pull request as ready for review December 16, 2024 23:00
@auto-assign auto-assign bot requested a review from jfldde December 16, 2024 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement eth_getProof
3 participants