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

fix: eth_estimateGas opts to fallback estimation for contract revert #2834

Conversation

victor-yanev
Copy link
Contributor

@victor-yanev victor-yanev commented Aug 16, 2024

Description:

This PR modifies the EthImpl class to improve error handling for gas estimation.

  • Add logic to return predefined.CONTRACT_REVERT JSON-RPC error with the revert reason in case of contract revert
  • Extend predefined.CONTRACT_REVERT to cover more edge cases when decoding error messages
  • Extend tests for decodeErrorMessage and predefined.CONTRACT_REVERT
  • Extend tests for eth_estimateGas with the new logic for reverting calls

Related issue(s):

Fixes #2830

Solves problem in #2821

Notes for reviewer:

image

Checklist

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

@victor-yanev victor-yanev added bug Something isn't working enhancement New feature or request labels Aug 16, 2024
@victor-yanev victor-yanev added this to the 0.55.0 milestone Aug 16, 2024
@victor-yanev victor-yanev self-assigned this Aug 16, 2024
Copy link

github-actions bot commented Aug 16, 2024

Acceptance Tests

  18 files  225 suites   28m 23s ⏱️
611 tests 591 ✔️ 4 💤 16
719 runs  699 ✔️ 4 💤 16

Results for commit a2d38ea.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

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

Shouldn't we include some tests for this new scenario?

@victor-yanev
Copy link
Contributor Author

Shouldn't we include some tests for this new scenario?

Good point, I will extend the tests for eth_estimateGas

Copy link

github-actions bot commented Aug 16, 2024

Tests

       3 files     203 suites   53s ⏱️
1 030 tests 1 029 ✔️ 1 💤 0
1 043 runs  1 042 ✔️ 1 💤 0

Results for commit a2d38ea.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@acuarica acuarica 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, left some questions

packages/relay/src/lib/eth.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/eth.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/eth.ts Outdated Show resolved Hide resolved
@acuarica
Copy link
Contributor

Nit: I think we should the remove the "Fixes #2821" from this PR's description, and replace it by something like "Related to #2821". The reason is that we wouldn't want to close #2821 when we merge this PR, but instead close it when the Axelar team can effectively confirm the issue is solved, which is when we make a new release containing this fix.

Copy link

sonarcloud bot commented Aug 16, 2024

Copy link
Collaborator

@natanasow natanasow left a comment

Choose a reason for hiding this comment

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

LGTM

@victor-yanev victor-yanev merged commit 5853e30 into main Aug 19, 2024
38 checks passed
@victor-yanev victor-yanev deleted the 2830-Calling-eth_estimateGas-opts-to-fallback-estimation-when-a-contract-revert-is-executed branch August 19, 2024 14:47
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.28%. Comparing base (59a808d) to head (a2d38ea).
Report is 5 commits behind head on main.

Files Patch % Lines
packages/relay/src/lib/eth.ts 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2834      +/-   ##
==========================================
+ Coverage   81.26%   81.28%   +0.02%     
==========================================
  Files          46       46              
  Lines        3394     3415      +21     
  Branches      707      716       +9     
==========================================
+ Hits         2758     2776      +18     
+ Misses        414      410       -4     
- Partials      222      229       +7     
Flag Coverage Δ
relay 80.94% <81.81%> (+0.03%) ⬆️
server 81.53% <ø> (ø)
ws-server 97.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
packages/relay/src/lib/errors/JsonRpcError.ts 70.58% <100.00%> (+4.63%) ⬆️
...ages/relay/src/lib/errors/MirrorNodeClientError.ts 87.50% <100.00%> (+1.13%) ⬆️
packages/relay/src/lib/eth.ts 81.04% <33.33%> (-0.53%) ⬇️

... and 2 files with indirect coverage changes

quiet-node pushed a commit that referenced this pull request Aug 19, 2024
#2834)

* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts

Signed-off-by: Victor Yanev <[email protected]>

* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts

Signed-off-by: Victor Yanev <[email protected]>

* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts

Signed-off-by: Victor Yanev <[email protected]>

* test: extend tests in eth_estimateGas.spec.ts to cover the new logic

Signed-off-by: Victor Yanev <[email protected]>

* test: fix failing postman tests due to missing 'from' field

Signed-off-by: Victor Yanev <[email protected]>

* fix: throw predefined.CONTRACT_REVERT only for CONTRACT_REVERT_EXECUTED error message

Signed-off-by: Victor Yanev <[email protected]>

* fix: use `Status.ContractRevertExecuted` from `@hashgraph/sdk`

Signed-off-by: Victor Yanev <[email protected]>

---------

Signed-off-by: Victor Yanev <[email protected]>
quiet-node pushed a commit that referenced this pull request Aug 19, 2024
#2834)

* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts

Signed-off-by: Victor Yanev <[email protected]>

* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts

Signed-off-by: Victor Yanev <[email protected]>

* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts

Signed-off-by: Victor Yanev <[email protected]>

* test: extend tests in eth_estimateGas.spec.ts to cover the new logic

Signed-off-by: Victor Yanev <[email protected]>

* test: fix failing postman tests due to missing 'from' field

Signed-off-by: Victor Yanev <[email protected]>

* fix: throw predefined.CONTRACT_REVERT only for CONTRACT_REVERT_EXECUTED error message

Signed-off-by: Victor Yanev <[email protected]>

* fix: use `Status.ContractRevertExecuted` from `@hashgraph/sdk`

Signed-off-by: Victor Yanev <[email protected]>

---------

Signed-off-by: Victor Yanev <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
quiet-node added a commit that referenced this pull request Aug 19, 2024
fix: `eth_estimateGas` opts to fallback estimation for contract revert (#2834)

* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts



* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts



* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts



* test: extend tests in eth_estimateGas.spec.ts to cover the new logic



* test: fix failing postman tests due to missing 'from' field



* fix: throw predefined.CONTRACT_REVERT only for CONTRACT_REVERT_EXECUTED error message



* fix: use `Status.ContractRevertExecuted` from `@hashgraph/sdk`



---------

Signed-off-by: Victor Yanev <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Co-authored-by: Victor Yanev <[email protected]>
ebadiere pushed a commit that referenced this pull request Aug 21, 2024
#2834)

* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts

Signed-off-by: Victor Yanev <[email protected]>

* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts

Signed-off-by: Victor Yanev <[email protected]>

* fix: `eth_estimateGas` opts to fallback estimation when a contract reverts

Signed-off-by: Victor Yanev <[email protected]>

* test: extend tests in eth_estimateGas.spec.ts to cover the new logic

Signed-off-by: Victor Yanev <[email protected]>

* test: fix failing postman tests due to missing 'from' field

Signed-off-by: Victor Yanev <[email protected]>

* fix: throw predefined.CONTRACT_REVERT only for CONTRACT_REVERT_EXECUTED error message

Signed-off-by: Victor Yanev <[email protected]>

* fix: use `Status.ContractRevertExecuted` from `@hashgraph/sdk`

Signed-off-by: Victor Yanev <[email protected]>

---------

Signed-off-by: Victor Yanev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling eth_estimateGas opts to fallback estimation when a contract revert is executed
4 participants