Added message string for require()

[balajipachai]

Enhancement #
Added message string for require statements.

Many a time it happens, a transaction reverts with the message
Error: VM Exception while processing transaction: revert, for reasons such as trying to access a variable that is not in existence, trying to call a contract's function where the contract's address has been address(0) and not yet initialized etc.

The ways to find the reason for revert may include:

• Adding events in the smart contract which emit the variable's value, catching them using web3js, finally logging them on the console to check the variable's value, which is time consuming and inefficient.

After the addition of message string for require statements, one can easily find the point of revocation and work on fixing the same.

The below approach has been used for adding message string

• If there is only 1 require statement in a function then the message string is:
  "from OpenZeppelin's:FileName.sol:functionName()."

• If there are multiple require statement in a function then the message string is:
  "from OpenZeppelin's:FileName.sol:functionName. Cause of the revert."

[nventuro]

Hey there @balajipachai! We have an outstanding issue open for this (#888), which we plan on tackling during these couple of days. I'm currently doing some testing regarding gas costs, user experience, etc., and trying to figure out what the best approach is for the messages themselves.

I think we can safely skip the 'from OpenZeppelin' bit, and just include the contract name: I'd expect that to be enough to pinpoint the offending contract. Your suggestion to only include the function name is intriguing, since it'd make it easy to look into the code and find the root cause of the revert. That said, I think I favor having a more descriptive message (e.g. null transfer recipient, closing date in the past, etc.), so that, at least in some cases, we can skip the going back to the library source code. We'd need to take care not to repeat messages inside a contract, but I think that's not a tall order. @frangio tagging you so you can share your thoughts regarding this.

[balajipachai]

@nventuro
Even though adding just the functionName() might be intriguing, however, since it is the case for functions with only 1 require statement, I thought of the aforesaid approach, and, yes, we can include descriptive text highlighting the cause of the error, this too can be accommodated.
Should I remove the from OpenZeppeling's from the message string?

[nventuro]

To prevent work duplication, I'd rather we settle on a format first, so that everyone can then later contribute by adding these reason strings and associated tests (which are quite a lot). Please, come join the discussion on our forum and share your thoughts!

[nventuro]

@balajipachai I opened #1709 with some guidelines regarding message format, among other things, including some examples. Could you review it and update your PR accordingly? Thanks!

[balajipachai]

@balajipachai I opened #1709 with some guidelines regarding message format, among other things, including some examples. Could you review it and update your PR accordingly? Thanks!

@nventuro Sure, let me have a look at those guidelines and I'll update my PR accordingly.

[balajipachai]

@balajipachai I opened #1709 with some guidelines regarding message format, among other things, including some examples. Could you review it and update your PR accordingly? Thanks!

@nventuro Sure, let me have a look at those guidelines and I'll update my PR accordingly.

@nventuro Updated the PR as per #1709, please have a look and let me know in case of any changes.

[balajipachai]

@nventuro Awaiting your reply, once you give a heads up, I can start with the changes in the test cases too for shouldFail.reverting.withMessage.
Thanks!!!

[nventuro]

Hello @balajipachai, thank you for your contribution! I left a couple comments to improve some of the messages :) I also noticed some of them are empty, did you not know what to put in those?

I may do a couple myself to serve as a guideline, since I fear the forum post may not be enough.

[balajipachai]

Hello @balajipachai, thank you for your contribution! I left a couple comments to improve some of the messages :) I also noticed some of them are empty, did you not know what to put in those?

I may do a couple myself to serve as a guideline, since I fear the forum post may not be enough.

I didn't notice what was left empty, my bad, however, as per your guideline, I have started to make changes and will update the PR shortly.
Thanks!!!! for your feedback, appreciated :)

[balajipachai]

@nventuro The PR has been updated as per below guidelines:

• The require messages now describe what went wrong as opposed to the requirement.

• Symbols have been removed and instead, words are used.

Note
If there is require(to != address(0)) in _burn() as well as in _transferFrom() in ERC20.sol
Below convention is used:
require(to != address(0), 'ERC20: to is address(0) in _burn()')
require(to != address(0), 'ERC20: to is address(0) in _transferFrom()')
This will help us to verify, exactly, which to != address(0) caused the revert.
The same thing has been applied to all such similar cases.

[nventuro]

Hello @balajipachai, thank you very much for your improvements! I did some changes to the revert strings, generally either making them more terse, or changing implementation details for the actual reason for failure.

Could you add these strings to the test suite, to assert the actual failure reason? Thanks!

[balajipachai]

Hello @balajipachai, thank you very much for your improvements! I did some changes to the revert strings, generally either making them more terse, or changing implementation details for the actual reason for failure.

Could you add these strings to the test suite, to assert the actual failure reason? Thanks!

Sure @nventuro , I'll add those changes and update !!!

[balajipachai]

@nventuro I have started working on the shouldFail.reverting.withMessage, however, I have noticed that :
In shouldFail.js: shouldFail.reverting.withMessage = withMessage;
The above statement in turn calls the shouldFailWithMessage(promise, message)
if (1 in matches && semver.satisfies(matches[1], '>=2.2.0'))
and the above if condition fails since
const matches = /TestRPC\/v([0-9.]+)\/ethereum-js/.exec(nodeInfo); matches == null

In order to be sure about shouldFail.reverting.withMessage is working as expected I did a way around for testing purpose only and modified the below line:
shouldFail.reverting.withMessage = withMessage;
to
shouldFail.reverting.withMessage = shouldFailWithMessage;
And the results were as expected. The test cases are passing, however, this is not the right way.
Guide me on getting things done via
shouldFail.reverting.withMessage = withMessage;
Thanks !!!

[nventuro]

@balajipachai That's weird, you shouldn't be having any issues with that if statement. It is there to check if the ganache version that is being used supports revert reasons (introduced in ganache-core after v2.2.0), and our package.json specifies ganache-cli v6.4.1, which bundles ganache-core v2.5.3.

If you have an older version installed, you could delete node_modules and rerun npm install. Try also running $ npx ganache-cli --version and see what the versions are for your setup. I tested locally and got the helper working just fine.

[nventuro]

@balajipachai I've now reviewed and updated all reason strings.

[balajipachai]

@nventuro Thanks!!! Test cases updation is in progress 👍

[nventuro]

@frangio could you take a look at the reason strings?

[nventuro]

Great work @balajipachai, this must have taken quite a while to get done! I left a couple comments, but most are very minor, we should be able to merge this very soon!

[frangio]

Awesome work everyone!

[balajipachai]

@nventuro & @frangio I have gone through both of your comments, Thank You both for your feedback.
Please bare me some time, to accommodate the required changes.

[balajipachai]

Great work @balajipachai, this must have taken quite a while to get done! I left a couple comments, but most are very minor, we should be able to merge this very soon!

Thank You @nventuro

[nventuro]

@balajipachai sorry we forgot to mention this, but we did some of the changes ourselves in the interest of getting this merged for the release, and to relieve some of the load from you, who has already done so much :)

Thanks a lot everyone!

[balajipachai]

@nventuro It's been more than couple of days, yet, I don't see my name listed amongst the Contributors.
😅😅😅😅

[nventuro]

Where are you looking for that? I do see you on the list:

[balajipachai]

I am looking at this link:
https://github.com/OpenZeppelin/openzeppelin-solidity/graphs/contributors

Could you share the link where my name is listed?

[nventuro]

That very same link, on number #56 when sorting by commits (PRs) made.

[balajipachai]

That very same link, on number #56 when sorting by commits (PRs) made.

Thank You 👍