Skip to content

Conversation

@lemastero
Copy link
Contributor

@lemastero lemastero commented Dec 21, 2020

Description

Add eth_getProof JSON-RPC method. EIP-1186

Proposed Solution

Uses getProof implemented in #857.

Testing

@lemastero lemastero changed the title [ETCM-468] add JSON-RPC method eth-getProof WIP [ETCM-468] add JSON-RPC method eth-getProof Dec 22, 2020
Copy link
Contributor

@dmitry-worker dmitry-worker left a comment

Choose a reason for hiding this comment

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

PR's looking good!
But if I would upgrade something - maybe I'd upgrade the testing

  • add at least one failed scenario - to be sure that error matches an expected one (3 error cases were defined)
  • maybe test the route itself, mocking the logic - in order to validate json it gives

@dzajkowski
Copy link
Contributor

Would it make sense to have the happy path merged into dev and work on the rest in a separate PR?

@bsuieric bsuieric self-requested a review January 13, 2021 12:57
@lemastero lemastero changed the title WIP [ETCM-468] add JSON-RPC method eth-getProof [ETCM-468] add JSON-RPC method eth-getProof Jan 13, 2021
@lemastero lemastero marked this pull request as ready for review January 13, 2021 16:40
@lemastero lemastero requested a review from dzajkowski January 13, 2021 16:52
@lemastero
Copy link
Contributor Author

lemastero commented Jan 13, 2021

@dmitry-worker

maybe test the route itself, mocking the logic - in order to validate json it gives

I have added sth like that in: dfd4ec3

add at least one failed scenario - to be sure that error matches an expected one (3 error cases were defined)

Each we could use more tests. Let's add it as separate PR. WDYT?

@dzajkowski
Copy link
Contributor

@AnastasiiaL @bsuieric would it be possible to bring this pr to a mergeable state and get it into dev? it's already large, further work can happen in separate PRs. WDYT?

@AnastasiiaL
Copy link
Contributor

@AnastasiiaL @bsuieric would it be possible to bring this pr to a mergeable state and get it into dev? it's already large, further work can happen in separate PRs. WDYT?

the PR is ready to be merged, waiting for approval

@bsuieric
Copy link
Contributor

How can I approve the PR?

@bsuieric
Copy link
Contributor

done

@lemastero lemastero requested a review from dzajkowski January 19, 2021 12:35
Piotr Paradziński and others added 22 commits January 20, 2021 00:08
Signed-off-by: Piotr Paradziński <[email protected]>
Signed-off-by: Piotr Paradziński <[email protected]>
Signed-off-by: Piotr Paradziński <[email protected]>
…orage proof (real tests for proof are on MPT)

Signed-off-by: Piotr Paradziński <[email protected]>
Signed-off-by: Piotr Paradziński <[email protected]>
… request is handled by refactor

Signed-off-by: Piotr Paradziński <[email protected]>
Signed-off-by: Piotr Paradziński <[email protected]>
Signed-off-by: Piotr Paradziński <[email protected]>
@lemastero lemastero force-pushed the feature/ETCM-468-eth-getProof branch from bb80baa to 0b66389 Compare January 19, 2021 23:39
Piotr Paradziński added 2 commits January 20, 2021 01:02
@lemastero lemastero merged commit 99491c7 into develop Jan 20, 2021
biandratti pushed a commit that referenced this pull request Jan 20, 2021
[ETCM-468] add eth_getProof JSON-RPC endpoint (without non-membership proofs)
@drospa drospa linked an issue Feb 10, 2021 that may be closed by this pull request
@AnastasiiaL AnastasiiaL deleted the feature/ETCM-468-eth-getProof branch March 29, 2021 13:31
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 (non-) membership proof for an MPT element with a given key

7 participants