MixinTransaction Unit Tests #2069
Conversation
jalextowle
force-pushed
the
feature/contracts/3.0/transaction-units
branch
3 times, most recently
from
9276787 to
3c9333c
Compare
…on to not require state
…hauling transaction_unit_tests.ts
jalextowle
force-pushed
the
feature/contracts/3.0/transaction-units
branch
from
a09c654 to
fb41818
Compare
…d the `Killed` error
jalextowle
force-pushed
the
feature/contracts/3.0/transaction-units
branch
from
0761efe to
ea950a5
Compare
| @@ -567,7 +577,10 @@ workflows: | |||
| # - build-website: | |||
| # requires: | |||
| # - build | |||
| - test-contracts-ganache-3.0: | |||
| - test-exchange-ganache-3.0: | |||
| return expect(tx).to.revertWith(expectedError); | ||
| }); | ||
|
|
||
| it('should revert if the same transaction is executed twice in a batch', async () => { |
|
|
||
| // Duplicate the first transaction. This should cause the call to `batchExecuteTransactions()` to fail | ||
| // because this transaction will have the same order hash as transaction1. | ||
| const transaction2 = { |
There was a problem hiding this comment.
Why not just set transaction2 = transaction1?
| '0x0000000000000000000000000000000000000000000000000000000000000020'.concat( | ||
| encodedDeadbeef.slice(2, encodedDeadbeef.length), | ||
| ), | ||
| ).to.be.true(); |
There was a problem hiding this comment.
Why not expect(result[0]).to.eq('0x00...'.concat(...)) so we can see the actual vs expected if it fails?
| expect(result.length).to.be.eq(2); | ||
| expect( | ||
| result[0] === | ||
| '0x0000000000000000000000000000000000000000000000000000000000000020'.concat( |
There was a problem hiding this comment.
I think it's probably worth it to refactor this encoding logic out, for readability.
There was a problem hiding this comment.
I agree. My rationale for not doing this is that a fix to the AbiEncoder will actually allow us to use that for this check. At the moment, I have to do this because AbiEncoder.create('bytes').decode(result[0]) will not correctly decode the bytes (it will actually just return the 32 byte slot containing the length). I've added an Asana backlog task for this, so it should actually be completed at some point. What do you think of this?
There was a problem hiding this comment.
If it's a hassle, nbd.
| const validSignature = randomSignature(); | ||
|
|
||
| const currentTimestamp = await getLatestBlockTimestampAsync(); | ||
| const transaction1 = { |
There was a problem hiding this comment.
Can we call these txs something like innerTx and outerTx?
|
|
||
| // Use the transaction in execute transaction. | ||
| await expect( | ||
| transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature), |
There was a problem hiding this comment.
This technically doesn't wait for the tx to be mined (even though it's probably all the same on ganache).
| transactionsContract.executeTransaction.sendTransactionAsync(transaction, validSignature), | |
| transactionsContract.executeTransaction.awaitTransactionSuccessAsync(transaction, validSignature), |
There's a couple other spots where this is done, too.
| const currentTimestamp = await getLatestBlockTimestampAsync(); | ||
| const transaction = { | ||
| ...EMPTY_ZERO_EX_TRANSACTION, | ||
| expirationTimeSeconds: new BigNumber(currentTimestamp).plus(10), |
There was a problem hiding this comment.
This is getting a little repetitive. How about having EMPTY_ZERO_EX_TRANSACTION hold some timestamp in the distant future? Like new BigNumber(Math.floor(_.now() / 1000) + 86400).
| const transactionHash = transactionHashUtils.getTransactionHashHex(transaction); | ||
|
|
||
| // Verify that the returndata of the transaction is 0xDEADBEEF | ||
| const result = await transactionsContract.executeTransaction.callAsync(transaction, validSignature, { |
There was a problem hiding this comment.
The TransactionHelper class in @0x/contracts-test-utils might reduce some of the code in blocks where you do a callAsync, sendTransactionAsync, and getTxWithDecodedLogsAsync since it does all three.
| public | ||
| returns (bytes memory) | ||
| { | ||
| emit ExecutableCalled(data, returnData); |
There was a problem hiding this comment.
I think it might be super cool to log the current context address, too.
Description
This PR introduces unit tests for the
MixinTransactioncontract.Testing instructions
yarn build:contracts && yarn test:contracts && yarn lint:contractsTypes of changes
Checklist:
[WIP]if necessary.