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

Properly handle execution reverts in eth_call JSON-RPC endpoint #297

Merged
merged 4 commits into from
Jun 12, 2024

Conversation

m-Peter
Copy link
Collaborator

@m-Peter m-Peter commented Jun 12, 2024

Closes: #296

Description

This can be used by other tools to correctly parse the revert reason to a human-friendly error message.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

Summary by CodeRabbit

  • New Features

    • Enhanced error handling for EVM execution, including support for specific error codes and custom error messages.
  • Bug Fixes

    • Corrected the return value in EVM calls to ensure accurate error data is provided.
  • Tests

    • Added validations for custom errors and assertion errors in smart contract interactions.
    • Updated test fixtures and bytecode to reflect new error handling mechanisms.
  • Chores

    • Updated dependencies to the latest versions for improved stability and performance.

@m-Peter m-Peter added this to the Flow-EVM-M2 milestone Jun 12, 2024
@m-Peter m-Peter self-assigned this Jun 12, 2024

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

return nil, getErrorForCode(evmResult.ErrorCode)
if evmResult.ErrorCode == evmTypes.ExecutionErrCodeExecutionReverted {
return nil, errs.NewRevertError(evmResult.ReturnedData)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for else

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, right 😅
Removed in 90d72ce

Comment on lines +28 to +34
function assertError() public pure {
require(false, "Assert Error Message");
}

function customError() public pure {
revert MyCustomError(5, "Value is too low");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@@ -103,4 +103,74 @@ it('deploy contract and interact', async() => {
let block = await web3.eth.getBlock(latestHeight)
assert.equal(block.logsBloom, res.receipt.logsBloom)

// check that revert reason for custom error is correctly returned for signed transaction
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Contributor

@devbugging devbugging left a comment

Choose a reason for hiding this comment

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

very nice!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range and nitpick comments (2)
services/requester/requester.go (2)

Line range hint 563-563: Refactor signAndSend to reduce complexity.

The signAndSend function is too long, which can make it difficult to maintain and understand. Consider breaking it down into smaller, more manageable functions. For example, the concurrent operations for fetching the latest block and signer info could be extracted into separate methods.


Line range hint 669-669: Refactor getErrorForCode to simplify.

The getErrorForCode function has too many statements, making it complex and hard to maintain. Consider using a map to associate error codes with their corresponding errors, which can simplify the function and improve readability.

+ var errorCodeMap = map[evmTypes.ErrorCode]error{
+     evmTypes.ValidationErrCodeGasUintOverflow: gethVM.ErrGasUintOverflow,
+     // Add other mappings here...
+ }
+
+ func getErrorForCode(errorCode evmTypes.ErrorCode) error {
+     if err, ok := errorCodeMap[errorCode]; ok {
+         return err
+     }
+     return fmt.Errorf("unknown error code: %d", errorCode)
+ }
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a1ee6d6 and 90d72ce.

Files selected for processing (1)
  • services/requester/requester.go (1 hunks)
Additional context used
golangci-lint
services/requester/requester.go

563-563: Function 'signAndSend' is too long (82 > 60) (funlen)


669-669: Function 'getErrorForCode' has too many statements (66 > 40) (funlen)

Additional comments not posted (1)
services/requester/requester.go (1)

329-333: Properly handle execution reverts in eth_call.

The updated error handling in the Call method correctly checks for execution reverts and returns a RevertError when appropriate. This aligns with the PR's objective to enhance error parsing for better user feedback.

@devbugging devbugging merged commit 573c763 into onflow:main Jun 12, 2024
2 checks passed
@m-Peter m-Peter deleted the eth-call-handle-execution-reverts branch June 12, 2024 15:13
@coderabbitai coderabbitai bot mentioned this pull request Sep 23, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Update eth_call to correctly handle execution reverts
2 participants