Replace revert strings with custom errors by ernestognw · Pull Request #4261 · OpenZeppelin/openzeppelin-contracts (original) (raw)

I want to touch again on the "where errors are declared", because I fell we never took the proper time to discuss the options. We discuss the syntax of the error at length, but I think there is more to it than just how they are named. Particularly since our codebase is designed to be reused.

AFAIK, there are 4 options to declaring errors

• In an interface
• In a contract
• In a library
• Globally (file level)

I don't really like defining them globally, because it pollutes the namespace, and can cause to identifier conflicts very easily (particularly if there are many errors)

For contracts vs interfaces vs library, there is the following:

• One cannot call an error defined in an interface that it doesn't inherit. That means that if a contract, of a library wants to use the error from an interface it needs to inherit the interface (not possible for libraries)
• One can call an error from a contract using the revert Contract.Error() syntax. This allows library to throw errors defined in interfaces using a contract implementation as a proxy (currently done in SafeERC20 → ERC20). The downside IMO is that you are bringing a possibly big contract that the compiler will a to process, and that will appear in the verified source code even though it may not actually be used.
  • An alternative is to use an abstract contract that contains just the errors, but that is very similar to the third point (library)
• One can call an error defined in a library using the revert Library.Error() syntax. It has similar downside to the "contract" approach, except that library tends to be smaller, with fewer dependencies, so the "bloat" is reduced.

In our codebase, we use (custom) errors in different cases:

• In contracts that inherit a public (IMO immutable) interface such as ERC20 → IERC20. I think we agree on the immutability of the IERC20 interface, which is why we came up with ERC6093.
  • In that case, the current PR either creates a new interface, or add the error to existing interfaces when they are not standard.
• In our libraries
  • In that case we either declare the errors in the library itself, or use interfaces declared elsewhere in an interface using a "proxy" implementation contract.

IMO there is real value to having clear "error" libraries, as it would improve reusing the errors in helper libraries (SafeERC20), or between contracts if/when that makes sens. All the libraries errors are already declared within the libraries, which IMO is good! Now its just a matter of moving the errors declared in contracts to dedicated libraries.

Example n°1:

I would propose that in the contracts/governance folder, next to Governor.sol and IGovernor.sol we have GovernorErrors.sol that would contain library GovernorErrors {...} and could be reused accross the code.
In particular, this file would declare all the errors for the Governor system, including the one that are used in modules but not in the core. Since 0.8.20, only the ones actually used would be part of the compiled in the ABI. So no more errors scaterred all over the modules with possible duplicates, and cleaner ABIs for the contracts.

Example n°2:

For ERC20 (and the other tokens), I would add a contracts/token/ERC20/ERC20Errors.sol that contains library ERC20Errors {...}. Note that while it doesn't match the reference implementation of ERC6093 (which we could update) it complies with the ERC, because nowhere does the ERC state that the errros must be in an interface (in the end they would be in the contract ABI just the same).
The contracts/interfaces/draft-ERC6093.sol could be built similarly to to other in this repo

pragma solidity ^0.8.0;

import "../token/ERC20/ERC20Errors.sol";
import "../token/ERC721/ERC721Errors.sol";
import "../token/ERC1155/ERC1155Errors.sol";

What do you think?

EDIT: my point includes a direct consequence, the pragma should be no lower than 0.8.20 so the custom errors get in the ABI