Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

EVM Error format #350

Merged
merged 7 commits into from
Jul 28, 2021
Merged

Conversation

thomas-nguy
Copy link
Contributor

@thomas-nguy thomas-nguy commented Jul 26, 2021

Closes: #247

Description

Return Geth error format for EVM failure


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

Merging #350 (add31af) into main (695027c) will increase coverage by 0.23%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #350      +/-   ##
==========================================
+ Coverage   51.97%   52.20%   +0.23%     
==========================================
  Files          47       47              
  Lines        4660     4668       +8     
==========================================
+ Hits         2422     2437      +15     
+ Misses       2139     2132       -7     
  Partials       99       99              
Impacted Files Coverage Δ
x/evm/keeper/grpc_query.go 71.20% <50.00%> (ø)
x/evm/types/errors.go 100.00% <100.00%> (+100.00%) ⬆️

}

reason, errUnpack := abi.UnpackRevert(result)
err := errors.New("execution reverted")
Copy link
Contributor

Choose a reason for hiding this comment

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

There are other types of errors, e.g. out of gas?

Copy link
Contributor Author

@thomas-nguy thomas-nguy Jul 26, 2021

Choose a reason for hiding this comment

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

hmm I could not find anything special in geth. Is out of gas include in exection reverted?

Copy link
Contributor

@yihuang yihuang Jul 26, 2021

Choose a reason for hiding this comment

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

https://github.com/ethereum/go-ethereum/blob/master/core/vm/errors.go#L31
there are several possible cases for the VmError. maybe just change it to errors.New(vmError) here is enough?

Copy link
Contributor Author

@thomas-nguy thomas-nguy Jul 26, 2021

Choose a reason for hiding this comment

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

I see, you are right, I have missread the if condition. Fixed

@thomas-nguy thomas-nguy changed the title [WIP] EVM Error format EVM Error format Jul 26, 2021
@thomas-nguy thomas-nguy changed the title EVM Error format EVM Error format [WIP] Jul 26, 2021
@thomas-nguy
Copy link
Contributor Author

thomas-nguy commented Jul 26, 2021

Currently, on transaction revert (with this PR)

Ethermint

{
 <     "jsonrpc": "2.0",
 <     "id": 1627276957344,
 <     "error": {
 <       "code": 3,
 <       "message": "execution reverted: COUNTER_TOO_LOW",
 <       "data": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000f434f554e5445525f544f4f5f4c4f570000000000000000000000000000000000"
 <     }
 <   }
 

Ganache

 <   {
<     "id": 1627277502538,
<     "jsonrpc": "2.0",
<     "error": {
<       "message": "VM Exception while processing transaction: revert COUNTER_TOO_LOW",
<       "code": -32000,
<       "data": {
<         "0x90264de254689f1d4e7f8670cd97f60d9bc803874fdecb34d249bd1cc3ca823a": {
<           "error": "revert",
<           "program_counter": 346,
<           "return": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000f434f554e5445525f544f4f5f4c4f570000000000000000000000000000000000",
<           "reason": "COUNTER_TOO_LOW"
<         },
<         "stack": "c: VM Exception while processing transaction: revert COUNTER_TOO_LOW\n    at Function.c.fromResults (/Users/thomasnguy/.config/yarn/global/node_modules/ganache-cli/build/ganache-core.node.cli.js:4:192416)\n    at /Users/thomasnguy/.config/yarn/global/node_modules/ganache-cli/build/ganache-core.node.cli.js:42:50402",
<         "name": "c"
<       }
<     }

I am not sure how compliant we needs to be. It seems that there is also differences between geth and nethermid
ethereum/go-ethereum#21886

@leejw51crypto
Copy link
Contributor

some lint issues ^^;

@thomas-nguy thomas-nguy force-pushed the thomas/314-evm-error-format branch from e4492c7 to baaae15 Compare July 26, 2021 05:53
@thomas-nguy thomas-nguy changed the title EVM Error format [WIP] EVM Error format Jul 26, 2021
@thomas-nguy thomas-nguy force-pushed the thomas/314-evm-error-format branch from 861a942 to 3bbfcce Compare July 26, 2021 06:06
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM, utACK

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better rpc error response
4 participants