OpenZeppelin / openzeppelin-test-helpers

Assertion library for Ethereum smart contract testing
https://docs.openzeppelin.com/test-helpers
MIT License
415 stars 132 forks source link

ExpectRevert breaks nonce calculation in truffle tests #154

Closed illuzen closed 2 years ago

illuzen commented 3 years ago

Everything is fine until I await expectRevert and then the nonces are always off by 1. Manually specifying {nonce: (await web3.eth.getTransactionCount(account)} fixes it, but is annoying.

abcoathup commented 3 years ago

Hi @illuzen!

I’m sorry that you had this issue. We would need more information so that we can reproduce it.

Unfortunately, I wasn’t able to reproduce this issue. I tried the following: I reordered the test from the Learn guides to use expertRevert prior to other tests but didn't have any errors: https://docs.openzeppelin.com/learn/writing-automated-tests#performing-complex-assertions

// test/Box.test.js
// Load dependencies
const { expect } = require('chai');

// Import utilities from Test Helpers
const { BN, expectEvent, expectRevert } = require('@openzeppelin/test-helpers');

// Load compiled artifacts
const Box = artifacts.require('Box');

// Start test block
contract('Box', function ([ owner, other ]) {

  // Use large integers ('big numbers')
  const value = new BN('42');

  beforeEach(async function () {
    this.box = await Box.new({ from: owner });
  });

  it('non owner cannot store a value', async function () {
    // Test a transaction reverts
    await expectRevert(
      this.box.store(value, { from: other }),
      'Ownable: caller is not the owner'
    );
  });

  it('retrieve returns a value previously stored', async function () {
    await this.box.store(value, { from: owner });

    // Use large integer comparisons
    expect(await this.box.retrieve()).to.be.bignumber.equal(value);
  });

  it('store emits an event', async function () {
    const receipt = await this.box.store(value, { from: owner });

    // Test that a ValueChanged event was emitted with the new value
    expectEvent(receipt, 'ValueChanged', { newValue: value });
  });
});
// contracts/Box.sol
// SPDX-License-Identifier: MIT
pragma solidity ^0.6.0;

// Import Ownable from the OpenZeppelin Contracts library
import "@openzeppelin/contracts/access/Ownable.sol";

// Make Box inherit from the Ownable contract
contract Box is Ownable {
    uint256 private value;

    event ValueChanged(uint256 newValue);

    // The onlyOwner modifier restricts who can call the store function
    function store(uint256 newValue) public onlyOwner {
        value = newValue;
        emit ValueChanged(newValue);
    }

    function retrieve() public view returns (uint256) {
        return value;
    }
}
$ npx truffle test

Compiling your contracts...
===========================
> Compiling ./contracts/Box.sol
> Compiling ./contracts/Migrations.sol
> Compiling @openzeppelin/contracts/access/Ownable.sol
> Compiling @openzeppelin/contracts/utils/Context.sol
> Artifacts written to /tmp/test--20606-5TSlWZeRaJpr
> Compiled successfully using:
   - solc: 0.6.12+commit.27d51765.Emscripten.clang

web3-shh package will be deprecated in version 1.3.5 and will no longer be supported.
web3-bzz package will be deprecated in version 1.3.5 and will no longer be supported.

  Contract: Box
    ✓ non owner cannot store a value (624ms)
    ✓ retrieve returns a value previously stored (72ms)
    ✓ store emits an event (50ms)

  3 passing (991ms)

I tried this on WSL2 (Ubuntu 18.04) on Windows 10, using version 0.5.10 of OpenZeppelin Test Helpers:

$ npx truffle version
Truffle v5.2.4 (core: 5.2.4)
Solidity - 0.6.12 (solc-js)
Node v10.23.2
Web3.js v1.2.9

Can you provide the version of the Test Helpers, Truffle you are using, as well as the operating system, node and npm versions? Any other details that might help us reproduce it would be appreciated! If you could create a small example (such as the above) which illustrates the issue that would be very helpful.

elod008 commented 2 years ago

Hello, I can also confirm the issue:

Truffle v5.4.17
@openzeppelin/test-helpers ^0.5.15
Solidity ^0.8.7
node v14.18.1
npm 8.1.3

@abcoathup the broker nonce error refers only to the account that was the msg.sender in the transaction in which the revert happened. That means if you try to use that account once again in a test cycle after your test "non owner cannot store a value" that specific error would emerge.

frangio commented 2 years ago

the nonces are always off by 1

What does this mean? A reverted transaction still consumes a nonce, there is nothing special that expectRevert is doing.

Closing until someone can provide an error reproduction.

illuzen commented 2 years ago

Try doing a transaction after expectRevert, in the same test case

frangio commented 2 years ago

I'm pretty sure you forgot to include await before expectRevert. Take a look at the examples in the docs.

illuzen commented 2 years ago

hmm maybe you're right...

zhTnecniv commented 2 years ago

I have the same issue, but I didn't use expectRevert. I am only catching the error message in a try...catch... block. But everytime after the transaction gets reverted, the nonce is off by one. It's very annoying.