Reason strings for all require statements

[nventuro]

Supersedes #888.

This is way overdue, but a big push during these last couple weeks for it took it to the point where we're now comfortable to include it in the library.

There was some discussion about formatting in our forum, which culminated in this post, which has some rough guidelines (pasting at the end of this message for reference).

I also did some testing and verified that we can safely add revert reasons to all of OpenZeppelin's contracts, without them going over the gas limit for a block. The situation may differ for setups with particularly large contracts, but most users should be unaffected. Nonetheless, I opened an issue on the Solidity repo suggesting they provide a way to remove these strings, so that they can be stripped for production builds, if desired.

Our goal is to have all require statements also include a human-readable revert reason, unique across all of OpenZeppelin. Contributions are welcome! Please also update the tests with the shouldFail.reverting.withMessage test helper, so that we can both be sure of the actual revert reason, and prevent regression errors by having these messages change.

Rough guidelines:

I think there's a fine line between machine-like messages and having bits of Shakespeare embedded in a smart contract. Some of 0x's messages are quite terse, but then again, in some cases the revert will take place deep down the stack (e.g. you'll get a SafeMath: subtraction overflow instead of ERC20: insufficient balance for transfer, or even LimitedCrowdsale: all tokens have been sold), and it'll be up to the developer to figure out how the call got there. As long as each message is unique in their contract, and they are descriptive enough so that one can get an idea of what the revert means (in the context of that contract) without looking at the code, we should be fine.

Here are some examples I think are aligned with this guideline:

• SafeMath: multiplication overflow
• SafeMath: division by zero
• ERC20: null transfer recipient
• ERC165: invalid interface id
• MinterRole: caller doesn't have the Minter role
• ConditionalEscrow: withdrawal disallowed
• RefundEscrow: can only close when active
• RefundEscrow: can only withdraw when closed
• PaymentSplitter: caller has no shares
• PaymentSplitter: empty payment

Note that some are a bit different (like the RefundEscrow ones) since they originate from a statement such as require(_state == State.Closed), in which the message cannot describe why the performed call is wrong since the state cannot be returned (e.g. cannot withdraw while active), so we must resort to simply reporting the conditions for a successful call.

[balajipachai]

@nventuro I have started working on it :)

[princesinha19]

@nventuro what about ERC721. I think it should be the same as ERC20: null transfer recipient

[princesinha19]

@nventuro what about other requtire statement other than require(owner != address(0)). Like require(_exists(tokenId)) Should I write token doesn't exist?

[nventuro]

@nventuro what about other requtire statement other than require(owner != address(0)). Like require(_exists(tokenId)) Should I write token doesn't exist?

I think so, yes (unless we can also add token to transfer doesn't exist, for mint, transfer, burn`, etc.).

[frangio]

ERC20: null transfer recipient

I thought we had agreed not to use the word "null" for the zero address. It's a bit confusing because in the context of Ethereum transactions a null value in the to field has a special meaning (makes it a contract creation transaction).

[nventuro]

zero-address transfer recipient? A bit of a mouthful.

[frangio]

What about just "zero" or "empty"?

[princesinha19]

What about 'empty or zero transaction recipient'.

[nventuro]

Closed via #1704.