Remove restrictions that maker/takerAssetData must be the same for each order in marketBuy/Sell functions

contracts/exchange/contracts/src/MixinWrapperFunctions.sol

@@ -87,7 +87,7 @@ contract MixinWrapperFunctions is
         return fillResults;
     }
 
-    /// @dev Executes multiple calls of fillOrKill.
+    /// @dev Executes multiple calls of fillOrKillOrder.
     /// @param orders Array of order specifications.
     /// @param takerAssetFillAmounts Array of desired amounts of takerAsset to sell in orders.
     /// @param signatures Proofs that orders have been created by makers.
@@ -115,7 +115,7 @@ contract MixinWrapperFunctions is
         return fillResults;
     }
 
-    /// @dev Executes multiple calls of fillOrderNoThrow.
+    /// @dev Executes multiple calls of fillOrder. If any fill reverts, the error is caught and ignored.
     /// @param orders Array of order specifications.
     /// @param takerAssetFillAmounts Array of desired amounts of takerAsset to sell in orders.
     /// @param signatures Proofs that orders have been created by makers.
@@ -142,7 +142,9 @@ contract MixinWrapperFunctions is
         return fillResults;
     }
 
-    /// @dev Executes multiple calls of fillOrderNoThrow until total amount of takerAsset is sold by taker.
+    /// @dev Executes multiple calls of fillOrder until total amount of takerAsset is sold by taker.
+    ///      If any fill reverts, the error is caught and ignored.
+    ///      NOTE: This function does not enforce that the takerAsset is the same for each order.
     /// @param orders Array of order specifications.
     /// @param takerAssetFillAmount Desired amount of takerAsset to sell.
     /// @param signatures Proofs that orders have been signed by makers.
