From ca28b8f93ef0bb89ef5481c7f297ea2bdb483293 Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Tue, 6 Aug 2019 15:26:05 -0400 Subject: [PATCH] `@0x/contracts-exchange`: Make `marketBuy/SellNoThrow` the default. `@0x/contracts-exchange`: Add more `wrapper_unit_tests` tests. --- .../contracts/src/MixinWrapperFunctions.sol | 102 +---- .../src/interfaces/IWrapperFunctions.sol | 30 +- .../exchange/test/utils/exchange_wrapper.ts | 32 +- contracts/exchange/test/wrapper.ts | 352 +----------------- contracts/exchange/test/wrapper_unit_tests.ts | 63 ++++ 5 files changed, 85 insertions(+), 494 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol index 9ff32159b3..7ceef3963c 100644 --- a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol @@ -164,57 +164,12 @@ contract MixinWrapperFunctions is return fillResults; } - /// @dev Executes multiple calls of fillOrder until total amount of takerAsset is sold by taker. - /// @param orders Array of order specifications. - /// @param takerAssetFillAmount Desired amount of takerAsset to sell. - /// @param signatures Proofs that orders have been created by makers. - /// @return Amounts filled and fees paid by makers and taker. - function marketSellOrders( - LibOrder.Order[] memory orders, - uint256 takerAssetFillAmount, - bytes[] memory signatures - ) - public - nonReentrant - returns (FillResults memory fillResults) - { - bytes memory takerAssetData = orders[0].takerAssetData; - - uint256 ordersLength = orders.length; - for (uint256 i = 0; i != ordersLength; i++) { - - // 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; - - // Calculate the remaining amount of takerAsset to sell - uint256 remainingTakerAssetFillAmount = _safeSub(takerAssetFillAmount, fillResults.takerAssetFilledAmount); - - // Attempt to sell the remaining amount of takerAsset - FillResults memory singleFillResults = _fillOrder( - orders[i], - remainingTakerAssetFillAmount, - signatures[i] - ); - - // Update amounts filled and fees paid by maker and taker - _addFillResults(fillResults, singleFillResults); - - // Stop execution if the entire amount of takerAsset has been sold - if (fillResults.takerAssetFilledAmount >= takerAssetFillAmount) { - break; - } - } - return fillResults; - } - /// @dev Executes multiple calls of fillOrderNoThrow until total amount of takerAsset is sold by taker. /// @param orders Array of order specifications. /// @param takerAssetFillAmount Desired amount of takerAsset to sell. /// @param signatures Proofs that orders have been signed by makers. /// @return Amounts filled and fees paid by makers and taker. - function marketSellOrdersNoThrow( + function marketSellOrders( LibOrder.Order[] memory orders, uint256 takerAssetFillAmount, bytes[] memory signatures @@ -253,65 +208,12 @@ contract MixinWrapperFunctions is return fillResults; } - /// @dev Executes multiple calls of fillOrder until total amount of makerAsset is bought by taker. - /// @param orders Array of order specifications. - /// @param makerAssetFillAmount Desired amount of makerAsset to buy. - /// @param signatures Proofs that orders have been signed by makers. - /// @return Amounts filled and fees paid by makers and taker. - function marketBuyOrders( - LibOrder.Order[] memory orders, - uint256 makerAssetFillAmount, - bytes[] memory signatures - ) - public - nonReentrant - returns (FillResults memory fillResults) - { - bytes memory makerAssetData = orders[0].makerAssetData; - - uint256 ordersLength = orders.length; - for (uint256 i = 0; i != ordersLength; i++) { - - // 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; - - // Calculate the remaining amount of makerAsset to buy - uint256 remainingMakerAssetFillAmount = _safeSub(makerAssetFillAmount, fillResults.makerAssetFilledAmount); - - // Convert the remaining amount of makerAsset to buy into remaining amount - // of takerAsset to sell, assuming entire amount can be sold in the current order - uint256 remainingTakerAssetFillAmount = _getPartialAmountFloor( - orders[i].takerAssetAmount, - orders[i].makerAssetAmount, - remainingMakerAssetFillAmount - ); - - // Attempt to sell the remaining amount of takerAsset - FillResults memory singleFillResults = _fillOrder( - orders[i], - remainingTakerAssetFillAmount, - signatures[i] - ); - - // Update amounts filled and fees paid by maker and taker - _addFillResults(fillResults, singleFillResults); - - // Stop execution if the entire amount of makerAsset has been bought - if (fillResults.makerAssetFilledAmount >= makerAssetFillAmount) { - break; - } - } - return fillResults; - } - /// @dev Executes multiple calls of fillOrderNoThrow until total amount of makerAsset is bought by taker. /// @param orders Array of order specifications. /// @param makerAssetFillAmount Desired amount of makerAsset to buy. /// @param signatures Proofs that orders have been signed by makers. /// @return Amounts filled and fees paid by makers and taker. - function marketBuyOrdersNoThrow( + function marketBuyOrders( LibOrder.Order[] memory orders, uint256 makerAssetFillAmount, bytes[] memory signatures diff --git a/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol b/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol index 5df1b0e806..70476d8043 100644 --- a/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol @@ -91,26 +91,13 @@ contract IWrapperFunctions { public returns (LibFillResults.FillResults[] memory fillResults); - /// @dev Synchronously executes multiple calls of fillOrder until total amount of takerAsset is sold by taker. - /// @param orders Array of order specifications. - /// @param takerAssetFillAmount Desired amount of takerAsset to sell. - /// @param signatures Proofs that orders have been created by makers. - /// @return Amounts filled and fees paid by makers and taker. - function marketSellOrders( - LibOrder.Order[] memory orders, - uint256 takerAssetFillAmount, - bytes[] memory signatures - ) - public - returns (LibFillResults.FillResults memory fillResults); - /// @dev Synchronously executes multiple calls of fillOrder until total amount of takerAsset is sold by taker. /// Returns false if the transaction would otherwise revert. /// @param orders Array of order specifications. /// @param takerAssetFillAmount Desired amount of takerAsset to sell. /// @param signatures Proofs that orders have been signed by makers. /// @return Amounts filled and fees paid by makers and taker. - function marketSellOrdersNoThrow( + function marketSellOrders( LibOrder.Order[] memory orders, uint256 takerAssetFillAmount, bytes[] memory signatures @@ -118,26 +105,13 @@ contract IWrapperFunctions { public returns (LibFillResults.FillResults memory fillResults); - /// @dev Synchronously executes multiple calls of fillOrder until total amount of makerAsset is bought by taker. - /// @param orders Array of order specifications. - /// @param makerAssetFillAmount Desired amount of makerAsset to buy. - /// @param signatures Proofs that orders have been signed by makers. - /// @return Amounts filled and fees paid by makers and taker. - function marketBuyOrders( - LibOrder.Order[] memory orders, - uint256 makerAssetFillAmount, - bytes[] memory signatures - ) - public - returns (LibFillResults.FillResults memory fillResults); - /// @dev Synchronously executes multiple fill orders in a single transaction until total amount is bought by taker. /// Returns false if the transaction would otherwise revert. /// @param orders Array of order specifications. /// @param makerAssetFillAmount Desired amount of makerAsset to buy. /// @param signatures Proofs that orders have been signed by makers. /// @return Amounts filled and fees paid by makers and taker. - function marketBuyOrdersNoThrow( + function marketBuyOrders( LibOrder.Order[] memory orders, uint256 makerAssetFillAmount, bytes[] memory signatures diff --git a/contracts/exchange/test/utils/exchange_wrapper.ts b/contracts/exchange/test/utils/exchange_wrapper.ts index da244e35e3..9467fba577 100644 --- a/contracts/exchange/test/utils/exchange_wrapper.ts +++ b/contracts/exchange/test/utils/exchange_wrapper.ts @@ -133,25 +133,11 @@ export class ExchangeWrapper { return tx; } public async marketSellOrdersAsync( - orders: SignedOrder[], - from: string, - opts: { takerAssetFillAmount: BigNumber }, - ): Promise { - const txHash = await this._exchange.marketSellOrders.sendTransactionAsync( - orders, - opts.takerAssetFillAmount, - orders.map(signedOrder => signedOrder.signature), - { from }, - ); - const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); - return tx; - } - public async marketSellOrdersNoThrowAsync( orders: SignedOrder[], from: string, opts: { takerAssetFillAmount: BigNumber; gas?: number }, ): Promise { - const txHash = await this._exchange.marketSellOrdersNoThrow.sendTransactionAsync( + const txHash = await this._exchange.marketSellOrders.sendTransactionAsync( orders, opts.takerAssetFillAmount, orders.map(signedOrder => signedOrder.signature), @@ -161,25 +147,11 @@ export class ExchangeWrapper { return tx; } public async marketBuyOrdersAsync( - orders: SignedOrder[], - from: string, - opts: { makerAssetFillAmount: BigNumber }, - ): Promise { - const txHash = await this._exchange.marketBuyOrders.sendTransactionAsync( - orders, - opts.makerAssetFillAmount, - orders.map(signedOrder => signedOrder.signature), - { from }, - ); - const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); - return tx; - } - public async marketBuyOrdersNoThrowAsync( orders: SignedOrder[], from: string, opts: { makerAssetFillAmount: BigNumber; gas?: number }, ): Promise { - const txHash = await this._exchange.marketBuyOrdersNoThrow.sendTransactionAsync( + const txHash = await this._exchange.marketBuyOrders.sendTransactionAsync( orders, opts.makerAssetFillAmount, orders.map(signedOrder => signedOrder.signature), diff --git a/contracts/exchange/test/wrapper.ts b/contracts/exchange/test/wrapper.ts index 9dcd4e0501..0df4805a26 100644 --- a/contracts/exchange/test/wrapper.ts +++ b/contracts/exchange/test/wrapper.ts @@ -925,167 +925,7 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); - const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); - const tx = exchangeWrapper.marketSellOrdersAsync([signedOrder], takerAddress, { - takerAssetFillAmount: signedOrder.takerAssetAmount, - }); - return expect(tx).to.revertWith(expectedError); - }); - } - }; - describe('marketSellOrders reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); - - it('should stop when the entire takerAssetFillAmount is filled', async () => { - const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus( - signedOrders[1].takerAssetAmount.div(2), - ); - - const fillResults = await exchange.marketSellOrders.callAsync( - signedOrders, - takerAssetFillAmount, - signedOrders.map(signedOrder => signedOrder.signature), - { from: takerAddress }, - ); - await exchangeWrapper.marketSellOrdersAsync(signedOrders, takerAddress, { - takerAssetFillAmount, - }); - const newBalances = await erc20Wrapper.getBalancesAsync(); - - const makerAssetFilledAmount = signedOrders[0].makerAssetAmount.plus( - signedOrders[1].makerAssetAmount.dividedToIntegerBy(2), - ); - const makerFee = signedOrders[0].makerFee.plus(signedOrders[1].makerFee.dividedToIntegerBy(2)); - const takerFee = signedOrders[0].takerFee.plus(signedOrders[1].takerFee.dividedToIntegerBy(2)); - - expect(fillResults.makerAssetFilledAmount).to.bignumber.equal(makerAssetFilledAmount); - expect(fillResults.takerAssetFilledAmount).to.bignumber.equal(takerAssetFillAmount); - expect(fillResults.makerFeePaid).to.bignumber.equal(makerFee); - expect(fillResults.takerFeePaid).to.bignumber.equal(takerFee); - - expect(newBalances[makerAddress][defaultMakerAssetAddress]).to.be.bignumber.equal( - erc20Balances[makerAddress][defaultMakerAssetAddress].minus(makerAssetFilledAmount), - ); - expect(newBalances[makerAddress][defaultTakerAssetAddress]).to.be.bignumber.equal( - erc20Balances[makerAddress][defaultTakerAssetAddress].plus(takerAssetFillAmount), - ); - expect(newBalances[makerAddress][feeToken.address]).to.be.bignumber.equal( - erc20Balances[makerAddress][feeToken.address].minus(makerFee), - ); - expect(newBalances[takerAddress][defaultTakerAssetAddress]).to.be.bignumber.equal( - erc20Balances[takerAddress][defaultTakerAssetAddress].minus(takerAssetFillAmount), - ); - expect(newBalances[takerAddress][defaultMakerAssetAddress]).to.be.bignumber.equal( - erc20Balances[takerAddress][defaultMakerAssetAddress].plus(makerAssetFilledAmount), - ); - expect(newBalances[takerAddress][feeToken.address]).to.be.bignumber.equal( - erc20Balances[takerAddress][feeToken.address].minus(takerFee), - ); - expect(newBalances[feeRecipientAddress][feeToken.address]).to.be.bignumber.equal( - erc20Balances[feeRecipientAddress][feeToken.address].plus(makerFee.plus(takerFee)), - ); - }); - - it('should fill all signedOrders if cannot fill entire takerAssetFillAmount', async () => { - const takerAssetFillAmount = Web3Wrapper.toBaseUnitAmount(new BigNumber(100000), 18); - _.forEach(signedOrders, signedOrder => { - erc20Balances[makerAddress][defaultMakerAssetAddress] = erc20Balances[makerAddress][ - defaultMakerAssetAddress - ].minus(signedOrder.makerAssetAmount); - erc20Balances[makerAddress][defaultTakerAssetAddress] = erc20Balances[makerAddress][ - defaultTakerAssetAddress - ].plus(signedOrder.takerAssetAmount); - erc20Balances[makerAddress][feeToken.address] = erc20Balances[makerAddress][feeToken.address].minus( - signedOrder.makerFee, - ); - erc20Balances[takerAddress][defaultMakerAssetAddress] = erc20Balances[takerAddress][ - defaultMakerAssetAddress - ].plus(signedOrder.makerAssetAmount); - erc20Balances[takerAddress][defaultTakerAssetAddress] = erc20Balances[takerAddress][ - defaultTakerAssetAddress - ].minus(signedOrder.takerAssetAmount); - erc20Balances[takerAddress][feeToken.address] = erc20Balances[takerAddress][feeToken.address].minus( - signedOrder.takerFee, - ); - erc20Balances[feeRecipientAddress][feeToken.address] = erc20Balances[feeRecipientAddress][ - feeToken.address - ].plus(signedOrder.makerFee.plus(signedOrder.takerFee)); - }); - - const fillResults = await exchange.marketSellOrders.callAsync( - signedOrders, - takerAssetFillAmount, - signedOrders.map(signedOrder => signedOrder.signature), - { from: takerAddress }, - ); - await exchangeWrapper.marketSellOrdersAsync(signedOrders, takerAddress, { - takerAssetFillAmount, - }); - const newBalances = await erc20Wrapper.getBalancesAsync(); - - const expectedFillResults = signedOrders - .map(signedOrder => ({ - makerAssetFilledAmount: signedOrder.makerAssetAmount, - takerAssetFilledAmount: signedOrder.takerAssetAmount, - makerFeePaid: signedOrder.makerFee, - takerFeePaid: signedOrder.takerFee, - })) - .reduce( - (totalFillResults, currentFillResults) => ({ - makerAssetFilledAmount: totalFillResults.makerAssetFilledAmount.plus( - currentFillResults.makerAssetFilledAmount, - ), - takerAssetFilledAmount: totalFillResults.takerAssetFilledAmount.plus( - currentFillResults.takerAssetFilledAmount, - ), - makerFeePaid: totalFillResults.makerFeePaid.plus(currentFillResults.makerFeePaid), - takerFeePaid: totalFillResults.takerFeePaid.plus(currentFillResults.takerFeePaid), - }), - nullFillResults, - ); - - expect(fillResults).to.deep.equal(expectedFillResults); - expect(newBalances).to.be.deep.equal(erc20Balances); - }); - - it('should revert when a signedOrder does not use the same takerAssetAddress', async () => { - signedOrders = [ - await orderFactory.newSignedOrderAsync(), - await orderFactory.newSignedOrderAsync({ - takerAssetData: assetDataUtils.encodeERC20AssetData(feeToken.address), - }), - await orderFactory.newSignedOrderAsync(), - ]; - const reconstructedOrder = { - ...signedOrders[1], - takerAssetData: signedOrders[0].takerAssetData, - }; - const orderHashHex = orderHashUtils.getOrderHashHex(reconstructedOrder); - const expectedError = new ExchangeRevertErrors.SignatureError( - ExchangeRevertErrors.SignatureErrorCode.BadSignature, - orderHashHex, - signedOrders[1].makerAddress, - signedOrders[1].signature, - ); - const tx = exchangeWrapper.marketSellOrdersAsync(signedOrders, takerAddress, { - takerAssetFillAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1000), 18), - }); - return expect(tx).to.revertWith(expectedError); - }); - }); - - describe('marketSellOrdersNoThrow', () => { - const reentrancyTest = (functionNames: string[]) => { - for (const [functionId, functionName] of functionNames.entries()) { - const description = `should not allow marketSellOrdersNoThrow to reenter the Exchange contract via ${functionName}`; - it(description, async () => { - const signedOrder = await orderFactory.newSignedOrderAsync({ - makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), - }); - await web3Wrapper.awaitTransactionSuccessAsync( - await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), - constants.AWAIT_TRANSACTION_MINED_MS, - ); - await exchangeWrapper.marketSellOrdersNoThrowAsync([signedOrder], takerAddress, { + await exchangeWrapper.marketSellOrdersAsync([signedOrder], takerAddress, { takerAssetFillAmount: signedOrder.takerAssetAmount, }); const newBalances = await erc20Wrapper.getBalancesAsync(); @@ -1093,7 +933,7 @@ describe('Exchange wrappers', () => { }); } }; - describe('marketSellOrdersNoThrow reentrancy tests', () => + describe('marketSellOrders reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); it('should stop when the entire takerAssetFillAmount is filled', async () => { @@ -1101,13 +941,13 @@ describe('Exchange wrappers', () => { signedOrders[1].takerAssetAmount.div(2), ); - const fillResults = await exchange.marketSellOrdersNoThrow.callAsync( + const fillResults = await exchange.marketSellOrders.callAsync( signedOrders, takerAssetFillAmount, signedOrders.map(signedOrder => signedOrder.signature), { from: takerAddress }, ); - await exchangeWrapper.marketSellOrdersNoThrowAsync(signedOrders, takerAddress, { + await exchangeWrapper.marketSellOrdersAsync(signedOrders, takerAddress, { takerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because // the Geth gas estimator doesn't work with the way we use @@ -1176,13 +1016,13 @@ describe('Exchange wrappers', () => { ].plus(signedOrder.makerFee.plus(signedOrder.takerFee)); }); - const fillResults = await exchange.marketSellOrdersNoThrow.callAsync( + const fillResults = await exchange.marketSellOrders.callAsync( signedOrders, takerAssetFillAmount, signedOrders.map(signedOrder => signedOrder.signature), { from: takerAddress }, ); - await exchangeWrapper.marketSellOrdersNoThrowAsync(signedOrders, takerAddress, { + await exchangeWrapper.marketSellOrdersAsync(signedOrders, takerAddress, { takerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because // the Geth gas estimator doesn't work with the way we use @@ -1250,13 +1090,13 @@ describe('Exchange wrappers', () => { ].plus(signedOrder.makerFee.plus(signedOrder.takerFee)); }); - const fillResults = await exchange.marketSellOrdersNoThrow.callAsync( + const fillResults = await exchange.marketSellOrders.callAsync( signedOrders, takerAssetFillAmount, signedOrders.map(signedOrder => signedOrder.signature), { from: takerAddress }, ); - await exchangeWrapper.marketSellOrdersNoThrowAsync(signedOrders, takerAddress, { + await exchangeWrapper.marketSellOrdersAsync(signedOrders, takerAddress, { takerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because // the Geth gas estimator doesn't work with the way we use @@ -1303,167 +1143,7 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); - const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); - const tx = exchangeWrapper.marketBuyOrdersAsync([signedOrder], takerAddress, { - makerAssetFillAmount: signedOrder.makerAssetAmount, - }); - return expect(tx).to.revertWith(expectedError); - }); - } - }; - describe('marketBuyOrders reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); - - it('should stop when the entire makerAssetFillAmount is filled', async () => { - const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus( - signedOrders[1].makerAssetAmount.div(2), - ); - - const fillResults = await exchange.marketBuyOrders.callAsync( - signedOrders, - makerAssetFillAmount, - signedOrders.map(signedOrder => signedOrder.signature), - { from: takerAddress }, - ); - await exchangeWrapper.marketBuyOrdersAsync(signedOrders, takerAddress, { - makerAssetFillAmount, - }); - const newBalances = await erc20Wrapper.getBalancesAsync(); - - const makerAmountBought = signedOrders[0].takerAssetAmount.plus( - signedOrders[1].takerAssetAmount.dividedToIntegerBy(2), - ); - const makerFee = signedOrders[0].makerFee.plus(signedOrders[1].makerFee.dividedToIntegerBy(2)); - const takerFee = signedOrders[0].takerFee.plus(signedOrders[1].takerFee.dividedToIntegerBy(2)); - - expect(fillResults.makerAssetFilledAmount).to.bignumber.equal(makerAssetFillAmount); - expect(fillResults.takerAssetFilledAmount).to.bignumber.equal(makerAmountBought); - expect(fillResults.makerFeePaid).to.bignumber.equal(makerFee); - expect(fillResults.takerFeePaid).to.bignumber.equal(takerFee); - - expect(newBalances[makerAddress][defaultMakerAssetAddress]).to.be.bignumber.equal( - erc20Balances[makerAddress][defaultMakerAssetAddress].minus(makerAssetFillAmount), - ); - expect(newBalances[makerAddress][defaultTakerAssetAddress]).to.be.bignumber.equal( - erc20Balances[makerAddress][defaultTakerAssetAddress].plus(makerAmountBought), - ); - expect(newBalances[makerAddress][feeToken.address]).to.be.bignumber.equal( - erc20Balances[makerAddress][feeToken.address].minus(makerFee), - ); - expect(newBalances[takerAddress][defaultTakerAssetAddress]).to.be.bignumber.equal( - erc20Balances[takerAddress][defaultTakerAssetAddress].minus(makerAmountBought), - ); - expect(newBalances[takerAddress][defaultMakerAssetAddress]).to.be.bignumber.equal( - erc20Balances[takerAddress][defaultMakerAssetAddress].plus(makerAssetFillAmount), - ); - expect(newBalances[takerAddress][feeToken.address]).to.be.bignumber.equal( - erc20Balances[takerAddress][feeToken.address].minus(takerFee), - ); - expect(newBalances[feeRecipientAddress][feeToken.address]).to.be.bignumber.equal( - erc20Balances[feeRecipientAddress][feeToken.address].plus(makerFee.plus(takerFee)), - ); - }); - - it('should fill all signedOrders if cannot fill entire makerAssetFillAmount', async () => { - const makerAssetFillAmount = Web3Wrapper.toBaseUnitAmount(new BigNumber(100000), 18); - _.forEach(signedOrders, signedOrder => { - erc20Balances[makerAddress][defaultMakerAssetAddress] = erc20Balances[makerAddress][ - defaultMakerAssetAddress - ].minus(signedOrder.makerAssetAmount); - erc20Balances[makerAddress][defaultTakerAssetAddress] = erc20Balances[makerAddress][ - defaultTakerAssetAddress - ].plus(signedOrder.takerAssetAmount); - erc20Balances[makerAddress][feeToken.address] = erc20Balances[makerAddress][feeToken.address].minus( - signedOrder.makerFee, - ); - erc20Balances[takerAddress][defaultMakerAssetAddress] = erc20Balances[takerAddress][ - defaultMakerAssetAddress - ].plus(signedOrder.makerAssetAmount); - erc20Balances[takerAddress][defaultTakerAssetAddress] = erc20Balances[takerAddress][ - defaultTakerAssetAddress - ].minus(signedOrder.takerAssetAmount); - erc20Balances[takerAddress][feeToken.address] = erc20Balances[takerAddress][feeToken.address].minus( - signedOrder.takerFee, - ); - erc20Balances[feeRecipientAddress][feeToken.address] = erc20Balances[feeRecipientAddress][ - feeToken.address - ].plus(signedOrder.makerFee.plus(signedOrder.takerFee)); - }); - - const fillResults = await exchange.marketBuyOrders.callAsync( - signedOrders, - makerAssetFillAmount, - signedOrders.map(signedOrder => signedOrder.signature), - { from: takerAddress }, - ); - await exchangeWrapper.marketBuyOrdersAsync(signedOrders, takerAddress, { - makerAssetFillAmount, - }); - const newBalances = await erc20Wrapper.getBalancesAsync(); - - const expectedFillResults = signedOrders - .map(signedOrder => ({ - makerAssetFilledAmount: signedOrder.makerAssetAmount, - takerAssetFilledAmount: signedOrder.takerAssetAmount, - makerFeePaid: signedOrder.makerFee, - takerFeePaid: signedOrder.takerFee, - })) - .reduce( - (totalFillResults, currentFillResults) => ({ - makerAssetFilledAmount: totalFillResults.makerAssetFilledAmount.plus( - currentFillResults.makerAssetFilledAmount, - ), - takerAssetFilledAmount: totalFillResults.takerAssetFilledAmount.plus( - currentFillResults.takerAssetFilledAmount, - ), - makerFeePaid: totalFillResults.makerFeePaid.plus(currentFillResults.makerFeePaid), - takerFeePaid: totalFillResults.takerFeePaid.plus(currentFillResults.takerFeePaid), - }), - nullFillResults, - ); - - expect(fillResults).to.deep.equal(expectedFillResults); - expect(newBalances).to.be.deep.equal(erc20Balances); - }); - - it('should revert when a signedOrder does not use the same makerAssetAddress', async () => { - signedOrders = [ - await orderFactory.newSignedOrderAsync(), - await orderFactory.newSignedOrderAsync({ - makerAssetData: assetDataUtils.encodeERC20AssetData(feeToken.address), - }), - await orderFactory.newSignedOrderAsync(), - ]; - const reconstructedOrder = { - ...signedOrders[1], - makerAssetData: signedOrders[0].makerAssetData, - }; - const orderHashHex = orderHashUtils.getOrderHashHex(reconstructedOrder); - const expectedError = new ExchangeRevertErrors.SignatureError( - ExchangeRevertErrors.SignatureErrorCode.BadSignature, - orderHashHex, - signedOrders[1].makerAddress, - signedOrders[1].signature, - ); - const tx = exchangeWrapper.marketBuyOrdersAsync(signedOrders, takerAddress, { - makerAssetFillAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(1000), 18), - }); - return expect(tx).to.revertWith(expectedError); - }); - }); - - describe('marketBuyOrdersNoThrow', () => { - const reentrancyTest = (functionNames: string[]) => { - for (const [functionId, functionName] of functionNames.entries()) { - const description = `should not allow marketBuyOrdersNoThrow to reenter the Exchange contract via ${functionName}`; - it(description, async () => { - const signedOrder = await orderFactory.newSignedOrderAsync({ - makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), - }); - await web3Wrapper.awaitTransactionSuccessAsync( - await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), - constants.AWAIT_TRANSACTION_MINED_MS, - ); - await exchangeWrapper.marketBuyOrdersNoThrowAsync([signedOrder], takerAddress, { + await exchangeWrapper.marketBuyOrdersAsync([signedOrder], takerAddress, { makerAssetFillAmount: signedOrder.makerAssetAmount, }); const newBalances = await erc20Wrapper.getBalancesAsync(); @@ -1471,7 +1151,7 @@ describe('Exchange wrappers', () => { }); } }; - describe('marketBuyOrdersNoThrow reentrancy tests', () => + describe('marketBuyOrders reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); it('should stop when the entire makerAssetFillAmount is filled', async () => { @@ -1479,13 +1159,13 @@ describe('Exchange wrappers', () => { signedOrders[1].makerAssetAmount.div(2), ); - const fillResults = await exchange.marketBuyOrdersNoThrow.callAsync( + const fillResults = await exchange.marketBuyOrders.callAsync( signedOrders, makerAssetFillAmount, signedOrders.map(signedOrder => signedOrder.signature), { from: takerAddress }, ); - await exchangeWrapper.marketBuyOrdersNoThrowAsync(signedOrders, takerAddress, { + await exchangeWrapper.marketBuyOrdersAsync(signedOrders, takerAddress, { makerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because // the Geth gas estimator doesn't work with the way we use @@ -1554,13 +1234,13 @@ describe('Exchange wrappers', () => { ].plus(signedOrder.makerFee.plus(signedOrder.takerFee)); }); - const fillResults = await exchange.marketBuyOrdersNoThrow.callAsync( + const fillResults = await exchange.marketBuyOrders.callAsync( signedOrders, makerAssetFillAmount, signedOrders.map(signedOrder => signedOrder.signature), { from: takerAddress }, ); - await exchangeWrapper.marketBuyOrdersNoThrowAsync(signedOrders, takerAddress, { + await exchangeWrapper.marketBuyOrdersAsync(signedOrders, takerAddress, { makerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because // the Geth gas estimator doesn't work with the way we use @@ -1629,13 +1309,13 @@ describe('Exchange wrappers', () => { ].plus(signedOrder.makerFee.plus(signedOrder.takerFee)); }); - const fillResults = await exchange.marketBuyOrdersNoThrow.callAsync( + const fillResults = await exchange.marketBuyOrders.callAsync( signedOrders, makerAssetFillAmount, signedOrders.map(signedOrder => signedOrder.signature), { from: takerAddress }, ); - await exchangeWrapper.marketBuyOrdersNoThrowAsync(signedOrders, takerAddress, { + await exchangeWrapper.marketBuyOrdersAsync(signedOrders, takerAddress, { makerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because // the Geth gas estimator doesn't work with the way we use diff --git a/contracts/exchange/test/wrapper_unit_tests.ts b/contracts/exchange/test/wrapper_unit_tests.ts index 8777886dce..52837157e8 100644 --- a/contracts/exchange/test/wrapper_unit_tests.ts +++ b/contracts/exchange/test/wrapper_unit_tests.ts @@ -17,6 +17,7 @@ import { artifacts, TestWrapperFunctionsContract, TestWrapperFunctionsFillOrderCalledEventArgs as FillOrderCalledEventArgs, + TestWrapperFunctionsCancelOrderCalledEventArgs as CancelOrderCalledEventArgs, } from '../src'; blockchainTests.only('Exchange wrapper functions unit tests.', env => { @@ -37,8 +38,10 @@ blockchainTests.only('Exchange wrapper functions unit tests.', env => { }; let testContract: TestWrapperFunctionsContract; let txHelper: TransactionHelper; + let senderAddress: string; before(async () => { + [ senderAddress ] = await env.getAccountAddressesAsync(); txHelper = new TransactionHelper(env.web3Wrapper, artifacts); testContract = await TestWrapperFunctionsContract.deployFrom0xArtifactAsync( artifacts.TestWrapperFunctions, @@ -585,6 +588,66 @@ blockchainTests.only('Exchange wrapper functions unit tests.', env => { }); }); + // Asserts that `_cancelOrder()` was called in the same order and with the same + // arguments as given by examining receipt logs. + function assertCancelOrderCallsFromLogs( + logs: LogEntry[], + calls: Order[], + ): void { + expect(logs.length).to.eq(calls.length); + for (const i of _.times(calls.length)) { + const log = logs[i] as LogWithDecodedArgs; + const expectedOrder = calls[i]; + expect(log.event).to.eq('CancelOrderCalled'); + assertSameOrderFromEvent(log.args.order as any, expectedOrder); + } + } + + describe('batchCancelOrders', () => { + it('works with no orders', async () => { + const [ , receipt ] = await txHelper.getResultAndReceiptAsync( + testContract.batchCancelOrders, + [], + ); + assertCancelOrderCallsFromLogs(receipt.logs, []); + }); + + it('works with many orders', async () => { + const COUNT = 8; + const orders = _.times(COUNT, () => randomOrder({ makerAddress: senderAddress })); + const [ , receipt ] = await txHelper.getResultAndReceiptAsync( + testContract.batchCancelOrders, + orders, + ); + assertCancelOrderCallsFromLogs(receipt.logs, orders); + }); + + it('works with duplicate orders', async () => { + const COUNT = 3; + const order = randomOrder({ makerAddress: senderAddress }); + const orders = _.times(COUNT, () => order); + const [ , receipt ] = await txHelper.getResultAndReceiptAsync( + testContract.batchCancelOrders, + orders, + ); + assertCancelOrderCallsFromLogs(receipt.logs, orders); + }); + + it('reverts if one `_cancelOrder()` reverts', async () => { + const COUNT = 8; + const FAILING_ORDER_INDEX = 4; + const orders = _.times(COUNT, () => randomOrder({ makerAddress: senderAddress })); + const failingOrder = orders[FAILING_ORDER_INDEX]; + failingOrder.salt = ALWAYS_FAILING_SALT; + const expectedError = ALWAYS_FAILING_SALT_REVERT_ERROR; + const tx = txHelper.getResultAndReceiptAsync( + testContract.batchCancelOrders, + orders, + ); + return expect(tx).to.revertWith(expectedError); + }); + }); + describe('getOrdersInfo', () => { // Computes the expected (fake) order info generated by the `TestWrapperFunctions` contract. function getExpectedOrderInfo(order: Order): OrderInfo {