diff options
Diffstat (limited to 'packages/contracts')
17 files changed, 184 insertions, 188 deletions
diff --git a/packages/contracts/package.json b/packages/contracts/package.json index 1f5c15674..067a20775 100644 --- a/packages/contracts/package.json +++ b/packages/contracts/package.json @@ -62,7 +62,7 @@ "copyfiles": "^1.2.0", "dirty-chai": "^2.0.1", "make-promises-safe": "^1.1.0", - "mocha": "^4.0.1", + "mocha": "^4.1.0", "npm-run-all": "^4.1.2", "shx": "^0.2.2", "solc": "^0.4.24", @@ -86,6 +86,6 @@ "ethereumjs-util": "^5.1.1", "ethers": "3.0.22", "js-combinatorics": "^0.5.3", - "lodash": "^4.17.4" + "lodash": "^4.17.5" } } diff --git a/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol b/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol index 5cf5f831b..e06f9a8e3 100644 --- a/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol +++ b/packages/contracts/src/2.0.0/forwarder/MixinAssets.sol @@ -36,28 +36,25 @@ contract MixinAssets is bytes4 constant internal ERC20_TRANSFER_SELECTOR = bytes4(keccak256("transfer(address,uint256)")); - /// @dev Withdraws ERC20 tokens from this contract. The contract requires a ZRX balance in order to + /// @dev Withdraws assets from this contract. The contract requires a ZRX balance in order to /// function optimally, and this function allows the ZRX to be withdrawn by owner. It may also be - /// used to withdraw tokens that were accidentally sent to this contract. - /// @param token Address of ERC20 token to withdraw. + /// used to withdraw assets that were accidentally sent to this contract. + /// @param assetData Byte array encoded for the respective asset proxy. /// @param amount Amount of ERC20 token to withdraw. - function withdrawERC20( - address token, + function withdrawAsset( + bytes assetData, uint256 amount ) external onlyOwner { - require( - IERC20Token(token).transfer(msg.sender, amount), - "WITHDRAWAL_FAILED" - ); + transferAssetToSender(assetData, amount); } /// @dev Transfers given amount of asset to sender. /// @param assetData Byte array encoded for the respective asset proxy. /// @param amount Amount of asset to transfer to sender. - function transferPurchasedAssetToSender( + function transferAssetToSender( bytes memory assetData, uint256 amount ) diff --git a/packages/contracts/src/2.0.0/forwarder/MixinExchangeWrapper.sol b/packages/contracts/src/2.0.0/forwarder/MixinExchangeWrapper.sol index f3aa483c5..4584bb840 100644 --- a/packages/contracts/src/2.0.0/forwarder/MixinExchangeWrapper.sol +++ b/packages/contracts/src/2.0.0/forwarder/MixinExchangeWrapper.sol @@ -139,7 +139,7 @@ contract MixinExchangeWrapper is /// @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 marketBuyWithWeth( + function marketBuyExactAmountWithWeth( LibOrder.Order[] memory orders, uint256 makerAssetFillAmount, bytes[] memory signatures @@ -180,10 +180,16 @@ contract MixinExchangeWrapper is addFillResults(totalFillResults, singleFillResults); // Stop execution if the entire amount of makerAsset has been bought - if (totalFillResults.makerAssetFilledAmount >= makerAssetFillAmount) { + uint256 makerAssetFilledAmount = totalFillResults.makerAssetFilledAmount; + if (makerAssetFilledAmount >= makerAssetFillAmount) { break; } } + + require( + makerAssetFilledAmount >= makerAssetFillAmount, + "COMPLETE_FILL_FAILED" + ); return totalFillResults; } @@ -196,7 +202,7 @@ contract MixinExchangeWrapper is /// @param zrxBuyAmount Desired amount of ZRX to buy. /// @param signatures Proofs that orders have been created by makers. /// @return totalFillResults Amounts filled and fees paid by maker and taker. - function marketBuyZrxWithWeth( + function marketBuyExactZrxWithWeth( LibOrder.Order[] memory orders, uint256 zrxBuyAmount, bytes[] memory signatures @@ -248,6 +254,10 @@ contract MixinExchangeWrapper is } } + require( + zrxPurchased >= zrxBuyAmount, + "COMPLETE_FILL_FAILED" + ); return totalFillResults; } } diff --git a/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol b/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol index 1164ae919..93cbf79be 100644 --- a/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol +++ b/packages/contracts/src/2.0.0/forwarder/MixinForwarderCore.sol @@ -23,7 +23,7 @@ import "./libs/LibConstants.sol"; import "./mixins/MWeth.sol"; import "./mixins/MAssets.sol"; import "./mixins/MExchangeWrapper.sol"; -import "./mixins/MForwarderCore.sol"; +import "./interfaces/IForwarderCore.sol"; import "../utils/LibBytes/LibBytes.sol"; import "../protocol/Exchange/libs/LibOrder.sol"; import "../protocol/Exchange/libs/LibFillResults.sol"; @@ -37,7 +37,7 @@ contract MixinForwarderCore is MWeth, MAssets, MExchangeWrapper, - MForwarderCore + IForwarderCore { using LibBytes for bytes; @@ -117,7 +117,7 @@ contract MixinForwarderCore is ); // Buy back all ZRX spent on fees. zrxBuyAmount = orderFillResults.takerFeePaid; - feeOrderFillResults = marketBuyZrxWithWeth( + feeOrderFillResults = marketBuyExactZrxWithWeth( feeOrders, zrxBuyAmount, feeSignatures @@ -125,13 +125,6 @@ contract MixinForwarderCore is makerAssetAmountPurchased = orderFillResults.makerAssetFilledAmount; } - // Ensure that all ZRX fees have been repurchased and no extra WETH owned by this contract has been sold. - assertValidFillResults( - orderFillResults, - feeOrderFillResults, - zrxBuyAmount - ); - // Transfer feePercentage of total ETH spent on primary orders to feeRecipient. // Refund remaining ETH to msg.sender. transferEthFeeAndRefund( @@ -142,7 +135,7 @@ contract MixinForwarderCore is ); // Transfer purchased assets to msg.sender. - transferPurchasedAssetToSender(orders[0].makerAssetData, makerAssetAmountPurchased); + transferAssetToSender(orders[0].makerAssetData, makerAssetAmountPurchased); } /// @dev Attempt to purchase makerAssetFillAmount of makerAsset by selling ETH provided with transaction. @@ -180,7 +173,7 @@ contract MixinForwarderCore is if (orders[0].makerAssetData.equals(ZRX_ASSET_DATA)) { // If the makerAsset is ZRX, it is not necessary to pay fees out of this // contracts's ZRX balance because fees are factored into the price of the order. - orderFillResults = marketBuyZrxWithWeth( + orderFillResults = marketBuyExactZrxWithWeth( orders, makerAssetFillAmount, signatures @@ -190,14 +183,14 @@ contract MixinForwarderCore is } else { // Attemp to purchase desired amount of makerAsset. // ZRX fees are payed with this contract's balance. - orderFillResults = marketBuyWithWeth( + orderFillResults = marketBuyExactAmountWithWeth( orders, makerAssetFillAmount, signatures ); // Buy back all ZRX spent on fees. zrxBuyAmount = orderFillResults.takerFeePaid; - feeOrderFillResults = marketBuyZrxWithWeth( + feeOrderFillResults = marketBuyExactZrxWithWeth( feeOrders, zrxBuyAmount, feeSignatures @@ -205,13 +198,6 @@ contract MixinForwarderCore is makerAssetAmountPurchased = orderFillResults.makerAssetFilledAmount; } - // Ensure that all ZRX fees have been repurchased and no extra WETH owned by this contract has been sold. - assertValidFillResults( - orderFillResults, - feeOrderFillResults, - zrxBuyAmount - ); - // Transfer feePercentage of total ETH spent on primary orders to feeRecipient. // Refund remaining ETH to msg.sender. transferEthFeeAndRefund( @@ -222,33 +208,6 @@ contract MixinForwarderCore is ); // Transfer purchased assets to msg.sender. - transferPurchasedAssetToSender(orders[0].makerAssetData, makerAssetAmountPurchased); - } - - /// @dev Ensures that all ZRX fees have been repurchased and no extra WETH owned by this contract has been sold. - /// @param orderFillResults Amounts filled and fees paid for primary orders. - /// @param feeOrderFillResults Amounts filled and fees paid for fee orders. - /// @param zrxBuyAmount The amount of ZRX that needed to be repurchased after filling primary orders. - function assertValidFillResults( - FillResults memory orderFillResults, - FillResults memory feeOrderFillResults, - uint256 zrxBuyAmount - ) - internal - view - { - // Ensure that all ZRX spent while filling primary orders has been repurchased. - uint256 zrxPurchased = safeSub(feeOrderFillResults.makerAssetFilledAmount, feeOrderFillResults.takerFeePaid); - require( - zrxPurchased >= zrxBuyAmount, - "COMPLETE_FILL_FAILED" - ); - - // Ensure that no extra WETH owned by this contract has been sold. - uint256 wethSold = safeAdd(orderFillResults.takerAssetFilledAmount, feeOrderFillResults.takerAssetFilledAmount); - require( - wethSold <= msg.value, - "OVERSOLD_WETH" - ); + transferAssetToSender(orders[0].makerAssetData, makerAssetAmountPurchased); } } diff --git a/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol b/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol index 8ba236e7f..e07940776 100644 --- a/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol +++ b/packages/contracts/src/2.0.0/forwarder/MixinWeth.sol @@ -71,12 +71,16 @@ contract MixinWeth is "FEE_PERCENTAGE_TOO_LARGE" ); - // Calculate amount of WETH that hasn't been sold. - uint256 wethRemaining = safeSub( - msg.value, - safeAdd(wethSoldExcludingFeeOrders, wethSoldForZrx) + // Ensure that no extra WETH owned by this contract has been sold. + uint256 wethSold = safeAdd(wethSoldExcludingFeeOrders, wethSoldForZrx); + require( + wethSold <= msg.value, + "OVERSOLD_WETH" ); + // Calculate amount of WETH that hasn't been sold. + uint256 wethRemaining = safeSub(msg.value, wethSold); + // Calculate ETH fee to pay to feeRecipient. uint256 ethFee = getPartialAmount( feePercentage, diff --git a/packages/contracts/src/2.0.0/forwarder/interfaces/IAssets.sol b/packages/contracts/src/2.0.0/forwarder/interfaces/IAssets.sol index 9b0d995eb..1e034c003 100644 --- a/packages/contracts/src/2.0.0/forwarder/interfaces/IAssets.sol +++ b/packages/contracts/src/2.0.0/forwarder/interfaces/IAssets.sol @@ -21,13 +21,13 @@ pragma solidity 0.4.24; contract IAssets { - /// @dev Withdraws ERC20 tokens from this contract. The contract requires a ZRX balance in order to + /// @dev Withdraws assets from this contract. The contract requires a ZRX balance in order to /// function optimally, and this function allows the ZRX to be withdrawn by owner. It may also be - /// used to withdraw tokens that were accidentally sent to this contract. - /// @param token Address of ERC20 token to withdraw. + /// used to withdraw assets that were accidentally sent to this contract. + /// @param assetData Byte array encoded for the respective asset proxy. /// @param amount Amount of ERC20 token to withdraw. - function withdrawERC20( - address token, + function withdrawAsset( + bytes assetData, uint256 amount ) external; diff --git a/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol b/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol index 340ee0bcb..83636432a 100644 --- a/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol +++ b/packages/contracts/src/2.0.0/forwarder/mixins/MAssets.sol @@ -28,7 +28,7 @@ contract MAssets is /// @dev Transfers given amount of asset to sender. /// @param assetData Byte array encoded for the respective asset proxy. /// @param amount Amount of asset to transfer to sender. - function transferPurchasedAssetToSender( + function transferAssetToSender( bytes memory assetData, uint256 amount ) diff --git a/packages/contracts/src/2.0.0/forwarder/mixins/MExchangeWrapper.sol b/packages/contracts/src/2.0.0/forwarder/mixins/MExchangeWrapper.sol index 5a2def7e5..360dea0e4 100644 --- a/packages/contracts/src/2.0.0/forwarder/mixins/MExchangeWrapper.sol +++ b/packages/contracts/src/2.0.0/forwarder/mixins/MExchangeWrapper.sol @@ -60,7 +60,7 @@ contract MExchangeWrapper { /// @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 marketBuyWithWeth( + function marketBuyExactAmountWithWeth( LibOrder.Order[] memory orders, uint256 makerAssetFillAmount, bytes[] memory signatures @@ -77,7 +77,7 @@ contract MExchangeWrapper { /// @param zrxBuyAmount Desired amount of ZRX to buy. /// @param signatures Proofs that orders have been created by makers. /// @return totalFillResults Amounts filled and fees paid by maker and taker. - function marketBuyZrxWithWeth( + function marketBuyExactZrxWithWeth( LibOrder.Order[] memory orders, uint256 zrxBuyAmount, bytes[] memory signatures diff --git a/packages/contracts/src/2.0.0/forwarder/mixins/MForwarderCore.sol b/packages/contracts/src/2.0.0/forwarder/mixins/MForwarderCore.sol deleted file mode 100644 index 0f5cd9c66..000000000 --- a/packages/contracts/src/2.0.0/forwarder/mixins/MForwarderCore.sol +++ /dev/null @@ -1,42 +0,0 @@ -/* - - Copyright 2018 ZeroEx Intl. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. - -*/ - -pragma solidity 0.4.24; -pragma experimental ABIEncoderV2; - -import "../../protocol/Exchange/libs/LibOrder.sol"; -import "../../protocol/Exchange/libs/LibFillResults.sol"; -import "../interfaces/IForwarderCore.sol"; - - -contract MForwarderCore is - IForwarderCore -{ - - /// @dev Ensures that all ZRX fees have been repurchased and no extra WETH owned by this contract has been sold. - /// @param orderFillResults Amounts filled and fees paid for primary orders. - /// @param feeOrderFillResults Amounts filled and fees paid for fee orders. - /// @param zrxBuyAmount The amount of ZRX that needed to be repurchased after filling primary orders. - function assertValidFillResults( - LibFillResults.FillResults memory orderFillResults, - LibFillResults.FillResults memory feeOrderFillResults, - uint256 zrxBuyAmount - ) - internal - view; -} diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol index b5de1a5de..821d30279 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol @@ -142,7 +142,6 @@ contract MixinTransactions is // Compute hash result := keccak256(memPtr, 128) } - return result; } diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibAbiEncoder.sol b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibAbiEncoder.sol index 704c7061c..4aad37709 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibAbiEncoder.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/libs/LibAbiEncoder.sol @@ -24,20 +24,17 @@ import "./LibOrder.sol"; contract LibAbiEncoder { - /// @dev ABI encodes calldata for `fillOrder` in memory and returns the address range. - /// This range can be passed into `call` or `delegatecall` to invoke an external - /// call to `fillOrder`. + /// @dev ABI encodes calldata for `fillOrder`. /// @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. - /// @return calldataBegin Memory address of ABI encoded calldata. - /// @return calldataLength Lenfgth of ABI encoded calldata. + /// @return ABI encoded calldata for `fillOrder`. function abiEncodeFillOrder( LibOrder.Order memory order, uint256 takerAssetFillAmount, bytes memory signature ) - public + internal pure returns (bytes memory fillOrderCalldata) { @@ -207,10 +204,10 @@ contract LibAbiEncoder { } // Set length of calldata - mstore( - fillOrderCalldata, - sub(dataAreaEnd, add(fillOrderCalldata, 0x20)) - ) + mstore(fillOrderCalldata, sub(dataAreaEnd, add(fillOrderCalldata, 0x20))) + + // Increment free memory pointer + mstore(0x40, dataAreaEnd) } return fillOrderCalldata; diff --git a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol index 923bac97d..d9cec9edc 100644 --- a/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol +++ b/packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol @@ -22,6 +22,7 @@ pragma experimental ABIEncoderV2; import "../../protocol/Exchange/Exchange.sol"; +// solhint-disable no-empty-blocks contract TestExchangeInternals is Exchange { diff --git a/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol b/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol index 5a349527b..4a99dd9c1 100644 --- a/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol +++ b/packages/contracts/src/2.0.0/test/TestLibs/TestLibs.sol @@ -22,13 +22,33 @@ pragma experimental ABIEncoderV2; import "../../protocol/Exchange/libs/LibMath.sol"; import "../../protocol/Exchange/libs/LibOrder.sol"; import "../../protocol/Exchange/libs/LibFillResults.sol"; +import "../../protocol/Exchange/libs/LibAbiEncoder.sol"; contract TestLibs is LibMath, LibOrder, - LibFillResults + LibFillResults, + LibAbiEncoder { + + function publicAbiEncodeFillOrder( + Order memory order, + uint256 takerAssetFillAmount, + bytes memory signature + ) + public + pure + returns (bytes memory fillOrderCalldata) + { + fillOrderCalldata = abiEncodeFillOrder( + order, + takerAssetFillAmount, + signature + ); + return fillOrderCalldata; + } + function publicGetPartialAmount( uint256 numerator, uint256 denominator, diff --git a/packages/contracts/test/forwarder/forwarder.ts b/packages/contracts/test/forwarder/forwarder.ts index 19639d3aa..28ffdeabe 100644 --- a/packages/contracts/test/forwarder/forwarder.ts +++ b/packages/contracts/test/forwarder/forwarder.ts @@ -36,6 +36,7 @@ describe(ContractName.Forwarder, () => { let feeRecipientAddress: string; let otherAddress: string; let defaultMakerAssetAddress: string; + let zrxAssetData: string; let weth: DummyERC20TokenContract; let zrxToken: DummyERC20TokenContract; @@ -90,7 +91,7 @@ describe(ContractName.Forwarder, () => { erc20Wrapper.addDummyTokenContract(weth); const wethAssetData = assetDataUtils.encodeERC20AssetData(wethContract.address); - const zrxAssetData = assetDataUtils.encodeERC20AssetData(zrxToken.address); + zrxAssetData = assetDataUtils.encodeERC20AssetData(zrxToken.address); const exchangeInstance = await ExchangeContract.deployFrom0xArtifactAsync( artifacts.Exchange, provider, @@ -722,25 +723,18 @@ describe(ContractName.Forwarder, () => { ); expect(forwarderEthBalance).to.be.bignumber.equal(constants.ZERO_AMOUNT); }); - it('should not change balances if the amount of ETH sent is too low to fill the makerAssetAmount', async () => { + it('should revert if the amount of ETH sent is too low to fill the makerAssetAmount', async () => { const ordersWithoutFee = [orderWithoutFee]; const feeOrders: SignedOrder[] = []; const makerAssetFillAmount = orderWithoutFee.makerAssetAmount.dividedToIntegerBy(2); const ethValue = orderWithoutFee.takerAssetAmount.dividedToIntegerBy(4); - - tx = await forwarderWrapper.marketBuyOrdersWithEthAsync(ordersWithoutFee, feeOrders, makerAssetFillAmount, { - value: ethValue, - from: takerAddress, - }); - const takerEthBalanceAfter = await web3Wrapper.getBalanceInWeiAsync(takerAddress); - const forwarderEthBalance = await web3Wrapper.getBalanceInWeiAsync(forwarderContract.address); - const newBalances = await erc20Wrapper.getBalancesAsync(); - - const totalEthSpent = gasPrice.times(tx.gasUsed); - - expect(takerEthBalanceAfter).to.be.bignumber.equal(takerEthBalanceBefore.minus(totalEthSpent)); - expect(newBalances).to.deep.equal(erc20Balances); - expect(forwarderEthBalance).to.be.bignumber.equal(constants.ZERO_AMOUNT); + return expectTransactionFailedAsync( + forwarderWrapper.marketBuyOrdersWithEthAsync(ordersWithoutFee, feeOrders, makerAssetFillAmount, { + value: ethValue, + from: takerAddress, + }), + RevertReason.CompleteFillFailed, + ); }); it('should buy an ERC721 asset from a single order', async () => { const makerAssetId = erc721MakerAssetIds[0]; @@ -775,7 +769,7 @@ describe(ContractName.Forwarder, () => { ); expect(forwarderEthBalance).to.be.bignumber.equal(constants.ZERO_AMOUNT); }); - it('should buy an ERC721 asset and ignore later orders with different makerAssetData', async () => { + it('should revert if buying an ERC721 asset when later orders contain different makerAssetData', async () => { const makerAssetId = erc721MakerAssetIds[0]; orderWithoutFee = await orderFactory.newSignedOrderAsync({ makerAssetAmount: new BigNumber(1), @@ -786,33 +780,12 @@ describe(ContractName.Forwarder, () => { const feeOrders: SignedOrder[] = []; const makerAssetFillAmount = new BigNumber(1).plus(differentMakerAssetDataOrder.makerAssetAmount); const ethValue = orderWithFee.takerAssetAmount; - - tx = await forwarderWrapper.marketBuyOrdersWithEthAsync(ordersWithoutFee, feeOrders, makerAssetFillAmount, { - from: takerAddress, - value: ethValue, - }); - const takerEthBalanceAfter = await web3Wrapper.getBalanceInWeiAsync(takerAddress); - const forwarderEthBalance = await web3Wrapper.getBalanceInWeiAsync(forwarderContract.address); - const newOwner = await erc721Token.ownerOf.callAsync(makerAssetId); - const newBalances = await erc20Wrapper.getBalancesAsync(); - - const primaryTakerAssetFillAmount = ethValue; - const totalEthSpent = primaryTakerAssetFillAmount.plus(gasPrice.times(tx.gasUsed)); - expect(newOwner).to.be.bignumber.equal(takerAddress); - expect(takerEthBalanceAfter).to.be.bignumber.equal(takerEthBalanceBefore.minus(totalEthSpent)); - expect(newBalances[makerAddress][weth.address]).to.be.bignumber.equal( - erc20Balances[makerAddress][weth.address].plus(primaryTakerAssetFillAmount), - ); - expect(newBalances[forwarderContract.address][weth.address]).to.be.bignumber.equal(constants.ZERO_AMOUNT); - expect(newBalances[forwarderContract.address][defaultMakerAssetAddress]).to.be.bignumber.equal( - constants.ZERO_AMOUNT, - ); - expect(forwarderEthBalance).to.be.bignumber.equal(constants.ZERO_AMOUNT); - expect(newBalances[makerAddress][defaultMakerAssetAddress]).to.be.bignumber.equal( - erc20Balances[makerAddress][defaultMakerAssetAddress], - ); - expect(newBalances[takerAddress][defaultMakerAssetAddress]).to.be.bignumber.equal( - erc20Balances[takerAddress][defaultMakerAssetAddress], + return expectTransactionFailedAsync( + forwarderWrapper.marketBuyOrdersWithEthAsync(ordersWithoutFee, feeOrders, makerAssetFillAmount, { + value: ethValue, + from: takerAddress, + }), + RevertReason.CompleteFillFailed, ); }); it('should buy an ERC721 asset and pay ZRX fees from a single fee order', async () => { @@ -998,6 +971,26 @@ describe(ContractName.Forwarder, () => { ); }); }); + describe('withdrawAsset', () => { + it('should allow owner to withdraw ERC20 tokens', async () => { + const zrxWithdrawAmount = erc20Balances[forwarderContract.address][zrxToken.address]; + await forwarderWrapper.withdrawAssetAsync(zrxAssetData, zrxWithdrawAmount, { from: owner }); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(newBalances[owner][zrxToken.address]).to.be.bignumber.equal( + erc20Balances[owner][zrxToken.address].plus(zrxWithdrawAmount), + ); + expect(newBalances[forwarderContract.address][zrxToken.address]).to.be.bignumber.equal( + erc20Balances[forwarderContract.address][zrxToken.address].minus(zrxWithdrawAmount), + ); + }); + it('should revert if not called by owner', async () => { + const zrxWithdrawAmount = erc20Balances[forwarderContract.address][zrxToken.address]; + await expectTransactionFailedAsync( + forwarderWrapper.withdrawAssetAsync(zrxAssetData, zrxWithdrawAmount, { from: makerAddress }), + RevertReason.OnlyContractOwner, + ); + }); + }); }); // tslint:disable:max-file-line-count // tslint:enable:no-unnecessary-type-assertion diff --git a/packages/contracts/test/utils/exchange_wrapper.ts b/packages/contracts/test/utils/exchange_wrapper.ts index 490ea959b..d57592d6d 100644 --- a/packages/contracts/test/utils/exchange_wrapper.ts +++ b/packages/contracts/test/utils/exchange_wrapper.ts @@ -8,7 +8,7 @@ import { ExchangeContract } from '../../generated_contract_wrappers/exchange'; import { formatters } from './formatters'; import { LogDecoder } from './log_decoder'; import { orderUtils } from './order_utils'; -import { OrderInfo, SignedTransaction } from './types'; +import { FillResults, OrderInfo, SignedTransaction } from './types'; export class ExchangeWrapper { private readonly _exchange: ExchangeContract; @@ -243,4 +243,27 @@ export class ExchangeWrapper { const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } + public async getFillOrderResultsAsync( + signedOrder: SignedOrder, + from: string, + opts: { takerAssetFillAmount?: BigNumber } = {}, + ): Promise<FillResults> { + const params = orderUtils.createFill(signedOrder, opts.takerAssetFillAmount); + const fillResults = await this._exchange.fillOrder.callAsync( + params.order, + params.takerAssetFillAmount, + params.signature, + { from }, + ); + return fillResults; + } + public abiEncodeFillOrder(signedOrder: SignedOrder, opts: { takerAssetFillAmount?: BigNumber } = {}): string { + const params = orderUtils.createFill(signedOrder, opts.takerAssetFillAmount); + const data = this._exchange.fillOrder.getABIEncodedTransactionData( + params.order, + params.takerAssetFillAmount, + params.signature, + ); + return data; + } } diff --git a/packages/contracts/test/utils/fill_order_combinatorial_utils.ts b/packages/contracts/test/utils/fill_order_combinatorial_utils.ts index bdd1e62a2..284c4a2db 100644 --- a/packages/contracts/test/utils/fill_order_combinatorial_utils.ts +++ b/packages/contracts/test/utils/fill_order_combinatorial_utils.ts @@ -15,6 +15,7 @@ import * as _ from 'lodash'; import 'make-promises-safe'; import { ExchangeContract, ExchangeFillEventArgs } from '../../generated_contract_wrappers/exchange'; +import { TestLibsContract } from '../../generated_contract_wrappers/test_libs'; import { artifacts } from './artifacts'; import { expectTransactionFailedAsync } from './assertions'; @@ -123,6 +124,8 @@ export async function fillOrderCombinatorialUtilsFactoryAsync( exchangeContract.address, ); + const testLibsContract = await TestLibsContract.deployFrom0xArtifactAsync(artifacts.TestLibs, provider, txDefaults); + const fillOrderCombinatorialUtils = new FillOrderCombinatorialUtils( orderFactory, ownerAddress, @@ -132,6 +135,7 @@ export async function fillOrderCombinatorialUtilsFactoryAsync( zrxAssetData, exchangeWrapper, assetWrapper, + testLibsContract, ); return fillOrderCombinatorialUtils; } @@ -145,6 +149,7 @@ export class FillOrderCombinatorialUtils { public zrxAssetData: string; public exchangeWrapper: ExchangeWrapper; public assetWrapper: AssetWrapper; + public testLibsContract: TestLibsContract; public static generateFillOrderCombinations(): FillScenario[] { const takerScenarios = [ TakerScenario.Unspecified, @@ -329,6 +334,7 @@ export class FillOrderCombinatorialUtils { zrxAssetData: string, exchangeWrapper: ExchangeWrapper, assetWrapper: AssetWrapper, + testLibsContract: TestLibsContract, ) { this.orderFactory = orderFactory; this.ownerAddress = ownerAddress; @@ -338,6 +344,7 @@ export class FillOrderCombinatorialUtils { this.zrxAssetData = zrxAssetData; this.exchangeWrapper = exchangeWrapper; this.assetWrapper = assetWrapper; + this.testLibsContract = testLibsContract; } public async testFillOrderScenarioAsync( provider: Provider, @@ -410,6 +417,8 @@ export class FillOrderCombinatorialUtils { lazyStore, fillRevertReasonIfExists, ); + + await this._abiEncodeFillOrderAndAssertOutcomeAsync(signedOrder, takerAssetFillAmount); } private async _fillOrderAndAssertOutcomeAsync( signedOrder: SignedOrder, @@ -456,6 +465,29 @@ export class FillOrderCombinatorialUtils { signedOrder.takerAssetAmount, signedOrder.makerAssetAmount, ); + const expMakerFeePaid = orderUtils.getPartialAmount( + expFilledTakerAmount, + signedOrder.takerAssetAmount, + signedOrder.makerFee, + ); + const expTakerFeePaid = orderUtils.getPartialAmount( + expFilledTakerAmount, + signedOrder.takerAssetAmount, + signedOrder.takerFee, + ); + const fillResults = await this.exchangeWrapper.getFillOrderResultsAsync(signedOrder, this.takerAddress, { + takerAssetFillAmount, + }); + expect(fillResults.takerAssetFilledAmount).to.be.bignumber.equal( + expFilledTakerAmount, + 'takerAssetFilledAmount', + ); + expect(fillResults.makerAssetFilledAmount).to.be.bignumber.equal( + expFilledMakerAmount, + 'makerAssetFilledAmount', + ); + expect(fillResults.takerFeePaid).to.be.bignumber.equal(expTakerFeePaid, 'takerFeePaid'); + expect(fillResults.makerFeePaid).to.be.bignumber.equal(expMakerFeePaid, 'makerFeePaid'); // - Let's fill the order! const txReceipt = await this.exchangeWrapper.fillOrderAsync(signedOrder, this.takerAddress, { @@ -479,17 +511,7 @@ export class FillOrderCombinatorialUtils { expFilledTakerAmount, 'log.args.takerAssetFilledAmount', ); - const expMakerFeePaid = orderUtils.getPartialAmount( - expFilledTakerAmount, - signedOrder.takerAssetAmount, - signedOrder.makerFee, - ); expect(log.args.makerFeePaid).to.be.bignumber.equal(expMakerFeePaid, 'log.args.makerFeePaid'); - const expTakerFeePaid = orderUtils.getPartialAmount( - expFilledTakerAmount, - signedOrder.takerAssetAmount, - signedOrder.takerFee, - ); expect(log.args.takerFeePaid).to.be.bignumber.equal(expTakerFeePaid, 'logs.args.takerFeePaid'); expect(log.args.orderHash).to.be.equal(orderHash, 'log.args.orderHash'); expect(log.args.makerAssetData).to.be.equal(makerAssetData, 'log.args.makerAssetData'); @@ -571,6 +593,19 @@ export class FillOrderCombinatorialUtils { 'ZRXAssetBalanceOfFeeRecipient', ); } + private async _abiEncodeFillOrderAndAssertOutcomeAsync( + signedOrder: SignedOrder, + takerAssetFillAmount: BigNumber, + ): Promise<void> { + const params = orderUtils.createFill(signedOrder, takerAssetFillAmount); + const expectedAbiEncodedData = this.exchangeWrapper.abiEncodeFillOrder(signedOrder, { takerAssetFillAmount }); + const libsAbiEncodedData = await this.testLibsContract.publicAbiEncodeFillOrder.callAsync( + params.order, + params.takerAssetFillAmount, + params.signature, + ); + expect(libsAbiEncodedData).to.be.equal(expectedAbiEncodedData, 'ABIEncodedFillOrderData'); + } private async _getTakerAssetFillAmountAsync( signedOrder: SignedOrder, takerAssetFillAmountScenario: TakerAssetFillAmountScenario, diff --git a/packages/contracts/test/utils/forwarder_wrapper.ts b/packages/contracts/test/utils/forwarder_wrapper.ts index ef7476e36..5b9a63ddf 100644 --- a/packages/contracts/test/utils/forwarder_wrapper.ts +++ b/packages/contracts/test/utils/forwarder_wrapper.ts @@ -106,12 +106,12 @@ export class ForwarderWrapper { const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } - public async withdrawERC20Async( - tokenAddress: string, + public async withdrawAssetAsync( + assetData: string, amount: BigNumber, txData: TxDataPayable, ): Promise<TransactionReceiptWithDecodedLogs> { - const txHash = await this._forwarderContract.withdrawERC20.sendTransactionAsync(tokenAddress, amount, txData); + const txHash = await this._forwarderContract.withdrawAsset.sendTransactionAsync(assetData, amount, txData); const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } |