safeGetPartialAmount (#1035)

* Added Test "Should transfer correct amounts when left order is fully filled and values pass isRoundingErrorCeil but fail isRoundingErrorFloor"

* Added RoundingError exception to reference function for getPartialAmount

* Added RoundingError exception to reference function for getPartialAmount

* Added isRoundingErrorCeil to getPartialAmountCeil reference funtion

* Computed new values  for "Should give right maker a better buy price when correct price is not integral" that does not have a rounding error

* Almost all tests for match orders are passing after adding isRoundingErrorCeil check

* WIP commit: Added rounding error checks to getPartialAmount

* WIP commit: Added rounding error checks to getPartialAmount

* Use safe versions of getPartialAmount

* Update Exchange internals tests

* Run linter

* Found new values for "Should transfer correct amounts when right order fill amount deviates from amount derived by `Exchange.fillOrder`"

* Fixed merge conflicts

* Run all tests

* Cleaned up some comments on match Orders tests

* Fix tests for geth

5 files changed, 128 insertions, 23 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 be163ec97..64b1f7665 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
@@ -404,16 +404,6 @@ contract MixinExchangeCore is
             safeMul(order.makerAssetAmount, takerAssetFilledAmount),
             "INVALID_FILL_PRICE"
         );
-        
-        // Validate fill order rounding
-        require(
-            !isRoundingErrorFloor(
-                takerAssetFilledAmount,
-                order.takerAssetAmount,
-                order.makerAssetAmount
-            ),
-            "ROUNDING_ERROR"
-        );
     }
 
     /// @dev Validates context for cancelOrder. Succeeds or throws.
