Misuse of BigNumber instances in tests

[frangio]

Most of the values obtained interacting with web3 are bignums, for example the web3.eth.getBalance return value. In most of the assertions throughout the tests we compare them with JavaScript numbers. This coerces them into numbers and possibly introduces numerical errors. (Because JavaScript's native numbers are floating point, and e.g. 1000000000000000000 is indistinguishable from 1000000000000000001.)

I find it unlikely that this is causing false positives, but it should be fixed nevertheless. The numbers we work with in Ethereum can be pretty big and prone to this kind of error. For example the number shown in the previous paragraph is equivalent to 1 ether.

Apart from that, we should probably establish some convention to avoid this problem in the future, and document it in the contribution guidelines. I think it might be good to wrap assert.equal, etc., to treat bignums specially.

• Establish how to deal with this. Possibly defining some helper functions.
• Find all coercions to number (implicit or explicit) and fix them.
• Add a section in CONTRIBUTING.md.

[maraoz]

Great idea @frangio, thanks!

[frangio]

I've fixed some of these assertions but we should check the rest of the tests in case I missed other errors.

@misteraverin has pointed out that the issue is not only numerical error but also flat out wrong comparisons when two BigNumber objects are compared.

[misteraverin]

@frangio Nice fix. But it can be confusing for somebody to see that in one place you compare it as BigNumbers and in another as Strings (equal case).

[frangio]

The thing with that is that assertion error messages are more useful when they show the expected and actual values. Using assert.equal with strings provides that benefit. The same goes for "greater than" tests, actually...

I pushed my quick fix to get this going, but like I said before we should probably write an assert wrapper to take care of it automatically. I had a quick look and didn't find anything that we could just import and use in this regard. I won't have time to implement it myself for a few days.

[frangio]

We're incorporating chai into our tests. For now only crowdsale and TokenTimelock tests are written in this way. The rest will have to be gradually ported.

This issue will be addressed by using chai-bignumber as seen in the Crowdsale tests.

[Aniket-Engg]

I think this has been fixed.

[frangio]

@Aniket-Engg Perhaps! If you've checked would you mind sharing what things you searched for?

I found a few more cases of this in ERC721Full.

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/7361ffd26b6354499d0b68d6d2d9da1f8cb21c8b/test/token/ERC721/ERC721Full.test.js#L67

[Aniket-Engg]

@frangio I haven't checked it specifically. I supposed that as per the last comment chai-bignumber has been introduced, so required things would have been implemented. I will check it now.

[Aniket-Engg]

@frangio I have gone through almost each test file to introduce changes. Have a look to changelog and let me know if anything has left.