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

treat all vm errors the same as reverted #276

Merged
merged 1 commit into from
Jul 15, 2021
Merged

treat all vm errors the same as reverted #276

merged 1 commit into from
Jul 15, 2021

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Jul 13, 2021 •
edited
Loading

Closes: #274

Description

  • currently only reverted is treated as error, there are several other error reason need to be treated in the same way.
  • Changed return type of ApplyMessage, also moved gas refund logic into ApplyMessage, to make it more reusable in EthCall.

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 13, 2021 •
edited
Loading

Codecov Report

Merging #276 (3878861) into main (9d1ce30) will increase coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head 3878861 differs from pull request most recent head 86c259d. Consider uploading reports for the commit 86c259d to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main     #276   +/-   ##
=======================================
  Coverage   47.60%   47.60%           
=======================================
  Files          45       45           
  Lines        3151     3132   -19     
=======================================
- Hits         1500     1491    -9     
+ Misses       1576     1566   -10     
  Partials       75       75
Impacted Files Coverage Δ
x/evm/keeper/grpc_query.go 76.13% <0.00%> (+2.10%) ⬆️
x/evm/keeper/msg_server.go 0.00% <0.00%> (ø)
x/evm/keeper/state_transition.go 0.00% <0.00%> (-6.34%) ⬇️
app/ante/eth.go 90.04% <0.00%> (ø)

@yihuang yihuang force-pushed the vmerror branch 2 times, most recently from d3ae2b9 to aa31409 Compare July 13, 2021 08:48
@yihuang
Copy link
Contributor Author

yihuang commented Jul 13, 2021

It seems not trivial to test this in unit tests, I have to build smart contract tx that fails?

@yihuang yihuang mentioned this pull request Jul 13, 2021
Merged
11 tasks
leejw51crypto
leejw51crypto approved these changes Jul 13, 2021
View reviewed changes
Copy link
Contributor

@leejw51crypto leejw51crypto left a comment

Choose a reason for hiding this comment

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

lgtm

@leejw51crypto
Copy link
Contributor

i'll add smart contract test using etherjs

@yijiasu-crypto
Copy link
Contributor

It seems not trivial to test this in unit tests, I have to build smart contract tx that fails?

There's Opcode test in the coming PR #273 so it's good to go

@thomas-nguy
Copy link
Contributor

might need to solve conflict after
#258

@yihuang
Copy link
Contributor Author

yihuang commented Jul 14, 2021

might need to solve conflict after
#258

rebased.

@yihuang yihuang force-pushed the vmerror branch 2 times, most recently from a299047 to dbcc720 Compare July 14, 2021 10:07
fedekunze
fedekunze reviewed Jul 14, 2021
View reviewed changes
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.

Generally looks good. I'd just suggest adding extra comments both on the code and on top of the functions so that we can provide the most context possible

x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
x/evm/types/utils.go Outdated Show resolved Hide resolved
x/evm/keeper/grpc_query.go Show resolved Hide resolved
x/evm/keeper/state_transition.go Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
@yihuang yihuang force-pushed the vmerror branch 4 times, most recently from 31d19c8 to d6ffac5 Compare July 14, 2021 15:19
fedekunze
fedekunze approved these changes Jul 14, 2021
View reviewed changes
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.

Looks good, can you add a Changelog entry too? @thomas-nguy should also review this and give his ACK

x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
@fedekunze fedekunze requested a review from thomas-nguy July 14, 2021 21:29
@yihuang
Copy link
Contributor Author

yihuang commented Jul 14, 2021

Looks good, can you add a Changelog entry too? @thomas-nguy should also review this and give his ACK

changelog added now.

thomas-nguy
thomas-nguy approved these changes Jul 15, 2021
View reviewed changes
Copy link
Contributor

@thomas-nguy thomas-nguy left a comment

Choose a reason for hiding this comment

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

lgtm

@yihuang
treat all vm errors the same as reverted
86c259d 
Closes: #274

evm: fix `ExtraEIP` activation (#288)

Closes: #287

Update x/evm/types/utils.go

Co-authored-by: Federico Kunze Küllmer <[email protected]>

Add `Failed` utility function and changelog
@fedekunze fedekunze merged commit 297a35d into evmos:main Jul 15, 2021
@yihuang yihuang deleted the vmerror branch July 15, 2021 06:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thomas-nguy thomas-nguy thomas-nguy approved these changes

@leejw51crypto leejw51crypto leejw51crypto approved these changes

@fedekunze fedekunze fedekunze approved these changes

@khoslaventures khoslaventures Awaiting requested review from khoslaventures

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assembly instruction invalid() was not behaving correctly
5 participants
@yihuang @leejw51crypto @yijiasu-crypto @thomas-nguy @fedekunze