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