@@ -463,17 +453,17 @@ contract MixinExchangeCore is
     {
         // Compute proportional transfer amounts
         fillResults.takerAssetFilledAmount = takerAssetFilledAmount;
-        fillResults.makerAssetFilledAmount = getPartialAmountFloor(
+        fillResults.makerAssetFilledAmount = safeGetPartialAmountFloor(
             takerAssetFilledAmount,
             order.takerAssetAmount,
             order.makerAssetAmount
         );
-        fillResults.makerFeePaid = getPartialAmountFloor(
+        fillResults.makerFeePaid = safeGetPartialAmountFloor(
             takerAssetFilledAmount,
             order.takerAssetAmount,
             order.makerFee
         );
-        fillResults.takerFeePaid = getPartialAmountFloor(
+        fillResults.takerFeePaid = safeGetPartialAmountFloor(
             takerAssetFilledAmount,
             order.takerAssetAmount,
             order.takerFee
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 075a610b5..b4f6bdb26 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinMatchOrders.sol
@@ -177,13 +177,13 @@ contract MixinMatchOrders is
     {
         // Derive maker asset amounts for left & right orders, given store taker assert amounts
         uint256 leftTakerAssetAmountRemaining = safeSub(leftOrder.takerAssetAmount, leftOrderTakerAssetFilledAmount);
-        uint256 leftMakerAssetAmountRemaining = getPartialAmountFloor(
+        uint256 leftMakerAssetAmountRemaining = safeGetPartialAmountFloor(
             leftOrder.makerAssetAmount,
             leftOrder.takerAssetAmount,
             leftTakerAssetAmountRemaining
         );
         uint256 rightTakerAssetAmountRemaining = safeSub(rightOrder.takerAssetAmount, rightOrderTakerAssetFilledAmount);
-        uint256 rightMakerAssetAmountRemaining = getPartialAmountFloor(
+        uint256 rightMakerAssetAmountRemaining = safeGetPartialAmountFloor(
             rightOrder.makerAssetAmount,
             rightOrder.takerAssetAmount,
             rightTakerAssetAmountRemaining
@@ -205,7 +205,7 @@ contract MixinMatchOrders is
             matchedFillResults.left.takerAssetFilledAmount = matchedFillResults.right.makerAssetFilledAmount;
             // Round down to ensure the maker's exchange rate does not exceed the price specified by the order. 
             // We favor the maker when the exchange rate must be rounded.
-            matchedFillResults.left.makerAssetFilledAmount = getPartialAmountFloor(
+            matchedFillResults.left.makerAssetFilledAmount = safeGetPartialAmountFloor(
                 leftOrder.makerAssetAmount,
                 leftOrder.takerAssetAmount,
                 matchedFillResults.left.takerAssetFilledAmount
@@ -217,7 +217,7 @@ contract MixinMatchOrders is
             matchedFillResults.right.makerAssetFilledAmount = matchedFillResults.left.takerAssetFilledAmount;
             // Round up to ensure the maker's exchange rate does not exceed the price specified by the order.
             // We favor the maker when the exchange rate must be rounded.
-            matchedFillResults.right.takerAssetFilledAmount = getPartialAmountCeil(
+            matchedFillResults.right.takerAssetFilledAmount = safeGetPartialAmountCeil(
                 rightOrder.takerAssetAmount,
                 rightOrder.makerAssetAmount,
                 matchedFillResults.right.makerAssetFilledAmount
@@ -231,24 +231,24 @@ contract MixinMatchOrders is
         );
 
         // Compute fees for left order
-        matchedFillResults.left.makerFeePaid = getPartialAmountFloor(
+        matchedFillResults.left.makerFeePaid = safeGetPartialAmountFloor(
             matchedFillResults.left.makerAssetFilledAmount,
             leftOrder.makerAssetAmount,
             leftOrder.makerFee
         );
-        matchedFillResults.left.takerFeePaid = getPartialAmountFloor(
+        matchedFillResults.left.takerFeePaid = safeGetPartialAmountFloor(
             matchedFillResults.left.takerAssetFilledAmount,
             leftOrder.takerAssetAmount,
             leftOrder.takerFee
         );
 
         // Compute fees for right order
-        matchedFillResults.right.makerFeePaid = getPartialAmountFloor(
+        matchedFillResults.right.makerFeePaid = safeGetPartialAmountFloor(
             matchedFillResults.right.makerAssetFilledAmount,
             rightOrder.makerAssetAmount,
             rightOrder.makerFee
         );
-        matchedFillResults.right.takerFeePaid = getPartialAmountFloor(
+        matchedFillResults.right.takerFeePaid = safeGetPartialAmountFloor(
             matchedFillResults.right.takerAssetFilledAmount,
             rightOrder.takerAssetAmount,
             rightOrder.takerFee
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol
index 0e0fba5d2..57fd53f29 100644
--- a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol
+++ b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibMath.sol
@@ -26,11 +26,48 @@ contract LibMath is
 {
 
     /// @dev Calculates partial value given a numerator and denominator rounded down.
+    ///      Reverts if rounding error is >= 0.1%
     /// @param numerator Numerator.
     /// @param denominator Denominator.
     /// @param target Value to calculate partial of.
     /// @return Partial value of target rounded down.
-    function getPartialAmountFloor(
+    function safeGetPartialAmountFloor(
+        uint256 numerator,
+        uint256 denominator,
+        uint256 target
+    )
+        internal
+        pure
+        returns (uint256 partialAmount)
+    {
+        require(
+            denominator > 0,
+            "DIVISION_BY_ZERO"
+        );
+
+        require(
+            !isRoundingErrorFloor(
+                numerator,
+                denominator,
+                target
+            ),
+            "ROUNDING_ERROR"
+        );
+        
+        partialAmount = safeDiv(
+            safeMul(numerator, target),
+            denominator
+        );
+        return partialAmount;
+    }
+
+    /// @dev Calculates partial value given a numerator and denominator rounded down.
+    ///      Reverts if rounding error is >= 0.1%
+    /// @param numerator Numerator.
+    /// @param denominator Denominator.
+    /// @param target Value to calculate partial of.
+    /// @return Partial value of target rounded up.
+    function safeGetPartialAmountCeil(
         uint256 numerator,
         uint256 denominator,
         uint256 target
@@ -43,7 +80,48 @@ contract LibMath is
             denominator > 0,
             "DIVISION_BY_ZERO"
         );
+
+        require(
+            !isRoundingErrorCeil(
+                numerator,
+                denominator,
+                target
+            ),
+            "ROUNDING_ERROR"
+        );
         
+        // safeDiv computes `floor(a / b)`. We use the identity (a, b integer):
+        //       ceil(a / b) = floor((a + b - 1) / b)
+        // To implement `ceil(a / b)` using safeDiv.
+        partialAmount = safeDiv(
+            safeAdd(
+                safeMul(numerator, target),
+                safeSub(denominator, 1)
+            ),
+            denominator
+        );
+        return partialAmount;
+    }
+
+    /// @dev Calculates partial value given a numerator and denominator rounded down.
+    /// @param numerator Numerator.
+    /// @param denominator Denominator.
+    /// @param target Value to calculate partial of.
+    /// @return Partial value of target rounded down.
+    function getPartialAmountFloor(
+        uint256 numerator,
+        uint256 denominator,
+        uint256 target
+    )
+        internal
+        pure
+        returns (uint256 partialAmount)
+    {
+        require(
+            denominator > 0,
+            "DIVISION_BY_ZERO"
+        );
+
         partialAmount = safeDiv(
             safeMul(numerator, target),
             denominator
@@ -69,7 +147,7 @@ contract LibMath is
             denominator > 0,
             "DIVISION_BY_ZERO"
         );
-        
+
         // safeDiv computes `floor(a / b)`. We use the identity (a, b integer):
         //       ceil(a / b) = floor((a + b - 1) / b)
         // To implement `ceil(a / b)` using safeDiv.
diff --git a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
index 8bfdd2e66..4f4e28302 100644
--- a/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
+++ b/packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
@@ -25,6 +25,7 @@ import "../../protocol/Exchange/interfaces/IExchange.sol";
 import "../../protocol/Exchange/libs/LibOrder.sol";
 
 
+// solhint-disable no-unused-vars
 contract ReentrantERC20Token is
     ERC20Token
 {
diff --git a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
index da9313e02..27187f8f8 100644
--- a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
+++ b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
@@ -63,6 +63,42 @@ contract TestExchangeInternals is
     }
 
     /// @dev Calculates partial value given a numerator and denominator.
+    ///      Reverts if rounding error is >= 0.1%
+    /// @param numerator Numerator.
+    /// @param denominator Denominator.
+    /// @param target Value to calculate partial of.
+    /// @return Partial value of target.
+    function publicSafeGetPartialAmountFloor(
+        uint256 numerator,
+        uint256 denominator,
+        uint256 target
+    )
+        public
+        pure
+        returns (uint256 partialAmount)
+    {
+        return safeGetPartialAmountFloor(numerator, denominator, target);
+    }
+
+    /// @dev Calculates partial value given a numerator and denominator.
+    ///      Reverts if rounding error is >= 0.1%
+    /// @param numerator Numerator.
+    /// @param denominator Denominator.
+    /// @param target Value to calculate partial of.
+    /// @return Partial value of target.
+    function publicSafeGetPartialAmountCeil(
+        uint256 numerator,
+        uint256 denominator,
+        uint256 target
+    )
+        public
+        pure
+        returns (uint256 partialAmount)
+    {
+        return safeGetPartialAmountCeil(numerator, denominator, target);
+    }
+
+    /// @dev Calculates partial value given a numerator and denominator.
     /// @param numerator Numerator.
     /// @param denominator Denominator.
     /// @param target Value to calculate partial of.