diff --git a/contracts/exchange-forwarder/contracts/src/MixinAssets.sol b/contracts/exchange-forwarder/contracts/src/MixinAssets.sol index 4d0313a02c..eb0e5c1392 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinAssets.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinAssets.sol @@ -23,6 +23,7 @@ import "@0x/contracts-utils/contracts/src/LibRichErrors.sol"; import "@0x/contracts-utils/contracts/src/Ownable.sol"; import "@0x/contracts-erc20/contracts/src/LibERC20Token.sol"; import "@0x/contracts-erc721/contracts/src/interfaces/IERC721Token.sol"; +import "@0x/contracts-asset-proxy/contracts/src/interfaces/IAssetData.sol"; import "./libs/LibConstants.sol"; import "./libs/LibForwarderRichErrors.sol"; import "./interfaces/IAssets.sol"; @@ -59,9 +60,11 @@ contract MixinAssets is external { bytes4 proxyId = assetData.readBytes4(0); + bytes4 erc20ProxyId = IAssetData(address(0)).ERC20Token.selector; + // For now we only care about ERC20, since percentage fees on ERC721 tokens are invalid. - if (proxyId == ERC20_PROXY_ID) { - address proxyAddress = EXCHANGE.getAssetProxy(ERC20_PROXY_ID); + if (proxyId == erc20ProxyId) { + address proxyAddress = EXCHANGE.getAssetProxy(erc20ProxyId); if (proxyAddress == address(0)) { LibRichErrors.rrevert(LibForwarderRichErrors.UnregisteredAssetProxyError()); } @@ -81,12 +84,13 @@ contract MixinAssets is { bytes4 proxyId = assetData.readBytes4(0); - if (proxyId == ERC20_PROXY_ID) { + if ( + proxyId == IAssetData(address(0)).ERC20Token.selector || + proxyId == IAssetData(address(0)).ERC20Bridge.selector + ) { _transferERC20Token(assetData, amount); - } else if (proxyId == ERC721_PROXY_ID) { + } else if (proxyId == IAssetData(address(0)).ERC721Token.selector) { _transferERC721Token(assetData, amount); - } else if (proxyId == ERC20_BRIDGE_PROXY_ID) { - _transferERC20BridgeAsset(assetData, amount); } else { LibRichErrors.rrevert(LibForwarderRichErrors.UnsupportedAssetProxyError( proxyId @@ -94,7 +98,7 @@ contract MixinAssets is } } - /// @dev Decodes ERC20 assetData and transfers given amount to sender. + /// @dev Decodes ERC20 or ERC20Bridge assetData and transfers given amount to sender. /// @param assetData Byte array encoded for the respective asset proxy. /// @param amount Amount of asset to transfer to sender. function _transferERC20Token( @@ -133,18 +137,4 @@ contract MixinAssets is tokenId ); } - - /// @dev Decodes ERC20Bridge assetData and transfers given amount to sender. - /// @param assetData Byte array encoded for the respective asset proxy. - /// @param amount Amount of asset to transfer to sender. - function _transferERC20BridgeAsset( - bytes memory assetData, - uint256 amount - ) - internal - { - address token = assetData.readAddress(16); - // Transfer tokens. - LibERC20Token.transfer(token, msg.sender, amount); - } } diff --git a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol index 0ca8ead916..16782c5cc3 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinExchangeWrapper.sol @@ -19,12 +19,15 @@ pragma solidity ^0.5.9; pragma experimental ABIEncoderV2; +import "@0x/contracts-utils/contracts/src/LibBytes.sol"; import "@0x/contracts-utils/contracts/src/LibRichErrors.sol"; import "@0x/contracts-utils/contracts/src/LibSafeMath.sol"; import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; import "@0x/contracts-exchange-libs/contracts/src/LibFillResults.sol"; import "@0x/contracts-exchange-libs/contracts/src/LibMath.sol"; import "@0x/contracts-exchange/contracts/src/interfaces/IExchange.sol"; +import "@0x/contracts-asset-proxy/contracts/src/interfaces/IAssetData.sol"; +import "@0x/contracts-erc20/contracts/src/interfaces/IERC20Token.sol"; import "./libs/LibConstants.sol"; import "./libs/LibForwarderRichErrors.sol"; import "./MixinAssets.sol"; @@ -34,6 +37,7 @@ contract MixinExchangeWrapper is LibConstants, MixinAssets { + using LibBytes for bytes; using LibSafeMath for uint256; /// @dev Fills the input order. @@ -88,7 +92,10 @@ contract MixinExchangeWrapper is ) { // No taker fee or percentage fee - if (order.takerFee == 0 || _isPercentageFee(order.takerFeeAssetData, order.makerAssetData)) { + if ( + order.takerFee == 0 || + _areUnderlyingAssetsEqual(order.takerFeeAssetData, order.makerAssetData) + ) { // Attempt to sell the remaining amount of WETH LibFillResults.FillResults memory singleFillResults = _fillOrderNoThrow( order, @@ -103,7 +110,7 @@ contract MixinExchangeWrapper is makerAssetAcquiredAmount = singleFillResults.makerAssetFilledAmount .safeSub(singleFillResults.takerFeePaid); // WETH fee - } else if (order.takerFeeAssetData.equals(order.takerAssetData)) { + } else if (_areUnderlyingAssetsEqual(order.takerFeeAssetData, order.takerAssetData)) { // We will first sell WETH as the takerAsset, then use it to pay the takerFee. // This ensures that we reserve enough to pay the taker and protocol fees. @@ -150,9 +157,10 @@ contract MixinExchangeWrapper is uint256 totalMakerAssetAcquiredAmount ) { - uint256 ordersLength = orders.length; uint256 protocolFee = tx.gasprice.safeMul(EXCHANGE.protocolFeeMultiplier()); + bytes4 erc20BridgeProxyId = IAssetData(address(0)).ERC20Bridge.selector; + uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { // Preemptively skip to avoid division by zero in _marketSellSingleOrder if (orders[i].makerAssetAmount == 0 || orders[i].takerAssetAmount == 0) { @@ -164,6 +172,15 @@ contract MixinExchangeWrapper is .safeSub(totalWethSpentAmount) .safeSub(protocolFee); + // If the maker asset is ERC20Bridge, take a snapshot of the Forwarder contract's balance. + bytes4 makerAssetProxyId = orders[i].makerAssetData.readBytes4(0); + address tokenAddress; + uint256 balanceBefore; + if (makerAssetProxyId == erc20BridgeProxyId) { + tokenAddress = orders[i].makerAssetData.readAddress(16); + balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this)); + } + ( uint256 wethSpentAmount, uint256 makerAssetAcquiredAmount @@ -173,6 +190,15 @@ contract MixinExchangeWrapper is remainingTakerAssetFillAmount ); + // Account for the ERC20Bridge transfering more of the maker asset than expected. + if (makerAssetProxyId == erc20BridgeProxyId) { + uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this)); + makerAssetAcquiredAmount = LibSafeMath.max256( + balanceAfter.safeSub(balanceBefore), + makerAssetAcquiredAmount + ); + } + _transferAssetToSender(orders[i].makerAssetData, makerAssetAcquiredAmount); totalWethSpentAmount = totalWethSpentAmount @@ -206,7 +232,10 @@ contract MixinExchangeWrapper is ) { // No taker fee or WETH fee - if (order.takerFee == 0 || order.takerFeeAssetData.equals(order.takerAssetData)) { + if ( + order.takerFee == 0 || + _areUnderlyingAssetsEqual(order.takerFeeAssetData, order.takerAssetData) + ) { // Calculate the remaining amount of takerAsset to sell uint256 remainingTakerAssetFillAmount = LibMath.getPartialAmountCeil( order.takerAssetAmount, @@ -228,7 +257,7 @@ contract MixinExchangeWrapper is makerAssetAcquiredAmount = singleFillResults.makerAssetFilledAmount; // Percentage fee - } else if (_isPercentageFee(order.takerFeeAssetData, order.makerAssetData)) { + } else if (_areUnderlyingAssetsEqual(order.takerFeeAssetData, order.makerAssetData)) { // Calculate the remaining amount of takerAsset to sell uint256 remainingTakerAssetFillAmount = LibMath.getPartialAmountCeil( order.takerAssetAmount, @@ -277,6 +306,8 @@ contract MixinExchangeWrapper is uint256 totalMakerAssetAcquiredAmount ) { + bytes4 erc20BridgeProxyId = IAssetData(address(0)).ERC20Bridge.selector; + uint256 ordersLength = orders.length; for (uint256 i = 0; i != ordersLength; i++) { // Preemptively skip to avoid division by zero in _marketBuySingleOrder @@ -287,6 +318,15 @@ contract MixinExchangeWrapper is uint256 remainingMakerAssetFillAmount = makerAssetBuyAmount .safeSub(totalMakerAssetAcquiredAmount); + // If the maker asset is ERC20Bridge, take a snapshot of the Forwarder contract's balance. + bytes4 makerAssetProxyId = orders[i].makerAssetData.readBytes4(0); + address tokenAddress; + uint256 balanceBefore; + if (makerAssetProxyId == erc20BridgeProxyId) { + tokenAddress = orders[i].makerAssetData.readAddress(16); + balanceBefore = IERC20Token(tokenAddress).balanceOf(address(this)); + } + ( uint256 wethSpentAmount, uint256 makerAssetAcquiredAmount @@ -296,6 +336,15 @@ contract MixinExchangeWrapper is remainingMakerAssetFillAmount ); + // Account for the ERC20Bridge transfering more of the maker asset than expected. + if (makerAssetProxyId == erc20BridgeProxyId) { + uint256 balanceAfter = IERC20Token(tokenAddress).balanceOf(address(this)); + makerAssetAcquiredAmount = LibSafeMath.max256( + balanceAfter.safeSub(balanceBefore), + makerAssetAcquiredAmount + ); + } + _transferAssetToSender(orders[i].makerAssetData, makerAssetAcquiredAmount); totalWethSpentAmount = totalWethSpentAmount @@ -317,38 +366,34 @@ contract MixinExchangeWrapper is } } - /// @dev Checks whether an order's taker fee is effectively denominated in the maker asset. - /// This is the case if they have the same ERC20Proxy asset data, or if the makerAssetData - /// is the ERC20Bridge equivalent of the takerFeeAssetData. - /// @param takerFeeAssetData Byte array encoded for the takerFee asset proxy. - /// @param makerAssetData Byte array encoded for the maker asset proxy. - /// @return isPercentageFee Whether or not the taker fee asset matches the maker asset. - function _isPercentageFee( - bytes memory takerFeeAssetData, - bytes memory makerAssetData + /// @dev Checks whether one asset is effectively equal to another asset. + /// This is the case if they have the same ERC20Proxy/ERC20BridgeProxy asset data, or if + /// one is the ERC20Bridge equivalent of the other. + /// @param assetData1 Byte array encoded for the takerFee asset proxy. + /// @param assetData2 Byte array encoded for the maker asset proxy. + /// @return areEqual Whether or not the underlying assets are equal. + function _areUnderlyingAssetsEqual( + bytes memory assetData1, + bytes memory assetData2 ) internal pure returns (bool) { - bytes4 takerFeeAssetProxyId = takerFeeAssetData.readBytes4(0); - // If the takerFee asset is not ERC20, it cannot be a percentage fee (and will revert with - // an UnsupportedFeeError in the calling function). - if (takerFeeAssetProxyId != ERC20_PROXY_ID) { - return false; - } + bytes4 assetProxyId1 = assetData1.readBytes4(0); + bytes4 assetProxyId2 = assetData2.readBytes4(0); + bytes4 erc20ProxyId = IAssetData(address(0)).ERC20Token.selector; + bytes4 erc20BridgeProxyId = IAssetData(address(0)).ERC20Bridge.selector; - bytes4 makerAssetProxyId = makerAssetData.readBytes4(0); - if (makerAssetProxyId == ERC20_PROXY_ID) { - // If the maker asset is ERC20, we can directly compare the asset data. - return takerFeeAssetData.equals(makerAssetData); - } else if (makerAssetProxyId == ERC20_BRIDGE_PROXY_ID) { - // If the maker asset is from an ERC20Bridge, compare the underlying token addresses. - address takerFeeToken = takerFeeAssetData.readAddress(16); - address makerToken = makerAssetData.readAddress(16); - return (takerFeeToken == makerToken); + if ( + (assetProxyId1 == erc20ProxyId || assetProxyId1 == erc20BridgeProxyId) && + (assetProxyId2 == erc20ProxyId || assetProxyId2 == erc20BridgeProxyId) + ) { + // Compare the underlying token addresses. + address token1 = assetData1.readAddress(16); + address token2 = assetData2.readAddress(16); + return (token1 == token2); } else { - // If the maker asset is of any other type, the taker fee cannot be a percentage fee. return false; } } diff --git a/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol b/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol index aefa944efb..8b1ba3b804 100644 --- a/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol +++ b/contracts/exchange-forwarder/contracts/src/MixinForwarderCore.sol @@ -24,6 +24,7 @@ import "@0x/contracts-utils/contracts/src/LibRichErrors.sol"; import "@0x/contracts-utils/contracts/src/LibSafeMath.sol"; import "@0x/contracts-exchange-libs/contracts/src/LibOrder.sol"; import "@0x/contracts-exchange-libs/contracts/src/LibMath.sol"; +import "@0x/contracts-asset-proxy/contracts/src/interfaces/IAssetData.sol"; import "./libs/LibConstants.sol"; import "./libs/LibForwarderRichErrors.sol"; import "./interfaces/IAssets.sol"; @@ -46,7 +47,7 @@ contract MixinForwarderCore is constructor () public { - address proxyAddress = EXCHANGE.getAssetProxy(ERC20_PROXY_ID); + address proxyAddress = EXCHANGE.getAssetProxy(IAssetData(address(0)).ERC20Token.selector); if (proxyAddress == address(0)) { LibRichErrors.rrevert(LibForwarderRichErrors.UnregisteredAssetProxyError()); } diff --git a/contracts/exchange-forwarder/contracts/src/libs/LibConstants.sol b/contracts/exchange-forwarder/contracts/src/libs/LibConstants.sol index 390094d6f7..8a195e5428 100644 --- a/contracts/exchange-forwarder/contracts/src/libs/LibConstants.sol +++ b/contracts/exchange-forwarder/contracts/src/libs/LibConstants.sol @@ -27,10 +27,6 @@ contract LibConstants { using LibBytes for bytes; - bytes4 constant internal ERC20_PROXY_ID = bytes4(keccak256("ERC20Token(address)")); - bytes4 constant internal ERC721_PROXY_ID = bytes4(keccak256("ERC721Token(address,uint256)")); - bytes4 constant internal ERC20_BRIDGE_PROXY_ID = bytes4(keccak256("ERC20Bridge(address,address,bytes)")); - uint256 constant internal MAX_UINT = 2**256 - 1; uint256 constant internal PERCENTAGE_DENOMINATOR = 10**18; uint256 constant internal MAX_FEE_PERCENTAGE = 5 * PERCENTAGE_DENOMINATOR / 100; // 5% diff --git a/contracts/exchange-forwarder/contracts/test/TestForwarder.sol b/contracts/exchange-forwarder/contracts/test/TestForwarder.sol index 490ecebf98..593597da32 100644 --- a/contracts/exchange-forwarder/contracts/test/TestForwarder.sol +++ b/contracts/exchange-forwarder/contracts/test/TestForwarder.sol @@ -36,16 +36,16 @@ contract TestForwarder is ) {} - function isPercentageFee( - bytes memory takerFeeAssetData, - bytes memory makerAssetData + function areUnderlyingAssetsEqual( + bytes memory assetData1, + bytes memory assetData2 ) public returns (bool) { - return _isPercentageFee( - takerFeeAssetData, - makerAssetData + return _areUnderlyingAssetsEqual( + assetData1, + assetData2 ); } diff --git a/contracts/exchange-forwarder/test/asset_test.ts b/contracts/exchange-forwarder/test/asset_test.ts index 91df6e3f29..e1bad1b61c 100644 --- a/contracts/exchange-forwarder/test/asset_test.ts +++ b/contracts/exchange-forwarder/test/asset_test.ts @@ -84,33 +84,43 @@ blockchainTests('Supported asset type unit tests', env => { .getABIEncodedTransactionData(); }); - describe('_isPercentageFee', () => { - it('returns true if takerFeeAssetData == makerAssetData are ERC20', async () => { - const result = await forwarder.isPercentageFee(erc20AssetData, erc20AssetData).callAsync(); + describe('_areUnderlyingAssetsEqual', () => { + it('returns true if assetData1 == assetData2 are ERC20', async () => { + const result = await forwarder.areUnderlyingAssetsEqual(erc20AssetData, erc20AssetData).callAsync(); expect(result).to.be.true(); }); - it('returns true if makerAssetData is the ERC20Bridge equivalent of takerFeeAssetData', async () => { - const result = await forwarder.isPercentageFee(erc20AssetData, erc20BridgeAssetData).callAsync(); + it('returns true if assetData1 == assetData2 are ERC20Bridge', async () => { + const result = await forwarder + .areUnderlyingAssetsEqual(erc20BridgeAssetData, erc20BridgeAssetData) + .callAsync(); expect(result).to.be.true(); }); - it('returns false if takerFeeAssetData != makerAssetData are ERC20', async () => { + it('returns true if assetData2 is the ERC20Bridge equivalent of assetData1', async () => { + const result = await forwarder.areUnderlyingAssetsEqual(erc20AssetData, erc20BridgeAssetData).callAsync(); + expect(result).to.be.true(); + }); + it('returns false if assetData1 != assetData2 are ERC20', async () => { const differentERC20AssetData = assetDataEncoder.ERC20Token(randomAddress()).getABIEncodedTransactionData(); - const result = await forwarder.isPercentageFee(erc20AssetData, differentERC20AssetData).callAsync(); + const result = await forwarder + .areUnderlyingAssetsEqual(erc20AssetData, differentERC20AssetData) + .callAsync(); expect(result).to.be.false(); }); - it('returns false if takerFeeAssetData is ERC20 and makerAssetData is ERC721', async () => { - const result = await forwarder.isPercentageFee(erc20AssetData, erc721AssetData).callAsync(); + it('returns false if assetData1 is ERC20 and assetData2 is ERC721', async () => { + const result = await forwarder.areUnderlyingAssetsEqual(erc20AssetData, erc721AssetData).callAsync(); expect(result).to.be.false(); }); - it('returns false if makerAssetData us ERC20Bridge, but for a different token than takerFeeAssetData', async () => { + it('returns false if assetData2 is ERC20Bridge, but for a different token than assetData1', async () => { const mismatchedErc20BridgeAssetData = assetDataEncoder .ERC20Bridge(randomAddress(), bridgeAddress, bridgeData) .getABIEncodedTransactionData(); - const result = await forwarder.isPercentageFee(erc20AssetData, mismatchedErc20BridgeAssetData).callAsync(); + const result = await forwarder + .areUnderlyingAssetsEqual(erc20AssetData, mismatchedErc20BridgeAssetData) + .callAsync(); expect(result).to.be.false(); }); - it('returns false if takerFeeAssetData is not ERC20', async () => { - const result = await forwarder.isPercentageFee(erc721AssetData, erc721AssetData).callAsync(); + it('returns false if assetData1 == assetData2 are ERC721', async () => { + const result = await forwarder.areUnderlyingAssetsEqual(erc721AssetData, erc721AssetData).callAsync(); expect(result).to.be.false(); }); }); diff --git a/contracts/integrations/contracts/test/TestEth2Dai.sol b/contracts/integrations/contracts/test/TestEth2Dai.sol index 0a38d7d93a..011906472c 100644 --- a/contracts/integrations/contracts/test/TestEth2Dai.sol +++ b/contracts/integrations/contracts/test/TestEth2Dai.sol @@ -26,6 +26,14 @@ import "@0x/contracts-erc20/contracts/test/DummyERC20Token.sol"; contract TestEth2Dai is IEth2Dai { + uint256 private _excessBuyAmount; + + function setExcessBuyAmount(uint256 amount) + external + { + _excessBuyAmount = amount; + } + function sellAllAmount( address sellTokenAddress, uint256 sellTokenAmount, @@ -41,11 +49,11 @@ contract TestEth2Dai is sellTokenAmount ); DummyERC20Token buyToken = DummyERC20Token(buyTokenAddress); - buyToken.mint(minimumFillAmount); + buyToken.mint(minimumFillAmount + _excessBuyAmount); buyToken.transfer( msg.sender, - minimumFillAmount + minimumFillAmount + _excessBuyAmount ); - return minimumFillAmount; + return minimumFillAmount + _excessBuyAmount; } } diff --git a/contracts/integrations/contracts/test/TestUniswapExchange.sol b/contracts/integrations/contracts/test/TestUniswapExchange.sol index 68938de793..256d58a035 100644 --- a/contracts/integrations/contracts/test/TestUniswapExchange.sol +++ b/contracts/integrations/contracts/test/TestUniswapExchange.sol @@ -27,6 +27,7 @@ contract TestUniswapExchange is IUniswapExchange { DummyERC20Token public token; + uint256 private _excessBuyAmount; constructor(address _tokenAddress) public { token = DummyERC20Token(_tokenAddress); @@ -39,6 +40,12 @@ contract TestUniswapExchange is payable {} + function setExcessBuyAmount(uint256 amount) + external + { + _excessBuyAmount = amount; + } + function ethToTokenTransferInput( uint256 minTokensBought, uint256, /* deadline */ @@ -48,9 +55,9 @@ contract TestUniswapExchange is payable returns (uint256 tokensBought) { - token.mint(minTokensBought); - token.transfer(recipient, minTokensBought); - return minTokensBought; + token.mint(minTokensBought + _excessBuyAmount); + token.transfer(recipient, minTokensBought + _excessBuyAmount); + return minTokensBought + _excessBuyAmount; } function tokenToEthSwapInput( @@ -66,8 +73,8 @@ contract TestUniswapExchange is address(this), tokensSold ); - msg.sender.transfer(minEthBought); - return minEthBought; + msg.sender.transfer(minEthBought + _excessBuyAmount); + return minEthBought + _excessBuyAmount; } function tokenToTokenTransferInput( @@ -87,9 +94,9 @@ contract TestUniswapExchange is tokensSold ); DummyERC20Token toToken = DummyERC20Token(toTokenAddress); - toToken.mint(minTokensBought); - toToken.transfer(recipient, minTokensBought); - return minTokensBought; + toToken.mint(minTokensBought + _excessBuyAmount); + toToken.transfer(recipient, minTokensBought + _excessBuyAmount); + return minTokensBought + _excessBuyAmount; } function toTokenAddress() diff --git a/contracts/integrations/test/forwarder/bridge_test.ts b/contracts/integrations/test/forwarder/bridge_test.ts index 58b59082e0..bc63ef302b 100644 --- a/contracts/integrations/test/forwarder/bridge_test.ts +++ b/contracts/integrations/test/forwarder/bridge_test.ts @@ -21,17 +21,21 @@ import { Taker } from '../framework/actors/taker'; import { actorAddressesByName } from '../framework/actors/utils'; import { BlockchainBalanceStore } from '../framework/balances/blockchain_balance_store'; import { DeploymentManager } from '../framework/deployment_manager'; +import { TestEth2DaiContract, TestUniswapExchangeContract } from '../wrappers'; import { deployForwarderAsync } from './deploy_forwarder'; import { ForwarderTestFactory } from './forwarder_test_factory'; -blockchainTests.resets('Forwarder <> ERC20Bridge integration tests', env => { +blockchainTests.resets.only('Forwarder <> ERC20Bridge integration tests', env => { let deployment: DeploymentManager; - let forwarder: ForwarderContract; - let assetDataEncoder: IAssetDataContract; let balanceStore: BlockchainBalanceStore; let testFactory: ForwarderTestFactory; + let forwarder: ForwarderContract; + let assetDataEncoder: IAssetDataContract; + let eth2Dai: TestEth2DaiContract; + let uniswapExchange: TestUniswapExchangeContract; + let erc721Token: DummyERC721TokenContract; let nftId: BigNumber; let makerTokenAssetData: string; @@ -58,10 +62,12 @@ blockchainTests.resets('Forwarder <> ERC20Bridge integration tests', env => { [erc721Token] = deployment.tokens.erc721; forwarder = await deployForwarderAsync(deployment, env); - const [eth2DaiBridge] = await deployEth2DaiBridgeAsync(deployment, env); - const [uniswapBridge, [uniswapMakerTokenExchange]] = await deployUniswapBridgeAsync(deployment, env, [ - makerToken.address, - ]); + const eth2DaiContracts = await deployEth2DaiBridgeAsync(deployment, env); + const [eth2DaiBridge] = eth2DaiContracts; + [, eth2Dai] = eth2DaiContracts; + const uniswapContracts = await deployUniswapBridgeAsync(deployment, env, [makerToken.address]); + const [uniswapBridge] = uniswapContracts; + [, [uniswapExchange]] = uniswapContracts; makerTokenAssetData = assetDataEncoder.ERC20Token(makerToken.address).getABIEncodedTransactionData(); makerFeeTokenAssetData = assetDataEncoder.ERC20Token(makerFeeToken.address).getABIEncodedTransactionData(); @@ -129,7 +135,7 @@ blockchainTests.resets('Forwarder <> ERC20Bridge integration tests', env => { [nftId] = await maker.configureERC721TokenAsync(erc721Token); // We need to top up the TestUniswapExchange with some ETH so that it can perform tokenToEthSwapInput - await uniswapMakerTokenExchange.topUpEth().awaitTransactionSuccessAsync({ + await uniswapExchange.topUpEth().awaitTransactionSuccessAsync({ from: forwarderFeeRecipient.address, value: constants.ONE_ETHER.times(10), }); @@ -160,6 +166,11 @@ blockchainTests.resets('Forwarder <> ERC20Bridge integration tests', env => { it('should partially fill a single Eth2DaiBridge order without a taker fee', async () => { await testFactory.marketSellTestAsync([eth2DaiBridgeOrder], 0.34); }); + it('should correctly handle excess maker asset acquired from Eth2Dai', async () => { + const bridgeExcessBuyAmount = new BigNumber(1); + await eth2Dai.setExcessBuyAmount(bridgeExcessBuyAmount).awaitTransactionSuccessAsync(); + await testFactory.marketSellTestAsync([eth2DaiBridgeOrder], 0.34, { bridgeExcessBuyAmount }); + }); it('should fill a single Eth2DaiBridge order with a WETH taker fee', async () => { const order = { ...eth2DaiBridgeOrder, @@ -196,6 +207,11 @@ blockchainTests.resets('Forwarder <> ERC20Bridge integration tests', env => { it('should partially fill a single UniswapBridge order without a taker fee', async () => { await testFactory.marketSellTestAsync([uniswapBridgeOrder], 0.34); }); + it('should correctly handle excess maker asset acquired from Uniswap', async () => { + const bridgeExcessBuyAmount = new BigNumber(1); + await uniswapExchange.setExcessBuyAmount(bridgeExcessBuyAmount).awaitTransactionSuccessAsync(); + await testFactory.marketSellTestAsync([uniswapBridgeOrder], 0.34, { bridgeExcessBuyAmount }); + }); it('should fill a single UniswapBridge order with a WETH taker fee', async () => { const order = { ...uniswapBridgeOrder, @@ -249,6 +265,11 @@ blockchainTests.resets('Forwarder <> ERC20Bridge integration tests', env => { it('should return excess ETH', async () => { await testFactory.marketBuyTestAsync([eth2DaiBridgeOrder], 1, { ethValueAdjustment: 1 }); }); + it('should correctly handle excess maker asset acquired from Eth2Dai', async () => { + const bridgeExcessBuyAmount = new BigNumber(1); + await eth2Dai.setExcessBuyAmount(bridgeExcessBuyAmount).awaitTransactionSuccessAsync(); + await testFactory.marketBuyTestAsync([eth2DaiBridgeOrder], 0.34, { bridgeExcessBuyAmount }); + }); it('should fill a single Eth2DaiBridge order with a WETH taker fee', async () => { const order = { ...eth2DaiBridgeOrder, @@ -295,6 +316,11 @@ blockchainTests.resets('Forwarder <> ERC20Bridge integration tests', env => { it('should partially fill a single UniswapBridge order without a taker fee', async () => { await testFactory.marketBuyTestAsync([uniswapBridgeOrder], 0.34); }); + it('should correctly handle excess maker asset acquired from Uniswap', async () => { + const bridgeExcessBuyAmount = new BigNumber(1); + await uniswapExchange.setExcessBuyAmount(bridgeExcessBuyAmount).awaitTransactionSuccessAsync(); + await testFactory.marketBuyTestAsync([uniswapBridgeOrder], 0.34, { bridgeExcessBuyAmount }); + }); it('should fill a single UniswapBridge order with a WETH taker fee', async () => { const order = { ...uniswapBridgeOrder, diff --git a/contracts/integrations/test/forwarder/forwarder_test_factory.ts b/contracts/integrations/test/forwarder/forwarder_test_factory.ts index 4f61a67168..7d77641d4f 100644 --- a/contracts/integrations/test/forwarder/forwarder_test_factory.ts +++ b/contracts/integrations/test/forwarder/forwarder_test_factory.ts @@ -29,28 +29,32 @@ interface ForwarderFillState { interface MarketSellOptions { forwarderFeePercentage: Numberish; revertError: RevertError; + bridgeExcessBuyAmount: BigNumber; } interface MarketBuyOptions extends MarketSellOptions { ethValueAdjustment: number; // Used to provided insufficient/excess ETH } -function isPercentageFee(takerFeeAssetData: string, makerAssetData: string): boolean { - const makerAssetProxyId = hexSlice(makerAssetData, 0, 4); - const takerFeeAssetProxyId = hexSlice(takerFeeAssetData, 0, 4); - if (makerAssetProxyId === AssetProxyId.ERC20Bridge && takerFeeAssetProxyId === AssetProxyId.ERC20) { +function areUnderlyingAssetsEqual(assetData1: string, assetData2: string): boolean { + const assetProxyId1 = hexSlice(assetData1, 0, 4); + const assetProxyId2 = hexSlice(assetData2, 0, 4); + if ( + (assetProxyId1 === AssetProxyId.ERC20 || assetProxyId1 === AssetProxyId.ERC20Bridge) && + (assetProxyId2 === AssetProxyId.ERC20 || assetProxyId2 === AssetProxyId.ERC20Bridge) + ) { const assetDataDecoder = new IAssetDataContract(constants.NULL_ADDRESS, provider); - const [makerTokenAddress] = assetDataDecoder.getABIDecodedTransactionData( - 'ERC20Bridge', - makerAssetData, - ); - const takerFeeTokenAddress = assetDataDecoder.getABIDecodedTransactionData( - 'ERC20Token', - takerFeeAssetData, - ); - return makerTokenAddress === takerFeeTokenAddress; + const tokenAddress1 = + assetProxyId1 === AssetProxyId.ERC20 + ? assetDataDecoder.getABIDecodedTransactionData('ERC20Token', assetData1) + : assetDataDecoder.getABIDecodedTransactionData<[string]>('ERC20Bridge', assetData1)[0]; + const tokenAddress2 = + assetProxyId2 === AssetProxyId.ERC20 + ? assetDataDecoder.getABIDecodedTransactionData('ERC20Token', assetData2) + : assetDataDecoder.getABIDecodedTransactionData<[string]>('ERC20Bridge', assetData2)[0]; + return tokenAddress2 === tokenAddress1; } else { - return makerAssetData === takerFeeAssetData; + return false; } } @@ -92,7 +96,7 @@ export class ForwarderTestFactory { const tx = this._forwarder .marketBuyOrdersWithEth( orders, - makerAssetAcquiredAmount, + makerAssetAcquiredAmount.minus(options.bridgeExcessBuyAmount || 0), orders.map(signedOrder => signedOrder.signature), feePercentage, this._forwarderFeeRecipient.address, @@ -208,6 +212,7 @@ export class ForwarderTestFactory { order, ordersInfoBefore[i].orderTakerAssetFilledAmount, Math.min(remainingOrdersToFill, 1), + options.bridgeExcessBuyAmount || constants.ZERO_AMOUNT, ); remainingOrdersToFill = Math.max(remainingOrdersToFill - 1, 0); @@ -232,6 +237,7 @@ export class ForwarderTestFactory { order: SignedOrder, takerAssetFilled: BigNumber, fillFraction: number, + bridgeExcessBuyAmount: BigNumber, ): Promise { let { makerAssetAmount, takerAssetAmount, makerFee, takerFee } = order; makerAssetAmount = makerAssetAmount.times(fillFraction).integerValue(BigNumber.ROUND_CEIL); @@ -251,9 +257,10 @@ export class ForwarderTestFactory { const takerFeeFilled = takerAssetFilled.times(order.takerFee).dividedToIntegerBy(order.takerAssetAmount); takerFee = BigNumber.max(takerFee.minus(takerFeeFilled), 0); + makerAssetAmount = makerAssetAmount.plus(bridgeExcessBuyAmount); let wethSpentAmount = takerAssetAmount.plus(DeploymentManager.protocolFee); let makerAssetAcquiredAmount = makerAssetAmount; - if (isPercentageFee(order.takerFeeAssetData, order.makerAssetData)) { + if (areUnderlyingAssetsEqual(order.takerFeeAssetData, order.makerAssetData)) { makerAssetAcquiredAmount = makerAssetAcquiredAmount.minus(takerFee); } else if (order.takerFeeAssetData === order.takerAssetData) { wethSpentAmount = wethSpentAmount.plus(takerFee);