@0x/contracts-exchange: Avoid redundant transfer in matchOrders() when maker/feeRecipient and takerAssetData/makerFeeAssetData are the same.

`@0x/conracts-exchange`: Disable combinatorial tests by default. Can be run by setting env var `TEST_ALL=1`.

contracts/exchange/contracts/src/MixinMatchOrders.sol

@@ -15,6 +15,7 @@ pragma solidity ^0.5.5;
 pragma experimental ABIEncoderV2;
 
 import "@0x/contracts-utils/contracts/src/ReentrancyGuard.sol";
+import "@0x/contracts-utils/contracts/src/LibBytes.sol";
 import "@0x/contracts-exchange-libs/contracts/src/LibMath.sol";
 import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol";
 import "@0x/contracts-exchange-libs/contracts/src/LibFillResults.sol";
@@ -35,6 +36,8 @@ contract MixinMatchOrders is
     MTransactions,
     MExchangeRichErrors
 {
+    using LibBytes for bytes;
+
     /// @dev Match two complementary orders that have a profitable spread.
     ///      Each order is filled at their respective price point. However, the calculations are
     ///      carried out as though the orders are both being filled at the right order's price point.
@@ -280,21 +283,82 @@ contract MixinMatchOrders is
     )
         private
     {
-        // Order makers and taker
+        address leftFeeRecipientAddress = leftOrder.feeRecipientAddress;
-        _dispatchTransferFrom(
+        address rightFeeRecipientAddress = rightOrder.feeRecipientAddress;
-            leftOrderHash,
+
-            leftOrder.makerAssetData,
+        // Settle left order taker asset and maker fees.
-            leftOrder.makerAddress,
+        if (
-            rightOrder.makerAddress,
+            leftOrder.makerAddress == leftFeeRecipientAddress &&
-            matchedFillResults.right.takerAssetFilledAmount
+            leftOrder.makerFeeAssetData.equals(leftOrder.takerAssetData)
-        );
+        ) {
-        _dispatchTransferFrom(
+            // Maker is fee recipient and the maker fee asset is the same as the taker asset.
-            rightOrderHash,
+            // We can transfer the taker asset and maker fees in one go.
-            rightOrder.makerAssetData,
+            _dispatchTransferFrom(
-            rightOrder.makerAddress,
+                rightOrderHash,
-            leftOrder.makerAddress,
+                rightOrder.makerAssetData,
-            matchedFillResults.left.takerAssetFilledAmount
+                rightOrder.makerAddress,
-        );
+                leftFeeRecipientAddress,
+                _safeAdd(
+                    matchedFillResults.left.takerAssetFilledAmount,
+                    matchedFillResults.right.makerFeePaid
+                )
+            );
+        } else {
+            // Right maker asset -> left maker
+            _dispatchTransferFrom(
+                rightOrderHash,
+                rightOrder.makerAssetData,
+                rightOrder.makerAddress,
+                leftOrder.makerAddress,
+                matchedFillResults.left.takerAssetFilledAmount
+            );
+            // Left maker fee -> left fee recipient
+            _dispatchTransferFrom(
+                leftOrderHash,
+                leftOrder.makerFeeAssetData,
+                leftOrder.makerAddress,
+                leftFeeRecipientAddress,
+                matchedFillResults.left.makerFeePaid
+            );
+        }
+
+        // Settle right order taker asset and maker fees.
+        if (
+            rightOrder.makerAddress == rightFeeRecipientAddress &&
+            rightOrder.makerFeeAssetData.equals(rightOrder.takerAssetData)
+        ) {
+            // Maker is fee recipient and the maker fee asset is the same as the taker asset.
+            // We can transfer the taker asset and maker fees in one go.
+            _dispatchTransferFrom(
+                leftOrderHash,
+                leftOrder.makerAssetData,
+                leftOrder.makerAddress,
+                rightOrder.makerAddress,
+                _safeAdd(
+                    matchedFillResults.right.takerAssetFilledAmount,
+                    matchedFillResults.left.makerFeePaid
+                )
+            );
+        } else {
+            // 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
+            );
+        }
+
+        // Settle taker profits.
         _dispatchTransferFrom(
             leftOrderHash,
             leftOrder.makerAssetData,
@@ -303,61 +367,40 @@ contract MixinMatchOrders is
             matchedFillResults.leftMakerAssetSpreadAmount
         );
 
