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

Optimize eth_getLogs() #616

Merged
merged 3 commits into from
Oct 14, 2022
Merged

Optimize eth_getLogs() #616

merged 3 commits into from
Oct 14, 2022

Conversation

dimitrovmaksim
Copy link
Collaborator

@dimitrovmaksim dimitrovmaksim commented Oct 13, 2022

Signed-off-by: Maksim Dimitrov [email protected]

Description:
Removes now obsolete queries to the mirror node for each log to fetch block and transaction data

Related issue(s):

Fixes #389

Notes for reviewer:

Checklist

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

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2022

Codecov Report

Base: 71.21% // Head: 70.74% // Decreases project coverage by -0.46% ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #616      +/-   ##
==========================================
- Coverage   71.21%   70.74%   -0.47%     
==========================================
  Files          16       16              
  Lines        1157     1135      -22     
  Branches      198      195       -3     
==========================================
- Hits          824      803      -21     
  Misses        279      279              
+ Partials       54       53       -1     
Impacted Files Coverage Δ
packages/relay/src/lib/eth.ts 83.23% <100.00%> (-0.53%) ⬇️

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.

@dimitrovmaksim
Copy link
Collaborator Author

dimitrovmaksim commented Oct 13, 2022

I'm not sure if 567 is the correct timeout status code https://github.com/hashgraph/hedera-json-rpc-relay/blob/main/packages/relay/src/lib/errors/MirrorNodeClientError.ts#L26. Currently I think 2 eth_getLogs tests are using this as a timeout code, but it also returns 567 if a mock for a specific request is not found. One of the tests now passes, but actually the error reason is that there's no mock for the request -

mock.onGet(`contracts/${contractAddress1}/results/logs`).reply(567, timeoutError);

MirrorNodeClientError: Could not find mock for: 
{
  "method": "get",
  "url": "contracts/0x000000000000000000000000000000000000055f/results/logs?timestamp=gte:1651560386.060890949&timestamp=lte:1651560389.060890949"
}
    at MirrorNodeClient.handleError (/Users/maksimdimitrov/Projects/hedera/hedera-json-rpc-relay/packages/relay/src/lib/clients/mirrorNodeClient.ts:21:2947)
    at MirrorNodeClient.<anonymous> (/Users/maksimdimitrov/Projects/hedera/hedera-json-rpc-relay/packages/relay/src/lib/clients/mirrorNodeClient.ts:21:1894)
    at Generator.throw (<anonymous>)
    at rejected (/Users/maksimdimitrov/Projects/hedera/hedera-json-rpc-relay/packages/relay/src/lib/clients/mirrorNodeClient.ts:19:4) {
  statusCode: 567
}

The actual mocked requests should be

mock.onGet(`contracts/${contractAddress1}/results/logs?timestamp=gte:${defaultBlock.timestamp.from}&timestamp=lte:${defaultBlock.timestamp.to}`).reply(567, timeoutError);

but the tests till passes

CC: @Nana-EC

@dimitrovmaksim dimitrovmaksim requested review from Nana-EC and removed request for Nana-EC October 13, 2022 14:07
@dimitrovmaksim
Copy link
Collaborator Author

I think I figured it out. Timeouts will not have a response property so it will always return unknownServerErrorHttpStatusCode which is 567

const effectiveStatusCode = error.response !== undefined ? error.response.status : MirrorNodeClient.unknownServerErrorHttpStatusCode;

Timeouts will have error.code == ECONNABORTED

@Nana-EC Nana-EC requested review from Nana-EC, Ivo-Yankov, ar-conmit and lukelee-sl and removed request for Nana-EC and Ivo-Yankov October 13, 2022 17:13
@Nana-EC Nana-EC added limechain enhancement New feature or request P2 labels Oct 13, 2022
@Nana-EC Nana-EC added this to the 0.10 milestone Oct 13, 2022
Signed-off-by: Maksim Dimitrov <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG, just waiting on passing tests

Copy link
Member

@georgi-l95 georgi-l95 left a comment

Choose a reason for hiding this comment

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

LGTM!

@dimitrovmaksim dimitrovmaksim merged commit fa98536 into main Oct 14, 2022
@dimitrovmaksim dimitrovmaksim deleted the 389-optimize-eth-get-logs branch October 14, 2022 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request limechain P2
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Optimize eth_getLogs
4 participants