Conversation
b8a13e8 to
d9e0d4b
Compare
facuspagnuolo
left a comment
There was a problem hiding this comment.
Glad that we are dropping all this code! Well, actually reusing it from somewhere else haha
I think it would be nice to also update the test suite to Truffle 5 + web3 1.x, please take a look at my comments on the helpers PR :)
|
|
||
| const APP_ID = hash('stub.aragonpm.test') | ||
| const ZERO_ADDR = '0x0000000000000000000000000000000000000000' | ||
| const EMPTY_BYTES = '0x' |
There was a problem hiding this comment.
Wonder if we can replace EMPTY_BYTES everywhere for the one exported here: https://github.com/aragon/contract-helpers/blob/master/packages/test-helpers/src/bytes.js#L1
There was a problem hiding this comment.
Wasn't sure, but perhaps we forgot to do this? Although it is different as the one in the package is 0x00.
There was a problem hiding this comment.
All the tests using 0x00 as empty bytes were failing, we should change it in the helpers I think
sohkai
left a comment
There was a problem hiding this comment.
🙌 👏 Absolutely brilliant.
|
|
||
| const APP_ID = hash('stub.aragonpm.test') | ||
| const ZERO_ADDR = '0x0000000000000000000000000000000000000000' | ||
| const EMPTY_BYTES = '0x' |
There was a problem hiding this comment.
Wasn't sure, but perhaps we forgot to do this? Although it is different as the one in the package is 0x00.
| assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance - approvedAmount, 'Balance of owner should be correct') | ||
| assert.equal((await tokenMock.balanceOf(receiver)).valueOf(), approvedAmount, 'Balance of receiver should be correct') | ||
| assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should stay the same') | ||
| assertBn(await tokenMock.balanceOf(owner), initialBalance - approvedAmount, 'Balance of owner should be correct') |
There was a problem hiding this comment.
Not sure, but are we able to do math on BNs like this? We don't need to use sub()?
There was a problem hiding this comment.
Yea, it shouldn't work this way, let me fix it anyway
| it('checks block number', async () => { | ||
| assert.equal((await timeHelpersMock.getBlockNumberExt()).toString(), (await timeHelpersMock.getBlockNumber64Ext()).toString(), "block numbers should match") | ||
| assert.equal((await timeHelpersMock.getBlockNumberExt()).toString(), (await timeHelpersMock.getBlockNumberDirect()).toString(), web3.eth.blockNumber, "block number should match with real one", "block number should match with real one") | ||
| assert.equal((await timeHelpersMock.getBlockNumberExt()).toString(), (await timeHelpersMock.getBlockNumberDirect()).toString(), web3.eth.getBlockNumber, "block number should match with real one", "block number should match with real one") |
There was a problem hiding this comment.
A bit confused with this (although it's more with the original test)—do we not need to call getBlockNumber()?
There was a problem hiding this comment.
I think the original was malformed, let me change it
| assert.equal( | ||
| await web3.eth.getStorageAt(appStorage.address, (await appStorage.getKernelPosition())), | ||
| kernel.address, | ||
| kernel.address.toLowerCase(), |
There was a problem hiding this comment.
I'm starting to wonder if we should have some sort of assertAddressEquals() or etc. so we don't have to do this 😄
There was a problem hiding this comment.
Haha this one was a bit particular
Depends on aragon/contract-helpers#53