Openzeppelin-solidity requires do not have string "reason" messages

General
1

I'm curious, why is it that require statements inside the OZ-solidity contracts don't have a string "reason" message? In practice, this seems to make it so that things can be confusing to debug

2

I think I can give you an answer for this. The problem is that those messages (let’s call them reason strings in require statements are very expensive in terms of bytecode usage. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive

2 Likes
3

That makes a lot of sense @madjarevicn. Thanks for responding!

It makes sense that OZ, being a library of low-level base contracts, would want to hyper-optimize for gas usage. However these reason messages would be super handy. It’d be awesome if there were some mechanism to have require “reasons”, but strip them out out as a separate step before shipping to a real chain.

4

To be honest we have wanted this for some time but just haven’t got around to it. Performance is a concern, yes, but it was more a matter of having the time to do it.

2 Likes
5

@zachlysobey we’ve long postponed this, but plan on tackling it as part of v2.3 (which should be coming out in about a month): see the corresponding GitHub issue here.

I share @madjarevicn’s concern regarding gas usage though - it’d be great if we could strip all require reasons before production deployment, but keep them while testing (similar to how other languages/build systems have Debug and Release settings). I think eventually we’ll have to introduce some sort of code-autogeneration into OpenZeppelin, but I’d like to postpone that moment as much as possible. An option I hadn’t considered until today is having Solidity itself be able to remove these strings: I’ve opened an issue in their repo to discuss this.

2 Likes
6

Before moving forward with actually adding the revert reasons throughout the project, a pending task is defining the format and tone they will have. This was what initially blocked us.

What do you guys think about the style used by 0x?

4 Likes
7

I like that style quite a bit; it makes the reasons easy to map to an enum in a web3 application hooking into the contracts.

1 Like
8

Hi, I’m glad to see this is being addressed. I need all the require() error messages for a project I’m working on now, but can’t wait for OZ 2.3 to come out. Assuming that the work has been done but just hasn’t been integrated yet… to avoid duplicating work, is there a repository with only these changes that I could make use of right now?
Thanks!

  • DJ
9

About tone and style, I would simply consider the audience who would be viewing the error, as the error only really needs to guide the user as to what they need to do to resolve the problem. For example, if a math underflow were to happen during transfer(), an error saying “Insufficient funds.” I think would be good enough. Certainly way better than having no message at all!
Thanks,

  • DJ
10

Speaking of this, where can I find information about the timing of the OZ 3.2 release?

11

It hasn't :frowning: I intend for this to be one of the first things we work on for the upcoming release, but for now we still have to settle on a message format. Contributions are more than welcome :slight_smile: That said, I'd usually advice against copying code from the repo directly, and suggest only working with published versions.

An issue to consider is that an error may happen way down the call stack, so a message saying 'integer overflow' may not be very useful when e.g. participating on a crowdsale. I don't think we'll be able to address this though with just these messages.

Our release cycle post has the information you're looking for. In the case of v2.3, it is scheduled to come out on April 20th.

12

Hi! I also like the format used by 0x. I think you are doing a great job in Zeppelin and it would be a pleasure for me to help and contribute to this issue. Of course when the decision on the message format will be made.

5 Likes
13

Does anyone have experience debugging with 0x-style revert reasons? I initially liked them because of the reasons mentioned earlier in this thread, but now I’m thinking that more human-like sentences will result in clearer error messages and a better developer experience.

Anyway, thanks everyone for the comments! We will be analyzing this issue in our current sprint. Hopefully we’ll have some guidelines next week for error messages and anyone interested will be able to contribute adding them throughout the contracts!

1 Like
14

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.

3 Likes
15

Great! Thank you.
Wishing you all the best with this release.

16

I have one additional thought on this thread, which has come from personal experience writing ProxyToken. It has to do with the simple matter that storage of error message strings costs gas. Error messages longer than 32 bytes should be avoided at all cost, and in general, I think error messages should be as brief as possible with an emphasis on conserving space, but without loss of meaning. Error codes or very short phrases would work best to this end, with lengthier variants of the messages being available for lookup by the user from another lest costly resource (such as the online docs), or perhaps programatically from somewhere else on the blockchain by another smart contract.

This is not about being cheap about using gas, though that is one factor. The bigger issue is that in development of our ProxyToken contract, we ran smack into the block gas limit for simply deploying the contract. Yes, it costs 6720000 gas just to deploy it! For a brief period it was actually too big to deploy (gas required to deploy exceeds block gas limit) until… you guessed it, I shortened all our error messages.

1 Like
17

Is this because after 32 bytes they will take two storage slots instead of one?

Yes, this worries me somewhat. However, our intent for reason strings is for them to help during development, and not necessarily in production code (where I'd expect other, more robust measures), which is why I suggested the Solidity team to provide an option to remove them from compiled code.

1 Like
18

@nventuro You missed the ERC721. Also, These require statement like:
require(_exists(tokenId), require(to != msg.sender), require(ownerOf(tokenId) == owner), require(ownerOf(tokenId) == from), require(_isApprovedOrOwner(msg.sender, tokenId)), require(to != owner), require(msg.sender == owner || isApprovedForAll(owner, msg.sender)).
These are different from require(to != address(0)). So, we need to guideline these too. Like:

  • require(_exists(tokenId): tokenId doesn't exist
  • require(to != msg.sender): sender can't be receipent
  • require(ownerOf(tokenId) == owner): can only performed by owner
  • require(ownerOf(tokenId) == from): can only performed by owner
  • require(to != owner): receipent must be owner

can you please describe the error message for these:

  • require(_isApprovedOrOwner(msg.sender, tokenId)):
  • require(msg.sender == owner || isApprovedForAll(owner, msg.sender)):
1 Like
19

Unless the same require applies to all cases, I'd also include for which action the recipient is being targeted, e.g.: transfer recipient must be owner, mint recipient must be owner, etc. In this case, it probably should be transfer recipient must not be current owner (I think you missed the != :stuck_out_tongue:)

Hm, we could do a detailed message (e.g. sender is not owner nor is approved, like the MinterRole one (sender doesn't have the MinterRole), but now I think it may be better to simply say something like sender lacks permission, and leave out what 'having permission' means for that contract (i.e. owner or approved on ERC20, owner, operator or default non-revoked operator on ERC777, role-haver on roles, etc.). What do you think?

1 Like