diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index a993dede86..7a8d67afcb 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/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 - _dispatchTransferFrom( - leftOrderHash, - leftOrder.makerAssetData, - leftOrder.makerAddress, - rightOrder.makerAddress, - matchedFillResults.right.takerAssetFilledAmount - ); - _dispatchTransferFrom( - rightOrderHash, - rightOrder.makerAssetData, - rightOrder.makerAddress, - leftOrder.makerAddress, - matchedFillResults.left.takerAssetFilledAmount - ); + address leftFeeRecipientAddress = leftOrder.feeRecipientAddress; + address rightFeeRecipientAddress = rightOrder.feeRecipientAddress; + + // Settle left order taker asset and maker fees. + if ( + leftOrder.makerAddress == leftFeeRecipientAddress && + leftOrder.makerFeeAssetData.equals(leftOrder.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( + rightOrderHash, + rightOrder.makerAssetData, + 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; - address rightFeeRecipientAddress = rightOrder.feeRecipientAddress; - - // Maker fees - _dispatchTransferFrom( - leftOrderHash, - leftOrder.makerFeeAssetData, - leftOrder.makerAddress, - leftFeeRecipientAddress, - matchedFillResults.left.makerFeePaid - ); - _dispatchTransferFrom( - rightOrderHash, - rightOrder.makerFeeAssetData, - rightOrder.makerAddress, - rightFeeRecipientAddress, - matchedFillResults.right.makerFeePaid - ); - - // Taker fees - bytes memory leftTakerFeeAssetData = leftOrder.takerFeeAssetData; - bytes memory rightTakerFeeAssetData = rightOrder.takerFeeAssetData; - - if (leftFeeRecipientAddress == rightFeeRecipientAddress) { - bytes32 leftTakerFeeAssetDataHash = keccak256(leftTakerFeeAssetData); - bytes32 rightTakerFeeAssetDataHash = keccak256(rightTakerFeeAssetData); - if (leftTakerFeeAssetDataHash == rightTakerFeeAssetDataHash) { - // Fee recipients and taker fee assets are identical, so we can - // transfer them in one go. - _dispatchTransferFrom( - leftOrderHash, - leftTakerFeeAssetData, - takerAddress, - leftFeeRecipientAddress, - _safeAdd( - matchedFillResults.left.takerFeePaid, - matchedFillResults.right.takerFeePaid - ) - ); - return; - } + // 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 { + // taker fee -> left fee recipient + _dispatchTransferFrom( + leftOrderHash, + leftOrder.takerFeeAssetData, + takerAddress, + leftFeeRecipientAddress, + matchedFillResults.left.takerFeePaid + ); + // taker fee -> right fee recipient + _dispatchTransferFrom( + rightOrderHash, + rightOrder.takerFeeAssetData, + takerAddress, + rightFeeRecipientAddress, + matchedFillResults.right.takerFeePaid + ); } - _dispatchTransferFrom( - leftOrderHash, - leftTakerFeeAssetData, - takerAddress, - leftFeeRecipientAddress, - matchedFillResults.left.takerFeePaid - ); - _dispatchTransferFrom( - rightOrderHash, - rightTakerFeeAssetData, - takerAddress, - rightFeeRecipientAddress, - matchedFillResults.right.takerFeePaid - ); } } diff --git a/contracts/exchange/test/fill_order.ts b/contracts/exchange/test/fill_order.ts index d8b17710b5..b4379c1f46 100644 --- a/contracts/exchange/test/fill_order.ts +++ b/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)}`; diff --git a/contracts/exchange/test/match_orders.ts b/contracts/exchange/test/match_orders.ts index a0f720f5df..92c5db227c 100644 --- a/contracts/exchange/test/match_orders.ts +++ b/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; diff --git a/contracts/exchange/test/utils/match_order_tester.ts b/contracts/exchange/test/utils/match_order_tester.ts index 1f5c9b24bc..53d2f6945b 100644 --- a/contracts/exchange/test/utils/match_order_tester.ts +++ b/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, - transferAmounts.rightMakerAssetBoughtByLeftMakerAmount, - orders.rightOrder.makerAssetData, + orders.rightOrder.feeRecipientAddress, + transferAmounts.rightMakerFeeAssetPaidByRightMakerAmount, + 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, - orders.leftOrder.makerFeeAssetData, + transferAmounts.leftTakerFeeAssetPaidByTakerAmount, + orders.leftOrder.takerFeeAssetData, matchResults, ); - // Right maker fees + // Right taker fees transferAsset( - orders.rightOrder.makerAddress, + takerAddress, orders.rightOrder.feeRecipientAddress, - transferAmounts.rightMakerFeeAssetPaidByRightMakerAmount, - orders.rightOrder.makerFeeAssetData, + transferAmounts.rightTakerFeeAssetPaidByTakerAmount, + 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; }