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

Support revert with custom errors (EIP 838) #464

Merged

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Jun 22, 2021

What was wrong?

We don't support custom errors as specified by EIP-838 (solidity docs have more info)

How was it fixed?

  1. Added support for revert <expression> where <expression> has to be a struct. We might change that in the future to allow more flexibility with reverts. For now structs are the only way to return an EIP-838 compatible error.

E.g.

struct Error:
    msg: u256
    val: bool
    
pub def revert_custom_error():
    revert Error(msg=100, val=true)

The revert data for the above would be:

  --sig--|-----------------------------100------------------------------|---------------------------------true----------------------------
0x3253e3c700000000000000000000000000000000000000000000000000000000000000640000000000000000000000000000000000000000000000000000000000000001
  1. This PR doesn't change anything about assert statements but as a recap we currently encode assert false, "foo" as if it were assert false, Error(msg="foo"). We could use the lowering pass to do the conversion but since we do not yet support String<N> in structs this continues to be a special case. But again, this PR doesn't change the existing encoding of assert it only refactors the underlying code to be more flexible and shared across revert and assert statements.

  2. I added a bunch of new solidity tests that prove that the error coding on their end works similar to ours

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2021

Codecov Report

Merging #464 (f8a8809) into master (7c08919) will increase coverage by 0.00%.
The diff coverage is 97.02%.

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

@@           Coverage Diff           @@
##           master     #464   +/-   ##
=======================================
  Coverage   87.31%   87.31%           
=======================================
  Files          75       76    +1     
  Lines        4966     5046   +80     
=======================================
+ Hits         4336     4406   +70     
- Misses        630      640   +10     
Impacted Files Coverage Δ
crates/parser/src/ast.rs 48.88% <ø> (ø)
crates/yulgen/src/runtime/functions/data.rs 100.00% <ø> (ø)
crates/yulgen/src/runtime/functions/mod.rs 100.00% <ø> (ø)
crates/abi/src/utils.rs 71.42% <66.66%> (ø)
crates/test-utils/src/lib.rs 82.72% <93.54%> (-1.76%) ⬇️
crates/analyzer/src/namespace/types.rs 82.90% <100.00%> (+0.17%) ⬆️
crates/analyzer/src/traversal/functions.rs 91.12% <100.00%> (ø)
crates/lowering/src/mappers/functions.rs 99.16% <100.00%> (ø)
crates/parser/src/grammar/functions.rs 90.16% <100.00%> (+0.33%) ⬆️
crates/yulgen/src/context.rs 100.00% <100.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c08919...b289056. Read the comment docs.

@cburgdorf cburgdorf force-pushed the christoph/tests/solidity-custom-errors branch 6 times, most recently from 64d4dce to d869266 Compare June 30, 2021 11:22
@cburgdorf cburgdorf force-pushed the christoph/tests/solidity-custom-errors branch 3 times, most recently from febc76e to cb6eab6 Compare July 2, 2021 09:34
@cburgdorf cburgdorf force-pushed the christoph/tests/solidity-custom-errors branch 2 times, most recently from b289056 to 256b91a Compare July 2, 2021 10:05
@cburgdorf cburgdorf changed the title Christoph/tests/solidity custom errors Support revert with custom errors (EIP 838) Jul 2, 2021
@cburgdorf cburgdorf marked this pull request as ready for review July 2, 2021 10:27
Copy link
Member

@g-r-a-n-t g-r-a-n-t 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 😎

@g-r-a-n-t g-r-a-n-t merged commit ff5df97 into ethereum:master Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants