Add simple cache on `eth_call` #880

georgi-l95 · 2023-02-07T13:03:20Z

Signed-off-by: georgi-l95 glazarov95@gmail.com

Description:
Adds a cache on eth_call request, based on to and data parameters for 200ms.

Related issue(s):

Fixes #877

Notes for reviewer:

Checklist

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

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

codecov-commenter · 2023-02-07T13:07:42Z

Codecov Report

Base: 73.88% // Head: 74.16% // Increases project coverage by +0.28% 🎉

Coverage data is based on head (3af5d1a) compared to base (213d7cb).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 3af5d1a differs from pull request most recent head 024588c. Consider uploading reports for the commit 024588c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #880      +/-   ##
==========================================
+ Coverage   73.88%   74.16%   +0.28%     
==========================================
  Files          29       29              
  Lines        1815     1827      +12     
  Branches      336      339       +3     
==========================================
+ Hits         1341     1355      +14     
+ Misses        375      374       -1     
+ Partials       99       98       -1

Impacted Files	Coverage Δ
packages/relay/src/lib/eth.ts	`82.84% <100.00%> (+0.71%)`	⬆️

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.

Nana-EC

LG
Some suggestions

Nana-EC · 2023-02-07T13:47:56Z

packages/relay/src/lib/eth.ts

@@ -1017,8 +1017,17 @@ export class EthImpl implements Eth {
        }
      }

+      let cachedResponse = this.cache.get(`eth_call: ${call.data} from ${call.to}`);


nit: add const for label. Maybe shorten key also

Suggested change

let cachedResponse = this.cache.get(`eth_call: ${call.data} from ${call.to}`);

const cacheKey = `eth_call:.${call.to}.${call.data}`;

let cachedResponse = this.cache.get(cacheKey);

Nana-EC · 2023-02-07T13:48:22Z

packages/relay/src/lib/eth.ts

-      return EthImpl.prepend0x(Buffer.from(contractCallResponse.asBytes()).toString('hex'));
+      const formattedCallReponse = EthImpl.prepend0x(Buffer.from(contractCallResponse.asBytes()).toString('hex'));
+
+      this.cache.set(`eth_call: ${call.data} from ${call.to}`, formattedCallReponse, { ttl: 200 });


use cacheKey here

Nana-EC · 2023-02-07T13:50:22Z

packages/relay/src/lib/eth.ts

@@ -1017,8 +1017,17 @@ export class EthImpl implements Eth {
        }
      }

+      let cachedResponse = this.cache.get(`eth_call: ${call.data} from ${call.to}`);


I'm concerned the call.data can get quite long.
In my exploration I went down the path of taking a hashing of the data to reduce it's length.

Maybe since the cache is so short lived it's not a biggie but let's make sure the key itself doesn't have length limits.
Maybe a simple approach of taking the first n characters also works if the hash approach is costly

I just wanna avoid any potential error cases from a long data. maybe add a test case with a long data field if you stay with this approach.

Nana-EC

Let's make the ttl configureable

Nana-EC · 2023-02-07T13:59:11Z

packages/relay/src/lib/eth.ts

-      return EthImpl.prepend0x(Buffer.from(contractCallResponse.asBytes()).toString('hex'));
+      const formattedCallReponse = EthImpl.prepend0x(Buffer.from(contractCallResponse.asBytes()).toString('hex'));
+
+      this.cache.set(`eth_call: ${call.data} from ${call.to}`, formattedCallReponse, { ttl: 200 });


Please make the ttl value a config to allow for on demand tuning

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

sonarcloud · 2023-02-07T15:22:56Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

Ivo-Yankov · 2023-02-07T15:36:17Z

I think this cache could cause issues in the case where the result of a method from a contract depends not on its own state, but on the state of another contract.

Such is the case with a Multicaller contract, where it only aggregates and returns results from multiple other contracts, but it doesn't update its own state.

georgi-l95 · 2023-02-07T16:02:29Z

Hmm, I think you're right, but we have a ttl of 200ms, which is not so long, because those multicalls might take some time. Also this is a temporary solution and it's configurable if there is indeed problem with multicall queries.

Nana-EC

LG

lukelee-sl · 2023-02-07T20:39:31Z

packages/relay/src/lib/eth.ts

+        data = crypto.createHash('sha1').update(call.data).digest('hex'); // NOSONAR
+      }
+
+      const cacheKey = `eth_call:.${call.to}.${data}`;


what happens if data is null/undefined?

No problems there we continue with the query as normal, I just didn’t want to calculate hash for a undefined, now that I think, I should’ve added a or call.data.length is bigger than 64(for example).

Adds a cache on eth_call request, based on `to` and `data` parameters for 200ms. Signed-off-by: georgi-l95 <glazarov95@gmail.com>

