Merge pull request #2043 from jalextowle/feature/contracts/3.0/order-matching-unit-tests

MatchOrders Unit Tests

contracts/exchange/contracts/src/MixinMatchOrders.sol

@@ -473,6 +473,116 @@ contract MixinMatchOrders is
         );
     }
 
+    /// @dev Settles matched order by transferring appropriate funds between order makers, taker, and fee recipient.
+    /// @param leftOrderHash First matched order hash.
+    /// @param rightOrderHash Second matched order hash.
+    /// @param leftOrder First matched order.
+    /// @param rightOrder Second matched order.
+    /// @param takerAddress Address that matched the orders. The taker receives the spread between orders as profit.
+    /// @param matchedFillResults Struct holding amounts to transfer between makers, taker, and fee recipients.
+    function _settleMatchedOrders(
+        bytes32 leftOrderHash,
+        bytes32 rightOrderHash,
+        LibOrder.Order memory leftOrder,
+        LibOrder.Order memory rightOrder,
+        address takerAddress,
+        LibFillResults.MatchedFillResults memory matchedFillResults
+    )
+        internal
+    {
+        address leftFeeRecipientAddress = leftOrder.feeRecipientAddress;
+        address rightFeeRecipientAddress = rightOrder.feeRecipientAddress;
+
+        // Right maker asset -> left maker
+        _dispatchTransferFrom(
+            rightOrderHash,
+            rightOrder.makerAssetData,
+            rightOrder.makerAddress,
+            leftOrder.makerAddress,
+            matchedFillResults.left.takerAssetFilledAmount
+        );
+
+        // Left maker asset -> right maker
+        _dispatchTransferFrom(
+            leftOrderHash,
+            leftOrder.makerAssetData,
+            leftOrder.makerAddress,
+            rightOrder.makerAddress,
+            matchedFillResults.right.takerAssetFilledAmount
+        );
+
+        // Right maker fee -> right fee recipient
+        _dispatchTransferFrom(
+            rightOrderHash,
+            rightOrder.makerFeeAssetData,
+            rightOrder.makerAddress,
+            rightFeeRecipientAddress,
+            matchedFillResults.right.makerFeePaid
+        );
+
+        // Left maker fee -> left fee recipient
+        _dispatchTransferFrom(
+            leftOrderHash,
+            leftOrder.makerFeeAssetData,
+            leftOrder.makerAddress,
+            leftFeeRecipientAddress,
+            matchedFillResults.left.makerFeePaid
+        );
+
+        // Settle taker profits.
+        _dispatchTransferFrom(
+            leftOrderHash,
+            leftOrder.makerAssetData,
+            leftOrder.makerAddress,
+            takerAddress,
+            matchedFillResults.profitInLeftMakerAsset
+        );
+        _dispatchTransferFrom(
+            rightOrderHash,
+            rightOrder.makerAssetData,
+            rightOrder.makerAddress,
+            takerAddress,
+            matchedFillResults.profitInRightMakerAsset
+        );
+
+        // Settle taker fees.
+        if (
+            leftFeeRecipientAddress == rightFeeRecipientAddress &&
+            leftOrder.takerFeeAssetData.equals(rightOrder.takerFeeAssetData)
+        ) {
+            // Fee recipients and taker fee assets are identical, so we can
+            // transfer them in one go.
+            _dispatchTransferFrom(
+                leftOrderHash,
+                leftOrder.takerFeeAssetData,
+                takerAddress,
+                leftFeeRecipientAddress,
+                _safeAdd(
+                    matchedFillResults.left.takerFeePaid,
+                    matchedFillResults.right.takerFeePaid
+                )
+            );
+        } else {
+            // Right taker fee -> right fee recipient
+            _dispatchTransferFrom(
+                rightOrderHash,
+                rightOrder.takerFeeAssetData,
+                takerAddress,
+                rightFeeRecipientAddress,
+                matchedFillResults.right.takerFeePaid
+            );
+
+            // Left taker fee -> left fee recipient
+            _dispatchTransferFrom(
+                leftOrderHash,
+                leftOrder.takerFeeAssetData,
+                takerAddress,
+                leftFeeRecipientAddress,
+                matchedFillResults.left.takerFeePaid
+            );
+        }
+    }
+
     /// @dev Match complementary orders that have a profitable spread.
     ///      Each order is filled at their respective price point, and
     ///      the matcher receives a profit denominated in the left maker asset.
@@ -708,115 +818,4 @@ contract MixinMatchOrders is
 
         return matchedFillResults;
     }
