diff --git a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol index c4996112ef..2bd0792138 100644 --- a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol +++ b/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. diff --git a/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol b/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol index 28d35236b4..b0273d72e6 100644 --- a/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol +++ b/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. - /// Returns false if the transaction would otherwise revert. + /// @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. @@ -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 diff --git a/contracts/exchange/test/wrapper.ts b/contracts/exchange/test/wrapper.ts index 399186ad50..70916b43c4 100644 --- a/contracts/exchange/test/wrapper.ts +++ b/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(filledSignedOrders, signedOrder => { + _.forEach(signedOrders, signedOrder => { + const takerAssetAddress = + signedOrder.takerAssetData === differentTakerAssetData + ? feeToken.address + : defaultTakerAssetAddress; erc20Balances[makerAddress][defaultMakerAssetAddress] = erc20Balances[makerAddress][ defaultMakerAssetAddress ].minus(signedOrder.makerAssetAmount); - erc20Balances[makerAddress][defaultTakerAssetAddress] = erc20Balances[makerAddress][ - defaultTakerAssetAddress + erc20Balances[makerAddress][takerAssetAddress] = erc20Balances[makerAddress][ + 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][ - defaultTakerAssetAddress + erc20Balances[takerAddress][takerAssetAddress] = erc20Balances[takerAddress][ + 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(filledSignedOrders, signedOrder => { - erc20Balances[makerAddress][defaultMakerAssetAddress] = erc20Balances[makerAddress][ - defaultMakerAssetAddress + _.forEach(signedOrders, signedOrder => { + const makerAssetAddress = + signedOrder.makerAssetData === differentMakerAssetData + ? 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][ - defaultMakerAssetAddress + erc20Balances[takerAddress][makerAssetAddress] = erc20Balances[takerAddress][ + 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, diff --git a/contracts/exchange/test/wrapper_unit_tests.ts b/contracts/exchange/test/wrapper_unit_tests.ts index b070e0bd78..a071f95bc9 100644 --- a/contracts/exchange/test/wrapper_unit_tests.ts +++ b/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 () => { - const expectedError = new AnyRevertError(); // Just a generic revert. - const tx = txHelper.getResultAndReceiptAsync(getContractFn(), [], constants.ZERO_AMOUNT, []); - return expect(tx).to.revertWith(expectedError); + it('returns empty fill results with no orders', async () => { + const [expectedResult, expectedCalls] = simulator([], constants.ZERO_AMOUNT, []); + expect(expectedCalls.length).to.eq(0); + 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([modifiedOrder, remainingTakerAssetFillAmount, signature]); + fillOrderCalls.push([order, 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 () => { - const expectedError = new AnyRevertError(); // Just a generic revert. - const tx = txHelper.getResultAndReceiptAsync(getContractFn(), [], constants.ZERO_AMOUNT, []); - return expect(tx).to.revertWith(expectedError); + it('returns empty fill results with no orders', async () => { + const [expectedResult, expectedCalls] = simulator([], constants.ZERO_AMOUNT, []); + expect(expectedCalls.length).to.eq(0); + 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([modifiedOrder, remainingTakerAssetFillAmount, signature]); + fillOrderCalls.push([order, remainingTakerAssetFillAmount, signature]); } fillResults = addFillResults(fillResults, getExpectedFillResults(order, signature)); if (fillResults.makerAssetFilledAmount.gte(makerAssetFillAmount)) {