* Add simple cache on `eth_call` (#880) Adds a cache on eth_call request, based on `to` and `data` parameters for 200ms. Signed-off-by: georgi-l95 <glazarov95@gmail.com> * retrigger checks --------- Signed-off-by: georgi-l95 <glazarov95@gmail.com> Co-authored-by: Georgi Lazarov <glazarov95@gmail.com>

add cache

9a301ca

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

georgi-l95 self-assigned this Feb 7, 2023

format

4e3e864

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

georgi-l95 requested review from Nana-EC, AlfredoG87, ebadiere, Ivo-Yankov, lukelee-sl and natanasow February 7, 2023 13:06

georgi-l95 marked this pull request as ready for review February 7, 2023 13:07

Nana-EC reviewed Feb 7, 2023

View reviewed changes

georgi-l95 added 3 commits February 7, 2023 16:51

hash call data

024588c

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

suppress warning

500ace3

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

suppress warning

982515f

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

georgi-l95 requested a review from Nana-EC February 7, 2023 15:25

Nana-EC approved these changes Feb 7, 2023

View reviewed changes

Nana-EC added enhancement New feature or request limechain P1 labels Feb 7, 2023

Nana-EC added this to the 0.18.0 milestone Feb 7, 2023

georgi-l95 merged commit 1f9b860 into main Feb 7, 2023

georgi-l95 deleted the add-cache-on-eth-call branch February 7, 2023 16:43

lukelee-sl reviewed Feb 7, 2023

View reviewed changes

lukelee-sl pushed a commit that referenced this pull request Feb 7, 2023

Add simple cache on eth_call (#880)

1647aeb

Adds a cache on eth_call request, based on `to` and `data` parameters for 200ms. Signed-off-by: georgi-l95 <glazarov95@gmail.com>

lukelee-sl mentioned this pull request Feb 7, 2023

cherry pick #880 - simple cache for eth_call #885

Merged

lukelee-sl pushed a commit that referenced this pull request Feb 7, 2023

Add simple cache on eth_call (#880)

b03d80f

Adds a cache on eth_call request, based on `to` and `data` parameters for 200ms. Signed-off-by: georgi-l95 <glazarov95@gmail.com>

lukelee-sl modified the milestones: 0.18.0, 0.17.1 Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simple cache on `eth_call` #880

Add simple cache on `eth_call` #880

georgi-l95 commented Feb 7, 2023 •

edited

Loading

codecov-commenter commented Feb 7, 2023 •

edited

Loading

Nana-EC left a comment

Nana-EC Feb 7, 2023

Nana-EC Feb 7, 2023

Nana-EC Feb 7, 2023

Nana-EC Feb 7, 2023

Nana-EC left a comment

Nana-EC Feb 7, 2023

sonarcloud bot commented Feb 7, 2023

Ivo-Yankov commented Feb 7, 2023 •

edited

Loading

georgi-l95 commented Feb 7, 2023

Nana-EC left a comment

lukelee-sl Feb 7, 2023 •

edited

Loading

georgi-l95 Feb 7, 2023

	let cachedResponse = this.cache.get(`eth_call: ${call.data} from ${call.to}`);
	const cacheKey = `eth_call:.${call.to}.${call.data}`;
	let cachedResponse = this.cache.get(cacheKey);

Add simple cache on eth_call #880

Add simple cache on eth_call #880

Conversation

georgi-l95 commented Feb 7, 2023 • edited Loading

codecov-commenter commented Feb 7, 2023 • edited Loading

Codecov Report

Nana-EC left a comment

Choose a reason for hiding this comment

Nana-EC Feb 7, 2023

Choose a reason for hiding this comment

Nana-EC Feb 7, 2023

Choose a reason for hiding this comment

Nana-EC Feb 7, 2023

Choose a reason for hiding this comment

Nana-EC Feb 7, 2023

Choose a reason for hiding this comment

Nana-EC left a comment

Choose a reason for hiding this comment

Nana-EC Feb 7, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Feb 7, 2023

Ivo-Yankov commented Feb 7, 2023 • edited Loading

georgi-l95 commented Feb 7, 2023

Nana-EC left a comment

Choose a reason for hiding this comment

lukelee-sl Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

georgi-l95 Feb 7, 2023

Choose a reason for hiding this comment

Add simple cache on `eth_call` #880

Add simple cache on `eth_call` #880

georgi-l95 commented Feb 7, 2023 •

edited

Loading

codecov-commenter commented Feb 7, 2023 •

edited

Loading

Ivo-Yankov commented Feb 7, 2023 •

edited

Loading

lukelee-sl Feb 7, 2023 •

edited

Loading