[Actionable List] Replace revert strings with custom errors

[Amxx]

🧐 Motivation

Custom Errors, which were introduced in 0.8.4, contribute to reduce the contract size and improve clarity of revert reasons.

A quick study shows that replacing a (short) revert string with a custom error can save 8.5k gas, and slightly decrease the run cost.

error SomeError();
error SomeErrorWithArgs(address);

contract A { function test() external { revert("some short message"); } }
// deploy: 90599
// run: 21288

contract B { function test() external { revert("some long message that will cause deployment to be more expensive, but hopefully not execution"); } }
// deploy: 109401
// run: 21318

contract C { function test() external { revert SomeError(); } }
// deploy: 81953
// run: 21228

contract D { function test() external { revert SomeErrorWithArgs(msg.sender); } }
// deploy: 83225
// run: 21245

📝 Next steps

There are some question to resolve before moving forward with that change:

• Check/validate custom error support by developer tools (hardhat + truffle / hardhat + waffle, etherscan, ...)
  • Adapt our tests if needed
• List all revert string, and decide what custom error to use for each one
  • some custom errors might be reusable between contracts (ex: InsufficientBalance for ERCs 20, 777, 1155)
  • decide which parameter (if any) to include in the custom error
• Decide where to define the errors
  • In each file: makes re-use difficult
  • In a global file: messy
  • Other?

See

• Add utilities for CrossChain messaging #3183 (uses custom errors)
• Add a MissingRole custom error in AccessControl #2725 (rewrite of access control around custom errors)

[Amxx]

#2839

[akhil-is-watching]

Hi I'd really like to take up this issue.

[Amxx]

Thank you @akhil-is-watching, but I think the next steps for now are mostly internal discussion with the team. While let everyone know on the issue if we can use any help.

[wiasliaw]

There are two testing development tools had supported the custom error.

• waffle
• hardhat-chai-matchers

I recommend hardhat-chai-matchers. it provide better readability.

// hardhat-chai-matchers
await expect(contract.call()).to.be.revertedWithCustomError(
  contract,
  "SomeCustomError"
);

but waffle provider a matcher combined reason string and custom error

// waffle
// reason string
await expect(token.checkRole('ADMIN'))
  .to.be.revertedWith(/AccessControl: account .* is missing role .*/);

// custom error
await expect(token.transfer(receiver, 100))
    .to.be.revertedWith('InsufficientBalance')
    .withArgs(0, 100);

[Amxx]

I like waffle's "withArgs" (its the same for events)

[wiasliaw]

@Amxx Hardhat supports withArgs too.

await expect(contract.call())
  .to.be.revertedWithCustomError(contract, "SomeCustomError")
  .withArgs(anyValue, "some error data string");

[frangio]

hardhat-chai-matchers should be preferred over Waffle nowadays.

[ItsShadowl]

hardhat-chai-matchers should be preferred over Waffle nowadays.

I believe both supports Custom Errors, and the implementation can be tested with either.

We use Custom Error all over our codebase where possible. It is lighter.

[ernestognw]

Closing in favor of #2839 since there's more activity and attention over there

[radeksvarz]

There are two testing development tools had supported the custom error.

• waffle
• hardhat-chai-matchers

I recommend hardhat-chai-matchers. it provide better readability.

// hardhat-chai-matchers
await expect(contract.call()).to.be.revertedWithCustomError(
  contract,
  "SomeCustomError"
);

but waffle provider a matcher combined reason string and custom error

// waffle
// reason string
await expect(token.checkRole('ADMIN'))
  .to.be.revertedWith(/AccessControl: account .* is missing role .*/);

// custom error
await expect(token.transfer(receiver, 100))
    .to.be.revertedWith('InsufficientBalance')
    .withArgs(0, 100);

Foundry can be also included on the list with vm.expectRevert(CustomError.selector);

[Amxx]

@radeksvarz

Our tests are written in JS for many reasons (including ensuring compatibility with libraries frequently used by frontends). We are not going to migrate everything to foundry. That would require a tremendous effort, and the added value to our end users is debatable.

We do use foundry to fusing though.