Added require reasons to MixinMatchOrders and cleaned up some comments.

6 files changed, 75 insertions, 40 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
index 58b5fa6b1..981c5984e 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
@@ -53,7 +53,7 @@ contract MixinExchangeCore is
 
     ////// Core exchange functions //////
 
-    /// @dev Gets information about an order.
+    /// @dev Gets information about an order: status, hash, and amount filled.
     /// @param order Order to gather information on.
     /// @return status Status of order. Statuses are defined in the LibStatus.Status struct.
     /// @return orderHash Keccak-256 EIP712 hash of the order.
@@ -105,14 +105,12 @@ contract MixinExchangeCore is
             orderStatus = uint8(Status.ORDER_CANCELLED);
             return (orderStatus, orderHash, takerAssetFilledAmount);
         }
-
-        // Validate order is not cancelled
         if (makerEpoch[order.makerAddress] > order.salt) {
             orderStatus = uint8(Status.ORDER_CANCELLED);
             return (orderStatus, orderHash, takerAssetFilledAmount);
         }
 
-        // Order is Fillable
+        // All other statuses are ruled out: order is Fillable
         orderStatus = uint8(Status.ORDER_FILLABLE);
         return (orderStatus, orderHash, takerAssetFilledAmount);
     }
@@ -136,6 +134,8 @@ contract MixinExchangeCore is
         internal
     {
         // Ensure order is valid
+        // An order can only be filled if it is Status.FILLABLE;
+        // however, only invalid statuses result in a throw.
         require(
             orderStatus != uint8(Status.ORDER_INVALID_MAKER_ASSET_AMOUNT),
             INVALID_ORDER_MAKER_ASSET_AMOUNT
@@ -269,7 +269,7 @@ contract MixinExchangeCore is
         public
         returns (FillResults memory fillResults)
     {
-        // Fetch current order status
+        // Fetch order info
         bytes32 orderHash;
         uint8 orderStatus;
         uint256 takerAssetFilledAmount;
@@ -309,6 +309,8 @@ contract MixinExchangeCore is
         internal
     {
         // Ensure order is valid
+        // An order can only be cancelled if it is Status.FILLABLE;
+        // however, only invalid statuses result in a throw.
         require(
             orderStatus != uint8(Status.ORDER_INVALID_MAKER_ASSET_AMOUNT),
             INVALID_ORDER_MAKER_ASSET_AMOUNT
@@ -372,9 +374,11 @@ contract MixinExchangeCore is
     }
 
     /// @dev After calling, the order can not be filled anymore.
-    /// @param order Order struct containing order specifications.
+    ///      Throws if order is invalid or sender does not have permission to cancel.
+    /// @param order Order to cancel. Order must be Status.FILLABLE.
     /// @return True if the order state changed to cancelled.
-    ///         False if the transaction was already cancelled or expired.
+    ///         False if the order was order was in a valid, but
+    ///               unfillable state (see LibStatus.STATUS for order states)
     function cancelOrder(Order memory order)
         public
         returns (bool)
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
index 76b021919..a333b8221 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinMatchOrders.sol
@@ -23,6 +23,7 @@ import "./libs/LibMath.sol";
 import "./libs/LibOrder.sol";
 import "./libs/LibStatus.sol";
 import "../../utils/LibBytes/LibBytes.sol";
+import "./libs/LibExchangeErrors.sol";
 
 contract MixinMatchOrders is
     SafeMath,
@@ -30,6 +31,7 @@ contract MixinMatchOrders is
     LibMath,
     LibStatus,
     LibOrder,
+    LibExchangeErrors,
     MExchangeCore,
     MMatchOrders,
     MSettlement,
@@ -45,22 +47,29 @@ contract MixinMatchOrders is
         internal
     {
         // The leftOrder maker asset must be the same as the rightOrder taker asset.
-        require(areBytesEqual(leftOrder.makerAssetData, rightOrder.takerAssetData));
+        require(
+            areBytesEqual(leftOrder.makerAssetData, rightOrder.takerAssetData),
+            ASSET_MISMATCH_MAKER_TAKER
+        );
 
         // The leftOrder taker asset must be the same as the rightOrder maker asset.
-        require(areBytesEqual(leftOrder.takerAssetData, rightOrder.makerAssetData));
+        require(
+            areBytesEqual(leftOrder.takerAssetData, rightOrder.makerAssetData),
+            ASSET_MISMATCH_TAKER_MAKER
+        );
 
         // Make sure there is a positive spread.
         // There is a positive spread iff the cost per unit bought (OrderA.MakerAmount/OrderA.TakerAmount) for each order is greater
         // than the profit per unit sold of the matched order (OrderB.TakerAmount/OrderB.MakerAmount).
         // This is satisfied by the equations below:
-        // <leftOrder.makerAssetAmount> / <leftOrder.takerAssetAmount> >= <rightOrder.takerAssetAmount> / <rightOrder.makerAssetAmount>
+        // <leftOrder.makerAssetAmount> / <leftOrder.takerAssetAmount> = <rightOrder.takerAssetAmount> / <rightOrder.makerAssetAmount>
         // AND
         // <rightOrder.makerAssetAmount> / <rightOrder.takerAssetAmount> >= <leftOrder.takerAssetAmount> / <leftOrder.makerAssetAmount>
         // These equations can be combined to get the following:
         require(
             safeMul(leftOrder.makerAssetAmount, rightOrder.makerAssetAmount) >=
-            safeMul(leftOrder.takerAssetAmount, rightOrder.takerAssetAmount)
+            safeMul(leftOrder.takerAssetAmount, rightOrder.takerAssetAmount),
+            NEGATIVE_SPREAD
         );
     }
 
@@ -69,11 +78,21 @@ contract MixinMatchOrders is
     function validateMatchedOrderFillResultsOrThrow(MatchedFillResults memory matchedFillResults)
         internal
     {
-        // The right order must spend at least as much as we're transferring to the left order's maker.
-        // If the amount transferred from the right order is greater than what is transferred, it is a rounding error amount.
+        // The right order must spend at least as much as we're transferring to the left order.
+        require(
+            matchedFillResults.right.makerAssetFilledAmount >=
+            matchedFillResults.left.takerAssetFilledAmount,
+            MISCALCULATED_TRANSFER_AMOUNTS
+        );
+        // If the amount transferred from the right order is different than what is transferred, it is a rounding error amount.
         // Ensure this difference is negligible by dividing the values with each other. The result should equal to ~1.
-        require(matchedFillResults.right.makerAssetFilledAmount >= matchedFillResults.left.takerAssetFilledAmount);
-        require(!isRoundingError(matchedFillResults.right.makerAssetFilledAmount, matchedFillResults.left.takerAssetFilledAmount, 1));
+        require(
+            !isRoundingError(
+                matchedFillResults.right.makerAssetFilledAmount,
+                matchedFillResults.left.takerAssetFilledAmount,
+                1),
+            ROUNDING_ERROR_TRANSFER_AMOUNTS
+        );
     }
 
     /// @dev Calculates partial value given a numerator and denominator.
@@ -89,7 +108,10 @@ contract MixinMatchOrders is
         internal pure
         returns (uint256 partialAmount)
     {
-        require(!isRoundingError(numerator, denominator, target));
+        require(
+            !isRoundingError(numerator, denominator, target),
+            ROUNDING_ERROR_ON_PARTIAL_AMOUNT
+            );
         return getPartialAmount(numerator, denominator, target);
     }
 
@@ -136,7 +158,7 @@ contract MixinMatchOrders is
             leftOrderAmountToFill = leftTakerAssetAmountRemaining;
 
             // The right order receives an amount proportional to how much was spent.
-            // TODO: Ensure rounding error is in the correct direction.
+            // TODO: Can we ensure rounding error is in the correct direction?
             rightOrderAmountToFill = safeGetPartialAmount(
                 rightOrder.takerAssetAmount,
                 rightOrder.makerAssetAmount,
@@ -146,7 +168,7 @@ contract MixinMatchOrders is
             rightOrderAmountToFill = rightTakerAssetAmountRemaining;
 
             // The left order receives an amount proportional to how much was spent.
-            // TODO: Ensure rounding error is in the correct direction.
+            // TODO: Can we ensure rounding error is in the correct direction?
             leftOrderAmountToFill = safeGetPartialAmount(
                 rightOrder.makerAssetAmount,
                 rightOrder.takerAssetAmount,
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol
index 3b67051d2..cde64ba74 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSettlement.sol
@@ -23,9 +23,11 @@ import "./mixins/MAssetProxyDispatcher.sol";
 import "./libs/LibOrder.sol";
 import "./libs/LibMath.sol";
 import "./mixins/MMatchOrders.sol";
+import "./mixins/LibExchangeErrors.sol";
 
 contract MixinSettlement is
     LibMath,
+    LibExchangeErrors,
     MMatchOrders,
     MSettlement,
     MAssetProxyDispatcher
@@ -134,7 +136,11 @@ contract MixinSettlement is
         // rightOrder.MakerAsset == leftOrder.TakerAsset
         // leftOrder.takerAssetFilledAmount ~ rightOrder.makerAssetFilledAmount
         // The change goes to right, not to taker.
-        assert(matchedFillResults.right.makerAssetFilledAmount >= matchedFillResults.left.takerAssetFilledAmount);
+        require(
+            matchedFillResults.right.makerAssetFilledAmount >=
+            matchedFillResults.left.takerAssetFilledAmount,
+            MISCALCULATED_TRANSFER_AMOUNTS
+        );
         dispatchTransferFrom(
             rightOrder.makerAssetData,
             rightOrder.makerAddress,
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 bf048e815..ba055dd8e 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol
@@ -48,4 +48,12 @@ contract LibExchangeErrors {
     string constant INVALID_SIGNATURE_LENGTH = "Invalid signature length.";
     string constant ILLEGAL_SIGNATURE_TYPE = "Illegal signature type.";
     string constant UNSUPPORTED_SIGNATURE_TYPE = "Unsupported signature type.";
+
+    // Order matching revert reasons
+    string constant ASSET_MISMATCH_MAKER_TAKER = "Left order maker asset is different from right order taker asset.";
+    string constant ASSET_MISMATCH_TAKER_MAKER = "Left order taker asset is different from right order maker asset.";
+    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 ROUNDING_ERROR_ON_PARTIAL_AMOUNT = "A rounding error occurred when calculating partial transfer amounts.";
 }
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol
index 416cf426d..cfd2678ba 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol
@@ -65,7 +65,7 @@ contract MExchangeCore is
         LibFillResults.FillResults memory fillResults)
         internal;
 
-    /// @dev Gets information about an order.
+    /// @dev Gets information about an order: status, hash, and amount filled.
     /// @param order Order to gather information on.
     /// @return status Status of order. Statuses are defined in the LibStatus.Status struct.
     /// @return orderHash Keccak-256 EIP712 hash of the order.
diff --git a/packages/contracts/src/utils/types.ts b/packages/contracts/src/utils/types.ts
index ce7fbcbe1..0e3b2c9a8 100644
--- a/packages/contracts/src/utils/types.ts
+++ b/packages/contracts/src/utils/types.ts
@@ -75,26 +75,21 @@ export interface Token {
 }
 
 export enum ExchangeStatus {
-    /// Default Status ///
-    INVALID, // General invalid status
-
-    /// General Exchange Statuses ///
-    SUCCESS, // Indicates a successful operation
-    ROUNDING_ERROR_TOO_LARGE, // Rounding error too large
-    INSUFFICIENT_BALANCE_OR_ALLOWANCE, // Insufficient balance or allowance for token transfer
-    TAKER_ASSET_FILL_AMOUNT_TOO_LOW, // takerAssetFillAmount is <= 0
-    INVALID_SIGNATURE, // Invalid signature
-    INVALID_SENDER, // Invalid sender
-    INVALID_TAKER, // Invalid taker
-    INVALID_MAKER, // Invalid maker
-
-    /// Order State Statuses ///
-    ORDER_INVALID_MAKER_ASSET_AMOUNT, // Order does not have a valid maker asset amount
-    ORDER_INVALID_TAKER_ASSET_AMOUNT, // Order does not have a valid taker asset amount
-    ORDER_FILLABLE, // Order is fillable
-    ORDER_EXPIRED, // Order has already expired
-    ORDER_FULLY_FILLED, // Order is fully filled
-    ORDER_CANCELLED, // Order has been cancelled
+    INVALID,
+    SUCCESS,
+    ROUNDING_ERROR_TOO_LARGE,
+    INSUFFICIENT_BALANCE_OR_ALLOWANCE,
+    TAKER_ASSET_FILL_AMOUNT_TOO_LOW,
+    INVALID_SIGNATURE,
+    INVALID_SENDER,
+    INVALID_TAKER,
+    INVALID_MAKER,
+    ORDER_INVALID_MAKER_ASSET_AMOUNT,
+    ORDER_INVALID_TAKER_ASSET_AMOUNT,
+    ORDER_FILLABLE,
+    ORDER_EXPIRED,
+    ORDER_FULLY_FILLED,
+    ORDER_CANCELLED,
 }
 
 export enum ContractName {