Throw if the left or right orders do not compute the correct fill results. I like this better than just logging an error and failing silently.

5 files changed, 27 insertions, 37 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
index 9ea44beab..9f9ad6a52 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
@@ -78,19 +78,14 @@ contract MixinMatchOrders is
         validateMatchOrThrow(leftOrder, rightOrder);
 
         // Compute proportional fill amounts
-        uint8 matchedFillResultsStatus;
-        (   matchedFillResultsStatus,
-            matchedFillResults
-        ) = calculateMatchedFillResults(
+        matchedFillResults = calculateMatchedFillResults(
             leftOrder,
             rightOrder,
             leftOrderInfo.orderStatus,
             rightOrderInfo.orderStatus,
             leftOrderInfo.orderFilledAmount,
-            rightOrderInfo.orderFilledAmount);
-        if (matchedFillResultsStatus != uint8(Status.SUCCESS)) {
-            return matchedFillResults;
-        }
+            rightOrderInfo.orderFilledAmount
+        );
 
         // Validate fill contexts
         validateFillOrRevert(
@@ -231,7 +226,6 @@ contract MixinMatchOrders is
     /// @param rightOrderStatus Order status of right order.
     /// @param leftOrderFilledAmount Amount of left order already filled.
     /// @param rightOrderFilledAmount Amount of right order already filled.
-    /// @return status Return status of calculating fill amounts. Returns Status.SUCCESS on success.
     /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders.
     function calculateMatchedFillResults(
         Order memory leftOrder,
@@ -242,10 +236,7 @@ contract MixinMatchOrders is
         uint256 rightOrderFilledAmount
     )
         internal
-        returns (
-            uint8 status,
-            MatchedFillResults memory matchedFillResults
-        )
+        returns (MatchedFillResults memory matchedFillResults)
     {
         // We settle orders at the price point defined by the right order (profit goes to the order taker)
         // The constraint can be either on the left or on the right.
@@ -286,15 +277,17 @@ contract MixinMatchOrders is
         }
 
         // Calculate fill results for left order
+        uint8 status;
         (status, matchedFillResults.left) = calculateFillResults(
             leftOrder,
             leftOrderStatus,
             leftOrderFilledAmount,
             leftOrderAmountToFill
         );
-        if (status != uint8(Status.SUCCESS)) {
-            return (status, matchedFillResults);
-        }
+        require(
+            status == uint8(Status.SUCCESS),
+            FAILED_TO_CALCULATE_FILL_RESULTS_FOR_LEFT_ORDER
+        );
 
         // Calculate fill results for right order
         (status, matchedFillResults.right) = calculateFillResults(
@@ -303,9 +296,10 @@ contract MixinMatchOrders is
             rightOrderFilledAmount,
             rightOrderAmountToFill
         );
-        if (status != uint8(Status.SUCCESS)) {
-            return (status, matchedFillResults);
-        }
+        require(
+            status == uint8(Status.SUCCESS),
+            FAILED_TO_CALCULATE_FILL_RESULTS_FOR_RIGHT_ORDER
+        );
 
         // Calculate amount given to taker
         matchedFillResults.takerFillAmount = safeSub(
@@ -316,7 +310,7 @@ contract MixinMatchOrders is
         // Validate the fill results
         validateMatchOrThrow(matchedFillResults);
 
-        // Return status & fill results
-        return (status, matchedFillResults);
+        // Return fill results
+        return matchedFillResults;
     }
 }
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol
index 5ef0a52c5..84f621f64 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol
@@ -55,4 +55,6 @@ contract LibExchangeErrors {
     string constant NEGATIVE_SPREAD = "Matched orders must have a positive spread.";
     string constant MISCALCULATED_TRANSFER_AMOUNTS = "A miscalculation occurred: the left maker would receive more than the right maker would spend.";
     string constant ROUNDING_ERROR_TRANSFER_AMOUNTS = "A rounding error occurred when calculating transfer amounts for matched orders.";
+    string constant FAILED_TO_CALCULATE_FILL_RESULTS_FOR_LEFT_ORDER = "Failed to calculate fill results for left order.";
+    string constant FAILED_TO_CALCULATE_FILL_RESULTS_FOR_RIGHT_ORDER = "Failed to calculate fill results for right order.";
 }
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol
index 6bae03ff2..27d65c69f 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibMath.sol
@@ -61,7 +61,7 @@ contract LibMath is
         require(
             !isRoundingError(numerator, denominator, target),
             ROUNDING_ERROR_ON_PARTIAL_AMOUNT
-            );
+        );
         return getPartialAmount(numerator, denominator, target);
     }
 
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol
index 60a266250..d8b9a22c3 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MMatchOrders.sol
@@ -59,7 +59,6 @@ contract MMatchOrders is
     /// @param rightOrderStatus Order status of right order.
     /// @param leftOrderFilledAmount Amount of left order already filled.
     /// @param rightOrderFilledAmount Amount of right order already filled.
-    /// @return status Return status of calculating fill amounts. Returns Status.SUCCESS on success.
     /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders.
     function calculateMatchedFillResults(
         LibOrder.Order memory leftOrder,
@@ -70,8 +69,5 @@ contract MMatchOrders is
         uint256 rightOrderFilledAmount
     )
         internal
-        returns (
-            uint8 status,
-            LibFillResults.MatchedFillResults memory matchedFillResults
-        );
+        returns (LibFillResults.MatchedFillResults memory matchedFillResults);
 }
diff --git a/packages/contracts/test/exchange/match_orders.ts b/packages/contracts/test/exchange/match_orders.ts
index 6ba88b5fb..74e4f6760 100644
--- a/packages/contracts/test/exchange/match_orders.ts
+++ b/packages/contracts/test/exchange/match_orders.ts
@@ -647,7 +647,7 @@ describe('matchOrders', () => {
             );
         });
 
-        it('Should not transfer any amounts if left order is not fillable', async () => {
+        it('Should throw if left order is not fillable', async () => {
             // Create orders to match
             const signedOrderLeft = orderFactoryLeft.newSignedOrder({
                 makerAddress: makerAddressLeft,
@@ -668,13 +668,12 @@ describe('matchOrders', () => {
             // Cancel left order
             await exchangeWrapper.cancelOrderAsync(signedOrderLeft, signedOrderLeft.makerAddress);
             // Match orders
-            await exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress);
-            // Verify balances did not change
-            const newBalances = await erc20Wrapper.getBalancesAsync();
-            expect(newBalances).to.be.deep.equal(erc20BalancesByOwner);
+            return expect(
+                exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress),
+            ).to.be.rejectedWith(constants.REVERT);
         });
 
-        it('Should not transfer any amounts if right order is not fillable', async () => {
+        it('Should throw if right order is not fillable', async () => {
             // Create orders to match
             const signedOrderLeft = orderFactoryLeft.newSignedOrder({
                 makerAddress: makerAddressLeft,
@@ -695,10 +694,9 @@ describe('matchOrders', () => {
             // Cancel right order
             await exchangeWrapper.cancelOrderAsync(signedOrderRight, signedOrderRight.makerAddress);
             // Match orders
-            await exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress);
-            // Verify balances did not change
-            const newBalances = await erc20Wrapper.getBalancesAsync();
-            expect(newBalances).to.be.deep.equal(erc20BalancesByOwner);
+            return expect(
+                exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress),
+            ).to.be.rejectedWith(constants.REVERT);
         });
 
         it('should throw if there is not a positive spread', async () => {