From a794a3355164e9d24fba96fe898c0089b997ef60 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 15 Oct 2019 15:08:56 -0700 Subject: [PATCH] `@0x:contracts-integrations` Addressed review comments --- .../integrations/test/examples/simple.ts | 6 +- .../function_assertion_test.ts | 70 +++++++++---------- .../framework-unit-tests/getter_cache_test.ts | 15 ++-- .../exchange_integration_test.ts | 32 ++++----- .../test/utils/address_manager.ts | 43 ++---------- contracts/integrations/test/utils/cache.ts | 8 ++- .../test/utils/function_assertions.ts | 5 +- 7 files changed, 68 insertions(+), 111 deletions(-) diff --git a/contracts/integrations/test/examples/simple.ts b/contracts/integrations/test/examples/simple.ts index 57e7f78757..23f68f08b4 100644 --- a/contracts/integrations/test/examples/simple.ts +++ b/contracts/integrations/test/examples/simple.ts @@ -1,10 +1,8 @@ -import { blockchainTests, constants, expect } from '@0x/contracts-test-utils'; +import { blockchainTests, expect } from '@0x/contracts-test-utils'; import { BigNumber } from '@0x/utils'; -import { TxData, TransactionReceiptWithDecodedLogs } from 'ethereum-types'; import { artifacts, TestFrameworkContract } from '../../src'; -import { Condition, FunctionAssertion, Result } from '../utils/function_assertions'; -import { GetterCache } from '../utils/cache'; +import { FunctionAssertion, Result } from '../utils/function_assertions'; // These tests provide examples for how to use the "FunctionAssertion" class to write // tests for "payable" and "nonpayable" Solidity functions as well as "pure" and "view" functions. diff --git a/contracts/integrations/test/framework-unit-tests/function_assertion_test.ts b/contracts/integrations/test/framework-unit-tests/function_assertion_test.ts index c5a36bbf39..aa967246f9 100644 --- a/contracts/integrations/test/framework-unit-tests/function_assertion_test.ts +++ b/contracts/integrations/test/framework-unit-tests/function_assertion_test.ts @@ -1,5 +1,5 @@ import { blockchainTests, constants, expect, filterLogsToArguments, hexRandom } from '@0x/contracts-test-utils'; -import { BigNumber } from '@0x/utils'; +import { BigNumber, generatePseudoRandom256BitNumber } from '@0x/utils'; import { TransactionReceiptWithDecodedLogs } from 'ethereum-types'; import { artifacts, TestFrameworkContract, TestFrameworkSomeEventEventArgs, TestFrameworkEvents } from '../../src'; @@ -8,10 +8,6 @@ import { FunctionAssertion, Result } from '../utils/function_assertions'; blockchainTests.resets('FunctionAssertion Unit Tests', env => { let exampleContract: TestFrameworkContract; - function randomBigNumber(): BigNumber { - return new BigNumber(hexRandom(constants.WORD_LENGTH)); - } - before(async () => { exampleContract = await TestFrameworkContract.deployFrom0xArtifactAsync( artifacts.TestFramework, @@ -23,107 +19,107 @@ blockchainTests.resets('FunctionAssertion Unit Tests', env => { describe('runAsync', () => { it('should call the before function with the provided arguments', async () => { - let beforeIntrospectionObject = constants.ZERO_AMOUNT; + let sideEffectTarget = constants.ZERO_AMOUNT; const assertion = new FunctionAssertion(exampleContract.noEffect, { - before: async (returnValue: BigNumber) => { - beforeIntrospectionObject = returnValue; + before: async (inputValue: BigNumber) => { + sideEffectTarget = inputValue; }, after: async (beforeInfo: any, result: Result, returnValue: BigNumber) => {}, }); - const randomInput = randomBigNumber(); + const randomInput = generatePseudoRandom256BitNumber(); await assertion.runAsync(randomInput); - expect(beforeIntrospectionObject).bignumber.to.be.eq(randomInput); + expect(sideEffectTarget).bignumber.to.be.eq(randomInput); }); it('should call the after function with the provided arguments', async () => { - let afterIntrospectionObject = constants.ZERO_AMOUNT; + let sideEffectTarget = constants.ZERO_AMOUNT; const assertion = new FunctionAssertion(exampleContract.noEffect, { - before: async (returnValue: BigNumber) => {}, + before: async (inputValue: BigNumber) => {}, after: async (beforeInfo: any, result: Result, returnValue: BigNumber) => { - afterIntrospectionObject = returnValue; + sideEffectTarget = returnValue; }, }); - const randomInput = randomBigNumber(); + const randomInput = generatePseudoRandom256BitNumber(); await assertion.runAsync(randomInput); - expect(afterIntrospectionObject).bignumber.to.be.eq(randomInput); + expect(sideEffectTarget).bignumber.to.be.eq(randomInput); }); it('should not fail immediately if the wrapped function fails', async () => { await exampleContract.setCounter.awaitTransactionSuccessAsync(new BigNumber(1)); const assertion = new FunctionAssertion(exampleContract.revertSideEffect, { - before: async (returnValue: BigNumber) => {}, + before: async (inputValue: BigNumber) => {}, after: async (beforeInfo: any, result: Result, returnValue: BigNumber) => {}, }); - const randomInput = randomBigNumber(); + const randomInput = generatePseudoRandom256BitNumber(); await assertion.runAsync(randomInput); }); it('should pass the return value from "before" to "after"', async () => { - const randomInput = randomBigNumber(); - let afterIntrospectionObject = constants.ZERO_AMOUNT; + const randomInput = generatePseudoRandom256BitNumber(); + let sideEffectTarget = constants.ZERO_AMOUNT; const assertion = new FunctionAssertion(exampleContract.noEffect, { - before: async (returnValue: BigNumber) => { + before: async (inputValue: BigNumber) => { return randomInput; }, after: async (beforeInfo: any, result: Result, returnValue: BigNumber) => { - afterIntrospectionObject = beforeInfo; + sideEffectTarget = beforeInfo; }, }); await assertion.runAsync(randomInput); - expect(afterIntrospectionObject).bignumber.to.be.eq(randomInput); + expect(sideEffectTarget).bignumber.to.be.eq(randomInput); }); it('should pass the result from the function call to "after"', async () => { - let afterIntrospectionObject = constants.ZERO_AMOUNT; + let sideEffectTarget = constants.ZERO_AMOUNT; const assertion = new FunctionAssertion(exampleContract.revertSideEffect, { - before: async (returnValue: BigNumber) => {}, + before: async (inputValue: BigNumber) => {}, after: async (beforeInfo: any, result: Result, returnValue: BigNumber) => { - afterIntrospectionObject = result.data; + sideEffectTarget = result.data; }, }); - const randomInput = randomBigNumber(); + const randomInput = generatePseudoRandom256BitNumber(); await assertion.runAsync(randomInput); - expect(afterIntrospectionObject).bignumber.to.be.eq(randomInput); + expect(sideEffectTarget).bignumber.to.be.eq(randomInput); }); it('should pass the receipt from the function call to "after"', async () => { - let afterIntrospectionObject: TransactionReceiptWithDecodedLogs = {} as TransactionReceiptWithDecodedLogs; + let sideEffectTarget: TransactionReceiptWithDecodedLogs = {} as TransactionReceiptWithDecodedLogs; const assertion = new FunctionAssertion(exampleContract.revertSideEffect, { - before: async (returnValue: BigNumber) => {}, + before: async (inputValue: BigNumber) => {}, after: async (beforeInfo: any, result: Result, returnValue: BigNumber) => { if (result.receipt) { - afterIntrospectionObject = result.receipt; + sideEffectTarget = result.receipt; } else { throw new Error('No receipt received.'); } }, }); - const randomInput = randomBigNumber(); + const randomInput = generatePseudoRandom256BitNumber(); await assertion.runAsync(randomInput); // Ensure that the correct events were emitted. const [event] = filterLogsToArguments( - afterIntrospectionObject.logs, + sideEffectTarget.logs, TestFrameworkEvents.SomeEvent, ); expect(event).to.be.deep.eq({ someNumber: randomInput }); }); it('should pass the error to "after" if the function call fails', async () => { - let afterIntrospectionObject: Error = new Error(''); + let sideEffectTarget: Error = new Error(''); await exampleContract.setCounter.awaitTransactionSuccessAsync(new BigNumber(1)); const assertion = new FunctionAssertion(exampleContract.revertSideEffect, { - before: async (returnValue: BigNumber) => {}, + before: async (inputValue: BigNumber) => {}, after: async (beforeInfo: any, result: Result, returnValue: BigNumber) => { - afterIntrospectionObject = result.data; + sideEffectTarget = result.data; }, }); - const randomInput = randomBigNumber(); + const randomInput = generatePseudoRandom256BitNumber(); await assertion.runAsync(randomInput); const errorMessage = 'VM Exception while processing transaction: revert Revert'; - expect(afterIntrospectionObject.message).to.be.eq(errorMessage); + expect(sideEffectTarget.message).to.be.eq(errorMessage); }); }); }); diff --git a/contracts/integrations/test/framework-unit-tests/getter_cache_test.ts b/contracts/integrations/test/framework-unit-tests/getter_cache_test.ts index 001d20ba2c..a8c97a8333 100644 --- a/contracts/integrations/test/framework-unit-tests/getter_cache_test.ts +++ b/contracts/integrations/test/framework-unit-tests/getter_cache_test.ts @@ -45,7 +45,7 @@ blockchainTests.resets('Cache Tests', env => { // Update the counter to 1. await exampleContract.setCounter.awaitTransactionSuccessAsync(new BigNumber(1)); - // Ensure that the returned value is the cached counter. + // This should return "0" because a value was cached by the first call to "callAsync" expect(await cache.callAsync()).bignumber.to.be.eq(constants.ZERO_AMOUNT); }); }); @@ -74,7 +74,7 @@ blockchainTests.resets('Cache Tests', env => { // Update "counter" to "1", which will cause all calls of "isZeroOrFalse" to return "false". await exampleContract.setCounter.awaitTransactionSuccessAsync(new BigNumber(1)); - // This should return "true" because a value was cached. + // This should return "true" because a value was by the first call to "callAsync" expect(await cache.callAsync(constants.ZERO_AMOUNT)).to.be.true(); }); }); @@ -115,19 +115,16 @@ blockchainTests.resets('Cache Tests', env => { new BigNumber(1), ethUtil.bufferToHex(ethUtil.sha3(0)), ); + const hash = ethUtil.bufferToHex(ethUtil.sha3(hexSlice(hashData, 4))); // Ensure that the cache returns the correct value. - expect(await cache.callAsync(new BigNumber(1), ethUtil.bufferToHex(ethUtil.sha3(0)))).to.be.eq( - ethUtil.bufferToHex(ethUtil.sha3(hexSlice(hashData, 4))), - ); + expect(await cache.callAsync(new BigNumber(1), ethUtil.bufferToHex(ethUtil.sha3(0)))).to.be.eq(hash); // Update "counter" to "1", which will cause all calls to return the null hash. await exampleContract.setCounter.awaitTransactionSuccessAsync(new BigNumber(1)); - // Ensure that the cache returns the correct value. - expect(await cache.callAsync(new BigNumber(1), ethUtil.bufferToHex(ethUtil.sha3(0)))).to.be.eq( - ethUtil.bufferToHex(ethUtil.sha3(hexSlice(hashData, 4))), - ); + // The cache should return the same value as the first call to `callAsync` since a value was cached. + expect(await cache.callAsync(new BigNumber(1), ethUtil.bufferToHex(ethUtil.sha3(0)))).to.be.eq(hash); }); }); }); diff --git a/contracts/integrations/test/internal-integration-tests/exchange_integration_test.ts b/contracts/integrations/test/internal-integration-tests/exchange_integration_test.ts index f9ba5198e8..057999b3a5 100644 --- a/contracts/integrations/test/internal-integration-tests/exchange_integration_test.ts +++ b/contracts/integrations/test/internal-integration-tests/exchange_integration_test.ts @@ -11,8 +11,8 @@ import { DeploymentManager } from '../utils/deployment_manager'; blockchainTests('Exchange & Staking', env => { let accounts: string[]; let makerAddress: string; - let takers: string[]; - let delegators: string[]; + let takers: string[] = []; + let delegators: string[] = []; let feeRecipientAddress: string; let addressManager: AddressManager; let deploymentManager: DeploymentManager; @@ -21,15 +21,12 @@ blockchainTests('Exchange & Staking', env => { let takerAsset: DummyERC20TokenContract; let feeAsset: DummyERC20TokenContract; - const gasPrice = 1e9; + const GAS_PRICE = 1e9; before(async () => { const chainId = await env.getChainIdAsync(); accounts = await env.getAccountAddressesAsync(); - makerAddress = accounts[1]; - feeRecipientAddress = accounts[2]; - takers = [accounts[3], accounts[4]]; - delegators = [accounts[5], accounts[6], accounts[7]]; + [makerAddress, feeRecipientAddress, takers[0], takers[1], ...delegators] = accounts.slice(1); deploymentManager = await DeploymentManager.deployAsync(env); // Create a staking pool with the operator as a maker address. @@ -55,21 +52,20 @@ blockchainTests('Exchange & Staking', env => { ); // Set up two addresses for taking orders. - await addressManager.batchAddTakerAsync( - deploymentManager, - takers.map(takerAddress => { - return { - address: takerAddress, + await Promise.all( + takers.map(taker => + addressManager.addTakerAsync(deploymentManager, { + address: taker, mainToken: deploymentManager.tokens.erc20[1], feeToken: deploymentManager.tokens.erc20[2], - }; - }), + }), + ), ); }); describe('fillOrder', () => { it('should be able to fill an order', async () => { - const order = await addressManager.makerAddresses[0].orderFactory.newSignedOrderAsync({ + const order = await addressManager.makers[0].orderFactory.newSignedOrderAsync({ makerAddress, makerAssetAmount: new BigNumber(1), takerAssetAmount: new BigNumber(1), @@ -84,8 +80,8 @@ blockchainTests('Exchange & Staking', env => { order.signature, { from: takers[0], - gasPrice, - value: DeploymentManager.protocolFeeMultiplier.times(gasPrice), + gasPrice: GAS_PRICE, + value: DeploymentManager.protocolFeeMultiplier.times(GAS_PRICE), }, ); @@ -112,7 +108,7 @@ blockchainTests('Exchange & Staking', env => { takerAssetFilledAmount: order.takerAssetAmount, makerFeePaid: constants.ZERO_AMOUNT, takerFeePaid: constants.ZERO_AMOUNT, - protocolFeePaid: DeploymentManager.protocolFeeMultiplier.times(gasPrice), + protocolFeePaid: DeploymentManager.protocolFeeMultiplier.times(GAS_PRICE), }, ]); diff --git a/contracts/integrations/test/utils/address_manager.ts b/contracts/integrations/test/utils/address_manager.ts index f7d0aebf41..41bf3604b5 100644 --- a/contracts/integrations/test/utils/address_manager.ts +++ b/contracts/integrations/test/utils/address_manager.ts @@ -17,10 +17,10 @@ interface ConfigurationArgs { export class AddressManager { // A set of addresses that have been configured for market making. - public makerAddresses: MarketMaker[]; + public makers: MarketMaker[]; // A set of addresses that have been configured to take orders. - public takerAddresses: string[]; + public takers: string[]; /** * Sets up an address to take orders. @@ -31,19 +31,7 @@ export class AddressManager { await this._configureTokenForAddressAsync(deploymentManager, configArgs.address, configArgs.feeToken); // Add the taker to the list of configured taker addresses. - this.takerAddresses.push(configArgs.address); - } - - /** - * Sets up a list of addresses to take orders. - */ - public async batchAddTakerAsync( - deploymentManager: DeploymentManager, - configArgs: ConfigurationArgs[], - ): Promise { - for (const args of configArgs) { - await this.addTakerAsync(deploymentManager, args); - } + this.takers.push(configArgs.address); } /** @@ -79,35 +67,18 @@ export class AddressManager { await this._configureTokenForAddressAsync(deploymentManager, configArgs.address, configArgs.feeToken); // Add the maker to the list of configured maker addresses. - this.makerAddresses.push({ + this.makers.push({ address: configArgs.address, orderFactory, }); } - /** - * Sets up several market makers. - */ - public async batchAddMakerAsync( - deploymentManager: DeploymentManager, - configArgs: ConfigurationArgs[], - environment: BlockchainTestsEnvironment, - takerToken: DummyERC20TokenContract, - feeRecipientAddress: string, - chainId: number, - ): Promise { - for (const args of configArgs) { - await this.addMakerAsync(deploymentManager, args, environment, takerToken, feeRecipientAddress, chainId); - } - } - /** * Sets up initial account balances for a token and approves the ERC20 asset proxy * to transfer the token. */ protected async _configureTokenForAddressAsync( - deploymentManager: DeploymentManager, - address: string, + deploymentManager: DeploymentManager, address: string, token: DummyERC20TokenContract, ): Promise { await token.setBalance.awaitTransactionSuccessAsync(address, constants.INITIAL_ERC20_BALANCE); @@ -119,7 +90,7 @@ export class AddressManager { } constructor() { - this.makerAddresses = []; - this.takerAddresses = []; + this.makers = []; + this.takers = []; } } diff --git a/contracts/integrations/test/utils/cache.ts b/contracts/integrations/test/utils/cache.ts index 6f6dce70d8..fe8193a867 100644 --- a/contracts/integrations/test/utils/cache.ts +++ b/contracts/integrations/test/utils/cache.ts @@ -46,12 +46,14 @@ export class GetterCache { } } +export interface GetterCacheSet { + [getter: string]: GetterCache; +} + export class GetterCacheCollection { // A dictionary of getter cache's that allow the user of the collection to reference // the getter functions by name. - public getters: { - [getter: string]: GetterCache; - }; + public getters: GetterCacheSet; /** * Constructs a getter collection with pre-seeded getter names and getters. diff --git a/contracts/integrations/test/utils/function_assertions.ts b/contracts/integrations/test/utils/function_assertions.ts index 0c05c07c04..8251c93808 100644 --- a/contracts/integrations/test/utils/function_assertions.ts +++ b/contracts/integrations/test/utils/function_assertions.ts @@ -1,8 +1,5 @@ import { PromiseWithTransactionHash } from '@0x/base-contract'; -import { BlockParam, CallData, TransactionReceiptWithDecodedLogs, TxData } from 'ethereum-types'; -import * as _ from 'lodash'; - -import { DeploymentManager } from './'; +import { TransactionReceiptWithDecodedLogs } from 'ethereum-types'; export interface ContractGetterFunction { callAsync: (...args: any[]) => Promise;