-
-    /// @dev Settles matched order by transferring appropriate funds between order makers, taker, and fee recipient.
-    /// @param leftOrderHash First matched order hash.
-    /// @param rightOrderHash Second matched order hash.
-    /// @param leftOrder First matched order.
-    /// @param rightOrder Second matched order.
-    /// @param takerAddress Address that matched the orders. The taker receives the spread between orders as profit.
-    /// @param matchedFillResults Struct holding amounts to transfer between makers, taker, and fee recipients.
-    function _settleMatchedOrders(
-        bytes32 leftOrderHash,
-        bytes32 rightOrderHash,
-        LibOrder.Order memory leftOrder,
-        LibOrder.Order memory rightOrder,
-        address takerAddress,
-        LibFillResults.MatchedFillResults memory matchedFillResults
-    )
-        private
-    {
-        address leftFeeRecipientAddress = leftOrder.feeRecipientAddress;
-        address rightFeeRecipientAddress = rightOrder.feeRecipientAddress;
-
-        // Right maker asset -> left maker
-        _dispatchTransferFrom(
-            rightOrderHash,
-            rightOrder.makerAssetData,
-            rightOrder.makerAddress,
-            leftOrder.makerAddress,
-            matchedFillResults.left.takerAssetFilledAmount
-        );
-
-        // Left maker asset -> right maker
-        _dispatchTransferFrom(
-            leftOrderHash,
-            leftOrder.makerAssetData,
-            leftOrder.makerAddress,
-            rightOrder.makerAddress,
-            matchedFillResults.right.takerAssetFilledAmount
-        );
-
-        // Right maker fee -> right fee recipient
-        _dispatchTransferFrom(
-            rightOrderHash,
-            rightOrder.makerFeeAssetData,
-            rightOrder.makerAddress,
-            rightFeeRecipientAddress,
-            matchedFillResults.right.makerFeePaid
-        );
-
-        // Left maker fee -> left fee recipient
-        _dispatchTransferFrom(
-            leftOrderHash,
-            leftOrder.makerFeeAssetData,
-            leftOrder.makerAddress,
-            leftFeeRecipientAddress,
-            matchedFillResults.left.makerFeePaid
-        );
-
-        // Settle taker profits.
-        _dispatchTransferFrom(
-            leftOrderHash,
-            leftOrder.makerAssetData,
-            leftOrder.makerAddress,
-            takerAddress,
-            matchedFillResults.profitInLeftMakerAsset
-        );
-
-        _dispatchTransferFrom(
-            rightOrderHash,
-            rightOrder.makerAssetData,
-            rightOrder.makerAddress,
-            takerAddress,
-            matchedFillResults.profitInRightMakerAsset
-        );
-
-        // Settle taker fees.
-        if (
-            leftFeeRecipientAddress == rightFeeRecipientAddress &&
-            leftOrder.takerFeeAssetData.equals(rightOrder.takerFeeAssetData)
-        ) {
-            // Fee recipients and taker fee assets are identical, so we can
-            // transfer them in one go.
-            _dispatchTransferFrom(
-                leftOrderHash,
-                leftOrder.takerFeeAssetData,
-                takerAddress,
-                leftFeeRecipientAddress,
-                _safeAdd(
-                    matchedFillResults.left.takerFeePaid,
-                    matchedFillResults.right.takerFeePaid
-                )
-            );
-        } else {
-            // Right taker fee -> right fee recipient
-            _dispatchTransferFrom(
-                rightOrderHash,
-                rightOrder.takerFeeAssetData,
-                takerAddress,
-                rightFeeRecipientAddress,
-                matchedFillResults.right.takerFeePaid
-            );
-
-            // Left taker fee -> left fee recipient
-            _dispatchTransferFrom(
-                leftOrderHash,
-                leftOrder.takerFeeAssetData,
-                takerAddress,
-                leftFeeRecipientAddress,
-                matchedFillResults.left.takerFeePaid
-            );
-        }
-    }
 }

contracts/exchange/contracts/test/TestExchangeInternals.sol

@@ -39,6 +39,16 @@ contract TestExchangeInternals is
         Exchange(chainId)
     {}
 
+    function assertValidMatch(
+        LibOrder.Order memory leftOrder,
+        LibOrder.Order memory rightOrder
+    )
+        public
+        view
+    {
+        _assertValidMatch(leftOrder, rightOrder);
+    }
+
     function calculateFillResults(
         Order memory order,
         uint256 takerAssetFilledAmount
@@ -50,6 +60,43 @@ contract TestExchangeInternals is
         return _calculateFillResults(order, takerAssetFilledAmount);
     }
 
