Hello community,
Since the introduction custom errors in Solidity in v0.8.4, there’s now a more expressive and gas-efficient way of reverting changes during a transaction.
Given this new addition, we’re proposing a list of standard errors to be used for the standard tokens (ERC20, ERC721, and ERC1155), so the clients and implementers can expect an insightful and structured way from a transaction error.
This is a very good initiative. For the curious out here, it will be interesting to have a reference of the bytecode savings for switching from old revert strings (for example in ERC20) to error messages.
In the Rationale section for domain; perhaps the contract name itself could be suggested to help with the compiler DeclarationError in situations where the ErrorPrefix and Subject are the same.
This is a very good initiative. For the curious out here, it will be interesting to have a reference of the bytecode savings for switching from old revert strings (for example in ERC20) to error messages.
Thanks for your comments! Actually, I just made a quick repo test for comparing the gas savings.
You can find them here.
Look at the differences between custom errors and short strings vs long strings, which are the majority of the cases
TLDR is that EIP-6093’s custom errors are better in gas usage than general revert strings unless for reverts with empty strings.
In the Rationale section for domain ; perhaps the contract name itself could be suggested to help with the compiler DeclarationError in situations where the ErrorPrefix and Subject are the same.
I see what you mean. Although it can solve the DeclarationError, it’ll also change the error selector depending on the contract name, and that might affect standardization overall (eg. Metamask would need to know the contract name just to calculate the selector and show a proper error message in the UI).
What do you think?
Very nice report on the gas savings.
In my opinion, the change to custom errors should become the norm on new deployments of these common token implementations.
Assuming this makes its way to an OpenZeppelin implementation, would this change be implemented directly in the existing token contracts, for instance: ERC20, or would it be within the ./extensions folder?
We aim to implement these changes in OpenZeppelin Contracts for the next 5.0 version, so yes, it’s expected to make into OZ’s ERC20 implementation.
Also, the same rationale is going to be used for other errors, see this discussion.
I am wholeheartedly in favor of this, this is an awesome proposal. After reading the entire EIP, this is my feedback:
I am a bit confused by this statement in the “Specification” section:
This EIP defines standard errors that may be used by implementations in certain scenarios, but does not specify whether implementations should revert in those scenarios
But then you go on and say something like this underneath each error:
MUST be used when …
I might be getting the wrong end of the stick when it comes to what “MUST” means in this context, but don’t the statements above contradict one another?
Happy to see my proposal to use the name of the contract (in this case, the EIP number) as a custom error prefix. However, I like it better when the prefix is separated by an underscore, so that the contract name gets separated from the rest of the custom error name. Underscores in Solidity have become popular with the advent of Foundry (see the references to test naming here)
Here’s an example for what I mean:
error ERC20_InsufficientBalance(address sender, uint256 balance, uint256 needed);
error ERC20_InvalidSender(address sender);
error ERC20_InvalidReceiver(address receiver);
error ERC20_InsufficientAllowance(address spender, uint256 allowance, uint256 needed);
error ERC20_InvalidApprover(address approver);
error ERC20_InvalidSpender(address spender);
I wonder if it isn’t a bit too restrictive to demand that implementors MUST revert when the subject is the zero address (e.g. ERC721InvalidSender)? I personally never had a need for allowing transfers to and from the zero address, but I also can’t think of any good reason why this should be prevented at the EIP level (besides maybe preventing fat-finger errors?).
Besides the points above and a few minor wording suggestions I left in this PR, the EIP looks great to me. I can’t wait for OpenZeppelin v5 to be out.
Thanks for your feedback @PaulRBerg, I’m glad the proposal makes sense to you.
Let me answer your comments:
but don’t the statements above contradict one another?
In a sense, yes. We discussed this internally and realized there’s no way of making every previous token implementation use these errors always (backwards compatibility), so we can’t say MUST since it’s not even possible.
Still, we put MUST to highlight the absolute requirement of using one of the standard errors when its characteristics are those of an EIP-6093 error.
What do you think would be the most accurate approach?
Underscores in Solidity have become popular with the advent of Foundry (see the references to test naming here)
I think the test reference is becoming popular when it comes to testing, I haven’t seen many verified contracts adopting the mix between PascalCase and snake_case (I’ve seen double __, tho).
I searched for a reference, and the Solidity docs guide doesn’t include such a case.
https://docs.soliditylang.org/en/v0.8.17/style-guide.html#naming-styles
It also adds reasons to turn off Solhint, which may be dangerous for newbies.
I wonder if it isn’t a bit too restrictive to demand that implementors MUST revert when the subject is the zero address
This is also related to the ambiguity in the Must vs Must not category. The idea is that errors MAY be added, but when they’re added, they MUST be used for the described cases.
In any case, EIP-712 and EIP-1155 explicitly state zero address reverts, so the idea is to cover those cases in which the original EIP requires them to revert.
We also thought about a ZeroAddress() standard error but we think it loses important context information. For example, does ZeroAddress means canceling an action?, is the ZeroAddress coming from a bad implemented ecrecover? etc.
How do you see the zero address case addressed?
Besides the points above and a few minor wording suggestions I left in this PR
Thanks! I just approved 
Get rid of that statement? It’s super ambiguous. I think that it is an implicit assumption that EIP-6093-compatible token implementations will NOT be backward compatible with older implementations.
Rewrite the language to say something like this:
Fair enough!
Oh, I didn’t know this. Makes sense to also apply this rule to EIP-20, too, then.
I agree.
I was just thinking out load. Given your answers, I’m happy to keep it as is.
Thanks for your answers and for merging the PR!
Any reason this isn’t moved forward?
It’s going to be included in OpenZeppelin Contracts 5.0 next summer’s release, but we want to hear you out if there’s any feedback. Aside from that, the EIP reviewers will eventually peer-review it, but it takes some time.
It would be nice also to have custom errors for the commonly-used ERC-2612 “permit” function. The goal is to enhance the error handling and user experience when utilizing the permit function for approving token transfers. ERC-2612, also known as the “permit” function, has gained widespread adoption in Ethereum because of its gas efficiency. Additionally, the OpenZeppelin library provides an ERC20permit extension, to ensure consistency and compatibility across projects, it is essential to define a set of custom errors for the permit functionality. Having custom errors aligned with the standard would greatly benefit developers.
Specification: The proposed custom errors for ERC-2612 are as follows:
Implementation
interface ERC2612Errors {
error ERC2612ExpiredDeadline(uint256 deadline, uint256 blockTimestamp);
error ERC2612InvalidSignature(address owner, address spender);
}
Hey @mattiascaricato, thanks for your feedback!
These are already two considered errors we have on the current custom errors Pull Request for OpenZeppelin Contracts, you’ll see we have:
/**
* @dev Permit deadline has expired.
*/
error ERC2612ExpiredDeadline(uint256 deadline);
/**
* @dev Mismatched signature.
*/
error ERC2612InvalidSignature(address signer, address owner);
However, I think we might not want to decide on extensions to the base token implementations since we’re still agreeing on what would be the best way to create a common errors library in which these two errors may fit.
In any case, the criteria we’re using for naming tokens is that if an error is directly derived from an ERC specification, it should be prefixed by ERC<number>.
Curious to know, what’s your criteria for including block.timestamp as part of the arguments. I think it’ll be implicit in the block, do you see a strong reason to keep it?