Potential error in expectEvent.contains

[terenceyak]

For the expectEvent.contains function, if we look at the else-if condition

openzeppelin-test-helpers/src/expectEvent.js

Lines 129 to 134 in ab4b867

	} else if (isBN(args[key]) || isBN(value)) {
	const actual = isBN(args[key]) ? args[key].toString() : args[key];
	const expected = isBN(value) ? value.toString() : value;
	expect(args[key]).to.be.bignumber.equal(value,
	`expected event argument '${key}' to have value ${expected} but got ${actual}`);
	} else {

This condition is executed if either args[key] or the value is a BN object.

However, the assertion on line 132 fails in the case args[key] is not a BN but of other types like a string, throwing an error like the following:

AssertionError: expected '3' to be an instance of BN

A potential fix would be to ensure args[key] is a BN before the assertion.

    if(!isBN(args[key])) {
      args[key] = new BN(args[key]);
    }

---

Sidenote:

I encountered this issue when I was using expectEvent.inTransaction for an event with BN arguments. I notice that decodeLogs in expectEvent uses web3.eth.abi.decodeLogs, which based on the docs returns a string for all variables even uint. This is shown below:

web3.eth.abi.decodeLog([{
    type: 'string',
    name: 'myString'
},{
    type: 'uint256',
    name: 'myNumber',
    indexed: true
},{
    type: 'uint8',
    name: 'mySmallNumber',
    indexed: true
}],
'0x0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000748656c6c6f252100000000000000000000000000000000000000000000000000',
['0x000000000000000000000000000000000000000000000000000000000000f310', '0x0000000000000000000000000000000000000000000000000000000000000010']);
> Result {
    '0': 'Hello%!',
    '1': '62224',
    '2': '16',
    myString: 'Hello%!',
    myNumber: '62224',
    mySmallNumber: '16'
}

[abcoathup]

Hi @terenceyak! Unfortunately, I wasn’t able to reproduce this issue. I tried the following:

I created a contract MyContract and used expectEvent in the test with the event as you described. Apologies if I am missing something.

MyContract.sol

pragma solidity ^0.5.0;

contract MyContract {
    
    event MyEvent(string myString, uint256 indexed myNumber, uint8 mySmallNumber);
    
    function doStuff() public {
        emit MyEvent("Hello%!", 62224, 16);
    }
}

MyContract.test.js

const { accounts, contract } = require('@openzeppelin/test-environment');
const { expect } = require('chai');

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

const MyContract = contract.fromArtifact('MyContract');

describe('MyContract', function () {
  const [ owner, other ] = accounts;

  // Use large integers ('big numbers')
  const myNumberValue = new BN('62224');
  const mySmallNumberValue = new BN('16');

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

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

    console.log(receipt.logs[0]);
    expectEvent(receipt, 'MyEvent', { myString: 'Hello%!', myNumber: myNumberValue, mySmallNumber: mySmallNumberValue });
    expectEvent(receipt, 'MyEvent', { 0: 'Hello%!', 1: myNumberValue, 2: mySmallNumberValue });
  });
});

Thanks so much for reporting it! The project owner will review and triage this issue during the next week.

[frangio]

I think the bug would happen when one does: expectEvent(receipt, 'MyEvent', { myNumber: '62224' }). So when the argument is a string and not already converted to BN.

We want it to work with strings so we may be missing a conversion.

[terenceyak]

Hi both,

I have debugged the issue further, and have narrowed the problem down to the following line.

openzeppelin-test-helpers/src/expectEvent.js

Line 132 in ab4b867

expect(args[key]).to.be.bignumber.equal(value,

With reference to the following example:

      expect('11').to.be.bignumber.equal(new BN(11));

The above line passes tests when the web3 is version 1.2.2 but gives the following error when web3 is version 1.2.6

     AssertionError: expected '11' to be an instance of BN

Let me know if you are not able to reproduce, i will create a repo just for this.

[nventuro]

That's very interesting, the recent BN releases shouldn't have altered that behavior. Could you share a simple snippet of your tests so we can reproduce the issue? Thanks!

[frangio]

I added the test that I thought would catch this but it succeeds on my computer.

See frangio@671da75.

[terenceyak]

Hi all,

@nventuro is right, it is not web3 that caused the issue. I dug deeper, and found that the main issue is due to a different version of chai-bn on my repo. I was using v0.1.1 while the version used in this repo is v0.2.1.

I have reproduced the bug by replacing the chai-bn version used in the test helpers here.

It appears that there are more errors that pop up with the current test.

I also note that in setup.js, you guys have noted:

// The chai module used internally by us may not be the same one that the user
// has in their own tests. This can happen if the version ranges required don't
// intersect, or if the package manager doesn't dedupe the modules for any
// other reason. We do our best to install chai-bn for the user.

Not sure if you guys would like to enforce the use of the chai-bn version in this repo to prevent the above issue from happening. Otherwise, the repo has to be compatible with all other chai-bn versions as well.

[frangio]

I'm pretty sure that comment is not related to this issue.

Even if you have installed chai-bn@0.1.1 in your repo, test-helpers has its own dependency of chai-bn@0.2.1, and expectEvent should be using that.

Can you provide a full repository of a project where we can reproduce the issue? It should be a project that installs test-helpers as a dependency, not a fork of test-helpers itself.

[terenceyak]

Hi @frangio,

The bug is probably due to my own poor management of dependencies in my code. I have reproduced a passing and failing branch to showcase this explained below.

I found that using your own chai-bn after importing the test helpers would overwrite the chai-bn version in the helpers and use the local one instead. You can view the failing branch here.

Changing the import order and importing the test helpers after a local chai-bn import would lead to the test passing. You can view the passing branch here.

Hope it helps!

[nventuro]

I think this is a non-issue given the comment above, but could you share a bit on why you're installing and importing chai-bn yourself? It is meant to be an internal package that you don't need to know about.