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

Limit the ethCall gas to 15_000_000, otherwise the call will fail #513

Closed
wants to merge 4 commits into from

Conversation

dimitrovmaksim
Copy link
Collaborator

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

Description:
I'm not sure if this is the best way to do this, but ethCalls with gas over 15_000_000 will fail with BUSY error, because the Hedera limit is set to 15_000_000. This breaks the integration with TheGraph, when executing eth_calls from the event handlers, because the graph-node reserves all 50_000_000 gas.

Related issue(s):

Fixes #511

Notes for reviewer:

Checklist

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

@dimitrovmaksim dimitrovmaksim self-assigned this Sep 12, 2022
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.

Looking good.
Missing log and test

@Nana-EC Nana-EC added this to the 0.8.0 milestone Sep 12, 2022
@Nana-EC Nana-EC added enhancement New feature or request limechain P3 process Build, test and deployment-process related tasks and removed process Build, test and deployment-process related tasks labels Sep 12, 2022
Signed-off-by: Maksim Dimitrov <[email protected]>
Nana-EC
Nana-EC previously approved these changes Sep 13, 2022
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

// With values over 15_000_000 the call will fail with BUSY error
if (gas > 15_000_000) {
this.logger.trace(`${requestIdPrefix} eth_call gas amount (${gas}) exceeds network limit, capping gas to 15_000_000`);
gas = 15_000_000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: could have made 15_000_000 a const
Can make this change in a future PR, no biggie

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll still probably have to merge main at some point to fix the dapp tests, I can push that change too

Signed-off-by: Maksim Dimitrov <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Base: 76.38% // Head: 76.67% // Increases project coverage by +0.29% 🎉

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #513      +/-   ##
==========================================
+ Coverage   76.38%   76.67%   +0.29%     
==========================================
  Files          12       12              
  Lines         923      926       +3     
  Branches      144      145       +1     
==========================================
+ Hits          705      710       +5     
+ Misses        165      164       -1     
+ Partials       53       52       -1     
Impacted Files Coverage Δ
packages/relay/src/lib/eth.ts 84.64% <100.00%> (+0.54%) ⬆️

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.

@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

@Nana-EC
Copy link
Collaborator

Nana-EC commented Sep 15, 2022

@dimitrovmaksim, all your commits need to be signed

@dimitrovmaksim
Copy link
Collaborator Author

@dimitrovmaksim, all your commits need to be signed

All of them seem to have the "Signed-off-by:" message

@dimitrovmaksim
Copy link
Collaborator Author

Closing in favor of #526

@dimitrovmaksim dimitrovmaksim deleted the limit-gas-for-ethCall branch September 16, 2022 17:23
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 P3
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Set gas limit to 15_000_000
3 participants