Openzeppelin-contracts: A logical error in assertRevert.js

[barakman]

🎉 Description

• [x] 🐛 This is a bug report.
• [ ] 📈 This is a feature request.

💻 Environment

Agnostic (platform-independent)

📝 Details

If the promise is completed successfully, then you throw an exception with an error message containing "revert".
You then immediately catch this exception, and since its error message contains "revert", the assert completes successfully and no exception is thrown.

The thing to understand is that the Mocha framework reports a failure only when the tested scenario (it) throws an exception.
When that happens, the Mocha framework catches this exception and reports failure on the specific test at hand.
When you throw an exception and immediately catch it, the Mocha framework is not even aware of it.

For example, let's analyze a scenario in which the promise completes successfully, hence the test should fail:

• In the try part, the promise completes successfully
• In the try part, you throw an exception with "revert" in its error message
• In the catch part, you immediately catch that exception
• In the catch part, the assert completes successfully because the error message contains "revert"
• No exception is thrown, and the test does not fail

In order to understand the logical confusion, keep in mind that there are two different failure scenarios which you need to detect and handle:

1. No error has occurred (the promise has completed successfully)
2. An error without "revert" has occurred (the promise has failed but not because of require)

In your code, I believe that you detect the first scenario correctly, but you handle it incorrectly.

I therefore suggest the following alternative:

try {
    await promise;
    throw null;
}
catch (error) {
    assert(error, `Expected an error but did not get one`);
    assert(error.message.includes("revert"), `Expected an error containing "revert" but got "${error.message}" instead`);
}

[frangio]

Thanks for the detailed report @barakman. I understand your reasoning and it made me quite concerned. However, in practice it doesn't seem to be like this.

If you try to trigger the bug by, for example, removing a require line from some contract, you will see that the tests do correctly detect it. The AssertionError object that is caught will have these fields:

  message: 'assert.fail()',
  actual: 'Expected revert not received',

Thus, error.message will not contain "revert".

I still haven't looked deeper into why this is so. The assert that we're using does not seem to be Node's, but one injected by Truffle.

I'm going to keep looking into this later. I've wanted to improve this helper for a while now. Suggestions are appreciated!

[barakman]

@frangio:
NP, but if that is indeed the reason (using some proprietary version of assert), then I would strongly recommend that you change it to the code I suggested above (which is also cleaner IMO, but that debatable I suppose).
Thanks!

BTW, I've implemented this as a single helper for all "VM exception" errors:

module.exports.errTypes = {revert: "revert", invalidOpcode: "invalid opcode", outOfGas: "out of gas", invalidJump: "invalid JUMP"};

module.exports.tryCatch = async function(promise, errType) {
    try {
        await promise;
        throw null;
    }
    catch (error) {
        assert(error, "Expected an error but did not get one");
        assert(error.message.includes(errType), "Expected an error containing '" + errType + "' but got '" + error.message + "' instead");
    }
};

Perhaps it will inspire you to do something similar....

[cgewecke]

@barakman @frangio Truffle injects chai assert.

[barakman]

@cgewecke:

So is my analysis wrong then?

[cgewecke]

@barakman I think I would phrase this - luckily it works!! :) Really think the issue in that thread at truffle is related to block.timestamp which is always a little tricky.

[barakman]

@cgewecke:

I completely lost you on this one.

Any chance you're talking about the other thread, where I originally posted this issue? (I went into that thread while looking for a remedy on the infamous ganache issue BTW, and from there I "accidentally" continued to the assert_revert.js file on the related GIT repository (and from there eventually to the assert_revert.js file on this repository)).

I'm asking strictly with regards to Zeppelin's assertRevert.js - @frangio claims that it works because truffle uses some other assert. You said (or implied) that it is not the reason.

So I am asking if my analysis of a problem in Zeppelin's assertRevert.js is wrong.

Thanks

[cgewecke]

@barakman Zeppelin's works because the chai error has the structure @frangio identified.

[frangio]

@cgewecke in fact confirmed what I said. Your analysis was likely using Node.js assert, whereas Truffle uses Chai assert.

[barakman]

@cgewecke , @fabioberger:

A(nother) good reason to change the implementation there if you ask me :)