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

feat(smart-contracts): replace revert strings by error codes #8867

Merged
merged 24 commits into from
Jun 3, 2022

Conversation

clemsos
Copy link
Member

@clemsos clemsos commented May 30, 2022

Description

Here we replace require statement by Solidity Custom Errors. This decrease contract size and also improve gas consumption.

·---------------------------|-------------|---------------·
 |  Contract Name            ·  Size (KB)  ·  Change (KB)  │
 ····························|·············|················
 |  MixinConvenienceOwnable  ·      5.840  ·       -0.483  │
 ····························|·············|················
 |  MixinDisable             ·      0.063  ·               │
 ····························|·············|················
 |  MixinERC721Enumerable    ·      8.821  ·       -0.788  │
 ····························|·············|················
 |  MixinErrors              ·      0.063  ·       +0.063  │
 ····························|·············|················
 |  MixinFunds               ·      0.145  ·               │
 ····························|·············|················
 |  MixinGrantKeys           ·      9.643  ·       -0.861  │
 ····························|·············|················
 |  MixinKeys                ·      8.690  ·       -0.759  │
 ····························|·············|················
 |  MixinLockCore            ·      5.590  ·       -0.439  │
 ····························|·············|················
 |  MixinLockMetadata        ·     11.607  ·       -0.759  │
 ····························|·············|················
 |  MixinPurchase            ·     13.981  ·       -1.509  │
 ····························|·············|················
 |  MixinRefunds             ·     14.965  ·       -1.509  │
 ····························|·············|················
 |  MixinRoles               ·      2.679  ·       -0.035  │
 ····························|·············|················
 |  MixinTransfer            ·     16.370  ·       -1.701  │
 ····························|·············|················
 |  PublicLock               ·     22.181  ·       -1.815  │
 ·---------------------------|-------------|---------------·

Issues

Fixes
Refs #8380

Checklist:

  • 1 PR, 1 purpose: my Pull Request applies to a single purpose
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the docs to reflect my changes if applicable
  • I have added tests (and stories for frontend components) that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • If my code involves visual changes, I am adding applicable screenshots to this thread

Release Note Draft Snippet

@cla-bot cla-bot bot added the cla-signed label May 30, 2022
@clemsos clemsos changed the title feat(smart-contracts): replace revert strings by errors feat(smart-contracts): replace revert strings by error codes May 30, 2022
@clemsos
Copy link
Member Author

clemsos commented May 30, 2022

It seems that Hardhat test runner is not catching the error messages properly - while solidity-coverage does. Instead we got a

AssertionError: Expected to fail with NOT_ENOUGH_FUNDS, but failed with: 
Error: VM Exception while processing transaction: reverted with an unrecognized custom error

Hardhat should support custom errors, so maybe sth in passing around the errors defs

@clemsos
Copy link
Member Author

clemsos commented May 30, 2022

Here is a repro repo to see where the problem is https://github.com/clemsos/custom-errors-repro

@clemsos clemsos self-assigned this Jun 2, 2022
@clemsos clemsos requested a review from julien51 June 2, 2022 13:44
@julien51
Copy link
Member

julien51 commented Jun 2, 2022

/rebase

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

Let's use a more explicit error name but then LG!

@clemsos clemsos merged commit 3121df4 into master Jun 3, 2022
@clemsos clemsos deleted the strip-errors branch June 3, 2022 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants