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.

4 files changed, 19 insertions, 27 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);
 }