-        address leftFeeRecipientAddress = leftOrder.feeRecipientAddress;
+        // Settle taker fees.
-        address rightFeeRecipientAddress = rightOrder.feeRecipientAddress;
+        if (
-
+            leftFeeRecipientAddress == rightFeeRecipientAddress &&
-        // Maker fees
+            leftOrder.takerFeeAssetData.equals(rightOrder.takerFeeAssetData)
-        _dispatchTransferFrom(
+        ) {
-            leftOrderHash,
+            // Fee recipients and taker fee assets are identical, so we can
-            leftOrder.makerFeeAssetData,
+            // transfer them in one go.
-            leftOrder.makerAddress,
+            _dispatchTransferFrom(
-            leftFeeRecipientAddress,
+                leftOrderHash,
-            matchedFillResults.left.makerFeePaid
+                leftOrder.takerFeeAssetData,
-        );
+                takerAddress,
-        _dispatchTransferFrom(
+                leftFeeRecipientAddress,
-            rightOrderHash,
+                _safeAdd(
-            rightOrder.makerFeeAssetData,
+                    matchedFillResults.left.takerFeePaid,
-            rightOrder.makerAddress,
+                    matchedFillResults.right.takerFeePaid
-            rightFeeRecipientAddress,
+                )
-            matchedFillResults.right.makerFeePaid
+            );
-        );
+        } else {
-
+            // taker fee -> left fee recipient
-        // Taker fees
+            _dispatchTransferFrom(
-        bytes memory leftTakerFeeAssetData = leftOrder.takerFeeAssetData;
+                leftOrderHash,
-        bytes memory rightTakerFeeAssetData = rightOrder.takerFeeAssetData;
+                leftOrder.takerFeeAssetData,
-
+                takerAddress,
-        if (leftFeeRecipientAddress == rightFeeRecipientAddress) {
+                leftFeeRecipientAddress,
-            bytes32 leftTakerFeeAssetDataHash = keccak256(leftTakerFeeAssetData);
+                matchedFillResults.left.takerFeePaid
-            bytes32 rightTakerFeeAssetDataHash = keccak256(rightTakerFeeAssetData);
+            );
-            if (leftTakerFeeAssetDataHash == rightTakerFeeAssetDataHash) {
+            // taker fee -> right fee recipient
-                // Fee recipients and taker fee assets are identical, so we can
+            _dispatchTransferFrom(
-                // transfer them in one go.
+                rightOrderHash,
-                _dispatchTransferFrom(
+                rightOrder.takerFeeAssetData,
-                    leftOrderHash,
+                takerAddress,
-                    leftTakerFeeAssetData,
+                rightFeeRecipientAddress,
-                    takerAddress,
+                matchedFillResults.right.takerFeePaid
-                    leftFeeRecipientAddress,
+            );
-                    _safeAdd(
-                        matchedFillResults.left.takerFeePaid,
-                        matchedFillResults.right.takerFeePaid
-                    )
-                );
-                return;
-            }
         }
-        _dispatchTransferFrom(
-            leftOrderHash,
-            leftTakerFeeAssetData,
-            takerAddress,
-            leftFeeRecipientAddress,
-            matchedFillResults.left.takerFeePaid
-        );
-        _dispatchTransferFrom(
-            rightOrderHash,
-            rightTakerFeeAssetData,
-            takerAddress,
-            rightFeeRecipientAddress,
-            matchedFillResults.right.takerFeePaid
-        );
     }
 }

contracts/exchange/test/fill_order.ts

@@ -1,6 +1,8 @@
 import { chaiSetup, txDefaults, web3Wrapper } from '@0x/contracts-test-utils';
 import { BlockchainLifecycle } from '@0x/dev-utils';
+import { logUtils } from '@0x/utils';
 import * as _ from 'lodash';
