Expand `eth_call` validation to support blockParam object #817

georgi-l95 · 2023-01-17T15:46:39Z

Signed-off-by: georgi-l95 [email protected]

Description:
Expand eth_call validation to support blockParam object, introduced with EIP-1898. The Graph is passing blockHash as parameter and currently validations only accepts blockNumber or tag. More info can be found in this issue from this comment onwards.

Fix some minor intermittent bugs in ERC20 tests.

Bump network images to 0.33.2 and mirror-node images to 0.72.0.

Related issue(s):

Fixes #816

Notes for reviewer:

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

Signed-off-by: georgi-l95 <[email protected]>

codecov-commenter · 2023-01-17T18:00:01Z

Codecov Report

Base: 72.62% // Head: 73.88% // Increases project coverage by +1.26% 🎉

Coverage data is based on head (9fdec83) compared to base (b3b7650).
Patch coverage: 94.11% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #817      +/-   ##
==========================================
+ Coverage   72.62%   73.88%   +1.26%     
==========================================
  Files          29       29              
  Lines        1746     1773      +27     
  Branches      319      322       +3     
==========================================
+ Hits         1268     1310      +42     
+ Misses        387      371      -16     
- Partials       91       92       +1

Impacted Files	Coverage Δ
packages/relay/src/lib/eth.ts	`82.67% <ø> (+0.20%)`	⬆️
packages/server/src/validator/methods.ts	`100.00% <ø> (ø)`
packages/server/src/validator/types.ts	`95.83% <83.33%> (-4.17%)`	⬇️
packages/server/src/validator/constants.ts	`100.00% <100.00%> (ø)`
packages/server/src/validator/objectTypes.ts	`100.00% <100.00%> (ø)`
packages/relay/src/lib/errors/SDKClientError.ts	`66.66% <0.00%> (+2.38%)`	⬆️
packages/relay/src/lib/clients/sdkClient.ts	`20.61% <0.00%> (+14.47%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lukelee-sl · 2023-01-17T20:39:28Z

packages/server/tests/acceptance/rpc.spec.ts

+                expect(res).to.eq(BASIC_CONTRACT_PING_RESULT);
+            });
+
+            it('should fail to execute "eth_call" with correct block hash', async function () {


is this test calling with blockNumber rather than blockHash?
Succeed rather than fail?

lukelee-sl · 2023-01-17T20:41:44Z

packages/server/tests/acceptance/rpc.spec.ts

@@ -1299,6 +1299,101 @@ describe('@api RPC Server Acceptance Tests', function () {
                const res = await relay.call('eth_call', [callData, 'latest'], requestId);
                expect(res).to.eq(BASIC_CONTRACT_PING_RESULT);
            });
+
+            it('should fail to execute "eth_call" with correct block number', async function () {


It looks like this test is supposed to succeed rather than fail?

lukelee-sl · 2023-01-17T20:43:53Z

packages/server/tests/acceptance/rpc.spec.ts

+                expect(res).to.eq(BASIC_CONTRACT_PING_RESULT);
+            });
+
+            it('should fail to execute "eth_call" with correct block hash', async function () {


Succeed rather than fail?

Aw, yes. Copy paste error on my side. Thanks for the feedback. Those are positive cases.

lukelee-sl · 2023-01-17T21:29:57Z

packages/server/src/validator/types.ts

+        }
+        return new Validator.BlockNumberObject(param).validate();
+      }
+      return /^0[xX]([1-9A-Fa-f]+[0-9A-Fa-f]{0,13}|0)$/.test(param) && Number.MAX_SAFE_INTEGER >= Number(param) || ["earliest", "latest", "pending"].includes(param)


The regex seems odd to me. It allows for 12 bytes and the first nibble cannot be 0? I tested 0x1204567890abcdef123456 for example and the regex does not match for me.

Maybe also add some of the test cases noted in the EIP for additional coverage

First nibble cannot be 0, because block numbers are not starting with 0, that would mean to be padded, which we have to trim afterwards. For example infura doesn't accepts padded blocknumbers, but alchemy accepts them. We can change the regex to accepts, but we may have to trim it afterwards. I mean it could work both ways.
It allows for this many bytes only, because otherwise it will exceeds javascript max integer size, this is the reason your example is not working. If you cut it down to 0x1204567890abcde, it should work with this regular expression.

I guess my question is whether the regex should not be /^0[xX]([1-9A-Fa-f][0-9A-Fa-f]{0,13}|0)$/? Unless I am misunderstanding the purpose of what the regex is trying to test. My guess is that it is trying to say the first nibble should not be zero but should be any other hex character and the next up to 13 characters can be any hex character.

Because of the + after [1-9A-Fa-f] I think this allows for any number of hex characters as long as they are not 0 to be part of the first match group. So things like 0x123456789abcedf123456789abcedf123456789abcedf123456789abcedf123456789abcedf123456789abcedf will still match (which I don't think is the intention but correct me if I am wrong). The count of up to 13 characters starts after the first 0is encountered.

I may have "over-insured" in this case. Generally it should be fine to have a longer than 16 character hex for block number, my reason was that at one point all hex values will always convert to JS's MAX_SAFE_INTEGER, but then again it would take quite the long amout of time until we reach that point, so in the end that size limit may be uneeded.

Yes, you're right, I'll change the regex to /^0[xX]([1-9A-Fa-f][0-9A-Fa-f]{0,13}|0)$/, which will work for our needs.

Thanks for double checking.

Thank you for the feedback, Luke.

packages/server/tests/integration/server.spec.ts

Nana-EC · 2023-01-18T00:14:38Z

packages/server/src/validator/types.ts

+        }
+        return new Validator.BlockNumberObject(param).validate();
+      }
+      return /^0[xX]([1-9A-Fa-f]+[0-9A-Fa-f]{0,13}|0)$/.test(param) && Number.MAX_SAFE_INTEGER >= Number(param) || ["earliest", "latest", "pending"].includes(param)


Maybe also add some of the test cases noted in the EIP for additional coverage

Signed-off-by: georgi-l95 <[email protected]>

georgi-l95 · 2023-01-18T09:31:17Z

@Nana-EC Tests case examples in the EIP are about eth_getStorageAt, I've added similar about eth_call, but we are currently not using blockParam parameter, so we can't add all, for now.
According to the EIP we should make the following endpoints accept block parameter ( tag, block number or object with block hash or block number ):

eth_getBalance
eth_getStorageAt
eth_getTransactionCount
eth_getCode
eth_call
eth_getProof

But only eth_call is accepting those in Alchemy, for example. The other endpoints only accept tag or block number (the same as us, currently), both in Alchemy and Infura. Should we follow the EIP fully or only for eth_call ? If yes, maybe in a another PR, as this one will get a lot larger ?

Signed-off-by: georgi-l95 <[email protected]>

Nana-EC · 2023-01-19T01:01:09Z

@Nana-EC Tests case examples in the EIP are about eth_getStorageAt, I've added similar about eth_call, but we are currently not using blockParam parameter, so we can't add all, for now. According to the EIP we should make the following endpoints accept block parameter ( tag, block number or object with block hash or block number ):

eth_getBalance

eth_getStorageAt

eth_getTransactionCount

eth_getCode

eth_call

eth_getProof

But only eth_call is accepting those in Alchemy, for example. The other endpoints only accept tag or block number (the same as us, currently), both in Alchemy and Infura. Should we follow the EIP fully or only for eth_call ? If yes, maybe in a another PR, as this one will get a lot larger ?

@georgi-l95 Let's match Alchemy and Infura for now.
Open a ticket for the backlog to capture this for other endpoints are you've noted.

Signed-off-by: georgi-l95 <[email protected]>

sonarqubecloud · 2023-01-19T08:02:03Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.3% Duplication

lukelee-sl · 2023-01-19T15:52:51Z

LGTM

Expand `eth_call` validation to support `blockParam` object, introduced with [EIP-1898](https://eips.ethereum.org/EIPS/eip-1898). The Graph is passing blockHash as parameter and currently validations only accepts `blockNumber` or `tag`. More info can be found in this issue from this [comment](#626 (comment)) onwards. Fix some minor intermittent bugs in ERC20 tests. Bump network images to `0.33.2` and mirror-node images to `0.72.0`. Signed-off-by: georgi-l95 <[email protected]>

Expand `eth_call` validation to support `blockParam` object, introduced with [EIP-1898](https://eips.ethereum.org/EIPS/eip-1898). The Graph is passing blockHash as parameter and currently validations only accepts `blockNumber` or `tag`. More info can be found in this issue from this [comment](#626 (comment)) onwards. Fix some minor intermittent bugs in ERC20 tests. Bump network images to `0.33.2` and mirror-node images to `0.72.0`. Signed-off-by: georgi-l95 <[email protected]> Signed-off-by: georgi-l95 <[email protected]>

add blockParam to eth_call validation

515b566

Signed-off-by: georgi-l95 <[email protected]>

georgi-l95 linked an issue Jan 17, 2023 that may be closed by this pull request

Indexing eth_call requests from graph-node fail in release 0.15.0 #816

Closed

bump image versions

fa34e09

Signed-off-by: georgi-l95 <[email protected]>

georgi-l95 requested review from Nana-EC, natanasow, Ivo-Yankov and lukelee-sl January 17, 2023 16:38

georgi-l95 self-assigned this Jan 17, 2023

georgi-l95 marked this pull request as ready for review January 17, 2023 16:38

revert previous change

f3f8c32

Signed-off-by: georgi-l95 <[email protected]>

lukelee-sl reviewed Jan 17, 2023

View reviewed changes

Nana-EC added bug Something isn't working limechain P1 labels Jan 17, 2023

Nana-EC added this to the 0.17.0 milestone Jan 17, 2023

Nana-EC requested changes Jan 18, 2023

View reviewed changes

georgi-l95 added 3 commits January 18, 2023 10:18

fix typo and add more tests

22bfc3a

Signed-off-by: georgi-l95 <[email protected]>

bump images

834befe

Signed-off-by: georgi-l95 <[email protected]>

revert some changes

cd2d6fc

Signed-off-by: georgi-l95 <[email protected]>

fix some bugs in erc20 test

e74a683

Signed-off-by: georgi-l95 <[email protected]>

georgi-l95 requested review from Nana-EC and lukelee-sl January 18, 2023 14:12

Nana-EC previously approved these changes Jan 19, 2023

View reviewed changes

change regex

9fdec83

Signed-off-by: georgi-l95 <[email protected]>

georgi-l95 dismissed Nana-EC’s stale review via 9fdec83 January 19, 2023 08:01

georgi-l95 requested a review from Nana-EC January 19, 2023 11:43

lukelee-sl approved these changes Jan 19, 2023

View reviewed changes

georgi-l95 merged commit 381f09e into main Jan 19, 2023

georgi-l95 deleted the 816-indexing-eth_call-requests-from-graph-node-fail-in-release-0150 branch January 19, 2023 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand `eth_call` validation to support blockParam object #817

Expand `eth_call` validation to support blockParam object #817

georgi-l95 commented Jan 17, 2023 •

edited

Loading

codecov-commenter commented Jan 17, 2023 •

edited

Loading

lukelee-sl Jan 17, 2023 •

edited

Loading

georgi-l95 Jan 18, 2023

lukelee-sl Jan 17, 2023

georgi-l95 Jan 18, 2023

lukelee-sl Jan 17, 2023

georgi-l95 Jan 17, 2023

georgi-l95 Jan 18, 2023

lukelee-sl Jan 17, 2023 •

edited

Loading

Nana-EC Jan 18, 2023

georgi-l95 Jan 18, 2023

lukelee-sl Jan 18, 2023

dimitrovmaksim Jan 19, 2023 •

edited

Loading

georgi-l95 Jan 19, 2023

lukelee-sl Jan 19, 2023

georgi-l95 Jan 19, 2023

Nana-EC Jan 18, 2023

georgi-l95 commented Jan 18, 2023

Nana-EC commented Jan 19, 2023

sonarqubecloud bot commented Jan 19, 2023

lukelee-sl commented Jan 19, 2023

Expand eth_call validation to support blockParam object #817

Expand eth_call validation to support blockParam object #817

Conversation

georgi-l95 commented Jan 17, 2023 • edited Loading

codecov-commenter commented Jan 17, 2023 • edited Loading

Codecov Report

lukelee-sl Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukelee-sl Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitrovmaksim Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgi-l95 commented Jan 18, 2023

Nana-EC commented Jan 19, 2023

sonarqubecloud bot commented Jan 19, 2023

lukelee-sl commented Jan 19, 2023

Expand `eth_call` validation to support blockParam object #817

Expand `eth_call` validation to support blockParam object #817

georgi-l95 commented Jan 17, 2023 •

edited

Loading

codecov-commenter commented Jan 17, 2023 •

edited

Loading

lukelee-sl Jan 17, 2023 •

edited

Loading

lukelee-sl Jan 17, 2023 •

edited

Loading

dimitrovmaksim Jan 19, 2023 •

edited

Loading