From 29971f36cf5c0096dfd3b52c014c52b812a9e9ac Mon Sep 17 00:00:00 2001
From: Remco Bloemen <remco@wicked.ventures>
Date: Fri, 24 Aug 2018 13:41:56 -0700
Subject: Split into assertFillable and assertValidFill

---
 .../2.0.0/protocol/Exchange/MixinExchangeCore.sol  | 54 ++++++++++++----------
 .../2.0.0/protocol/Exchange/MixinMatchOrders.sol   | 18 ++++++--
 2 files changed, 43 insertions(+), 29 deletions(-)

diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
index b8b3899e8..4fd71092c 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
@@ -91,14 +91,11 @@ contract MixinExchangeCore is
         // Fetch order info
         OrderInfo memory orderInfo = getOrderInfo(order);
         
-        // An order can only be filled if its status is FILLABLE.
-        require(
-            orderInfo.orderStatus == uint8(OrderStatus.FILLABLE),
-            "ORDER_UNFILLABLE"
-        );
-
         // Fetch taker address
         address takerAddress = getCurrentContextAddress();
+        
+        // Assert that the order is fillable by taker
+        assertFillableOrder(order, orderInfo, taker, signature);
 
         // Get amount of takerAsset to fill
         uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount);
@@ -110,8 +107,6 @@ contract MixinExchangeCore is
         // Validate context
         assertValidFill(
             order,
-            orderInfo,
-            takerAddress,
             takerAssetFillAmount,
             takerAssetFilledAmount,
             fillResults.makerAssetFilledAmount,
@@ -266,22 +261,16 @@ contract MixinExchangeCore is
             order.takerAssetData
         );
     }
-
+    
     /// @dev Validates context for fillOrder. Succeeds or throws.
     /// @param order to be filled.
     /// @param orderInfo OrderStatus, orderHash, and amount already filled of order.
     /// @param takerAddress Address of order taker.
-    /// @param takerAssetFillAmount Desired amount of order to fill by taker.
-    /// @param takerAssetFilledAmount Amount of takerAsset that will be filled.
-    /// @param makerAssetFilledAmount Amount of makerAsset that will be transfered.
     /// @param signature Proof that the orders was created by its maker.
-    function assertValidFill(
+    function assertFillableOrder(
         Order memory order,
         OrderInfo memory orderInfo,
-        address takerAddress,
-        uint256 takerAssetFillAmount,
-        uint256 takerAssetFilledAmount,
-        uint256 makerAssetFilledAmount,
+        address taker,
         bytes memory signature
     )
         internal
@@ -292,13 +281,7 @@ contract MixinExchangeCore is
             orderInfo.orderStatus == uint8(OrderStatus.FILLABLE),
             "ORDER_UNFILLABLE"
         );
-
-        // Revert if fill amount is invalid
-        require(
-            takerAssetFillAmount != 0,
-            "INVALID_TAKER_AMOUNT"
-        );
-
+        
         // Validate sender is allowed to fill this order
         if (order.senderAddress != address(0)) {
             require(
@@ -332,6 +315,29 @@ contract MixinExchangeCore is
                 "INVALID_ORDER_SIGNATURE"
             );
         }
+    }
+    
+    /// @dev Validates context for fillOrder. Succeeds or throws.
+    /// @param order to be filled.
+    /// @param takerAssetFillAmount Desired amount of order to fill by taker.
+    /// @param takerAssetFilledAmount Amount of takerAsset that will be filled.
+    /// @param makerAssetFilledAmount Amount of makerAsset that will be transfered.
+    /// @param signature Proof that the orders was created by its maker.
+    function assertValidFill(
+        Order memory order,
+        uint256 takerAssetFillAmount,
+        uint256 takerAssetFilledAmount,
+        uint256 makerAssetFilledAmount,
+        bytes memory signature
+    )
+        internal
+        view
+    {
+        // Revert if fill amount is invalid
+        require(
+            takerAssetFillAmount != 0,
+            "INVALID_TAKER_AMOUNT"
+        );
         
         // Make sure taker does not pay more than desired amount
         // NOTE: This assertion should never fail, it is here
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
index 4d0411a63..5a5b0376e 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
@@ -61,8 +61,20 @@ contract MixinMatchOrders is
 
         // Fetch taker address
         address takerAddress = getCurrentContextAddress();
-
+        
         // Either our context is valid or we revert
+        assertFillableOrder(
+            leftOrder,
+            leftOrderInfo,
+            takerAddress,
+            leftSignature
+        );
+        assertFillableOrder(
+            righttOrder,
+            righttOrderInfo,
+            takerAddress,
+            leftSignature
+        );
         assertValidMatch(leftOrder, rightOrder);
 
         // Compute proportional fill amounts
@@ -76,8 +88,6 @@ contract MixinMatchOrders is
         // Validate fill contexts
         assertValidFill(
             leftOrder,
-            leftOrderInfo,
-            takerAddress,
             matchedFillResults.left.takerAssetFilledAmount,
             matchedFillResults.left.takerAssetFilledAmount,
             matchedFillResults.left.makerAssetFilledAmount,
@@ -85,8 +95,6 @@ contract MixinMatchOrders is
         );
         assertValidFill(
             rightOrder,
-            rightOrderInfo,
-            takerAddress,
             matchedFillResults.right.takerAssetFilledAmount,
             matchedFillResults.right.takerAssetFilledAmount,
             matchedFillResults.right.makerAssetFilledAmount,
-- 
cgit v1.2.3