diff --git a/contracts/asset-proxy/contracts/src/bridges/UniswapBridge.sol b/contracts/asset-proxy/contracts/src/bridges/UniswapBridge.sol index 1da6a3ea25..c5210d7e96 100644 --- a/contracts/asset-proxy/contracts/src/bridges/UniswapBridge.sol +++ b/contracts/asset-proxy/contracts/src/bridges/UniswapBridge.sol @@ -40,7 +40,6 @@ contract UniswapBridge is // Struct to hold `withdrawTo()` local variables in memory and to avoid // stack overflows. struct WithdrawToState { - address exchangeTokenAddress; IUniswapExchange exchange; uint256 fromTokenBalance; IEtherToken weth; @@ -82,15 +81,11 @@ contract UniswapBridge is return BRIDGE_SUCCESS; } - // Get the exchange token for the token pair. - state.exchangeTokenAddress = _getUniswapExchangeTokenAddressForTokenPair( + // Get the exchange for the token pair. + state.exchange = _getUniswapExchangeForTokenPair( fromTokenAddress, toTokenAddress ); - // Get the exchange. - state.exchange = _getUniswapExchangeForToken(state.exchangeTokenAddress); - // Grant an allowance to the exchange. - _grantExchangeAllowance(state.exchange, state.exchangeTokenAddress); // Get our balance of `fromTokenAddress` token. state.fromTokenBalance = IERC20Token(fromTokenAddress).balanceOf(address(this)); // Get the weth contract. @@ -113,6 +108,8 @@ contract UniswapBridge is // Convert from a token to WETH. } else if (toTokenAddress == address(state.weth)) { + // Grant the exchange an allowance. + _grantExchangeAllowance(state.exchange, fromTokenAddress); // Buy as much ETH with `fromTokenAddress` token as possible. uint256 ethBought = state.exchange.tokenToEthSwapInput( // Sell all tokens we hold. @@ -129,6 +126,8 @@ contract UniswapBridge is // Convert from one token to another. } else { + // Grant the exchange an allowance. + _grantExchangeAllowance(state.exchange, fromTokenAddress); // Buy as much `toTokenAddress` token with `fromTokenAddress` token // and transfer it to `to`. state.exchange.tokenToTokenTransferInput( @@ -193,36 +192,26 @@ contract UniswapBridge is IERC20Token(tokenAddress).approve(address(exchange), uint256(-1)); } - /// @dev Retrieves the uniswap exchange token address for a given token pair. + /// @dev Retrieves the uniswap exchange for a given token pair. /// In the case of a WETH-token exchange, this will be the non-WETH token. /// In th ecase of a token-token exchange, this will be the first token. /// @param fromTokenAddress The address of the token we are converting from. /// @param toTokenAddress The address of the token we are converting to. - /// @return exchangeTokenAddress The address of the token for the uniswap - /// exchange. - function _getUniswapExchangeTokenAddressForTokenPair( + /// @return exchange The uniswap exchange. + function _getUniswapExchangeForTokenPair( address fromTokenAddress, address toTokenAddress ) - private - view - returns (address exchangeTokenAddress) - { - // Whichever isn't WETH is the exchange token. - if (fromTokenAddress != address(getWethContract())) { - return fromTokenAddress; - } - return toTokenAddress; - } - - /// @dev Retrieves the uniswap exchange contract for a given token. - /// @return exchange The exchange contract for the token. - function _getUniswapExchangeForToken(address tokenAddress) private view returns (IUniswapExchange exchange) { - exchange = getUniswapExchangeFactoryContract().getExchange(tokenAddress); + address exchangeTokenAddress = fromTokenAddress; + // Whichever isn't WETH is the exchange token. + if (fromTokenAddress == address(getWethContract())) { + exchangeTokenAddress = toTokenAddress; + } + exchange = getUniswapExchangeFactoryContract().getExchange(exchangeTokenAddress); require(address(exchange) != address(0), "NO_UNISWAP_EXCHANGE_FOR_TOKEN"); return exchange; } diff --git a/contracts/asset-proxy/test/uniswap_bridge.ts b/contracts/asset-proxy/test/uniswap_bridge.ts index a284ff7fc7..92e70d5ca1 100644 --- a/contracts/asset-proxy/test/uniswap_bridge.ts +++ b/contracts/asset-proxy/test/uniswap_bridge.ts @@ -29,7 +29,7 @@ import { TestUniswapBridgeWethWithdrawEventArgs as WethWithdrawArgs, } from '../src'; -blockchainTests.resets('UniswapBridge unit tests', env => { +blockchainTests.resets.only('UniswapBridge unit tests', env => { const txHelper = new TransactionHelper(env.web3Wrapper, artifacts); let testContract: TestUniswapBridgeContract; let wethTokenAddress: string; @@ -324,27 +324,12 @@ blockchainTests.resets('UniswapBridge unit tests', env => { expect(calls[0].args.recipient).to.eq(opts.toAddress); }); - it('sets allowance for "to" token', async () => { - const { opts, logs } = await withdrawToAsync({ + it('does not set any allowance', async () => { + const { logs } = await withdrawToAsync({ fromTokenAddress: wethTokenAddress, }); const approvals = filterLogsToArguments(logs, ContractEvents.TokenApprove); - const exchangeAddress = await getExchangeForTokenAsync(opts.toTokenAddress); - expect(approvals.length).to.eq(1); - expect(approvals[0].spender).to.eq(exchangeAddress); - expect(approvals[0].allowance).to.bignumber.eq(constants.MAX_UINT256); - }); - - it('sets allowance for "to" token on subsequent calls', async () => { - const { opts } = await withdrawToAsync({ - fromTokenAddress: wethTokenAddress, - }); - const { logs } = await withdrawToAsync(opts); - const approvals = filterLogsToArguments(logs, ContractEvents.TokenApprove); - const exchangeAddress = await getExchangeForTokenAsync(opts.toTokenAddress); - expect(approvals.length).to.eq(1); - expect(approvals[0].spender).to.eq(exchangeAddress); - expect(approvals[0].allowance).to.bignumber.eq(constants.MAX_UINT256); + expect(approvals).to.be.empty(''); }); it('fails if "to" token does not exist', async () => {