@@ -157,19 +159,12 @@ contract MixinWrapperFunctions is
         disableRefundUntilEnd
         returns (LibFillResults.FillResults memory fillResults)
     {
-        bytes memory takerAssetData = orders[0].takerAssetData;
-
         uint256 ordersLength = orders.length;
         for (uint256 i = 0; i != ordersLength; i++) {
 
             // Calculate the remaining amount of takerAsset to sell
             uint256 remainingTakerAssetFillAmount = takerAssetFillAmount.safeSub(fillResults.takerAssetFilledAmount);
 
-            // The `takerAssetData` must be the same for each order.
-            // Rather than checking equality, we point the `takerAssetData` of each order to the same memory location.
-            // This is less expensive than checking equality.
-            orders[i].takerAssetData = takerAssetData;
-
             // Attempt to sell the remaining amount of takerAsset
             LibFillResults.FillResults memory singleFillResults = _fillOrderNoThrow(
                 orders[i],
@@ -188,7 +183,9 @@ contract MixinWrapperFunctions is
         return fillResults;
     }
 
-    /// @dev Executes multiple calls of fillOrderNoThrow until total amount of makerAsset is bought by taker.
+    /// @dev Executes multiple calls of fillOrder until total amount of makerAsset is bought by taker.
+    ///      If any fill reverts, the error is caught and ignored.
+    ///      NOTE: This function does not enforce that the makerAsset is the same for each order.
     /// @param orders Array of order specifications.
     /// @param makerAssetFillAmount Desired amount of makerAsset to buy.
     /// @param signatures Proofs that orders have been signed by makers.
@@ -203,8 +200,6 @@ contract MixinWrapperFunctions is
         disableRefundUntilEnd
         returns (LibFillResults.FillResults memory fillResults)
     {
-        bytes memory makerAssetData = orders[0].makerAssetData;
-
         uint256 ordersLength = orders.length;
         for (uint256 i = 0; i != ordersLength; i++) {
 
@@ -219,11 +214,6 @@ contract MixinWrapperFunctions is
                 remainingMakerAssetFillAmount
             );
 
-            // The `makerAssetData` must be the same for each order.
-            // Rather than checking equality, we point the `makerAssetData` of each order to the same memory location.
-            // This is less expensive than checking equality.
-            orders[i].makerAssetData = makerAssetData;
-
             // Attempt to sell the remaining amount of takerAsset
             LibFillResults.FillResults memory singleFillResults = _fillOrderNoThrow(
                 orders[i],
@@ -243,6 +233,7 @@ contract MixinWrapperFunctions is
     }
 
     /// @dev Calls marketSellOrdersNoThrow then reverts if < takerAssetFillAmount has been sold.
+    ///      NOTE: This function does not enforce that the takerAsset is the same for each order.
     /// @param orders Array of order specifications.
     /// @param takerAssetFillAmount Minimum amount of takerAsset to sell.
     /// @param signatures Proofs that orders have been signed by makers.
@@ -267,6 +258,7 @@ contract MixinWrapperFunctions is
     }
 
     /// @dev Calls marketBuyOrdersNoThrow then reverts if < makerAssetFillAmount has been bought.
+    ///      NOTE: This function does not enforce that the makerAsset is the same for each order.
     /// @param orders Array of order specifications.
     /// @param makerAssetFillAmount Minimum amount of makerAsset to buy.
     /// @param signatures Proofs that orders have been signed by makers.

contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol

@@ -26,7 +26,7 @@ import "@0x/contracts-exchange-libs/contracts/src/LibFillResults.sol";
 contract IWrapperFunctions {
 
     /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled.
-    /// @param order LibOrder.Order struct containing order specifications.
+    /// @param order Order struct containing order specifications.
     /// @param takerAssetFillAmount Desired amount of takerAsset to sell.
     /// @param signature Proof that order has been created by maker.
     function fillOrKillOrder(
@@ -52,7 +52,7 @@ contract IWrapperFunctions {
         payable
         returns (LibFillResults.FillResults[] memory fillResults);
 
-    /// @dev Synchronously executes multiple calls of fillOrKill.
+    /// @dev Executes multiple calls of fillOrKillOrder.
     /// @param orders Array of order specifications.
     /// @param takerAssetFillAmounts Array of desired amounts of takerAsset to sell in orders.
     /// @param signatures Proofs that orders have been created by makers.
@@ -66,8 +66,7 @@ contract IWrapperFunctions {
         payable
         returns (LibFillResults.FillResults[] memory fillResults);
 
-    /// @dev Fills an order with specified parameters and ECDSA signature.
+    /// @dev Executes multiple calls of fillOrder. If any fill reverts, the error is caught and ignored.
-    ///      Returns false if the transaction would otherwise revert.
     /// @param orders Array of order specifications.
     /// @param takerAssetFillAmounts Array of desired amounts of takerAsset to sell in orders.
     /// @param signatures Proofs that orders have been created by makers.
@@ -81,7 +80,9 @@ contract IWrapperFunctions {
         payable
         returns (LibFillResults.FillResults[] memory fillResults);
 
-    /// @dev Executes multiple calls of fillOrderNoThrow until total amount of takerAsset is sold by taker.
+    /// @dev Executes multiple calls of fillOrder until total amount of takerAsset is sold by taker.
+    ///      If any fill reverts, the error is caught and ignored.
+    ///      NOTE: This function does not enforce that the takerAsset is the same for each order.
     /// @param orders Array of order specifications.
     /// @param takerAssetFillAmount Desired amount of takerAsset to sell.
     /// @param signatures Proofs that orders have been signed by makers.
@@ -95,7 +96,9 @@ contract IWrapperFunctions {
         payable
         returns (LibFillResults.FillResults memory fillResults);
 
-    /// @dev Executes multiple calls of fillOrderNoThrow until total amount of makerAsset is bought by taker.
+    /// @dev Executes multiple calls of fillOrder until total amount of makerAsset is bought by taker.
+    ///      If any fill reverts, the error is caught and ignored.
+    ///      NOTE: This function does not enforce that the makerAsset is the same for each order.
     /// @param orders Array of order specifications.
     /// @param makerAssetFillAmount Desired amount of makerAsset to buy.
     /// @param signatures Proofs that orders have been signed by makers.
@@ -110,6 +113,7 @@ contract IWrapperFunctions {
         returns (LibFillResults.FillResults memory fillResults);
 
     /// @dev Calls marketSellOrdersNoThrow then reverts if < takerAssetFillAmount has been sold.
+    ///      NOTE: This function does not enforce that the takerAsset is the same for each order.
     /// @param orders Array of order specifications.
     /// @param takerAssetFillAmount Minimum amount of takerAsset to sell.
     /// @param signatures Proofs that orders have been signed by makers.
@@ -124,6 +128,7 @@ contract IWrapperFunctions {
         returns (LibFillResults.FillResults memory fillResults);
 
     /// @dev Calls marketBuyOrdersNoThrow then reverts if < makerAssetFillAmount has been bought.
+    ///      NOTE: This function does not enforce that the makerAsset is the same for each order.
     /// @param orders Array of order specifications.
     /// @param makerAssetFillAmount Minimum amount of makerAsset to buy.
     /// @param signatures Proofs that orders have been signed by makers.
@@ -137,7 +142,7 @@ contract IWrapperFunctions {
         payable
         returns (LibFillResults.FillResults memory fillResults);
 
-    /// @dev Synchronously cancels multiple orders in a single transaction.
+    /// @dev Executes multiple calls of cancelOrder.
     /// @param orders Array of order specifications.
     function batchCancelOrders(LibOrder.Order[] memory orders)
         public

contracts/exchange/test/wrapper.ts

@@ -1,4 +1,4 @@
-import { ERC20ProxyContract, ERC20Wrapper, ERC721ProxyContract, ERC721Wrapper } from '@0x/contracts-asset-proxy';
+import { ERC20ProxyContract, ERC20Wrapper } from '@0x/contracts-asset-proxy';
 import { DummyERC20TokenContract } from '@0x/contracts-erc20';
 import {
     blockchainTests,
@@ -30,11 +30,9 @@ blockchainTests.resets('Exchange wrappers', env => {
     let feeToken: DummyERC20TokenContract;
     let exchange: ExchangeContract;
     let erc20Proxy: ERC20ProxyContract;
-    let erc721Proxy: ERC721ProxyContract;
 
     let exchangeWrapper: ExchangeWrapper;
     let erc20Wrapper: ERC20Wrapper;
-    let erc721Wrapper: ERC721Wrapper;
     let erc20Balances: ERC20BalancesByOwner;
     let orderFactory: OrderFactory;
 
@@ -58,7 +56,6 @@ blockchainTests.resets('Exchange wrappers', env => {
         const usedAddresses = ([owner, makerAddress, takerAddress, feeRecipientAddress] = _.slice(accounts, 0, 4));
 
         erc20Wrapper = new ERC20Wrapper(env.provider, usedAddresses, owner);
-        erc721Wrapper = new ERC721Wrapper(env.provider, usedAddresses, owner);
 
         const numDummyErc20ToDeploy = 3;
         [erc20TokenA, erc20TokenB, feeToken] = await erc20Wrapper.deployDummyTokensAsync(
@@ -68,9 +65,6 @@ blockchainTests.resets('Exchange wrappers', env => {
         erc20Proxy = await erc20Wrapper.deployProxyAsync();
         await erc20Wrapper.setBalancesAndAllowancesAsync();
 
-        erc721Proxy = await erc721Wrapper.deployProxyAsync();
-        await erc721Wrapper.setBalancesAndAllowancesAsync();
-
         exchange = await ExchangeContract.deployFrom0xArtifactAsync(
             artifacts.Exchange,
             env.provider,
@@ -86,14 +80,10 @@ blockchainTests.resets('Exchange wrappers', env => {
 
         exchangeWrapper = new ExchangeWrapper(exchange, env.provider);
         await exchangeWrapper.registerAssetProxyAsync(erc20Proxy.address, owner);
-        await exchangeWrapper.registerAssetProxyAsync(erc721Proxy.address, owner);
 
         await erc20Proxy.addAuthorizedAddress.awaitTransactionSuccessAsync(exchange.address, {
             from: owner,
         });
-        await erc721Proxy.addAuthorizedAddress.awaitTransactionSuccessAsync(exchange.address, {
-            from: owner,
-        });
 
         defaultMakerAssetAddress = erc20TokenA.address;
         defaultTakerAssetAddress = erc20TokenB.address;
@@ -623,22 +613,26 @@ blockchainTests.resets('Exchange wrappers', env => {
                 expect(newBalances).to.be.deep.equal(erc20Balances);
             });
 
-            it('should not fill a signedOrder that does not use the same takerAssetAddress', async () => {
+            it('should fill a signedOrder that does not use the same takerAssetAddress', async () => {
+                const differentTakerAssetData = assetDataUtils.encodeERC20AssetData(feeToken.address);
                 signedOrders = [
                     await orderFactory.newSignedOrderAsync(),
                     await orderFactory.newSignedOrderAsync(),
                     await orderFactory.newSignedOrderAsync({
-                        takerAssetData: assetDataUtils.encodeERC20AssetData(feeToken.address),
+                        takerAssetData: differentTakerAssetData,
                     }),
                 ];
                 const takerAssetFillAmount = Web3Wrapper.toBaseUnitAmount(new BigNumber(100000), 18);
-                const filledSignedOrders = signedOrders.slice(0, -1);
+                _.forEach(signedOrders, signedOrder => {
-                _.forEach(filledSignedOrders, signedOrder => {
+                    const takerAssetAddress =
+                        signedOrder.takerAssetData === differentTakerAssetData
+                            ? feeToken.address
+                            : defaultTakerAssetAddress;
                     erc20Balances[makerAddress][defaultMakerAssetAddress] = erc20Balances[makerAddress][
                         defaultMakerAssetAddress
                     ].minus(signedOrder.makerAssetAmount);
-                    erc20Balances[makerAddress][defaultTakerAssetAddress] = erc20Balances[makerAddress][
+                    erc20Balances[makerAddress][takerAssetAddress] = erc20Balances[makerAddress][
-                        defaultTakerAssetAddress
+                        takerAssetAddress
                     ].plus(signedOrder.takerAssetAmount);
                     erc20Balances[makerAddress][feeToken.address] = erc20Balances[makerAddress][feeToken.address].minus(
                         signedOrder.makerFee,
@@ -646,8 +640,8 @@ blockchainTests.resets('Exchange wrappers', env => {
                     erc20Balances[takerAddress][defaultMakerAssetAddress] = erc20Balances[takerAddress][
                         defaultMakerAssetAddress
                     ].plus(signedOrder.makerAssetAmount);
-                    erc20Balances[takerAddress][defaultTakerAssetAddress] = erc20Balances[takerAddress][
+                    erc20Balances[takerAddress][takerAssetAddress] = erc20Balances[takerAddress][
-                        defaultTakerAssetAddress
+                        takerAssetAddress
                     ].minus(signedOrder.takerAssetAmount);
                     erc20Balances[takerAddress][feeToken.address] = erc20Balances[takerAddress][feeToken.address].minus(
                         signedOrder.takerFee,
@@ -668,7 +662,7 @@ blockchainTests.resets('Exchange wrappers', env => {
                 });
                 const newBalances = await erc20Wrapper.getBalancesAsync();
 
-                const expectedFillResults = filledSignedOrders
+                const expectedFillResults = signedOrders
                     .map(signedOrder => ({
                         makerAssetFilledAmount: signedOrder.makerAssetAmount,
                         takerAssetFilledAmount: signedOrder.takerAssetAmount,
@@ -811,20 +805,24 @@ blockchainTests.resets('Exchange wrappers', env => {
                 expect(newBalances).to.be.deep.equal(erc20Balances);
             });
 
-            it('should not fill a signedOrder that does not use the same makerAssetAddress', async () => {
+            it('should fill a signedOrder that does not use the same makerAssetAddress', async () => {
+                const differentMakerAssetData = assetDataUtils.encodeERC20AssetData(feeToken.address);
                 signedOrders = [
                     await orderFactory.newSignedOrderAsync(),
                     await orderFactory.newSignedOrderAsync(),
                     await orderFactory.newSignedOrderAsync({
-                        makerAssetData: assetDataUtils.encodeERC20AssetData(feeToken.address),
+                        makerAssetData: differentMakerAssetData,
                     }),
                 ];
 
                 const makerAssetFillAmount = Web3Wrapper.toBaseUnitAmount(new BigNumber(100000), 18);
-                const filledSignedOrders = signedOrders.slice(0, -1);
+                _.forEach(signedOrders, signedOrder => {
-                _.forEach(filledSignedOrders, signedOrder => {
+                    const makerAssetAddress =
-                    erc20Balances[makerAddress][defaultMakerAssetAddress] = erc20Balances[makerAddress][
+                        signedOrder.makerAssetData === differentMakerAssetData
-                        defaultMakerAssetAddress
+                            ? feeToken.address
+                            : defaultMakerAssetAddress;
+                    erc20Balances[makerAddress][makerAssetAddress] = erc20Balances[makerAddress][
+                        makerAssetAddress
                     ].minus(signedOrder.makerAssetAmount);
                     erc20Balances[makerAddress][defaultTakerAssetAddress] = erc20Balances[makerAddress][
                         defaultTakerAssetAddress
@@ -832,8 +830,8 @@ blockchainTests.resets('Exchange wrappers', env => {
                     erc20Balances[makerAddress][feeToken.address] = erc20Balances[makerAddress][feeToken.address].minus(
                         signedOrder.makerFee,
                     );
-                    erc20Balances[takerAddress][defaultMakerAssetAddress] = erc20Balances[takerAddress][
+                    erc20Balances[takerAddress][makerAssetAddress] = erc20Balances[takerAddress][
-                        defaultMakerAssetAddress
+                        makerAssetAddress
                     ].plus(signedOrder.makerAssetAmount);
                     erc20Balances[takerAddress][defaultTakerAssetAddress] = erc20Balances[takerAddress][
                         defaultTakerAssetAddress
@@ -857,7 +855,7 @@ blockchainTests.resets('Exchange wrappers', env => {
                 });
                 const newBalances = await erc20Wrapper.getBalancesAsync();
 
-                const expectedFillResults = filledSignedOrders
+                const expectedFillResults = signedOrders
                     .map(signedOrder => ({
                         makerAssetFilledAmount: signedOrder.makerAssetAmount,
                         takerAssetFilledAmount: signedOrder.takerAssetAmount,

contracts/exchange/test/wrapper_unit_tests.ts

@@ -739,10 +739,18 @@ blockchainTests('Exchange wrapper functions unit tests.', env => {
                 return expect(tx).to.revertWith(expectedError);
             });
 
-            it('reverts with no orders', async () => {
+            it('returns empty fill results with no orders', async () => {
-                const expectedError = new AnyRevertError(); // Just a generic revert.
+                const [expectedResult, expectedCalls] = simulator([], constants.ZERO_AMOUNT, []);
-                const tx = txHelper.getResultAndReceiptAsync(getContractFn(), [], constants.ZERO_AMOUNT, []);
+                expect(expectedCalls.length).to.eq(0);
-                return expect(tx).to.revertWith(expectedError);
+                const [actualResult, receipt] = await txHelper.getResultAndReceiptAsync(
+                    getContractFn(),
+                    [],
+                    constants.ZERO_AMOUNT,
+                    [],
+                );
+
+                expect(actualResult).to.deep.eq(expectedResult);
+                assertFillOrderCallsFromLogs(receipt.logs, expectedCalls);
             });
         }
 
@@ -753,12 +761,10 @@ blockchainTests('Exchange wrapper functions unit tests.', env => {
         ): [FillResults, ExpectedFillOrderCallArgs[]] {
             const fillOrderCalls = [];
             let fillResults = _.cloneDeep(EMPTY_FILL_RESULTS);
-            const takerAssetData = orders[0].takerAssetData;
             for (const [order, signature] of _.zip(orders, signatures) as [[Order, string]]) {
                 const remainingTakerAssetFillAmount = safeSub(takerAssetFillAmount, fillResults.takerAssetFilledAmount);
                 if (order.salt !== ALWAYS_FAILING_SALT) {
-                    const modifiedOrder = { ...order, takerAssetData };
+                    fillOrderCalls.push([order, remainingTakerAssetFillAmount, signature]);
-                    fillOrderCalls.push([modifiedOrder, remainingTakerAssetFillAmount, signature]);
                 }
                 fillResults = addFillResults(fillResults, getExpectedFillResults(order, signature));
                 if (fillResults.takerAssetFilledAmount.gte(takerAssetFillAmount)) {
@@ -1087,10 +1093,18 @@ blockchainTests('Exchange wrapper functions unit tests.', env => {
                 return expect(tx).to.revertWith(expectedError);
             });
 
-            it('reverts with no orders', async () => {
+            it('returns empty fill results with no orders', async () => {
-                const expectedError = new AnyRevertError(); // Just a generic revert.
+                const [expectedResult, expectedCalls] = simulator([], constants.ZERO_AMOUNT, []);
-                const tx = txHelper.getResultAndReceiptAsync(getContractFn(), [], constants.ZERO_AMOUNT, []);
+                expect(expectedCalls.length).to.eq(0);
-                return expect(tx).to.revertWith(expectedError);
+                const [actualResult, receipt] = await txHelper.getResultAndReceiptAsync(
+                    getContractFn(),
+                    [],
+                    constants.ZERO_AMOUNT,
+                    [],
+                );
+
+                expect(actualResult).to.deep.eq(expectedResult);
+                assertFillOrderCallsFromLogs(receipt.logs, expectedCalls);
             });
         }
 
@@ -1101,7 +1115,6 @@ blockchainTests('Exchange wrapper functions unit tests.', env => {
         ): [FillResults, ExpectedFillOrderCallArgs[]] {
             const fillOrderCalls = [];
             let fillResults = _.cloneDeep(EMPTY_FILL_RESULTS);
-            const makerAssetData = orders[0].makerAssetData;
             for (const [order, signature] of _.zip(orders, signatures) as [[Order, string]]) {
                 const remainingMakerAssetFillAmount = safeSub(makerAssetFillAmount, fillResults.makerAssetFilledAmount);
                 const remainingTakerAssetFillAmount = getPartialAmountFloor(
@@ -1110,8 +1123,7 @@ blockchainTests('Exchange wrapper functions unit tests.', env => {
                     remainingMakerAssetFillAmount,
                 );
                 if (order.salt !== ALWAYS_FAILING_SALT) {
-                    const modifiedOrder = { ...order, makerAssetData };
+                    fillOrderCalls.push([order, remainingTakerAssetFillAmount, signature]);
-                    fillOrderCalls.push([modifiedOrder, remainingTakerAssetFillAmount, signature]);
                 }
                 fillResults = addFillResults(fillResults, getExpectedFillResults(order, signature));
                 if (fillResults.makerAssetFilledAmount.gte(makerAssetFillAmount)) {