+    function calculateCompleteFillBoth(
+        uint256 leftMakerAssetAmountRemaining,
+        uint256 leftTakerAssetAmountRemaining,
+        uint256 rightMakerAssetAmountRemaining,
+        uint256 rightTakerAssetAmountRemaining
+    )
+        public
+        pure
+        returns (MatchedFillResults memory fillResults)
+    {
+        _calculateCompleteFillBoth(
+            fillResults,
+            leftMakerAssetAmountRemaining,
+            leftTakerAssetAmountRemaining,
+            rightMakerAssetAmountRemaining,
+            rightTakerAssetAmountRemaining
+        );
+        return fillResults;
+    }
+
+    function calculateCompleteRightFill(
+        LibOrder.Order memory leftOrder,
+        uint256 rightMakerAssetAmountRemaining,
+        uint256 rightTakerAssetAmountRemaining
+    )
+        public
+        pure
+        returns (MatchedFillResults memory fillResults)
+    {
+        _calculateCompleteRightFill(
+            fillResults,
+            leftOrder,
+            rightMakerAssetAmountRemaining,
+            rightTakerAssetAmountRemaining
+        );
+    }
+
     /// @dev Call `_updateFilledState()` but first set `filled[order]` to
     ///      `orderTakerAssetFilledAmount`.
     function testUpdateFilledState(
@@ -82,6 +129,26 @@ contract TestExchangeInternals is
         _settleOrder(orderHash, order, takerAddress, fillResults);
     }
 
+    function settleMatchOrders(
+        bytes32 leftOrderHash,
+        bytes32 rightOrderHash,
+        LibOrder.Order memory leftOrder,
+        LibOrder.Order memory rightOrder,
+        address takerAddress,
+        LibFillResults.MatchedFillResults memory matchedFillResults
+    )
+        public
+    {
+        _settleMatchedOrders(
+            leftOrderHash,
+            rightOrderHash,
+            leftOrder,
+            rightOrder,
+            takerAddress,
+            matchedFillResults
+        );
+    }
+
     /// @dev Overidden to only log arguments so we can test `_settleOrder()`.
     function _dispatchTransferFrom(
         bytes32 orderHash,

contracts/exchange/package.json

@@ -34,8 +34,8 @@
         "lint-contracts": "solhint -c ../.solhint.json contracts/**/**/**/**/*.sol"
     },
     "config": {
-        "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxyDispatcher|IEIP1271Wallet|IExchange|IExchangeCore|IMatchOrders|ISignatureValidator|ITransactions|IWallet|IWrapperFunctions|IsolatedExchange|ReentrancyTester|TestAssetProxyDispatcher|TestExchangeInternals|TestLibExchangeRichErrorDecoder|TestSignatureValidator|TestValidatorWallet|TestWrapperFunctions|Whitelist).json",
+        "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually.",
-        "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually."
+        "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxyDispatcher|IEIP1271Wallet|IExchange|IExchangeCore|IMatchOrders|ISignatureValidator|ITransactions|IWallet|IWrapperFunctions|IsolatedExchange|ReentrancyTester|TestAssetProxyDispatcher|TestExchangeInternals|TestLibExchangeRichErrorDecoder|TestSignatureValidator|TestValidatorWallet|TestWrapperFunctions|Whitelist).json"
     },
     "repository": {
         "type": "git",

contracts/exchange/test/dispatcher.ts

@@ -271,7 +271,7 @@ describe('AssetProxyDispatcher', () => {
             return expect(tx).to.revertWith(expectedError);
         });
 
-        it('should should revert with the correct error when assetData length < 4 bytes', async () => {
+        it('should revert with the correct error when assetData length < 4 bytes', async () => {
             await assetProxyDispatcher.registerAssetProxy.awaitTransactionSuccessAsync(erc20Proxy.address, {
                 from: owner,
             });

contracts/exchange/test/match_orders.ts

@@ -1336,7 +1336,7 @@ describe('matchOrders', () => {
         });
     });
     describe('matchOrdersWithMaximalFill', () => {
-        it('Should transfer correct amounts when right order is fully filled and values pass isRoundingErrorCeil but fail isRoundingErrorFloor', async () => {
+        it('should transfer correct amounts when right order is fully filled and values pass isRoundingErrorCeil but fail isRoundingErrorFloor', async () => {
             // Create orders to match
             const signedOrderLeft = await orderFactoryLeft.newSignedOrderAsync({
                 makerAddress: makerAddressLeft,