Updated batchMatchOrders to fix an edge case and added tests

2019-07-06 23:10:53 -05:00
parent c1985e6986
commit e0cff4b74e
5 changed files with 205 additions and 26 deletions
--- 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.
--- a/contracts/exchange/contracts/src/MixinMatchOrders.sol
+++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol
@@ -189,7 +189,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
                );
@@ -197,7 +197,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
                );
@@ -239,7 +239,7 @@ contract MixinMatchOrders is
            }

            // Calculate amount given to taker
-            matchedFillResults.leftMakerAssetSpreadAmount = _safeSub(
+            matchedFillResults.profitInLeftMakerAsset = _safeSub(
                matchedFillResults.left.makerAssetFilledAmount,
                matchedFillResults.right.takerAssetFilledAmount
            );
@@ -396,8 +396,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);

@@ -414,7 +414,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,
@@ -440,18 +440,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];
@@ -463,7 +465,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];
@@ -648,7 +649,7 @@ contract MixinMatchOrders is
            leftOrder.makerAssetData,
            leftOrder.makerAddress,
            takerAddress,
-            matchedFillResults.leftMakerAssetSpreadAmount
+            matchedFillResults.profitInLeftMakerAsset
        );

        _dispatchTransferFrom(
@@ -656,7 +657,7 @@ contract MixinMatchOrders is
            rightOrder.makerAssetData,
            rightOrder.makerAddress,
            takerAddress,
-            matchedFillResults.rightMakerAssetSpreadAmount
+            matchedFillResults.profitInRightMakerAsset
        );

        // Settle taker fees.
--- a/contracts/exchange/test/match_orders.ts
+++ b/contracts/exchange/test/match_orders.ts
@@ -2577,6 +2577,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 () => {
@@ -2737,6 +2814,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
--- 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;
 }
--- 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 {