From 564dbea1263aea8ea3a5617e6a8740897f2cf348 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Sat, 25 May 2019 17:58:54 -0700 Subject: [PATCH] Remove formatters and clarify comments about pointing assetData to the same memory location --- .../contracts/src/MixinMatchOrders.sol | 3 +- .../contracts/src/MixinWrapperFunctions.sol | 20 +++--- .../exchange/test/utils/exchange_wrapper.ts | 70 ++++++++----------- contracts/test-utils/src/formatters.ts | 68 ------------------ contracts/test-utils/src/index.ts | 1 - 5 files changed, 44 insertions(+), 118 deletions(-) delete mode 100644 contracts/test-utils/src/formatters.ts diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index 1c74b07b14..7fc8ecce9d 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -57,7 +57,8 @@ contract MixinMatchOrders is nonReentrant returns (LibFillResults.MatchedFillResults memory matchedFillResults) { - // We assume that rightOrder.takerAssetData == leftOrder.makerAssetData and rightOrder.makerAssetData == leftOrder.takerAssetData. + // We assume that rightOrder.takerAssetData == leftOrder.makerAssetData and rightOrder.makerAssetData == leftOrder.takerAssetData + // by pointing these values to the same location in memory. This is cheaper than checking equality. // If this assumption isn't true, the match will fail at signature validation. rightOrder.makerAssetData = leftOrder.takerAssetData; rightOrder.takerAssetData = leftOrder.makerAssetData; diff --git a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol index 02c46b59db..dc2d217cc7 100644 --- a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol @@ -199,8 +199,9 @@ contract MixinWrapperFunctions is uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - // We assume that asset being sold by taker is the same for each order. - // Rather than passing this in as calldata, we use the takerAssetData from the first order in all later orders. + // The `takerAssetData` must be the same for each order. + // Rather than checking equality, we point the `takerAssetData` of each order to the same memory location. + // This is less expensive than checking equality. orders[i].takerAssetData = takerAssetData; // Calculate the remaining amount of takerAsset to sell @@ -243,8 +244,9 @@ contract MixinWrapperFunctions is uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - // We assume that asset being sold by taker is the same for each order. - // Rather than passing this in as calldata, we use the takerAssetData from the first order in all later orders. + // The `takerAssetData` must be the same for each order. + // Rather than checking equality, we point the `takerAssetData` of each order to the same memory location. + // This is less expensive than checking equality. orders[i].takerAssetData = takerAssetData; // Calculate the remaining amount of takerAsset to sell @@ -287,8 +289,9 @@ contract MixinWrapperFunctions is uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - // We assume that asset being bought by taker is the same for each order. - // Rather than passing this in as calldata, we copy the makerAssetData from the first order onto all later orders. + // The `makerAssetData` must be the same for each order. + // Rather than checking equality, we point the `makerAssetData` of each order to the same memory location. + // This is less expensive than checking equality. orders[i].makerAssetData = makerAssetData; // Calculate the remaining amount of makerAsset to buy @@ -339,8 +342,9 @@ contract MixinWrapperFunctions is uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - // We assume that asset being bought by taker is the same for each order. - // Rather than passing this in as calldata, we copy the makerAssetData from the first order onto all later orders. + // The `makerAssetData` must be the same for each order. + // Rather than checking equality, we point the `makerAssetData` of each order to the same memory location. + // This is less expensive than checking equality. orders[i].makerAssetData = makerAssetData; // Calculate the remaining amount of makerAsset to buy diff --git a/contracts/exchange/test/utils/exchange_wrapper.ts b/contracts/exchange/test/utils/exchange_wrapper.ts index a69cd080b4..75cd042d86 100644 --- a/contracts/exchange/test/utils/exchange_wrapper.ts +++ b/contracts/exchange/test/utils/exchange_wrapper.ts @@ -1,14 +1,7 @@ import { artifacts as erc1155Artifacts } from '@0x/contracts-erc1155'; import { artifacts as erc20Artifacts } from '@0x/contracts-erc20'; import { artifacts as erc721Artifacts } from '@0x/contracts-erc721'; -import { - FillResults, - formatters, - LogDecoder, - OrderInfo, - orderUtils, - Web3ProviderEngine, -} from '@0x/contracts-test-utils'; +import { FillResults, LogDecoder, OrderInfo, orderUtils, Web3ProviderEngine } from '@0x/contracts-test-utils'; import { SignedOrder, SignedZeroExTransaction } from '@0x/types'; import { AbiEncoder, BigNumber } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; @@ -98,11 +91,12 @@ export class ExchangeWrapper { from: string, opts: { takerAssetFillAmounts?: BigNumber[] } = {}, ): Promise { - const params = formatters.createBatchFill(orders, opts.takerAssetFillAmounts); const txHash = await this._exchange.batchFillOrders.sendTransactionAsync( - params.orders, - params.takerAssetFillAmounts, - params.signatures, + orders, + opts.takerAssetFillAmounts === undefined + ? orders.map(signedOrder => signedOrder.takerAssetAmount) + : opts.takerAssetFillAmounts, + orders.map(signedOrder => signedOrder.signature), { from }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -113,11 +107,12 @@ export class ExchangeWrapper { from: string, opts: { takerAssetFillAmounts?: BigNumber[] } = {}, ): Promise { - const params = formatters.createBatchFill(orders, opts.takerAssetFillAmounts); const txHash = await this._exchange.batchFillOrKillOrders.sendTransactionAsync( - params.orders, - params.takerAssetFillAmounts, - params.signatures, + orders, + opts.takerAssetFillAmounts === undefined + ? orders.map(signedOrder => signedOrder.takerAssetAmount) + : opts.takerAssetFillAmounts, + orders.map(signedOrder => signedOrder.signature), { from }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -128,11 +123,12 @@ export class ExchangeWrapper { from: string, opts: { takerAssetFillAmounts?: BigNumber[]; gas?: number } = {}, ): Promise { - const params = formatters.createBatchFill(orders, opts.takerAssetFillAmounts); const txHash = await this._exchange.batchFillOrdersNoThrow.sendTransactionAsync( - params.orders, - params.takerAssetFillAmounts, - params.signatures, + orders, + opts.takerAssetFillAmounts === undefined + ? orders.map(signedOrder => signedOrder.takerAssetAmount) + : opts.takerAssetFillAmounts, + orders.map(signedOrder => signedOrder.signature), { from, gas: opts.gas }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -143,11 +139,10 @@ export class ExchangeWrapper { from: string, opts: { takerAssetFillAmount: BigNumber }, ): Promise { - const params = formatters.createMarketSellOrders(orders, opts.takerAssetFillAmount); const txHash = await this._exchange.marketSellOrders.sendTransactionAsync( - params.orders, - params.takerAssetFillAmount, - params.signatures, + orders, + opts.takerAssetFillAmount, + orders.map(signedOrder => signedOrder.signature), { from }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -158,11 +153,10 @@ export class ExchangeWrapper { from: string, opts: { takerAssetFillAmount: BigNumber; gas?: number }, ): Promise { - const params = formatters.createMarketSellOrders(orders, opts.takerAssetFillAmount); const txHash = await this._exchange.marketSellOrdersNoThrow.sendTransactionAsync( - params.orders, - params.takerAssetFillAmount, - params.signatures, + orders, + opts.takerAssetFillAmount, + orders.map(signedOrder => signedOrder.signature), { from, gas: opts.gas }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -173,11 +167,10 @@ export class ExchangeWrapper { from: string, opts: { makerAssetFillAmount: BigNumber }, ): Promise { - const params = formatters.createMarketBuyOrders(orders, opts.makerAssetFillAmount); const txHash = await this._exchange.marketBuyOrders.sendTransactionAsync( - params.orders, - params.makerAssetFillAmount, - params.signatures, + orders, + opts.makerAssetFillAmount, + orders.map(signedOrder => signedOrder.signature), { from }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -188,11 +181,10 @@ export class ExchangeWrapper { from: string, opts: { makerAssetFillAmount: BigNumber; gas?: number }, ): Promise { - const params = formatters.createMarketBuyOrders(orders, opts.makerAssetFillAmount); const txHash = await this._exchange.marketBuyOrdersNoThrow.sendTransactionAsync( - params.orders, - params.makerAssetFillAmount, - params.signatures, + orders, + opts.makerAssetFillAmount, + orders.map(signedOrder => signedOrder.signature), { from, gas: opts.gas }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -202,8 +194,7 @@ export class ExchangeWrapper { orders: SignedOrder[], from: string, ): Promise { - const params = formatters.createBatchCancel(orders); - const txHash = await this._exchange.batchCancelOrders.sendTransactionAsync(params.orders, { from }); + const txHash = await this._exchange.batchCancelOrders.sendTransactionAsync(orders, { from }); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } @@ -211,8 +202,7 @@ export class ExchangeWrapper { orders: SignedOrder[], from: string, ): Promise { - const params = formatters.createBatchCancel(orders); - const txHash = await this._exchange.batchCancelOrdersNoThrow.sendTransactionAsync(params.orders, { from }); + const txHash = await this._exchange.batchCancelOrdersNoThrow.sendTransactionAsync(orders, { from }); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } diff --git a/contracts/test-utils/src/formatters.ts b/contracts/test-utils/src/formatters.ts deleted file mode 100644 index a32e3e1eef..0000000000 --- a/contracts/test-utils/src/formatters.ts +++ /dev/null @@ -1,68 +0,0 @@ -import { SignedOrder } from '@0x/types'; -import { BigNumber } from '@0x/utils'; -import * as _ from 'lodash'; - -import { constants } from './constants'; -import { orderUtils } from './order_utils'; -import { BatchCancelOrders, BatchFillOrders, MarketBuyOrders, MarketSellOrders } from './types'; - -export const formatters = { - createBatchFill(signedOrders: SignedOrder[], takerAssetFillAmounts: BigNumber[] = []): BatchFillOrders { - const batchFill: BatchFillOrders = { - orders: [], - signatures: [], - takerAssetFillAmounts, - }; - _.forEach(signedOrders, signedOrder => { - const orderWithoutDomain = orderUtils.getOrderWithoutDomain(signedOrder); - batchFill.orders.push(orderWithoutDomain); - batchFill.signatures.push(signedOrder.signature); - if (takerAssetFillAmounts.length < signedOrders.length) { - batchFill.takerAssetFillAmounts.push(signedOrder.takerAssetAmount); - } - }); - return batchFill; - }, - createMarketSellOrders(signedOrders: SignedOrder[], takerAssetFillAmount: BigNumber): MarketSellOrders { - const marketSellOrders: MarketSellOrders = { - orders: [], - signatures: [], - takerAssetFillAmount, - }; - _.forEach(signedOrders, (signedOrder, i) => { - const orderWithoutDomain = orderUtils.getOrderWithoutDomain(signedOrder); - if (i !== 0) { - orderWithoutDomain.takerAssetData = constants.NULL_BYTES; - } - marketSellOrders.orders.push(orderWithoutDomain); - marketSellOrders.signatures.push(signedOrder.signature); - }); - return marketSellOrders; - }, - createMarketBuyOrders(signedOrders: SignedOrder[], makerAssetFillAmount: BigNumber): MarketBuyOrders { - const marketBuyOrders: MarketBuyOrders = { - orders: [], - signatures: [], - makerAssetFillAmount, - }; - _.forEach(signedOrders, (signedOrder, i) => { - const orderWithoutDomain = orderUtils.getOrderWithoutDomain(signedOrder); - if (i !== 0) { - orderWithoutDomain.makerAssetData = constants.NULL_BYTES; - } - marketBuyOrders.orders.push(orderWithoutDomain); - marketBuyOrders.signatures.push(signedOrder.signature); - }); - return marketBuyOrders; - }, - createBatchCancel(signedOrders: SignedOrder[]): BatchCancelOrders { - const batchCancel: BatchCancelOrders = { - orders: [], - }; - _.forEach(signedOrders, signedOrder => { - const orderWithoutDomain = orderUtils.getOrderWithoutDomain(signedOrder); - batchCancel.orders.push(orderWithoutDomain); - }); - return batchCancel; - }, -}; diff --git a/contracts/test-utils/src/index.ts b/contracts/test-utils/src/index.ts index 7e1c6b52f5..f98a6309f8 100644 --- a/contracts/test-utils/src/index.ts +++ b/contracts/test-utils/src/index.ts @@ -16,7 +16,6 @@ export { export { getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync } from './block_timestamp'; export { provider, txDefaults, web3Wrapper } from './web3_wrapper'; export { LogDecoder } from './log_decoder'; -export { formatters } from './formatters'; export { signingUtils } from './signing_utils'; export { orderUtils } from './order_utils'; export { typeEncodingUtils } from './type_encoding_utils';