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

Add eth proof generation utilities #928

Merged
merged 7 commits into from
Mar 28, 2024
Merged

Add eth proof generation utilities #928

merged 7 commits into from
Mar 28, 2024

Conversation

kiseln
Copy link
Contributor

@kiseln kiseln commented Mar 15, 2024

Replacement for https://github.com/aurora-is-near/rainbow-bridge/blob/master/eth2near/eth2near-block-relay/eth-proof-extractor.js#L86 written in Rust.

Javascript version is used here https://github.com/aurora-is-near/fast-bridge-service/blob/8ed92f1b9c051d49083726803d112aeaa5e51d52/src/ethereum/proof.rs#L40

Supporting older blocks
I tested this on post merge and post-4844 blocks. Older blocks may not work (for example because of different handling of difficulty field). If needed I'll test and add support for pre-merge.

Tests
I intended to mock RPC requests and add a few tests for the client as well ass more general test cases for proof generation. Would that be the correct approach?

Error handling
As one can see it's Box almost everywhere. I can put some work towards friendlier error types.

Types
I chose not to expose the types I added publicly. They are quite specifically designed for deserialization and support only required fields. In the future they can be improved and exposed to other components if that is deemed useful

eth2near/utilities/src/eth_proof_generator.rs Outdated Show resolved Hide resolved
eth2near/utilities/src/eth_proof_generator.rs Outdated Show resolved Hide resolved
eth2near/utilities/src/eth_proof_generator.rs Outdated Show resolved Hide resolved
eth2near/utilities/src/eth_proof_generator.rs Outdated Show resolved Hide resolved
eth2near/utilities/src/eth_proof_generator.rs Outdated Show resolved Hide resolved
eth2near/utilities/src/eth_proof_generator.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@olga24912 olga24912 left a comment

Choose a reason for hiding this comment

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

Could you please also provide more details in PR description: where this class will be used (why we need this) and the link to JS version

Also, I think the utilities better rename into eth_proof_extractor or something like this.

eth2near/utilities/src/eth_proof_generator.rs Outdated Show resolved Hide resolved
eth2near/utilities/src/eth_rpc_client.rs Show resolved Hide resolved
eth2near/utilities/src/eth_proof_generator.rs Show resolved Hide resolved
eth2near/utilities/src/eth_proof_generator.rs Outdated Show resolved Hide resolved
@kiseln
Copy link
Contributor Author

kiseln commented Mar 19, 2024

Also, I think the utilities better rename into eth_proof_extractor or something like this.

Depends whether proof generation is the only thing we are going to expose from this crate. If we, say, also want to expose the new ETH client then 'utilities' would work better

@kiseln kiseln requested a review from karim-en March 20, 2024 13:24
Copy link
Contributor

@olga24912 olga24912 left a comment

Choose a reason for hiding this comment

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

Thank you for your work! I like the code condition. I haven't tested it yet, but I like how it looks. Thank you for specifying in the PR description the origin code and the place of usage, it makes the reviewing process much easier.

I continue to think, that it is better to change the name of utilities for something more specific.

It would help if you prepared the PR for https://github.com/aurora-is-near/fast-bridge-service to start using this code instead of JS one.

eth2near/utilities/src/eth_proof_generator.rs Show resolved Hide resolved
eth2near/utilities/src/eth_proof_generator.rs Outdated Show resolved Hide resolved
@karim-en karim-en merged commit b54f0c9 into master Mar 28, 2024
23 of 27 checks passed
@karim-en karim-en deleted the eth-proof-generation branch March 28, 2024 21:46
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.

4 participants