Split out custom errors into separate interface

[KholdStare]

🧐 Motivation
We have been using custom errors for over a year in our contracts and have found some beneficial usage patterns. The most important is splitting out just the custom errors into their own interface so they can be re-used by final derived contracts.

With revert statements with strings, the errors were self-describing and did not need to be added to a contract's interface. Since custom errors have to be decoded by a user, all possible errors have to be known in advance. Some approaches that have not worked:

• Convince the solidity compiler to "bubble up" all the errors to the top: Include possible custom errors on ABI ethereum/solidity#13683 . This was rejected.
• Convince tools like Foundry to generate the set of possible errors: Support of custom-errors on ABI foundry-rs/foundry#3656 . This is still pending

At the end of the day it's up to the writers of the final contracts to ensure they accurately reflect what errors can be raised so users can effectively decode them. By splitting out just errors into their own interface, it is now possible for the final contract to inherit all the error interfaces for all direct and transitive dependencies. It is manual, but better than not specifying anything and leaving users of the final contract in the dark.

📝 Details

As an abstract example:

// Before
interface IFoo {
  error FooError(uint256 foo);

  function foo() external;
}

contract Foo is IFoo { };

contract FooCaller {
  Foo private foo;

  function useFoo() external {
    foo.foo(); // can revert with FooError, but FooCaller does not include it in its interface
  }
};

// After
interface IFooErrors {
  error FooError(uint256 foo);
}

interface IFoo is IFooErrors {
  function foo() external;
}

contract Foo is IFoo { };

contract FooCaller is IFooErrors {
  Foo private foo;

  function useFoo() external {
    foo.foo(); // FooError is part of the interface
  }
};

As you can see FooCaller's ABI accurately reflects the fact that FooError can be raised by calling the contract's functions. For OpenZeppelin, it should be a backward compatible change. The errors can be extracted out into their own interfaces and the main interfaces inherit from the error interface. The interfaceId for ERC165 detection should also not be affected, since errors do not affect the interfaceId.

[ernestognw]

Hey @KholdStare, thanks for opening the issue.

From the Solidity issue, I see the main argument against including the errors in the ABI is that it's not possible to determine all the possible thrown errors at compile time. In my opinion, it makes sense since an external call may throw any error (especially when bubbling up errors).

However, I see value in keeping the errors in a separate interface, but this is for standardization purposes. As an example, the ERC6093 errors are split in interfaces, but this is because they're standardized interfaces as opposed to almost every other error in the library which follow the same pattern but isn't standard.

Currently, I wouldn't feel comfortable porting every error to interfaces. We discussed in the past about ensuring API stability for custom errors but we decided not to commit to that yet because it's likely that errors will evolve (eg. adding parameters or information needed that we probably missed). Although we haven't seen meaningful changes required to custom errors, it may be too early to ensure error interface stability.

To be clear, I don't think we should discard this proposal but I think it requires more discussion. In the meantime, I'd be supportive of Foundry adding an option to show every custom error from the AST.