From 09b5018e6564496a067ec15e47f7710ebd3f6781 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Tue, 13 Aug 2019 22:34:53 +0200 Subject: [PATCH] Readability improvements --- .../assertion_wrappers/fill_order_wrapper.ts | 118 ++++++------------ .../test/balance_stores/balance_store.ts | 13 ++ .../blockchain_balance_store.ts | 3 +- .../balance_stores/local_balance_store.ts | 4 +- contracts/exchange/test/core.ts | 24 +--- 5 files changed, 61 insertions(+), 101 deletions(-) diff --git a/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts b/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts index 5aceb04da8..fa346eb544 100644 --- a/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts +++ b/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts @@ -1,9 +1,15 @@ -import { artifacts as proxyArtifacts , ERC1155ProxyWrapper, ERC20Wrapper, ERC721Wrapper } from '@0x/contracts-asset-proxy'; +import { + artifacts as proxyArtifacts, + ERC1155ProxyWrapper, + ERC20Wrapper, + ERC721Wrapper, +} from '@0x/contracts-asset-proxy'; import { artifacts as erc20Artifacts } from '@0x/contracts-erc20'; import { artifacts as erc721Artifacts } from '@0x/contracts-erc721'; import { - chaiSetup, + expect, FillEventArgs, + filterLogsToArguments, LogDecoder, OrderStatus, orderUtils, @@ -13,22 +19,20 @@ import { orderHashUtils } from '@0x/order-utils'; import { FillResults, SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; -import * as chai from 'chai'; -import { LogWithDecodedArgs, TransactionReceiptWithDecodedLogs, ZeroExProvider } from 'ethereum-types'; +import { TransactionReceiptWithDecodedLogs, ZeroExProvider } from 'ethereum-types'; import * as _ from 'lodash'; import { artifacts, ExchangeContract } from '../../src'; +import { calculateFillResults } from '../../src/reference_functions'; import { BalanceStore } from '../balance_stores/balance_store'; import { BlockchainBalanceStore } from '../balance_stores/blockchain_balance_store'; import { LocalBalanceStore } from '../balance_stores/local_balance_store'; -chaiSetup.configure(); -const expect = chai.expect; - export class FillOrderWrapper { private readonly _exchange: ExchangeContract; private readonly _blockchainBalanceStore: BlockchainBalanceStore; private readonly _web3Wrapper: Web3Wrapper; + /** * Simulates matching two orders by transferring amounts defined in * `transferAmounts` and returns the results. @@ -43,16 +47,11 @@ export class FillOrderWrapper { takerAddress: string, opts: { takerAssetFillAmount?: BigNumber } = {}, initBalanceStore: BalanceStore, - partialFillResults: Partial, ): [FillResults, FillEventArgs, BalanceStore] { const balanceStore = LocalBalanceStore.create(initBalanceStore); const takerAssetFillAmount = opts.takerAssetFillAmount !== undefined ? opts.takerAssetFillAmount : signedOrder.takerAssetAmount; - const fillResults = FillOrderWrapper._createFillResultsFromPartial( - partialFillResults, - signedOrder, - takerAssetFillAmount, - ); + const fillResults = calculateFillResults(signedOrder, takerAssetFillAmount); const fillEvent = FillOrderWrapper.simulateFillEvent(signedOrder, takerAddress, fillResults); // Taker -> Maker balanceStore.transferAsset( @@ -61,13 +60,6 @@ export class FillOrderWrapper { fillResults.takerAssetFilledAmount, signedOrder.takerAssetData, ); - // Taker -> Fee Recipient - balanceStore.transferAsset( - takerAddress, - signedOrder.feeRecipientAddress, - fillResults.takerFeePaid, - signedOrder.takerFeeAssetData, - ); // Maker -> Taker balanceStore.transferAsset( signedOrder.makerAddress, @@ -75,6 +67,13 @@ export class FillOrderWrapper { fillResults.makerAssetFilledAmount, signedOrder.makerAssetData, ); + // Taker -> Fee Recipient + balanceStore.transferAsset( + takerAddress, + signedOrder.feeRecipientAddress, + fillResults.takerFeePaid, + signedOrder.takerFeeAssetData, + ); // Maker -> Fee Recipient balanceStore.transferAsset( signedOrder.makerAddress, @@ -84,6 +83,7 @@ export class FillOrderWrapper { ); return [fillResults, fillEvent, balanceStore]; } + /** * Simulates the event emitted by the exchange contract when an order is filled. */ @@ -99,61 +99,24 @@ export class FillOrderWrapper { takerFeePaid: fillResults.takerFeePaid, }; } + /** * Extract the exchanges `Fill` event from a transaction receipt. */ - public static _extractFillEventsfromReceipt(receipt: TransactionReceiptWithDecodedLogs): FillEventArgs[] { - interface RawFillEventArgs { - orderHash: string; - makerAddress: string; - takerAddress: string; - makerAssetFilledAmount: string; - takerAssetFilledAmount: string; - makerFeePaid: string; - takerFeePaid: string; - } - const actualFills = (_.filter(receipt.logs, ['event', 'Fill']) as any) as Array< - LogWithDecodedArgs - >; - // Convert RawFillEventArgs to FillEventArgs. - return actualFills.map(fill => ({ - orderHash: fill.args.orderHash, - makerAddress: fill.args.makerAddress, - takerAddress: fill.args.takerAddress, - makerAssetFilledAmount: new BigNumber(fill.args.makerAssetFilledAmount), - takerAssetFilledAmount: new BigNumber(fill.args.takerAssetFilledAmount), - makerFeePaid: new BigNumber(fill.args.makerFeePaid), - takerFeePaid: new BigNumber(fill.args.takerFeePaid), - })); - } - /** - * Returns a completed `FillResults` given a partial instance. The correct values are inferred by the - * values in the `signedOrder` and `takerAssetFillAmount`. - */ - private static _createFillResultsFromPartial( - partialFillResults: Partial, - signedOrder: SignedOrder, - takerAssetFillAmount: BigNumber, - ): FillResults { - return { - makerAssetFilledAmount: - partialFillResults.makerAssetFilledAmount !== undefined - ? partialFillResults.makerAssetFilledAmount - : signedOrder.makerAssetAmount.times(takerAssetFillAmount).dividedBy(signedOrder.takerAssetAmount), - takerAssetFilledAmount: - partialFillResults.takerAssetFilledAmount !== undefined - ? partialFillResults.takerAssetFilledAmount - : signedOrder.takerAssetAmount.times(takerAssetFillAmount).dividedBy(signedOrder.takerAssetAmount), - makerFeePaid: - partialFillResults.makerFeePaid !== undefined - ? partialFillResults.makerFeePaid - : signedOrder.makerFee.times(takerAssetFillAmount).dividedBy(signedOrder.takerAssetAmount), - takerFeePaid: - partialFillResults.takerFeePaid !== undefined - ? partialFillResults.takerFeePaid - : signedOrder.takerFee.times(takerAssetFillAmount).dividedBy(signedOrder.takerAssetAmount), - }; + private static _extractFillEventsfromReceipt(receipt: TransactionReceiptWithDecodedLogs): FillEventArgs[] { + const events = filterLogsToArguments(receipt.logs, 'Fill'); + const fieldsOfInterest = [ + 'orderHash', + 'makerAddress', + 'takerAddress', + 'makerAssetFilledAmount', + 'takerAssetFilledAmount', + 'makerFeePaid', + 'takerFeePaid', + ]; + return events.map(event => _.pick(event, fieldsOfInterest)) as FillEventArgs[]; } + /** * Constructor. * @param exchangeContract Insstance of the deployed exchange contract @@ -173,12 +136,14 @@ export class FillOrderWrapper { this._blockchainBalanceStore = new BlockchainBalanceStore(erc20Wrapper, erc721Wrapper, erc1155ProxyWrapper); this._web3Wrapper = new Web3Wrapper(provider); } + /** * Returns the balance store used by this wrapper. */ public getBlockchainBalanceStore(): BlockchainBalanceStore { return this._blockchainBalanceStore; } + /** * Fills an order and asserts the effects. This includes * 1. The order info (via `getOrderInfo`) @@ -190,7 +155,6 @@ export class FillOrderWrapper { signedOrder: SignedOrder, from: string, opts: { takerAssetFillAmount?: BigNumber } = {}, - expectedFillResults: Partial = {}, ): Promise { // Get init state await this._blockchainBalanceStore.updateBalancesAsync(); @@ -204,13 +168,7 @@ export class FillOrderWrapper { simulatedFillResults, simulatedFillEvent, simulatedFinalBalanceStore, - ] = FillOrderWrapper.simulateFillOrder( - signedOrder, - from, - opts, - this._blockchainBalanceStore, - expectedFillResults, - ); + ] = FillOrderWrapper.simulateFillOrder(signedOrder, from, opts, this._blockchainBalanceStore); const [fillResults, fillEvent] = await this._fillOrderAsync(signedOrder, from, opts); // Assert state transition expect(simulatedFillResults, 'Fill Results').to.be.deep.equal(fillResults); @@ -221,6 +179,7 @@ export class FillOrderWrapper { const finalTakerAssetFilledAmount = initTakerAssetFilledAmount.plus(fillResults.takerAssetFilledAmount); await this._assertOrderStateAsync(signedOrder, finalTakerAssetFilledAmount); } + /** * Fills an order on-chain. As an optimization this function auto-updates the blockchain balance store * used by this contract. @@ -255,6 +214,7 @@ export class FillOrderWrapper { await this._blockchainBalanceStore.updateBalancesAsync(); return [fillResults, fillEvent]; } + /** * Asserts that the provided order's fill amount and order status * are the expected values. diff --git a/contracts/exchange/test/balance_stores/balance_store.ts b/contracts/exchange/test/balance_stores/balance_store.ts index 7d37f38f9a..6314aa8d1b 100644 --- a/contracts/exchange/test/balance_stores/balance_store.ts +++ b/contracts/exchange/test/balance_stores/balance_store.ts @@ -2,8 +2,15 @@ import { ERC1155Holdings, ERC1155HoldingsByOwner, TokenBalances } from '@0x/cont import { BigNumber } from '@0x/utils'; import * as _ from 'lodash'; +/** + * Note - this should live in `@0x/contracts-test-utils` but that would create a circular + * dependency in `BlockchainBlanceStore`. We should be able to mvoe this once we can rely + * solely on auto-generated wrappers opposed to the existing ERC20Wrapper, ERC721Wrapper, + * and ERC1155Wrapper. + */ export class BalanceStore { protected _balances: TokenBalances; + /** * Returns true iff balance stores do have the same entries. * @param lhs First balance store to compare @@ -12,6 +19,7 @@ export class BalanceStore { public static isEqual(lhs: BalanceStore, rhs: BalanceStore): boolean { return _.isEqual(lhs.getRawBalances(), rhs.getRawBalances()); } + /** * Throws iff balance stores do not have the same entries. * @param lhs First balance store to compare @@ -22,6 +30,7 @@ export class BalanceStore { throw new Error(`Balance stores are not equal:\n\nLeft:\n${lhs}\n\nRight:\n${rhs}`); } } + /** * Restructures `ERC1155HoldingsByOwner` to be compatible with `TokenBalances.erc1155`. * @param erc1155HoldingsByOwner Holdings returned by `ERC1155ProxyWrapper.getBalancesAsync()`. @@ -41,6 +50,7 @@ export class BalanceStore { } return result; } + /** * Encodes token balances in a way that they can be compared by lodash. */ @@ -57,12 +67,14 @@ export class BalanceStore { const keys = _.keys(obj).sort(); return _.zip(keys, keys.map(k => BalanceStore._encodeTokenBalances(obj[k]))); } + /** * Constructor. */ public constructor() { this._balances = { erc20: {}, erc721: {}, erc1155: {} }; } + /** * Copies the balance from an existing balance store. * @param balanceStore to copy balances from. @@ -70,6 +82,7 @@ export class BalanceStore { public copyBalancesFrom(balanceStore: BalanceStore): void { this._balances = _.cloneDeep(balanceStore._balances); } + /** * Returns the raw `TokenBalances` that this class encapsulates. */ diff --git a/contracts/exchange/test/balance_stores/blockchain_balance_store.ts b/contracts/exchange/test/balance_stores/blockchain_balance_store.ts index a377212385..ce994d2f0c 100644 --- a/contracts/exchange/test/balance_stores/blockchain_balance_store.ts +++ b/contracts/exchange/test/balance_stores/blockchain_balance_store.ts @@ -1,5 +1,4 @@ import { ERC1155ProxyWrapper, ERC20Wrapper, ERC721Wrapper } from '@0x/contracts-asset-proxy'; -import * as _ from 'lodash'; import { BalanceStore } from './balance_store'; @@ -7,6 +6,7 @@ export class BlockchainBalanceStore extends BalanceStore { private readonly _erc20Wrapper: ERC20Wrapper; private readonly _erc721Wrapper: ERC721Wrapper; private readonly _erc1155ProxyWrapper: ERC1155ProxyWrapper; + /** * Constructor. * @param erc20Wrapper The ERC20 Wrapper used to interface with deployed erc20 tokens. @@ -23,6 +23,7 @@ export class BlockchainBalanceStore extends BalanceStore { this._erc721Wrapper = erc721Wrapper; this._erc1155ProxyWrapper = erc1155ProxyWrapper; } + /** * Updates balances by querying on-chain values managed by the erc20, erc721, and erc1155 wrappers. */ diff --git a/contracts/exchange/test/balance_stores/local_balance_store.ts b/contracts/exchange/test/balance_stores/local_balance_store.ts index 87e41e9164..7d339c4182 100644 --- a/contracts/exchange/test/balance_stores/local_balance_store.ts +++ b/contracts/exchange/test/balance_stores/local_balance_store.ts @@ -17,12 +17,14 @@ export class LocalBalanceStore extends BalanceStore { } return localBalanceStore; } + /** * Constructor. */ public constructor() { super(); } + /** * Transfers assets from `fromAddress` to `toAddress`. * @param fromAddress Sender of asset(s) @@ -84,7 +86,7 @@ export class LocalBalanceStore extends BalanceStore { } } // sort NFT's by name - toBalances.nonFungible = _.sortBy(toBalances.nonFungible); + toBalances.nonFungible.sort(); break; } case AssetProxyId.MultiAsset: { diff --git a/contracts/exchange/test/core.ts b/contracts/exchange/test/core.ts index bef3e53911..c8d7f7ec2f 100644 --- a/contracts/exchange/test/core.ts +++ b/contracts/exchange/test/core.ts @@ -17,9 +17,11 @@ import { } from '@0x/contracts-erc20'; import { DummyERC721TokenContract } from '@0x/contracts-erc721'; import { - chaiSetup, + blockchainTests, constants, + describe, ERC20BalancesByOwner, + expect, getLatestBlockTimestampAsync, hexConcat, increaseTimeAndMineBlockAsync, @@ -29,12 +31,10 @@ import { txDefaults, web3Wrapper, } from '@0x/contracts-test-utils'; -import { BlockchainLifecycle } from '@0x/dev-utils'; import { assetDataUtils, ExchangeRevertErrors, LibMathRevertErrors, orderHashUtils } from '@0x/order-utils'; import { RevertReason, SignatureType, SignedOrder } from '@0x/types'; import { BigNumber, providerUtils, ReentrancyGuardRevertErrors, StringRevertError } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; -import * as chai from 'chai'; import { LogWithDecodedArgs } from 'ethereum-types'; import * as _ from 'lodash'; @@ -51,14 +51,10 @@ import { ValidatorWalletDataType, } from '../src'; -chaiSetup.configure(); -const expect = chai.expect; -const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); - import { FillOrderWrapper } from './assertion_wrappers/fill_order_wrapper'; // tslint:disable:no-unnecessary-type-assertion -describe('Exchange core', () => { +blockchainTests.resets('Exchange core', () => { let chainId: number; let makerAddress: string; let owner: string; @@ -102,12 +98,6 @@ describe('Exchange core', () => { let fillOrderWrapper: FillOrderWrapper; - before(async () => { - await blockchainLifecycle.startAsync(); - }); - after(async () => { - await blockchainLifecycle.revertAsync(); - }); before(async () => { chainId = await providerUtils.getChainIdAsync(provider); const accounts = await web3Wrapper.getAvailableAddressesAsync(); @@ -240,12 +230,6 @@ describe('Exchange core', () => { orderFactory = new OrderFactory(privateKey, defaultOrderParams); fillOrderWrapper = new FillOrderWrapper(exchange, erc20Wrapper, erc721Wrapper, erc1155ProxyWrapper, provider); }); - beforeEach(async () => { - await blockchainLifecycle.startAsync(); - }); - afterEach(async () => { - await blockchainLifecycle.revertAsync(); - }); describe('fillOrder', () => { beforeEach(async () => { erc20Balances = await erc20Wrapper.getBalancesAsync();