From 416b1aee98bdb63832075343b708c50204547276 Mon Sep 17 00:00:00 2001 From: James Towle Date: Wed, 3 Jul 2019 19:07:48 -0500 Subject: [PATCH] Improved the testing for matchOrders and batchMatchOrders --- .../contracts/src/MixinMatchOrders.sol | 2 + .../contracts/test/ReentrantERC20Token.sol | 14 -- .../exchange/test/utils/match_order_tester.ts | 131 +++++++++++++++++- contracts/test-utils/src/types.ts | 1 + 4 files changed, 130 insertions(+), 18 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index 8ef7ca46d5..6d7c0ebf70 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -265,6 +265,7 @@ contract MixinMatchOrders is // or break out of the loop if there are no more leftOrders to match. if (leftOrderInfo.orderTakerAssetFilledAmount >= leftOrder.takerAssetAmount) { if (++leftIdx == leftOrders.length) { + matchCount++; break; } else { leftOrder = leftOrders[leftIdx]; @@ -276,6 +277,7 @@ contract MixinMatchOrders is // or break out of the loop if there are no more rightOrders to match. if (rightOrderInfo.orderTakerAssetFilledAmount >= rightOrder.takerAssetAmount) { if (++rightIdx == rightOrders.length) { + matchCount++; break; } else { rightOrder = rightOrders[rightIdx]; diff --git a/contracts/exchange/contracts/test/ReentrantERC20Token.sol b/contracts/exchange/contracts/test/ReentrantERC20Token.sol index 8067127594..c9dc7e04e8 100644 --- a/contracts/exchange/contracts/test/ReentrantERC20Token.sol +++ b/contracts/exchange/contracts/test/ReentrantERC20Token.sol @@ -249,20 +249,6 @@ contract ReentrantERC20Token is orders[1].makerAssetAmount = orders[0].takerAssetAmount; } - /// @dev Create two complementary test orders. - function _createBatchMatchedOrders() - internal - view - returns (LibOrder.Order[][] memory orders) - { - - LibOrder.Order[] memory _orders = _createOrders(2); - orders[0][0] = _orders[0]; - orders[0][1] = _orders[1]; - orders[0][1].takerAssetAmount = orders[0][1].makerAssetAmount; - orders[0][1].makerAssetAmount = orders[0][0].takerAssetAmount; - } - function _getTakerFillAmounts( LibOrder.Order[] memory orders ) diff --git a/contracts/exchange/test/utils/match_order_tester.ts b/contracts/exchange/test/utils/match_order_tester.ts index bd801c8d9a..7f116fc0f6 100644 --- a/contracts/exchange/test/utils/match_order_tester.ts +++ b/contracts/exchange/test/utils/match_order_tester.ts @@ -1,5 +1,11 @@ import { ERC1155ProxyWrapper, ERC20Wrapper, ERC721Wrapper } from '@0x/contracts-asset-proxy'; -import { chaiSetup, ERC1155HoldingsByOwner, OrderStatus } from '@0x/contracts-test-utils'; +import { + BatchMatchedFillResults, + chaiSetup, + ERC1155HoldingsByOwner, + MatchedFillResults, + OrderStatus, +} from '@0x/contracts-test-utils'; import { assetDataUtils, orderHashUtils } from '@0x/order-utils'; import { AssetProxyId, SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; @@ -191,14 +197,25 @@ export class MatchOrderTester { ? initialTokenBalances : await this._initialTokenBalancesPromise; // Execute `batchMatchOrders()` + let actualBatchMatchResults; let transactionReceipt; if (withMaximalFill) { + actualBatchMatchResults = await this.exchangeWrapper.getBatchMatchOrdersWithMaximalFillResultsAsync( + orders.leftOrders, + orders.rightOrders, + takerAddress, + ); transactionReceipt = await this._executeBatchMatchOrdersWithMaximalFillAsync( orders.leftOrders, orders.rightOrders, takerAddress, ); } else { + actualBatchMatchResults = await this.exchangeWrapper.getBatchMatchOrdersResultsAsync( + orders.leftOrders, + orders.rightOrders, + takerAddress, + ); transactionReceipt = await this._executeBatchMatchOrdersAsync( orders.leftOrders, orders.rightOrders, @@ -213,6 +230,8 @@ export class MatchOrderTester { matchPairs, expectedTransferAmounts, ); + const expectedResults = convertToBatchMatchResults(expectedBatchMatchResults); + expect(actualBatchMatchResults).to.be.eql(expectedResults); // Validate the simulation against reality. await assertBatchMatchResultsAsync( expectedBatchMatchResults, @@ -248,31 +267,44 @@ export class MatchOrderTester { ? initialTokenBalances : await this._initialTokenBalancesPromise; // Execute `matchOrders()` + let actualMatchResults; let transactionReceipt; if (withMaximalFill) { + actualMatchResults = await this.exchangeWrapper.getMatchOrdersWithMaximalFillResultsAsync( + orders.leftOrder, + orders.rightOrder, + takerAddress, + ); transactionReceipt = await this._executeMatchOrdersWithMaximalFillAsync( orders.leftOrder, orders.rightOrder, takerAddress, ); } else { + actualMatchResults = await this.exchangeWrapper.getMatchOrdersResultsAsync( + orders.leftOrder, + orders.rightOrder, + takerAddress, + ); transactionReceipt = await this._executeMatchOrdersAsync(orders.leftOrder, orders.rightOrder, takerAddress); } // Simulate the fill. - const matchResults = simulateMatchOrders( + const expectedMatchResults = simulateMatchOrders( orders, takerAddress, _initialTokenBalances, toFullMatchTransferAmounts(expectedTransferAmounts), ); + const expectedResults = convertToMatchResults(expectedMatchResults); + expect(actualMatchResults).to.be.eql(expectedResults); // Validate the simulation against reality. await assertMatchResultsAsync( - matchResults, + expectedMatchResults, transactionReceipt, await this.getBalancesAsync(), this.exchangeWrapper, ); - return matchResults; + return expectedMatchResults; } /** @@ -1022,4 +1054,95 @@ function aggregateBalances( // TODO(jalextowle): Implement the same as the above for ERC1155 return totalBalances; } + +/** + * Converts a BatchMatchResults object to the associated value that correspondes to a value that could be + * returned by `batchMatchOrders` or `batchMatchOrdersWithMaximalFill`. + * @param results The results object to convert + * @return The associated object that can be compared to the return value of `batchMatchOrders` + */ +function convertToBatchMatchResults(results: BatchMatchResults): BatchMatchedFillResults { + // Initialize the results object + const batchMatchedFillResults: BatchMatchedFillResults = { + left: [], + right: [], + profitInLeftMakerAsset: ZERO, + profitInRightMakerAsset: ZERO, + }; + // Create the batchMatchedFillResults by aggreagating the data from the simulations matches + for (const match of results.matches) { + expect(match.fills.length).to.be.eq(2); + // Include the matches results in the left fills of the batch match results. + batchMatchedFillResults.left.push({ + makerAssetFilledAmount: match.fills[0].makerAssetFilledAmount, + takerAssetFilledAmount: match.fills[0].takerAssetFilledAmount, + makerFeePaid: match.fills[0].makerFeePaid, + takerFeePaid: match.fills[0].takerFeePaid, + }); + // Include the matches results in the right fills of the batch match results. + batchMatchedFillResults.right.push({ + makerAssetFilledAmount: match.fills[1].makerAssetFilledAmount, + takerAssetFilledAmount: match.fills[1].takerAssetFilledAmount, + makerFeePaid: match.fills[1].makerFeePaid, + takerFeePaid: match.fills[1].takerFeePaid, + }); + const leftSpread = match.fills[0].makerAssetFilledAmount.minus(match.fills[1].takerAssetFilledAmount); + // If the left maker spread is positive for match, update the profitInLeftMakerAsset + if (leftSpread.isGreaterThan(ZERO)) { + batchMatchedFillResults.profitInLeftMakerAsset = batchMatchedFillResults.profitInLeftMakerAsset.plus( + leftSpread, + ); + } + const rightSpread = match.fills[1].makerAssetFilledAmount.minus(match.fills[0].takerAssetFilledAmount); + // If the right maker spread is positive for match, update the profitInRightMakerAsset + if (rightSpread.isGreaterThan(ZERO)) { + batchMatchedFillResults.profitInRightMakerAsset = batchMatchedFillResults.profitInRightMakerAsset.plus( + rightSpread, + ); + } + } + return batchMatchedFillResults; +} + +/** + * Converts a MatchResults object to the associated value that correspondes to a value that could be + * returned by `matchOrders` or `matchOrdersWithMaximalFill`. + * @param results The results object to convert + * @return The associated object that can be compared to the return value of `matchOrders` + */ +function convertToMatchResults(result: MatchResults): MatchedFillResults { + // If the left spread is negative, set it to zero + let leftMakerAssetSpreadAmount = result.fills[0].makerAssetFilledAmount.minus( + result.fills[1].takerAssetFilledAmount, + ); + if (leftMakerAssetSpreadAmount.isLessThanOrEqualTo(ZERO)) { + leftMakerAssetSpreadAmount = ZERO; + } + + // If the right spread is negative, set it to zero + let rightMakerAssetSpreadAmount = result.fills[1].makerAssetFilledAmount.minus( + result.fills[0].takerAssetFilledAmount, + ); + if (rightMakerAssetSpreadAmount.isLessThanOrEqualTo(ZERO)) { + rightMakerAssetSpreadAmount = ZERO; + } + + const matchedFillResults: MatchedFillResults = { + left: { + makerAssetFilledAmount: result.fills[0].makerAssetFilledAmount, + takerAssetFilledAmount: result.fills[0].takerAssetFilledAmount, + makerFeePaid: result.fills[0].makerFeePaid, + takerFeePaid: result.fills[0].takerFeePaid, + }, + right: { + makerAssetFilledAmount: result.fills[1].makerAssetFilledAmount, + takerAssetFilledAmount: result.fills[1].takerAssetFilledAmount, + makerFeePaid: result.fills[1].makerFeePaid, + takerFeePaid: result.fills[1].takerFeePaid, + }, + leftMakerAssetSpreadAmount, + rightMakerAssetSpreadAmount, + }; + return matchedFillResults; +} // tslint:disable-line:max-file-line-count diff --git a/contracts/test-utils/src/types.ts b/contracts/test-utils/src/types.ts index 6560bdd1dc..8b115fbdff 100644 --- a/contracts/test-utils/src/types.ts +++ b/contracts/test-utils/src/types.ts @@ -155,6 +155,7 @@ export interface MatchedFillResults { left: FillResults; right: FillResults; leftMakerAssetSpreadAmount: BigNumber; + rightMakerAssetSpreadAmount: BigNumber; } export interface BatchMatchedFillResults {