+import { env } from 'process';
 
 import {
     AllowanceAmountScenario,
@@ -22,6 +24,12 @@ import {
 chaiSetup.configure();
 const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
 
+let describeOptional = describe;
+if (env.TEST_ALL === undefined || env.TEST_ALL === '0') {
+    describeOptional = describe.skip;
+    logUtils.warn(`Combinatorial tests are disabled. Run with "TEST_ALL=1" env variable to enable.`);
+}
+
 const defaultFillScenario = {
     orderScenario: {
         takerScenario: TakerScenario.Unspecified,
@@ -51,7 +59,7 @@ const defaultFillScenario = {
     },
 };
 
-describe.only('FillOrder Tests', () => {
+describe('FillOrder Tests', () => {
     let fillOrderCombinatorialUtils: FillOrderCombinatorialUtils;
 
     before(async () => {
@@ -631,7 +639,7 @@ describe.only('FillOrder Tests', () => {
         }
     });
 
-    describe('Combinatorially generated fills orders', () => {
+    describeOptional('Combinatorially generated fills orders', () => {
         const allFillScenarios = FillOrderCombinatorialUtils.generateFillOrderCombinations();
         for (const fillScenario of allFillScenarios) {
             const description = `Combinatorial OrderFill: ${JSON.stringify(fillScenario)}`;

contracts/exchange/test/match_orders.ts

@@ -38,6 +38,11 @@ const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
 chaiSetup.configure();
 const expect = chai.expect;
 
+// Reduce the number of tokens to deploy to speed up tests, since we don't need
+// so many.
+constants.NUM_DUMMY_ERC721_TO_DEPLOY = 1;
+constants.NUM_DUMMY_ERC1155_CONTRACTS_TO_DEPLOY = 1;
+
 describe('matchOrders', () => {
     let chainId: number;
     let makerAddressLeft: string;

contracts/exchange/test/utils/match_order_tester.ts

@@ -259,6 +259,22 @@ function simulateMatchOrders(
         fills: simulateFillEvents(orders, takerAddress, transferAmounts),
         balances: _.cloneDeep(tokenBalances),
     };
+    // Right maker asset -> left maker
+    transferAsset(
+        orders.rightOrder.makerAddress,
+        orders.leftOrder.makerAddress,
+        transferAmounts.rightMakerAssetBoughtByLeftMakerAmount,
+        orders.rightOrder.makerAssetData,
+        matchResults,
+    );
+    // Left maker fees
+    transferAsset(
+        orders.leftOrder.makerAddress,
+        orders.leftOrder.feeRecipientAddress,
+        transferAmounts.leftMakerFeeAssetPaidByLeftMakerAmount,
+        orders.leftOrder.makerFeeAssetData,
+        matchResults,
+    );
     // Left maker asset -> right maker
     transferAsset(
         orders.leftOrder.makerAddress,
@@ -267,12 +283,12 @@ function simulateMatchOrders(
         orders.leftOrder.makerAssetData,
         matchResults,
     );
-    // Right maker asset -> left maker
+    // Right maker fees
     transferAsset(
         orders.rightOrder.makerAddress,
-        orders.leftOrder.makerAddress,
+        orders.rightOrder.feeRecipientAddress,
-        transferAmounts.rightMakerAssetBoughtByLeftMakerAmount,
+        transferAmounts.rightMakerFeeAssetPaidByRightMakerAmount,
-        orders.rightOrder.makerAssetData,
+        orders.rightOrder.makerFeeAssetData,
         matchResults,
     );
     // Left taker profit
@@ -291,56 +307,22 @@ function simulateMatchOrders(
         orders.rightOrder.makerAssetData,
         matchResults,
     );
-    // Left maker fees
+    // Left taker fees
     transferAsset(
-        orders.leftOrder.makerAddress,
+        takerAddress,
         orders.leftOrder.feeRecipientAddress,
-        transferAmounts.leftMakerFeeAssetPaidByLeftMakerAmount,
+        transferAmounts.leftTakerFeeAssetPaidByTakerAmount,
-        orders.leftOrder.makerFeeAssetData,
+        orders.leftOrder.takerFeeAssetData,
         matchResults,
     );
-    // Right maker fees
+    // Right taker fees
     transferAsset(
-        orders.rightOrder.makerAddress,
+        takerAddress,
         orders.rightOrder.feeRecipientAddress,
-        transferAmounts.rightMakerFeeAssetPaidByRightMakerAmount,
+        transferAmounts.rightTakerFeeAssetPaidByTakerAmount,
-        orders.rightOrder.makerFeeAssetData,
+        orders.rightOrder.takerFeeAssetData,
         matchResults,
     );
-    // Taker fees.
-    if (
-        orders.leftOrder.feeRecipientAddress === orders.rightOrder.feeRecipientAddress &&
-        orders.leftOrder.takerFeeAssetData === orders.rightOrder.takerFeeAssetData
-    ) {
-        // Same asset data and recipients, so combine into a single transfer.
-        const totalTakerFeeAssetPaidByTakerAmount = transferAmounts.leftTakerFeeAssetPaidByTakerAmount.plus(
-            transferAmounts.rightTakerFeeAssetPaidByTakerAmount,
-        );
-        transferAsset(
-            takerAddress,
-            orders.leftOrder.feeRecipientAddress,
-            totalTakerFeeAssetPaidByTakerAmount,
-            orders.leftOrder.takerFeeAssetData,
-            matchResults,
-        );
-    } else {
-        // Left taker fees
-        transferAsset(
-            takerAddress,
-            orders.leftOrder.feeRecipientAddress,
-            transferAmounts.leftTakerFeeAssetPaidByTakerAmount,
-            orders.leftOrder.takerFeeAssetData,
-            matchResults,
-        );
-        // Right taker fees
-        transferAsset(
-            takerAddress,
-            orders.rightOrder.feeRecipientAddress,
-            transferAmounts.rightTakerFeeAssetPaidByTakerAmount,
-            orders.rightOrder.takerFeeAssetData,
-            matchResults,
-        );
-    }
     return matchResults;
 }