Openzeppelin-contracts: Add reason string for all assertions/requires

[spalladino]

The next solc version introduces support for reason strings for error messages. As soon as this release is available, migrate to that version and annotate all requires and asserts in the OpenZeppelin codebase with meaningful error messages.

• 📈 This is a feature request.

See https://github.com/ethereum/solidity/pull/3364

[elopio]

This was released on solidity 0.4.22. It can be included in openzeppelin now.

[pmosse]

@elopio @spalladino I can work on this. Do you have any guidelines for the messages (for example to make them as short as possible)? Or should I do a PR with a couple of sample cases so we can check?

[elopio]

@pmosse we have no guidelines yet, so why don't you go ahead and propose some? We can discuss on the PR.

[pmosse]

@elopio Done!

[shrugs]

I'd like to bring up the problem of having english error messages as return values instead of, say, status codes via https://eips.ethereum.org/EIPS/eip-1066

[shrugs]

At the very least, errors should be referred to as constants, imo

[pmosse]

Good idea. English error messages have more specific information but status codes facilitate automation. Should we have a global list of status codes constants? We can extend https://github.com/Finhaven/EthereumStatusCodes with others such as "Not Enough Funds". @elopio what are your thoughts on this?

[elopio]

Sorry for the late reply, I had to read the EIP. omg so many eips!!!
The idea sounds very nice, but it also sounds like it will take a long time for the community to agree on the form and the codes. We can update to that format once it's accepted.

About using constants, I have weird feelings towards constants that are upper-cased copies of strings, like:

ERROR_MESSAGE_NOT_ENOUGH_FUNDS_TO_DO_PAYMENT = "Not enough funds to do the payment";

If we have repeated messages, it could be useful. But from your PR we can't tell if we'll repeat any of them. I think it could be a premature optimization, but I'm not against it. If @shrugs wants them, go ahead.

We can also ask @frangio, he might have a strong opinion.
Anyway, your pr is a nice addition and we can iterate on it until it's perfect :) Thanks for that.

[shrugs]

I say it's worth waiting on the status codes; if we ship english descriptions, it'll create a bad precedent. Good point on the unnecessary variables, though, yeah; I can see how that wouldn't actually help much.

[frangio]

No strong opinion about constants unless the error messages are repeated.

I haven't been through the EIP in detail. Is there anything we can do in the meantime to improve devs' experience without harming the progress of the EIP by setting this bad precedent?

[pmosse]

@frangio We could limit the error messages to the status codes available in https://github.com/Finhaven/EthereumStatusCodes, or extend that list with some new codes. But if the codes are subject to change, we can't ensure forward compatibility. In that case it is worth it waiting.
About using constants, some error messages will be repeated but in different contracts.

[elopio]

On good http APIs you get an error code, and a human readable message. From that specification I'm missing the message, which is what we wanted to add here.

[frangio]

That seems to be true... We don't need the status codes necessarily, only the format. There is probably some discussion about that in the ethereum/solidity repo.

[shrugs]

Was talking with Ben from truffle about this today; generally a status code is a general error condition so your client can react accordingly (by retrying, redirecting, giving up, prompting the user to login, etc). The protocol itself doesn't dictate how error messages are transferred, so different layers on top of that (like REST, GraphQL) return errors differently.

But I expect we'll need to emulate a lot of that functionality in the status codes proposal as well. I'll go into more detail about this on that EIP.

[frangio]

We like the reason strings that the 0x project is using. For example:

require(
    currentAssetProxy == address(0),
    "ASSET_PROXY_ALREADY_EXISTS"
);

[nventuro]

Should we also add tests for these strings, to make sure they are not altered by mistake?

[frangio]

@nventuro 100%!

[nventuro]

The time has finally come to work on this! I'm closing this issue in favor of https://github.com/OpenZeppelin/openzeppelin-solidity/issues/1709, where we should track progress, further discuss any issues that arise, etc. Thanks everyone for your help!