From f5cdadb93deb6f48c22cdfd3a474caf627c32892 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 23 May 2019 22:37:32 -0500 Subject: [PATCH 1/7] Return FillResults[] for batch fill methods --- .../contracts/src/MixinWrapperFunctions.sol | 33 +++++++++---------- .../src/interfaces/IWrapperFunctions.sol | 12 +++---- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol index 5e4d921ee6..02c46b59db 100644 --- a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol @@ -106,8 +106,7 @@ contract MixinWrapperFunctions is /// @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. - /// @return Amounts filled and fees paid by makers and taker. - /// NOTE: makerAssetFilledAmount and takerAssetFilledAmount may include amounts filled of different assets. + /// @return Array of amounts filled and fees paid by makers and taker. function batchFillOrders( LibOrder.Order[] memory orders, uint256[] memory takerAssetFillAmounts, @@ -115,26 +114,25 @@ contract MixinWrapperFunctions is ) public nonReentrant - returns (FillResults memory totalFillResults) + returns (FillResults[] memory) { uint256 ordersLength = orders.length; + FillResults[] memory fillResults = new FillResults[](ordersLength); for (uint256 i = 0; i != ordersLength; i++) { - FillResults memory singleFillResults = _fillOrder( + fillResults[i] = _fillOrder( orders[i], takerAssetFillAmounts[i], signatures[i] ); - _addFillResults(totalFillResults, singleFillResults); } - return totalFillResults; + return fillResults; } /// @dev Synchronously executes multiple calls of fillOrKill. /// @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. - /// @return Amounts filled and fees paid by makers and taker. - /// NOTE: makerAssetFilledAmount and takerAssetFilledAmount may include amounts filled of different assets. + /// @return Array of amounts filled and fees paid by makers and taker. function batchFillOrKillOrders( LibOrder.Order[] memory orders, uint256[] memory takerAssetFillAmounts, @@ -142,18 +140,18 @@ contract MixinWrapperFunctions is ) public nonReentrant - returns (FillResults memory totalFillResults) + returns (FillResults[] memory) { uint256 ordersLength = orders.length; + FillResults[] memory fillResults = new FillResults[](ordersLength); for (uint256 i = 0; i != ordersLength; i++) { - FillResults memory singleFillResults = _fillOrKillOrder( + fillResults[i] = _fillOrKillOrder( orders[i], takerAssetFillAmounts[i], signatures[i] ); - _addFillResults(totalFillResults, singleFillResults); } - return totalFillResults; + return fillResults; } /// @dev Fills an order with specified parameters and ECDSA signature. @@ -161,26 +159,25 @@ contract MixinWrapperFunctions is /// @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. - /// @return Amounts filled and fees paid by makers and taker. - /// NOTE: makerAssetFilledAmount and takerAssetFilledAmount may include amounts filled of different assets. + /// @return Array of amounts filled and fees paid by makers and taker. function batchFillOrdersNoThrow( LibOrder.Order[] memory orders, uint256[] memory takerAssetFillAmounts, bytes[] memory signatures ) public - returns (FillResults memory totalFillResults) + returns (FillResults[] memory) { uint256 ordersLength = orders.length; + FillResults[] memory fillResults = new FillResults[](ordersLength); for (uint256 i = 0; i != ordersLength; i++) { - FillResults memory singleFillResults = fillOrderNoThrow( + fillResults[i] = fillOrderNoThrow( orders[i], takerAssetFillAmounts[i], signatures[i] ); - _addFillResults(totalFillResults, singleFillResults); } - return totalFillResults; + return fillResults; } /// @dev Synchronously executes multiple calls of fillOrder until total amount of takerAsset is sold by taker. diff --git a/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol b/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol index 9d801711db..a3c244f246 100644 --- a/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol @@ -55,41 +55,41 @@ contract IWrapperFunctions { /// @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. - /// @return Amounts filled and fees paid by makers and taker. + /// @return Array of amounts filled and fees paid by makers and taker. function batchFillOrders( LibOrder.Order[] memory orders, uint256[] memory takerAssetFillAmounts, bytes[] memory signatures ) public - returns (LibFillResults.FillResults memory totalFillResults); + returns (LibFillResults.FillResults[] memory); /// @dev Synchronously executes multiple calls of fillOrKill. /// @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. - /// @return Amounts filled and fees paid by makers and taker. + /// @return Array of amounts filled and fees paid by makers and taker. function batchFillOrKillOrders( LibOrder.Order[] memory orders, uint256[] memory takerAssetFillAmounts, bytes[] memory signatures ) public - returns (LibFillResults.FillResults memory totalFillResults); + returns (LibFillResults.FillResults[] memory); /// @dev Fills an order with specified parameters and ECDSA signature. /// 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. - /// @return Amounts filled and fees paid by makers and taker. + /// @return Array of amounts filled and fees paid by makers and taker. function batchFillOrdersNoThrow( LibOrder.Order[] memory orders, uint256[] memory takerAssetFillAmounts, bytes[] memory signatures ) public - returns (LibFillResults.FillResults memory totalFillResults); + returns (LibFillResults.FillResults[] memory); /// @dev Synchronously executes multiple calls of fillOrder until total amount of takerAsset is sold by taker. /// @param orders Array of order specifications. From 6ad9268dd69d24ccf1f198e933be6f984dc7ea06 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Sat, 25 May 2019 17:14:58 -0700 Subject: [PATCH 2/7] Add return value checks to wrapper tests --- contracts/exchange/test/wrapper.ts | 476 ++++++++++++++++++++++++++--- 1 file changed, 435 insertions(+), 41 deletions(-) diff --git a/contracts/exchange/test/wrapper.ts b/contracts/exchange/test/wrapper.ts index 2c9cb858b4..945ae1c0eb 100644 --- a/contracts/exchange/test/wrapper.ts +++ b/contracts/exchange/test/wrapper.ts @@ -5,6 +5,7 @@ import { chaiSetup, constants, ERC20BalancesByOwner, + FillResults, getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync, OrderFactory, @@ -64,6 +65,13 @@ describe('Exchange wrappers', () => { let defaultTakerAssetAddress: string; let defaultFeeAssetAddress: string; + const nullFillResults: FillResults = { + makerAssetFilledAmount: new BigNumber(0), + takerAssetFilledAmount: new BigNumber(0), + makerFeePaid: new BigNumber(0), + takerFeePaid: new BigNumber(0), + }; + before(async () => { await blockchainLifecycle.startAsync(); }); @@ -152,7 +160,7 @@ describe('Exchange wrappers', () => { }); describe('fillOrKillOrder', () => { const reentrancyTest = (functionNames: string[]) => { - _.forEach(functionNames, async (functionName: string, functionId: number) => { + for (const [functionId, functionName] of functionNames.entries()) { const description = `should not allow fillOrKillOrder to reenter the Exchange contract via ${functionName}`; it(description, async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ @@ -165,7 +173,7 @@ describe('Exchange wrappers', () => { const tx = exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress); return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); - }); + } }; describe('fillOrKillOrder reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); @@ -175,10 +183,16 @@ describe('Exchange wrappers', () => { takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(200), 18), }); const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); + + const fillResults = await exchange.fillOrKillOrder.callAsync( + signedOrder, + takerAssetFillAmount, + signedOrder.signature, + { from: takerAddress }, + ); await exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress, { takerAssetFillAmount, }); - const newBalances = await erc20Wrapper.getBalancesAsync(); const makerAssetFilledAmount = takerAssetFillAmount @@ -190,6 +204,12 @@ describe('Exchange wrappers', () => { const takerFee = signedOrder.takerFee .times(makerAssetFilledAmount) .dividedToIntegerBy(signedOrder.makerAssetAmount); + + 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), ); @@ -240,7 +260,7 @@ describe('Exchange wrappers', () => { describe('fillOrderNoThrow', () => { const reentrancyTest = (functionNames: string[]) => { - _.forEach(functionNames, async (functionName: string, functionId: number) => { + for (const [functionId, functionName] of functionNames.entries()) { const description = `should not allow fillOrderNoThrow to reenter the Exchange contract via ${functionName}`; it(description, async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ @@ -254,7 +274,7 @@ describe('Exchange wrappers', () => { const newBalances = await erc20Wrapper.getBalancesAsync(); expect(erc20Balances).to.deep.equal(newBalances); }); - }); + } }; describe('fillOrderNoThrow reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); @@ -265,6 +285,12 @@ describe('Exchange wrappers', () => { }); const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); + const fillResults = await exchange.fillOrderNoThrow.callAsync( + signedOrder, + takerAssetFillAmount, + signedOrder.signature, + { from: takerAddress }, + ); await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress, { takerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because @@ -272,8 +298,8 @@ describe('Exchange wrappers', () => { // delegatecall and swallow errors. gas: 250000, }); - const newBalances = await erc20Wrapper.getBalancesAsync(); + const makerAssetFilledAmount = takerAssetFillAmount .times(signedOrder.makerAssetAmount) .dividedToIntegerBy(signedOrder.takerAssetAmount); @@ -284,6 +310,11 @@ describe('Exchange wrappers', () => { .times(makerAssetFilledAmount) .dividedToIntegerBy(signedOrder.makerAssetAmount); + 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), ); @@ -312,8 +343,16 @@ describe('Exchange wrappers', () => { makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100000), 18), }); + const fillResults = await exchange.fillOrderNoThrow.callAsync( + signedOrder, + signedOrder.takerAssetAmount, + signedOrder.signature, + { from: takerAddress }, + ); await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress); const newBalances = await erc20Wrapper.getBalancesAsync(); + + expect(fillResults).to.deep.equal(nullFillResults); expect(newBalances).to.be.deep.equal(erc20Balances); }); @@ -322,19 +361,34 @@ describe('Exchange wrappers', () => { takerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100000), 18), }); + const fillResults = await exchange.fillOrderNoThrow.callAsync( + signedOrder, + signedOrder.takerAssetAmount, + signedOrder.signature, + { from: takerAddress }, + ); await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress); const newBalances = await erc20Wrapper.getBalancesAsync(); + + expect(fillResults).to.deep.equal(nullFillResults); expect(newBalances).to.be.deep.equal(erc20Balances); }); it('should not change erc20Balances if maker allowances are too low to fill order', async () => { const signedOrder = await orderFactory.newSignedOrderAsync(); + await web3Wrapper.awaitTransactionSuccessAsync( await erc20TokenA.approve.sendTransactionAsync(erc20Proxy.address, new BigNumber(0), { from: makerAddress, }), constants.AWAIT_TRANSACTION_MINED_MS, ); + const fillResults = await exchange.fillOrderNoThrow.callAsync( + signedOrder, + signedOrder.takerAssetAmount, + signedOrder.signature, + { from: takerAddress }, + ); await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress); await web3Wrapper.awaitTransactionSuccessAsync( await erc20TokenA.approve.sendTransactionAsync(erc20Proxy.address, constants.INITIAL_ERC20_ALLOWANCE, { @@ -342,19 +396,27 @@ describe('Exchange wrappers', () => { }), constants.AWAIT_TRANSACTION_MINED_MS, ); - const newBalances = await erc20Wrapper.getBalancesAsync(); + + expect(fillResults).to.deep.equal(nullFillResults); expect(newBalances).to.be.deep.equal(erc20Balances); }); it('should not change erc20Balances if taker allowances are too low to fill order', async () => { const signedOrder = await orderFactory.newSignedOrderAsync(); + await web3Wrapper.awaitTransactionSuccessAsync( await erc20TokenB.approve.sendTransactionAsync(erc20Proxy.address, new BigNumber(0), { from: takerAddress, }), constants.AWAIT_TRANSACTION_MINED_MS, ); + const fillResults = await exchange.fillOrderNoThrow.callAsync( + signedOrder, + signedOrder.takerAssetAmount, + signedOrder.signature, + { from: takerAddress }, + ); await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress); await web3Wrapper.awaitTransactionSuccessAsync( await erc20TokenB.approve.sendTransactionAsync(erc20Proxy.address, constants.INITIAL_ERC20_ALLOWANCE, { @@ -362,8 +424,9 @@ describe('Exchange wrappers', () => { }), constants.AWAIT_TRANSACTION_MINED_MS, ); - const newBalances = await erc20Wrapper.getBalancesAsync(); + + expect(fillResults).to.deep.equal(nullFillResults); expect(newBalances).to.be.deep.equal(erc20Balances); }); @@ -374,8 +437,17 @@ describe('Exchange wrappers', () => { makerFee: new BigNumber(1), makerAssetData: assetDataUtils.encodeERC20AssetData(feeToken.address), }); + + const fillResults = await exchange.fillOrderNoThrow.callAsync( + signedOrder, + signedOrder.takerAssetAmount, + signedOrder.signature, + { from: takerAddress }, + ); await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress); const newBalances = await erc20Wrapper.getBalancesAsync(); + + expect(fillResults).to.deep.equal(nullFillResults); expect(newBalances).to.be.deep.equal(erc20Balances); }); @@ -398,8 +470,17 @@ describe('Exchange wrappers', () => { takerFee: new BigNumber(1), takerAssetData: assetDataUtils.encodeERC20AssetData(feeToken.address), }); + + const fillResults = await exchange.fillOrderNoThrow.callAsync( + signedOrder, + signedOrder.takerAssetAmount, + signedOrder.signature, + { from: takerAddress }, + ); await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress); const newBalances = await erc20Wrapper.getBalancesAsync(); + + expect(fillResults).to.deep.equal(nullFillResults); expect(newBalances).to.be.deep.equal(erc20Balances); }); @@ -410,8 +491,17 @@ describe('Exchange wrappers', () => { takerFee: new BigNumber(1), takerAssetData: assetDataUtils.encodeERC20AssetData(feeToken.address), }); + + const fillResults = await exchange.fillOrderNoThrow.callAsync( + signedOrder, + signedOrder.takerAssetAmount, + signedOrder.signature, + { from: takerAddress }, + ); await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress); const newBalances = await erc20Wrapper.getBalancesAsync(); + + expect(fillResults).to.deep.equal(nullFillResults); expect(newBalances).to.be.deep.equal(erc20Balances); }); @@ -432,6 +522,13 @@ describe('Exchange wrappers', () => { expect(initialOwnerTakerAsset).to.be.bignumber.equal(takerAddress); // Call Exchange const takerAssetFillAmount = signedOrder.takerAssetAmount; + + const fillResults = await exchange.fillOrKillOrder.callAsync( + signedOrder, + takerAssetFillAmount, + signedOrder.signature, + { from: takerAddress }, + ); await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress, { takerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because @@ -439,7 +536,13 @@ describe('Exchange wrappers', () => { // delegatecall and swallow errors. gas: 280000, }); + // Verify post-conditions + expect(fillResults.makerAssetFilledAmount).to.bignumber.equal(signedOrder.makerAssetAmount); + expect(fillResults.takerAssetFilledAmount).to.bignumber.equal(signedOrder.takerAssetAmount); + expect(fillResults.makerFeePaid).to.bignumber.equal(signedOrder.makerFee); + expect(fillResults.takerFeePaid).to.bignumber.equal(signedOrder.takerFee); + const newOwnerMakerAsset = await erc721Token.ownerOf.callAsync(makerAssetId); expect(newOwnerMakerAsset).to.be.bignumber.equal(takerAddress); const newOwnerTakerAsset = await erc721Token.ownerOf.callAsync(takerAssetId); @@ -541,7 +644,7 @@ describe('Exchange wrappers', () => { describe('batchFillOrders', () => { const reentrancyTest = (functionNames: string[]) => { - _.forEach(functionNames, async (functionName: string, functionId: number) => { + for (const [functionId, functionName] of functionNames.entries()) { const description = `should not allow batchFillOrders to reenter the Exchange contract via ${functionName}`; it(description, async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ @@ -554,14 +657,17 @@ describe('Exchange wrappers', () => { const tx = exchangeWrapper.batchFillOrdersAsync([signedOrder], takerAddress); return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); - }); + } }; describe('batchFillOrders reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); it('should transfer the correct amounts', async () => { - const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; const takerAssetAddress = erc20TokenB.address; + + const takerAssetFillAmounts: BigNumber[] = []; + const expectedFillResults: FillResults[] = []; + _.forEach(signedOrders, signedOrder => { const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); const makerAssetFilledAmount = takerAssetFillAmount @@ -573,7 +679,15 @@ describe('Exchange wrappers', () => { const takerFee = signedOrder.takerFee .times(makerAssetFilledAmount) .dividedToIntegerBy(signedOrder.makerAssetAmount); + takerAssetFillAmounts.push(takerAssetFillAmount); + expectedFillResults.push({ + takerAssetFilledAmount: takerAssetFillAmount, + makerAssetFilledAmount, + makerFeePaid: makerFee, + takerFeePaid: takerFee, + }); + erc20Balances[makerAddress][makerAssetAddress] = erc20Balances[makerAddress][ makerAssetAddress ].minus(makerAssetFilledAmount); @@ -597,18 +711,25 @@ describe('Exchange wrappers', () => { ].plus(makerFee.plus(takerFee)); }); + const fillResults = await exchange.batchFillOrders.callAsync( + signedOrders, + takerAssetFillAmounts, + signedOrders.map(signedOrder => signedOrder.signature), + { from: takerAddress }, + ); await exchangeWrapper.batchFillOrdersAsync(signedOrders, takerAddress, { takerAssetFillAmounts, }); - const newBalances = await erc20Wrapper.getBalancesAsync(); + + expect(fillResults).to.deep.equal(expectedFillResults); expect(newBalances).to.be.deep.equal(erc20Balances); }); }); describe('batchFillOrKillOrders', () => { const reentrancyTest = (functionNames: string[]) => { - _.forEach(functionNames, async (functionName: string, functionId: number) => { + for (const [functionId, functionName] of functionNames.entries()) { const description = `should not allow batchFillOrKillOrders to reenter the Exchange contract via ${functionName}`; it(description, async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ @@ -621,15 +742,18 @@ describe('Exchange wrappers', () => { const tx = exchangeWrapper.batchFillOrKillOrdersAsync([signedOrder], takerAddress); return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); - }); + } }; describe('batchFillOrKillOrders reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); it('should transfer the correct amounts', async () => { - const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; const takerAssetAddress = erc20TokenB.address; + + const takerAssetFillAmounts: BigNumber[] = []; + const expectedFillResults: FillResults[] = []; + _.forEach(signedOrders, signedOrder => { const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); const makerAssetFilledAmount = takerAssetFillAmount @@ -641,7 +765,15 @@ describe('Exchange wrappers', () => { const takerFee = signedOrder.takerFee .times(makerAssetFilledAmount) .dividedToIntegerBy(signedOrder.makerAssetAmount); + takerAssetFillAmounts.push(takerAssetFillAmount); + expectedFillResults.push({ + takerAssetFilledAmount: takerAssetFillAmount, + makerAssetFilledAmount, + makerFeePaid: makerFee, + takerFeePaid: takerFee, + }); + erc20Balances[makerAddress][makerAssetAddress] = erc20Balances[makerAddress][ makerAssetAddress ].minus(makerAssetFilledAmount); @@ -665,11 +797,18 @@ describe('Exchange wrappers', () => { ].plus(makerFee.plus(takerFee)); }); + const fillResults = await exchange.batchFillOrKillOrders.callAsync( + signedOrders, + takerAssetFillAmounts, + signedOrders.map(signedOrder => signedOrder.signature), + { from: takerAddress }, + ); await exchangeWrapper.batchFillOrKillOrdersAsync(signedOrders, takerAddress, { takerAssetFillAmounts, }); - const newBalances = await erc20Wrapper.getBalancesAsync(); + + expect(fillResults).to.deep.equal(expectedFillResults); expect(newBalances).to.be.deep.equal(erc20Balances); }); @@ -692,7 +831,7 @@ describe('Exchange wrappers', () => { describe('batchFillOrdersNoThrow', async () => { const reentrancyTest = (functionNames: string[]) => { - _.forEach(functionNames, async (functionName: string, functionId: number) => { + for (const [functionId, functionName] of functionNames.entries()) { const description = `should not allow batchFillOrdersNoThrow to reenter the Exchange contract via ${functionName}`; it(description, async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ @@ -706,15 +845,18 @@ describe('Exchange wrappers', () => { const newBalances = await erc20Wrapper.getBalancesAsync(); expect(erc20Balances).to.deep.equal(newBalances); }); - }); + } }; describe('batchFillOrdersNoThrow reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); it('should transfer the correct amounts', async () => { - const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; const takerAssetAddress = erc20TokenB.address; + + const takerAssetFillAmounts: BigNumber[] = []; + const expectedFillResults: FillResults[] = []; + _.forEach(signedOrders, signedOrder => { const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); const makerAssetFilledAmount = takerAssetFillAmount @@ -726,7 +868,15 @@ describe('Exchange wrappers', () => { const takerFee = signedOrder.takerFee .times(makerAssetFilledAmount) .dividedToIntegerBy(signedOrder.makerAssetAmount); + takerAssetFillAmounts.push(takerAssetFillAmount); + expectedFillResults.push({ + takerAssetFilledAmount: takerAssetFillAmount, + makerAssetFilledAmount, + makerFeePaid: makerFee, + takerFeePaid: takerFee, + }); + erc20Balances[makerAddress][makerAssetAddress] = erc20Balances[makerAddress][ makerAssetAddress ].minus(makerAssetFilledAmount); @@ -750,6 +900,12 @@ describe('Exchange wrappers', () => { ].plus(makerFee.plus(takerFee)); }); + const fillResults = await exchange.batchFillOrdersNoThrow.callAsync( + signedOrders, + takerAssetFillAmounts, + signedOrders.map(signedOrder => signedOrder.signature), + { from: takerAddress }, + ); await exchangeWrapper.batchFillOrdersNoThrowAsync(signedOrders, takerAddress, { takerAssetFillAmounts, // HACK(albrow): We need to hardcode the gas estimate here because @@ -757,13 +913,13 @@ describe('Exchange wrappers', () => { // delegatecall and swallow errors. gas: 600000, }); - const newBalances = await erc20Wrapper.getBalancesAsync(); + + expect(fillResults).to.deep.equal(expectedFillResults); expect(newBalances).to.be.deep.equal(erc20Balances); }); it('should not throw if an order is invalid and fill the remaining orders', async () => { - const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; const takerAssetAddress = erc20TokenB.address; @@ -772,8 +928,9 @@ describe('Exchange wrappers', () => { signature: '0x00', }; const validOrders = signedOrders.slice(1); + const takerAssetFillAmounts: BigNumber[] = [invalidOrder.takerAssetAmount.div(2)]; + const expectedFillResults = [nullFillResults]; - takerAssetFillAmounts.push(invalidOrder.takerAssetAmount.div(2)); _.forEach(validOrders, signedOrder => { const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); const makerAssetFilledAmount = takerAssetFillAmount @@ -785,7 +942,15 @@ describe('Exchange wrappers', () => { const takerFee = signedOrder.takerFee .times(makerAssetFilledAmount) .dividedToIntegerBy(signedOrder.makerAssetAmount); + takerAssetFillAmounts.push(takerAssetFillAmount); + expectedFillResults.push({ + takerAssetFilledAmount: takerAssetFillAmount, + makerAssetFilledAmount, + makerFeePaid: makerFee, + takerFeePaid: takerFee, + }); + erc20Balances[makerAddress][makerAssetAddress] = erc20Balances[makerAddress][ makerAssetAddress ].minus(makerAssetFilledAmount); @@ -810,6 +975,12 @@ describe('Exchange wrappers', () => { }); const newOrders = [invalidOrder, ...validOrders]; + const fillResults = await exchange.batchFillOrdersNoThrow.callAsync( + newOrders, + takerAssetFillAmounts, + newOrders.map(signedOrder => signedOrder.signature), + { from: takerAddress }, + ); await exchangeWrapper.batchFillOrdersNoThrowAsync(newOrders, takerAddress, { takerAssetFillAmounts, // HACK(albrow): We need to hardcode the gas estimate here because @@ -817,15 +988,16 @@ describe('Exchange wrappers', () => { // delegatecall and swallow errors. gas: 450000, }); - const newBalances = await erc20Wrapper.getBalancesAsync(); + + expect(fillResults).to.deep.equal(expectedFillResults); expect(newBalances).to.be.deep.equal(erc20Balances); }); }); describe('marketSellOrders', () => { const reentrancyTest = (functionNames: string[]) => { - _.forEach(functionNames, async (functionName: string, functionId: number) => { + for (const [functionId, functionName] of functionNames.entries()) { const description = `should not allow marketSellOrders to reenter the Exchange contract via ${functionName}`; it(description, async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ @@ -840,7 +1012,7 @@ describe('Exchange wrappers', () => { }); return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); - }); + } }; describe('marketSellOrders reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); @@ -848,10 +1020,16 @@ describe('Exchange wrappers', () => { 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( @@ -859,6 +1037,12 @@ describe('Exchange wrappers', () => { ); 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), ); @@ -907,11 +1091,40 @@ describe('Exchange wrappers', () => { 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); }); @@ -943,7 +1156,7 @@ describe('Exchange wrappers', () => { describe('marketSellOrdersNoThrow', () => { const reentrancyTest = (functionNames: string[]) => { - _.forEach(functionNames, async (functionName: string, functionId: number) => { + 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({ @@ -959,7 +1172,7 @@ describe('Exchange wrappers', () => { const newBalances = await erc20Wrapper.getBalancesAsync(); expect(erc20Balances).to.deep.equal(newBalances); }); - }); + } }; describe('marketSellOrdersNoThrow reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); @@ -968,6 +1181,13 @@ describe('Exchange wrappers', () => { const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus( signedOrders[1].takerAssetAmount.div(2), ); + + const fillResults = await exchange.marketSellOrdersNoThrow.callAsync( + signedOrders, + takerAssetFillAmount, + signedOrders.map(signedOrder => signedOrder.signature), + { from: takerAddress }, + ); await exchangeWrapper.marketSellOrdersNoThrowAsync(signedOrders, takerAddress, { takerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because @@ -975,7 +1195,6 @@ describe('Exchange wrappers', () => { // delegatecall and swallow errors. gas: 6000000, }); - const newBalances = await erc20Wrapper.getBalancesAsync(); const makerAssetFilledAmount = signedOrders[0].makerAssetAmount.plus( @@ -983,6 +1202,12 @@ describe('Exchange wrappers', () => { ); 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), ); @@ -1031,6 +1256,13 @@ describe('Exchange wrappers', () => { feeToken.address ].plus(signedOrder.makerFee.plus(signedOrder.takerFee)); }); + + const fillResults = await exchange.marketSellOrdersNoThrow.callAsync( + signedOrders, + takerAssetFillAmount, + signedOrders.map(signedOrder => signedOrder.signature), + { from: takerAddress }, + ); await exchangeWrapper.marketSellOrdersNoThrowAsync(signedOrders, takerAddress, { takerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because @@ -1038,8 +1270,30 @@ describe('Exchange wrappers', () => { // delegatecall and swallow errors. gas: 600000, }); - 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); }); @@ -1076,6 +1330,13 @@ describe('Exchange wrappers', () => { feeToken.address ].plus(signedOrder.makerFee.plus(signedOrder.takerFee)); }); + + const fillResults = await exchange.marketSellOrdersNoThrow.callAsync( + signedOrders, + takerAssetFillAmount, + signedOrders.map(signedOrder => signedOrder.signature), + { from: takerAddress }, + ); await exchangeWrapper.marketSellOrdersNoThrowAsync(signedOrders, takerAddress, { takerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because @@ -1083,15 +1344,37 @@ describe('Exchange wrappers', () => { // delegatecall and swallow errors. gas: 600000, }); - const newBalances = await erc20Wrapper.getBalancesAsync(); + + const expectedFillResults = filledSignedOrders + .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); }); }); describe('marketBuyOrders', () => { const reentrancyTest = (functionNames: string[]) => { - _.forEach(functionNames, async (functionName: string, functionId: number) => { + for (const [functionId, functionName] of functionNames.entries()) { const description = `should not allow marketBuyOrders to reenter the Exchange contract via ${functionName}`; it(description, async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ @@ -1106,7 +1389,7 @@ describe('Exchange wrappers', () => { }); return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); }); - }); + } }; describe('marketBuyOrders reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); @@ -1114,10 +1397,16 @@ describe('Exchange wrappers', () => { 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( @@ -1125,6 +1414,12 @@ describe('Exchange wrappers', () => { ); 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), ); @@ -1173,11 +1468,40 @@ describe('Exchange wrappers', () => { 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); }); @@ -1209,7 +1533,7 @@ describe('Exchange wrappers', () => { describe('marketBuyOrdersNoThrow', () => { const reentrancyTest = (functionNames: string[]) => { - _.forEach(functionNames, async (functionName: string, functionId: number) => { + 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({ @@ -1225,7 +1549,7 @@ describe('Exchange wrappers', () => { const newBalances = await erc20Wrapper.getBalancesAsync(); expect(erc20Balances).to.deep.equal(newBalances); }); - }); + } }; describe('marketBuyOrdersNoThrow reentrancy tests', () => reentrancyTest(exchangeConstants.FUNCTIONS_WITH_MUTEX)); @@ -1234,6 +1558,13 @@ describe('Exchange wrappers', () => { const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus( signedOrders[1].makerAssetAmount.div(2), ); + + const fillResults = await exchange.marketBuyOrdersNoThrow.callAsync( + signedOrders, + makerAssetFillAmount, + signedOrders.map(signedOrder => signedOrder.signature), + { from: takerAddress }, + ); await exchangeWrapper.marketBuyOrdersNoThrowAsync(signedOrders, takerAddress, { makerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because @@ -1241,7 +1572,6 @@ describe('Exchange wrappers', () => { // delegatecall and swallow errors. gas: 600000, }); - const newBalances = await erc20Wrapper.getBalancesAsync(); const makerAmountBought = signedOrders[0].takerAssetAmount.plus( @@ -1249,6 +1579,12 @@ describe('Exchange wrappers', () => { ); 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), ); @@ -1297,6 +1633,13 @@ describe('Exchange wrappers', () => { feeToken.address ].plus(signedOrder.makerFee.plus(signedOrder.takerFee)); }); + + const fillResults = await exchange.marketBuyOrdersNoThrow.callAsync( + signedOrders, + makerAssetFillAmount, + signedOrders.map(signedOrder => signedOrder.signature), + { from: takerAddress }, + ); await exchangeWrapper.marketBuyOrdersNoThrowAsync(signedOrders, takerAddress, { makerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because @@ -1304,8 +1647,30 @@ describe('Exchange wrappers', () => { // delegatecall and swallow errors. gas: 600000, }); - 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); }); @@ -1343,6 +1708,13 @@ describe('Exchange wrappers', () => { feeToken.address ].plus(signedOrder.makerFee.plus(signedOrder.takerFee)); }); + + const fillResults = await exchange.marketBuyOrdersNoThrow.callAsync( + signedOrders, + makerAssetFillAmount, + signedOrders.map(signedOrder => signedOrder.signature), + { from: takerAddress }, + ); await exchangeWrapper.marketBuyOrdersNoThrowAsync(signedOrders, takerAddress, { makerAssetFillAmount, // HACK(albrow): We need to hardcode the gas estimate here because @@ -1350,8 +1722,30 @@ describe('Exchange wrappers', () => { // delegatecall and swallow errors. gas: 600000, }); - const newBalances = await erc20Wrapper.getBalancesAsync(); + + const expectedFillResults = filledSignedOrders + .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); }); }); From c709cf1e387e3c22369bdb8226a25ea1cb9882bf Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Sat, 25 May 2019 17:58:54 -0700 Subject: [PATCH 3/7] Remove formatters and clarify comments about pointing assetData to the same memory location --- .../contracts/src/MixinMatchOrders.sol | 3 +- .../contracts/src/MixinWrapperFunctions.sol | 20 +++--- .../exchange/test/utils/exchange_wrapper.ts | 70 ++++++++----------- contracts/test-utils/src/formatters.ts | 68 ------------------ contracts/test-utils/src/index.ts | 1 - 5 files changed, 44 insertions(+), 118 deletions(-) delete mode 100644 contracts/test-utils/src/formatters.ts diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index 1c74b07b14..7fc8ecce9d 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -57,7 +57,8 @@ contract MixinMatchOrders is nonReentrant returns (LibFillResults.MatchedFillResults memory matchedFillResults) { - // We assume that rightOrder.takerAssetData == leftOrder.makerAssetData and rightOrder.makerAssetData == leftOrder.takerAssetData. + // We assume that rightOrder.takerAssetData == leftOrder.makerAssetData and rightOrder.makerAssetData == leftOrder.takerAssetData + // by pointing these values to the same location in memory. This is cheaper than checking equality. // If this assumption isn't true, the match will fail at signature validation. rightOrder.makerAssetData = leftOrder.takerAssetData; rightOrder.takerAssetData = leftOrder.makerAssetData; diff --git a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol index 02c46b59db..dc2d217cc7 100644 --- a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol @@ -199,8 +199,9 @@ contract MixinWrapperFunctions is uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - // We assume that asset being sold by taker is the same for each order. - // Rather than passing this in as calldata, we use the takerAssetData from the first order in all later orders. + // 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 @@ -243,8 +244,9 @@ contract MixinWrapperFunctions is uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - // We assume that asset being sold by taker is the same for each order. - // Rather than passing this in as calldata, we use the takerAssetData from the first order in all later orders. + // 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 @@ -287,8 +289,9 @@ contract MixinWrapperFunctions is uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - // We assume that asset being bought by taker is the same for each order. - // Rather than passing this in as calldata, we copy the makerAssetData from the first order onto all later orders. + // 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 @@ -339,8 +342,9 @@ contract MixinWrapperFunctions is uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { - // We assume that asset being bought by taker is the same for each order. - // Rather than passing this in as calldata, we copy the makerAssetData from the first order onto all later orders. + // 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 diff --git a/contracts/exchange/test/utils/exchange_wrapper.ts b/contracts/exchange/test/utils/exchange_wrapper.ts index a69cd080b4..75cd042d86 100644 --- a/contracts/exchange/test/utils/exchange_wrapper.ts +++ b/contracts/exchange/test/utils/exchange_wrapper.ts @@ -1,14 +1,7 @@ import { artifacts as erc1155Artifacts } from '@0x/contracts-erc1155'; import { artifacts as erc20Artifacts } from '@0x/contracts-erc20'; import { artifacts as erc721Artifacts } from '@0x/contracts-erc721'; -import { - FillResults, - formatters, - LogDecoder, - OrderInfo, - orderUtils, - Web3ProviderEngine, -} from '@0x/contracts-test-utils'; +import { FillResults, LogDecoder, OrderInfo, orderUtils, Web3ProviderEngine } from '@0x/contracts-test-utils'; import { SignedOrder, SignedZeroExTransaction } from '@0x/types'; import { AbiEncoder, BigNumber } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; @@ -98,11 +91,12 @@ export class ExchangeWrapper { from: string, opts: { takerAssetFillAmounts?: BigNumber[] } = {}, ): Promise { - const params = formatters.createBatchFill(orders, opts.takerAssetFillAmounts); const txHash = await this._exchange.batchFillOrders.sendTransactionAsync( - params.orders, - params.takerAssetFillAmounts, - params.signatures, + orders, + opts.takerAssetFillAmounts === undefined + ? orders.map(signedOrder => signedOrder.takerAssetAmount) + : opts.takerAssetFillAmounts, + orders.map(signedOrder => signedOrder.signature), { from }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -113,11 +107,12 @@ export class ExchangeWrapper { from: string, opts: { takerAssetFillAmounts?: BigNumber[] } = {}, ): Promise { - const params = formatters.createBatchFill(orders, opts.takerAssetFillAmounts); const txHash = await this._exchange.batchFillOrKillOrders.sendTransactionAsync( - params.orders, - params.takerAssetFillAmounts, - params.signatures, + orders, + opts.takerAssetFillAmounts === undefined + ? orders.map(signedOrder => signedOrder.takerAssetAmount) + : opts.takerAssetFillAmounts, + orders.map(signedOrder => signedOrder.signature), { from }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -128,11 +123,12 @@ export class ExchangeWrapper { from: string, opts: { takerAssetFillAmounts?: BigNumber[]; gas?: number } = {}, ): Promise { - const params = formatters.createBatchFill(orders, opts.takerAssetFillAmounts); const txHash = await this._exchange.batchFillOrdersNoThrow.sendTransactionAsync( - params.orders, - params.takerAssetFillAmounts, - params.signatures, + orders, + opts.takerAssetFillAmounts === undefined + ? orders.map(signedOrder => signedOrder.takerAssetAmount) + : opts.takerAssetFillAmounts, + orders.map(signedOrder => signedOrder.signature), { from, gas: opts.gas }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -143,11 +139,10 @@ export class ExchangeWrapper { from: string, opts: { takerAssetFillAmount: BigNumber }, ): Promise { - const params = formatters.createMarketSellOrders(orders, opts.takerAssetFillAmount); const txHash = await this._exchange.marketSellOrders.sendTransactionAsync( - params.orders, - params.takerAssetFillAmount, - params.signatures, + orders, + opts.takerAssetFillAmount, + orders.map(signedOrder => signedOrder.signature), { from }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -158,11 +153,10 @@ export class ExchangeWrapper { from: string, opts: { takerAssetFillAmount: BigNumber; gas?: number }, ): Promise { - const params = formatters.createMarketSellOrders(orders, opts.takerAssetFillAmount); const txHash = await this._exchange.marketSellOrdersNoThrow.sendTransactionAsync( - params.orders, - params.takerAssetFillAmount, - params.signatures, + orders, + opts.takerAssetFillAmount, + orders.map(signedOrder => signedOrder.signature), { from, gas: opts.gas }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -173,11 +167,10 @@ export class ExchangeWrapper { from: string, opts: { makerAssetFillAmount: BigNumber }, ): Promise { - const params = formatters.createMarketBuyOrders(orders, opts.makerAssetFillAmount); const txHash = await this._exchange.marketBuyOrders.sendTransactionAsync( - params.orders, - params.makerAssetFillAmount, - params.signatures, + orders, + opts.makerAssetFillAmount, + orders.map(signedOrder => signedOrder.signature), { from }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -188,11 +181,10 @@ export class ExchangeWrapper { from: string, opts: { makerAssetFillAmount: BigNumber; gas?: number }, ): Promise { - const params = formatters.createMarketBuyOrders(orders, opts.makerAssetFillAmount); const txHash = await this._exchange.marketBuyOrdersNoThrow.sendTransactionAsync( - params.orders, - params.makerAssetFillAmount, - params.signatures, + orders, + opts.makerAssetFillAmount, + orders.map(signedOrder => signedOrder.signature), { from, gas: opts.gas }, ); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); @@ -202,8 +194,7 @@ export class ExchangeWrapper { orders: SignedOrder[], from: string, ): Promise { - const params = formatters.createBatchCancel(orders); - const txHash = await this._exchange.batchCancelOrders.sendTransactionAsync(params.orders, { from }); + const txHash = await this._exchange.batchCancelOrders.sendTransactionAsync(orders, { from }); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } @@ -211,8 +202,7 @@ export class ExchangeWrapper { orders: SignedOrder[], from: string, ): Promise { - const params = formatters.createBatchCancel(orders); - const txHash = await this._exchange.batchCancelOrdersNoThrow.sendTransactionAsync(params.orders, { from }); + const txHash = await this._exchange.batchCancelOrdersNoThrow.sendTransactionAsync(orders, { from }); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } diff --git a/contracts/test-utils/src/formatters.ts b/contracts/test-utils/src/formatters.ts deleted file mode 100644 index a32e3e1eef..0000000000 --- a/contracts/test-utils/src/formatters.ts +++ /dev/null @@ -1,68 +0,0 @@ -import { SignedOrder } from '@0x/types'; -import { BigNumber } from '@0x/utils'; -import * as _ from 'lodash'; - -import { constants } from './constants'; -import { orderUtils } from './order_utils'; -import { BatchCancelOrders, BatchFillOrders, MarketBuyOrders, MarketSellOrders } from './types'; - -export const formatters = { - createBatchFill(signedOrders: SignedOrder[], takerAssetFillAmounts: BigNumber[] = []): BatchFillOrders { - const batchFill: BatchFillOrders = { - orders: [], - signatures: [], - takerAssetFillAmounts, - }; - _.forEach(signedOrders, signedOrder => { - const orderWithoutDomain = orderUtils.getOrderWithoutDomain(signedOrder); - batchFill.orders.push(orderWithoutDomain); - batchFill.signatures.push(signedOrder.signature); - if (takerAssetFillAmounts.length < signedOrders.length) { - batchFill.takerAssetFillAmounts.push(signedOrder.takerAssetAmount); - } - }); - return batchFill; - }, - createMarketSellOrders(signedOrders: SignedOrder[], takerAssetFillAmount: BigNumber): MarketSellOrders { - const marketSellOrders: MarketSellOrders = { - orders: [], - signatures: [], - takerAssetFillAmount, - }; - _.forEach(signedOrders, (signedOrder, i) => { - const orderWithoutDomain = orderUtils.getOrderWithoutDomain(signedOrder); - if (i !== 0) { - orderWithoutDomain.takerAssetData = constants.NULL_BYTES; - } - marketSellOrders.orders.push(orderWithoutDomain); - marketSellOrders.signatures.push(signedOrder.signature); - }); - return marketSellOrders; - }, - createMarketBuyOrders(signedOrders: SignedOrder[], makerAssetFillAmount: BigNumber): MarketBuyOrders { - const marketBuyOrders: MarketBuyOrders = { - orders: [], - signatures: [], - makerAssetFillAmount, - }; - _.forEach(signedOrders, (signedOrder, i) => { - const orderWithoutDomain = orderUtils.getOrderWithoutDomain(signedOrder); - if (i !== 0) { - orderWithoutDomain.makerAssetData = constants.NULL_BYTES; - } - marketBuyOrders.orders.push(orderWithoutDomain); - marketBuyOrders.signatures.push(signedOrder.signature); - }); - return marketBuyOrders; - }, - createBatchCancel(signedOrders: SignedOrder[]): BatchCancelOrders { - const batchCancel: BatchCancelOrders = { - orders: [], - }; - _.forEach(signedOrders, signedOrder => { - const orderWithoutDomain = orderUtils.getOrderWithoutDomain(signedOrder); - batchCancel.orders.push(orderWithoutDomain); - }); - return batchCancel; - }, -}; diff --git a/contracts/test-utils/src/index.ts b/contracts/test-utils/src/index.ts index 7e1c6b52f5..f98a6309f8 100644 --- a/contracts/test-utils/src/index.ts +++ b/contracts/test-utils/src/index.ts @@ -16,7 +16,6 @@ export { export { getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync } from './block_timestamp'; export { provider, txDefaults, web3Wrapper } from './web3_wrapper'; export { LogDecoder } from './log_decoder'; -export { formatters } from './formatters'; export { signingUtils } from './signing_utils'; export { orderUtils } from './order_utils'; export { typeEncodingUtils } from './type_encoding_utils'; From 2a95bb1d38c9ec00cd9fad17377f0df9f3279e59 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Sun, 26 May 2019 11:35:18 -0700 Subject: [PATCH 4/7] Add names to return values and fix breaking transactions tests --- .../contracts/src/MixinWrapperFunctions.sol | 52 +++++++++---------- .../src/interfaces/IWrapperFunctions.sol | 14 ++--- contracts/exchange/test/transactions.ts | 8 +-- 3 files changed, 38 insertions(+), 36 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol index dc2d217cc7..dd1ae3bbdd 100644 --- a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol @@ -114,10 +114,10 @@ contract MixinWrapperFunctions is ) public nonReentrant - returns (FillResults[] memory) + returns (FillResults[] memory fillResults) { uint256 ordersLength = orders.length; - FillResults[] memory fillResults = new FillResults[](ordersLength); + fillResults = new FillResults[](ordersLength); for (uint256 i = 0; i != ordersLength; i++) { fillResults[i] = _fillOrder( orders[i], @@ -140,10 +140,10 @@ contract MixinWrapperFunctions is ) public nonReentrant - returns (FillResults[] memory) + returns (FillResults[] memory fillResults) { uint256 ordersLength = orders.length; - FillResults[] memory fillResults = new FillResults[](ordersLength); + fillResults = new FillResults[](ordersLength); for (uint256 i = 0; i != ordersLength; i++) { fillResults[i] = _fillOrKillOrder( orders[i], @@ -166,10 +166,10 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public - returns (FillResults[] memory) + returns (FillResults[] memory fillResults) { uint256 ordersLength = orders.length; - FillResults[] memory fillResults = new FillResults[](ordersLength); + fillResults = new FillResults[](ordersLength); for (uint256 i = 0; i != ordersLength; i++) { fillResults[i] = fillOrderNoThrow( orders[i], @@ -192,7 +192,7 @@ contract MixinWrapperFunctions is ) public nonReentrant - returns (FillResults memory totalFillResults) + returns (FillResults memory fillResults) { bytes memory takerAssetData = orders[0].takerAssetData; @@ -205,7 +205,7 @@ contract MixinWrapperFunctions is orders[i].takerAssetData = takerAssetData; // Calculate the remaining amount of takerAsset to sell - uint256 remainingTakerAssetFillAmount = _safeSub(takerAssetFillAmount, totalFillResults.takerAssetFilledAmount); + uint256 remainingTakerAssetFillAmount = _safeSub(takerAssetFillAmount, fillResults.takerAssetFilledAmount); // Attempt to sell the remaining amount of takerAsset FillResults memory singleFillResults = _fillOrder( @@ -215,14 +215,14 @@ contract MixinWrapperFunctions is ); // Update amounts filled and fees paid by maker and taker - _addFillResults(totalFillResults, singleFillResults); + _addFillResults(fillResults, singleFillResults); // Stop execution if the entire amount of takerAsset has been sold - if (totalFillResults.takerAssetFilledAmount >= takerAssetFillAmount) { + if (fillResults.takerAssetFilledAmount >= takerAssetFillAmount) { break; } } - return totalFillResults; + return fillResults; } /// @dev Synchronously executes multiple calls of fillOrder until total amount of takerAsset is sold by taker. @@ -237,7 +237,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public - returns (FillResults memory totalFillResults) + returns (FillResults memory fillResults) { bytes memory takerAssetData = orders[0].takerAssetData; @@ -250,7 +250,7 @@ contract MixinWrapperFunctions is orders[i].takerAssetData = takerAssetData; // Calculate the remaining amount of takerAsset to sell - uint256 remainingTakerAssetFillAmount = _safeSub(takerAssetFillAmount, totalFillResults.takerAssetFilledAmount); + uint256 remainingTakerAssetFillAmount = _safeSub(takerAssetFillAmount, fillResults.takerAssetFilledAmount); // Attempt to sell the remaining amount of takerAsset FillResults memory singleFillResults = fillOrderNoThrow( @@ -260,14 +260,14 @@ contract MixinWrapperFunctions is ); // Update amounts filled and fees paid by maker and taker - _addFillResults(totalFillResults, singleFillResults); + _addFillResults(fillResults, singleFillResults); // Stop execution if the entire amount of takerAsset has been sold - if (totalFillResults.takerAssetFilledAmount >= takerAssetFillAmount) { + if (fillResults.takerAssetFilledAmount >= takerAssetFillAmount) { break; } } - return totalFillResults; + return fillResults; } /// @dev Synchronously executes multiple calls of fillOrder until total amount of makerAsset is bought by taker. @@ -282,7 +282,7 @@ contract MixinWrapperFunctions is ) public nonReentrant - returns (FillResults memory totalFillResults) + returns (FillResults memory fillResults) { bytes memory makerAssetData = orders[0].makerAssetData; @@ -295,7 +295,7 @@ contract MixinWrapperFunctions is orders[i].makerAssetData = makerAssetData; // Calculate the remaining amount of makerAsset to buy - uint256 remainingMakerAssetFillAmount = _safeSub(makerAssetFillAmount, totalFillResults.makerAssetFilledAmount); + 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 @@ -313,14 +313,14 @@ contract MixinWrapperFunctions is ); // Update amounts filled and fees paid by maker and taker - _addFillResults(totalFillResults, singleFillResults); + _addFillResults(fillResults, singleFillResults); // Stop execution if the entire amount of makerAsset has been bought - if (totalFillResults.makerAssetFilledAmount >= makerAssetFillAmount) { + if (fillResults.makerAssetFilledAmount >= makerAssetFillAmount) { break; } } - return totalFillResults; + return fillResults; } /// @dev Synchronously executes multiple fill orders in a single transaction until total amount is bought by taker. @@ -335,7 +335,7 @@ contract MixinWrapperFunctions is bytes[] memory signatures ) public - returns (FillResults memory totalFillResults) + returns (FillResults memory fillResults) { bytes memory makerAssetData = orders[0].makerAssetData; @@ -348,7 +348,7 @@ contract MixinWrapperFunctions is orders[i].makerAssetData = makerAssetData; // Calculate the remaining amount of makerAsset to buy - uint256 remainingMakerAssetFillAmount = _safeSub(makerAssetFillAmount, totalFillResults.makerAssetFilledAmount); + 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 @@ -366,14 +366,14 @@ contract MixinWrapperFunctions is ); // Update amounts filled and fees paid by maker and taker - _addFillResults(totalFillResults, singleFillResults); + _addFillResults(fillResults, singleFillResults); // Stop execution if the entire amount of makerAsset has been bought - if (totalFillResults.makerAssetFilledAmount >= makerAssetFillAmount) { + if (fillResults.makerAssetFilledAmount >= makerAssetFillAmount) { break; } } - return totalFillResults; + return fillResults; } /// @dev After calling, the order can not be filled anymore. diff --git a/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol b/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol index a3c244f246..e28c45be62 100644 --- a/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol @@ -62,7 +62,7 @@ contract IWrapperFunctions { bytes[] memory signatures ) public - returns (LibFillResults.FillResults[] memory); + returns (LibFillResults.FillResults[] memory fillResults); /// @dev Synchronously executes multiple calls of fillOrKill. /// @param orders Array of order specifications. @@ -75,7 +75,7 @@ contract IWrapperFunctions { bytes[] memory signatures ) public - returns (LibFillResults.FillResults[] memory); + returns (LibFillResults.FillResults[] memory fillResults); /// @dev Fills an order with specified parameters and ECDSA signature. /// Returns false if the transaction would otherwise revert. @@ -89,7 +89,7 @@ contract IWrapperFunctions { bytes[] memory signatures ) public - returns (LibFillResults.FillResults[] memory); + 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. @@ -102,7 +102,7 @@ contract IWrapperFunctions { bytes[] memory signatures ) public - returns (LibFillResults.FillResults memory totalFillResults); + 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. @@ -116,7 +116,7 @@ contract IWrapperFunctions { bytes[] memory signatures ) public - returns (LibFillResults.FillResults memory totalFillResults); + 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. @@ -129,7 +129,7 @@ contract IWrapperFunctions { bytes[] memory signatures ) public - returns (LibFillResults.FillResults memory totalFillResults); + 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. @@ -143,7 +143,7 @@ contract IWrapperFunctions { bytes[] memory signatures ) public - returns (LibFillResults.FillResults memory totalFillResults); + returns (LibFillResults.FillResults memory fillResults); /// @dev Synchronously cancels multiple orders in a single transaction. /// @param orders Array of order specifications. diff --git a/contracts/exchange/test/transactions.ts b/contracts/exchange/test/transactions.ts index 63cdca60e7..c250fd8ae7 100644 --- a/contracts/exchange/test/transactions.ts +++ b/contracts/exchange/test/transactions.ts @@ -286,11 +286,13 @@ describe('Exchange transactions', () => { const abi = artifacts.Exchange.compilerOutput.abi; const methodAbi = abi.filter(abiItem => (abiItem as MethodAbi).name === fnName)[0] as MethodAbi; const abiEncoder = new AbiEncoder.Method(methodAbi); + const decodedReturnData = abiEncoder.decodeReturnValues(returnData); - const fillResults: FillResults = - decodedReturnData.fillResults === undefined - ? decodedReturnData.totalFillResults + const fillResults = + exchangeConstants.BATCH_FILL_FN_NAMES.indexOf(fnName) !== -1 + ? decodedReturnData.fillResults[0] : decodedReturnData.fillResults; + expect(fillResults.makerAssetFilledAmount).to.be.bignumber.eq(order.makerAssetAmount); expect(fillResults.takerAssetFilledAmount).to.be.bignumber.eq(order.takerAssetAmount); expect(fillResults.makerFeePaid).to.be.bignumber.eq(order.makerFee); From 107e4aacd7e7cfe28f035af00e398acc50054ae4 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Sun, 26 May 2019 13:52:24 -0700 Subject: [PATCH 5/7] Fix incorrect function call for fillOrderNoThrow test --- contracts/exchange/test/wrapper.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/exchange/test/wrapper.ts b/contracts/exchange/test/wrapper.ts index 945ae1c0eb..c19ffbf2ba 100644 --- a/contracts/exchange/test/wrapper.ts +++ b/contracts/exchange/test/wrapper.ts @@ -523,7 +523,7 @@ describe('Exchange wrappers', () => { // Call Exchange const takerAssetFillAmount = signedOrder.takerAssetAmount; - const fillResults = await exchange.fillOrKillOrder.callAsync( + const fillResults = await exchange.fillOrderNoThrow.callAsync( signedOrder, takerAssetFillAmount, signedOrder.signature, From 4caaeb35afaf44e3326ee5609af1474048a17c5a Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Sun, 26 May 2019 13:52:35 -0700 Subject: [PATCH 6/7] Update CHANGELOGs --- contracts/exchange/CHANGELOG.json | 4 ++++ contracts/test-utils/CHANGELOG.json | 6 +++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/contracts/exchange/CHANGELOG.json b/contracts/exchange/CHANGELOG.json index dfbd357441..9c924f2096 100644 --- a/contracts/exchange/CHANGELOG.json +++ b/contracts/exchange/CHANGELOG.json @@ -69,6 +69,10 @@ { "note": "Log an `TransactionExecuted` event when an `executeTransaction` call is successful", "pr": 1832 + }, + { + "note": "Return a FillResults array for batch fill variants", + "pr": 1834 } ] }, diff --git a/contracts/test-utils/CHANGELOG.json b/contracts/test-utils/CHANGELOG.json index 9688977b78..e72aea4a17 100644 --- a/contracts/test-utils/CHANGELOG.json +++ b/contracts/test-utils/CHANGELOG.json @@ -1,6 +1,6 @@ [ { - "version": "3.2.0", + "version": "4.0.0", "changes": [ { "note": "Add `chainId` to `TransactionFactory` constructor", @@ -25,6 +25,10 @@ { "note": "Update types for arbitrary fee tokens", "pr": 1819 + }, + { + "note": "Remove formatters", + "pr": 1834 } ] }, From e70b7f3385c266148ce083f02114e2aed4c3539d Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 28 May 2019 17:11:37 -0700 Subject: [PATCH 7/7] Fix outdated comments --- .../contracts/src/MixinWrapperFunctions.sol | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol index dd1ae3bbdd..20a02993ef 100644 --- a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol @@ -60,7 +60,7 @@ contract MixinWrapperFunctions is } /// @dev Fills the input order. - /// Returns false if the transaction would otherwise revert. + /// Returns a null FillResults instance if the transaction would otherwise revert. /// @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. @@ -102,7 +102,7 @@ contract MixinWrapperFunctions is return fillResults; } - /// @dev Synchronously executes multiple calls of fillOrder. + /// @dev Executes multiple calls of fillOrder. /// @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. @@ -128,7 +128,7 @@ contract MixinWrapperFunctions is return fillResults; } - /// @dev Synchronously executes multiple calls of fillOrKill. + /// @dev Executes multiple calls of fillOrKill. /// @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. @@ -154,8 +154,7 @@ contract MixinWrapperFunctions is return fillResults; } - /// @dev Fills an order with specified parameters and ECDSA signature. - /// Returns false if the transaction would otherwise revert. + /// @dev Executes multiple calls of fillOrderNoThrow. /// @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. @@ -180,7 +179,7 @@ contract MixinWrapperFunctions is return fillResults; } - /// @dev Synchronously executes multiple calls of fillOrder until total amount of takerAsset is sold by taker. + /// @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. @@ -225,8 +224,7 @@ contract MixinWrapperFunctions is return 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. + /// @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. @@ -270,7 +268,7 @@ contract MixinWrapperFunctions is return fillResults; } - /// @dev Synchronously executes multiple calls of fillOrder until total amount of makerAsset is bought by taker. + /// @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. @@ -323,8 +321,7 @@ contract MixinWrapperFunctions is return 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. + /// @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. @@ -377,8 +374,9 @@ contract MixinWrapperFunctions is } /// @dev After calling, the order can not be filled anymore. - /// @return True if the order was cancelled successfully. + // Returns false if the cancelOrder call would otherwise revert. /// @param order Order to cancel. Order must be OrderStatus.FILLABLE. + /// @return True if the order was cancelled successfully. function cancelOrderNoThrow(LibOrder.Order memory order) public returns (bool didCancel) @@ -388,7 +386,7 @@ contract MixinWrapperFunctions is return didCancel; } - /// @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 @@ -400,7 +398,7 @@ contract MixinWrapperFunctions is } } - /// @dev Synchronously cancels multiple orders in a single transaction. + /// @dev Executes multiple calls of canccelOrderNoThrow. /// @param orders Array of order specifications. /// @return Bool array containing results of each individual cancellation. function batchCancelOrdersNoThrow(LibOrder.Order[] memory orders)