From 5f47a3777178d3adb83987b578ec4b19f77da440 Mon Sep 17 00:00:00 2001 From: James Towle Date: Sat, 6 Jul 2019 23:10:53 -0500 Subject: [PATCH] Updated batchMatchOrders to fix an edge case and added tests --- .../contracts/src/LibFillResults.sol | 8 +- .../contracts/src/MixinMatchOrders.sol | 25 +-- contracts/exchange/test/match_orders.ts | 178 ++++++++++++++++++ .../exchange/test/utils/match_order_tester.ts | 16 +- contracts/test-utils/src/types.ts | 4 +- 5 files changed, 205 insertions(+), 26 deletions(-) diff --git a/contracts/exchange-libs/contracts/src/LibFillResults.sol b/contracts/exchange-libs/contracts/src/LibFillResults.sol index ad8869f5d2..b350d15b26 100644 --- a/contracts/exchange-libs/contracts/src/LibFillResults.sol +++ b/contracts/exchange-libs/contracts/src/LibFillResults.sol @@ -39,10 +39,10 @@ contract LibFillResults is } struct MatchedFillResults { - FillResults left; // Amounts filled and fees paid of left order. - FillResults right; // Amounts filled and fees paid of right order. - uint256 leftMakerAssetSpreadAmount; // Spread between price of left and right order, denominated in the left order's makerAsset, paid to taker. - uint256 rightMakerAssetSpreadAmount; // Spread between price of right and left order, denominated in the right order's makerAsset, paid to taker. + FillResults left; // Amounts filled and fees paid of left order. + FillResults right; // Amounts filled and fees paid of right order. + uint256 profitInLeftMakerAsset; // Profit taken from the left maker + uint256 profitInRightMakerAsset; // Profit taken from the right maker } /// @dev Adds properties of both FillResults instances. diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index 21e210da01..01005edcf2 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -188,7 +188,7 @@ contract MixinMatchOrders is // Calculate amount given to taker in the left order's maker asset if the left spread will be part of the profit. if (doesLeftMakerAssetProfitExist) { - matchedFillResults.leftMakerAssetSpreadAmount = _safeSub( + matchedFillResults.profitInLeftMakerAsset = _safeSub( matchedFillResults.left.makerAssetFilledAmount, matchedFillResults.right.takerAssetFilledAmount ); @@ -196,7 +196,7 @@ contract MixinMatchOrders is // Calculate amount given to taker in the right order's maker asset if the right spread will be part of the profit. if (doesRightMakerAssetProfitExist) { - matchedFillResults.rightMakerAssetSpreadAmount = _safeSub( + matchedFillResults.profitInRightMakerAsset = _safeSub( matchedFillResults.right.makerAssetFilledAmount, matchedFillResults.left.takerAssetFilledAmount ); @@ -238,7 +238,7 @@ contract MixinMatchOrders is } // Calculate amount given to taker - matchedFillResults.leftMakerAssetSpreadAmount = _safeSub( + matchedFillResults.profitInLeftMakerAsset = _safeSub( matchedFillResults.left.makerAssetFilledAmount, matchedFillResults.right.takerAssetFilledAmount ); @@ -395,8 +395,8 @@ contract MixinMatchOrders is // Without simulating all of the order matching, this program cannot know how many // matches there will be. To ensure that batchMatchedFillResults has enough memory // allocated for the left and the right side, we will allocate enough space for the - // maximum amount of matches (the maximum of the left and the right sides). - uint256 maxLength = _max256(leftOrders.length, rightOrders.length); + // maximum amount of matches (the left side length added to the right side length). + uint256 maxLength = leftOrders.length + rightOrders.length; batchMatchedFillResults.left = new LibFillResults.FillResults[](maxLength); batchMatchedFillResults.right = new LibFillResults.FillResults[](maxLength); @@ -413,7 +413,7 @@ contract MixinMatchOrders is // Loop infinitely (until broken inside of the loop), but keep a counter of how // many orders have been matched. - for (matchCount = 0;; matchCount++) { + for (matchCount = 0;;) { // Match the two orders that are pointed to by the left and right indices LibFillResults.MatchedFillResults memory matchResults = _matchOrders( leftOrder, @@ -439,18 +439,20 @@ contract MixinMatchOrders is batchMatchedFillResults.right[matchCount] = matchResults.right; batchMatchedFillResults.profitInLeftMakerAsset = _safeAdd( batchMatchedFillResults.profitInLeftMakerAsset, - matchResults.leftMakerAssetSpreadAmount + matchResults.profitInLeftMakerAsset ); batchMatchedFillResults.profitInRightMakerAsset = _safeAdd( batchMatchedFillResults.profitInRightMakerAsset, - matchResults.rightMakerAssetSpreadAmount + matchResults.profitInRightMakerAsset ); + // Increment the number of matches + matchCount++; + // If the leftOrder is filled, update the leftIdx, leftOrder, and leftSignature, // 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]; @@ -462,7 +464,6 @@ 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]; @@ -647,7 +648,7 @@ contract MixinMatchOrders is leftOrder.makerAssetData, leftOrder.makerAddress, takerAddress, - matchedFillResults.leftMakerAssetSpreadAmount + matchedFillResults.profitInLeftMakerAsset ); _dispatchTransferFrom( @@ -655,7 +656,7 @@ contract MixinMatchOrders is rightOrder.makerAssetData, rightOrder.makerAddress, takerAddress, - matchedFillResults.rightMakerAssetSpreadAmount + matchedFillResults.profitInRightMakerAsset ); // Settle taker fees. diff --git a/contracts/exchange/test/match_orders.ts b/contracts/exchange/test/match_orders.ts index c75635b56d..e7fd19718c 100644 --- a/contracts/exchange/test/match_orders.ts +++ b/contracts/exchange/test/match_orders.ts @@ -2576,6 +2576,83 @@ describe('matchOrders', () => { false, ); }); + it('should have three order matchings with only two left orders and two right orders', async () => { + const leftOrders = [ + await orderFactoryLeft.newSignedOrderAsync({ + makerAddress: makerAddressLeft, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(4, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 0), + feeRecipientAddress: feeRecipientAddressLeft, + }), + await orderFactoryLeft.newSignedOrderAsync({ + makerAddress: makerAddressLeft, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1, 0), + feeRecipientAddress: feeRecipientAddressLeft, + }), + ]; + const rightOrders = [ + await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(1, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 0), + feeRecipientAddress: feeRecipientAddressRight, + }), + await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(4, 0), + feeRecipientAddress: feeRecipientAddressRight, + }), + ]; + const expectedTransferAmounts = [ + { + // Left Maker + leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(2, 0), + leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50% + // Right Maker + rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0), + rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + // Taker + leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50% + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + }, + { + // Left Maker + leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(2, 0), + leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50% + // Right Maker + rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0), + rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50% + // Taker + leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50% + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50% + }, + { + // Left Maker + leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(2, 0), + leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + // Right Maker + rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0), + rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50% + // Taker + leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(50, 16), // 50% + }, + ]; + await matchOrderTester.batchMatchOrdersAndAssertEffectsAsync( + { + leftOrders, + rightOrders, + leftOrdersTakerAssetFilledAmounts: [ZERO, ZERO], + rightOrdersTakerAssetFilledAmounts: [ZERO, ZERO], + }, + takerAddress, + [[0, 0], [0, 1], [1, 1]], + expectedTransferAmounts, + false, + ); + }); }); describe('batchMatchOrdersWithMaximalFill', () => { it('should fully fill the the right order and pay the profit denominated in the left maker asset', async () => { @@ -2736,6 +2813,107 @@ describe('matchOrders', () => { true, ); }); + it('should correctly fill all four orders in three matches', async () => { + const leftOrders = [ + await orderFactoryLeft.newSignedOrderAsync({ + makerAddress: makerAddressLeft, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(2, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(1, 0), + feeRecipientAddress: feeRecipientAddressLeft, + }), + await orderFactoryLeft.newSignedOrderAsync({ + makerAddress: makerAddressLeft, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(72, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(36, 0), + feeRecipientAddress: feeRecipientAddressLeft, + }), + ]; + const rightOrders = [ + await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(15, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(30, 0), + feeRecipientAddress: feeRecipientAddressRight, + }), + await orderFactoryRight.newSignedOrderAsync({ + makerAddress: makerAddressRight, + makerAssetAmount: Web3Wrapper.toBaseUnitAmount(22, 0), + takerAssetAmount: Web3Wrapper.toBaseUnitAmount(44, 0), + feeRecipientAddress: feeRecipientAddressRight, + }), + ]; + const expectedTransferAmounts = [ + { + // Left Maker + leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(2, 0), + leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + // Right Maker + rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(1, 0), + rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('6.6666666666666666'), + 16, + ), // 6.66% + // Taker + leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('6.6666666666666666'), + 16, + ), // 6.66% + }, + { + // Left Maker + leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(28, 0), + leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('38.8888888888888888'), + 16, + ), // 38.88% + // Right Maker + rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(14, 0), + rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('93.3333333333333333'), + 16, + ), // 93.33% + // Taker + leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('38.8888888888888888'), + 16, + ), // 38.88% + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('93.3333333333333333'), + 16, + ), // 93.33% + }, + { + // Left Maker + leftMakerAssetSoldByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount(44, 0), + leftMakerFeeAssetPaidByLeftMakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('61.1111111111111111'), + 16, + ), // 61.11% + // Right Maker + rightMakerAssetSoldByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(22, 0), + rightMakerFeeAssetPaidByRightMakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + // Taker + leftTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount( + new BigNumber('61.1111111111111111'), + 16, + ), // 61.11% + rightTakerFeeAssetPaidByTakerAmount: Web3Wrapper.toBaseUnitAmount(100, 16), // 100% + }, + ]; + await matchOrderTester.batchMatchOrdersAndAssertEffectsAsync( + { + leftOrders, + rightOrders, + leftOrdersTakerAssetFilledAmounts: [ZERO, ZERO], + rightOrdersTakerAssetFilledAmounts: [ZERO, ZERO], + }, + takerAddress, + [[0, 0], [1, 0], [1, 1]], + expectedTransferAmounts, + true, + ); + }); }); }); // tslint:disable-line:max-file-line-count diff --git a/contracts/exchange/test/utils/match_order_tester.ts b/contracts/exchange/test/utils/match_order_tester.ts index 7f116fc0f6..7956e774c4 100644 --- a/contracts/exchange/test/utils/match_order_tester.ts +++ b/contracts/exchange/test/utils/match_order_tester.ts @@ -1112,19 +1112,19 @@ function convertToBatchMatchResults(results: BatchMatchResults): BatchMatchedFil */ function convertToMatchResults(result: MatchResults): MatchedFillResults { // If the left spread is negative, set it to zero - let leftMakerAssetSpreadAmount = result.fills[0].makerAssetFilledAmount.minus( + let profitInLeftMakerAsset = result.fills[0].makerAssetFilledAmount.minus( result.fills[1].takerAssetFilledAmount, ); - if (leftMakerAssetSpreadAmount.isLessThanOrEqualTo(ZERO)) { - leftMakerAssetSpreadAmount = ZERO; + if (profitInLeftMakerAsset.isLessThanOrEqualTo(ZERO)) { + profitInLeftMakerAsset = ZERO; } // If the right spread is negative, set it to zero - let rightMakerAssetSpreadAmount = result.fills[1].makerAssetFilledAmount.minus( + let profitInRightMakerAsset = result.fills[1].makerAssetFilledAmount.minus( result.fills[0].takerAssetFilledAmount, ); - if (rightMakerAssetSpreadAmount.isLessThanOrEqualTo(ZERO)) { - rightMakerAssetSpreadAmount = ZERO; + if (profitInRightMakerAsset.isLessThanOrEqualTo(ZERO)) { + profitInRightMakerAsset = ZERO; } const matchedFillResults: MatchedFillResults = { @@ -1140,8 +1140,8 @@ function convertToMatchResults(result: MatchResults): MatchedFillResults { makerFeePaid: result.fills[1].makerFeePaid, takerFeePaid: result.fills[1].takerFeePaid, }, - leftMakerAssetSpreadAmount, - rightMakerAssetSpreadAmount, + profitInLeftMakerAsset, + profitInRightMakerAsset, }; return matchedFillResults; } diff --git a/contracts/test-utils/src/types.ts b/contracts/test-utils/src/types.ts index 8b115fbdff..b085a34d0c 100644 --- a/contracts/test-utils/src/types.ts +++ b/contracts/test-utils/src/types.ts @@ -154,8 +154,8 @@ export interface FillResults { export interface MatchedFillResults { left: FillResults; right: FillResults; - leftMakerAssetSpreadAmount: BigNumber; - rightMakerAssetSpreadAmount: BigNumber; + profitInLeftMakerAsset: BigNumber; + profitInRightMakerAsset: BigNumber; } export interface BatchMatchedFillResults {