From b742e92502ea86d9ac2e4c6460ca1eb4db04fa9a Mon Sep 17 00:00:00 2001 From: Noah Zinsmeister Date: Mon, 17 Feb 2020 15:06:43 -0700 Subject: [PATCH] optimistic swaps (#53) * alternative flash lending (renting) design * add rent interface * fix stack too deep error rearrange order of k condition math ignore erroneous out of gas errors in tests * try removing rent in favor of monolithic swap IUniswapV2Borrower -> IUniswapV2Callee update tests * fix implementation * clean up math a bit * amount{0,1}In -> amount{0,1}InNet * charge on all inputs, not just net * removed unnecessary safemath * add to != token check don't indent in scope rename reserve{0,1}Next -> reserve{0,1}Adjusted * > instead of >= simplify algebra reserve{0,1}Adjusted -> balance{0,1}Adjusted add comments * add some optimistic swap test cases --- contracts/UniswapV2Exchange.sol | 64 ++++++++++++--------- contracts/interfaces/IUniswapV2Callee.sol | 5 ++ contracts/interfaces/IUniswapV2Exchange.sol | 11 +++- test/UniswapV2Exchange.spec.ts | 51 ++++++++++------ test/shared/fixtures.ts | 12 ++-- 5 files changed, 94 insertions(+), 49 deletions(-) create mode 100644 contracts/interfaces/IUniswapV2Callee.sol diff --git a/contracts/UniswapV2Exchange.sol b/contracts/UniswapV2Exchange.sol index 9727cf7..4b95c26 100644 --- a/contracts/UniswapV2Exchange.sol +++ b/contracts/UniswapV2Exchange.sol @@ -6,6 +6,7 @@ import './libraries/Math.sol'; import './libraries/UQ112x112.sol'; import './interfaces/IERC20.sol'; import './interfaces/IUniswapV2Factory.sol'; +import './interfaces/IUniswapV2Callee.sol'; contract UniswapV2Exchange is IUniswapV2Exchange, UniswapV2ERC20 { using SafeMath for uint; @@ -49,13 +50,21 @@ contract UniswapV2Exchange is IUniswapV2Exchange, UniswapV2ERC20 { event Mint(address indexed sender, uint amount0, uint amount1); event Burn(address indexed sender, uint amount0, uint amount1, address indexed to); - event Swap(address indexed sender, address indexed tokenIn, uint amountIn, uint amountOut, address indexed to); + event Swap( + address indexed sender, + uint amount0In, + uint amount1In, + uint amount0Out, + uint amount1Out, + address indexed to + ); event Sync(uint112 reserve0, uint112 reserve1); constructor() public { factory = msg.sender; } + // called once by the factory at time of deployment function initialize(address _token0, address _token1) external { require(msg.sender == factory, 'UniswapV2: FORBIDDEN'); // sufficient check token0 = _token0; @@ -99,6 +108,7 @@ contract UniswapV2Exchange is IUniswapV2Exchange, UniswapV2ERC20 { } } + // this low-level function should be called from a contract which performs important safety checks function mint(address to) external lock returns (uint liquidity) { (uint112 _reserve0, uint112 _reserve1,) = getReserves(); // gas savings uint balance0 = IERC20(token0).balanceOf(address(this)); @@ -118,10 +128,11 @@ contract UniswapV2Exchange is IUniswapV2Exchange, UniswapV2ERC20 { _mint(to, liquidity); _update(balance0, balance1, _reserve0, _reserve1); - if (feeOn) kLast = uint(reserve0).mul(reserve1); + if (feeOn) kLast = uint(reserve0).mul(reserve1); // reserve0 and reserve1 are up-to-date emit Mint(msg.sender, amount0, amount1); } + // this low-level function should be called from a contract which performs important safety checks function burn(address to) external lock returns (uint amount0, uint amount1) { (uint112 _reserve0, uint112 _reserve1,) = getReserves(); // gas savings address _token0 = token0; // gas savings @@ -142,40 +153,39 @@ contract UniswapV2Exchange is IUniswapV2Exchange, UniswapV2ERC20 { balance1 = IERC20(_token1).balanceOf(address(this)); _update(balance0, balance1, _reserve0, _reserve1); - if (feeOn) kLast = uint(reserve0).mul(reserve1); + if (feeOn) kLast = uint(reserve0).mul(reserve1); // reserve0 and reserve1 are up-to-date emit Burn(msg.sender, amount0, amount1, to); } - function swap(address tokenIn, uint amountOut, address to) external lock { - require(amountOut > 0, 'UniswapV2: INSUFFICIENT_OUTPUT_AMOUNT'); + // this low-level function should be called from a contract which performs important safety checks + function swap(uint amount0Out, uint amount1Out, address to, bytes calldata data) external lock { + require(amount0Out > 0 || amount1Out > 0, 'UniswapV2: INSUFFICIENT_OUTPUT_AMOUNT'); (uint112 _reserve0, uint112 _reserve1,) = getReserves(); // gas savings - address _token0 = token0; // gas savings - address _token1 = token1; // gas savings + require(amount0Out < _reserve0 && amount1Out < _reserve1, 'UniswapV2: INSUFFICIENT_LIQUIDITY'); + uint balance0; uint balance1; - uint amountIn; - - if (tokenIn == _token0) { - require(amountOut < _reserve1, 'UniswapV2: INSUFFICIENT_LIQUIDITY'); - balance0 = IERC20(_token0).balanceOf(address(this)); - amountIn = balance0.sub(_reserve0); - require(amountIn > 0, 'UniswapV2: INSUFFICIENT_INPUT_AMOUNT'); - require(amountIn.mul(_reserve1 - amountOut).mul(997) >= amountOut.mul(_reserve0).mul(1000), 'UniswapV2: K'); - _safeTransfer(_token1, to, amountOut); - balance1 = IERC20(_token1).balanceOf(address(this)); - } else { - require(tokenIn == _token1, 'UniswapV2: INVALID_INPUT_TOKEN'); - require(amountOut < _reserve0, 'UniswapV2: INSUFFICIENT_LIQUIDITY'); - balance1 = IERC20(_token1).balanceOf(address(this)); - amountIn = balance1.sub(_reserve1); - require(amountIn > 0, 'UniswapV2: INSUFFICIENT_INPUT_AMOUNT'); - require(amountIn.mul(_reserve0 - amountOut).mul(997) >= amountOut.mul(_reserve1).mul(1000), 'UniswapV2: K'); - _safeTransfer(_token0, to, amountOut); - balance0 = IERC20(_token0).balanceOf(address(this)); + { // scope for _token{0,1}, avoids stack too deep errors + address _token0 = token0; + address _token1 = token1; + require(to != _token0 && to != _token1, 'UniswapV2: INVALID_TO'); + if (amount0Out > 0) _safeTransfer(_token0, to, amount0Out); // optimistically transfer tokens + if (amount1Out > 0) _safeTransfer(_token1, to, amount1Out); // optimistically transfer tokens + if (data.length > 0) IUniswapV2Callee(to).uniswapV2Call(msg.sender, amount0Out, amount1Out, data); + balance0 = IERC20(_token0).balanceOf(address(this)); + balance1 = IERC20(_token1).balanceOf(address(this)); + } + uint amount0In = balance0 > _reserve0 - amount0Out ? balance0 - (_reserve0 - amount0Out) : 0; + uint amount1In = balance1 > _reserve1 - amount1Out ? balance1 - (_reserve1 - amount1Out) : 0; + require(amount0In > 0 || amount1In > 0, 'UniswapV2: INSUFFICIENT_INPUT_AMOUNT'); + { // scope for reserve{0,1}Adjusted, avoids stack too deep errors + uint balance0Adjusted = balance0.mul(1000).sub(amount0In.mul(3)); + uint balance1Adjusted = balance1.mul(1000).sub(amount1In.mul(3)); + require(balance0Adjusted.mul(balance1Adjusted) >= uint(_reserve0).mul(_reserve1).mul(1000**2), 'UniswapV2: K'); } _update(balance0, balance1, _reserve0, _reserve1); - emit Swap(msg.sender, tokenIn, amountIn, amountOut, to); + emit Swap(msg.sender, amount0In, amount1In, amount0Out, amount1Out, to); } // force balances to match reserves diff --git a/contracts/interfaces/IUniswapV2Callee.sol b/contracts/interfaces/IUniswapV2Callee.sol new file mode 100644 index 0000000..f067408 --- /dev/null +++ b/contracts/interfaces/IUniswapV2Callee.sol @@ -0,0 +1,5 @@ +pragma solidity =0.5.16; + +interface IUniswapV2Callee { + function uniswapV2Call(address sender, uint amount0, uint amount1, bytes calldata data) external; +} diff --git a/contracts/interfaces/IUniswapV2Exchange.sol b/contracts/interfaces/IUniswapV2Exchange.sol index 2f37c9a..100b49d 100644 --- a/contracts/interfaces/IUniswapV2Exchange.sol +++ b/contracts/interfaces/IUniswapV2Exchange.sol @@ -23,7 +23,14 @@ interface IUniswapV2Exchange { event Mint(address indexed sender, uint amount0, uint amount1); event Burn(address indexed sender, uint amount0, uint amount1, address indexed to); - event Swap(address indexed sender, address indexed tokenIn, uint amountIn, uint amountOut, address indexed to); + event Swap( + address indexed sender, + uint amount0In, + uint amount1In, + uint amount0Out, + uint amount1Out, + address indexed to + ); event Sync(uint112 reserve0, uint112 reserve1); function MINIMUM_LIQUIDITY() external pure returns (uint); @@ -37,7 +44,7 @@ interface IUniswapV2Exchange { function mint(address to) external returns (uint liquidity); function burn(address to) external returns (uint amount0, uint amount1); - function swap(address tokenIn, uint amountOut, address to) external; + function swap(uint amount0Out, uint amount1Out, address to, bytes calldata data) external; function skim(address to) external; function sync() external; diff --git a/test/UniswapV2Exchange.spec.ts b/test/UniswapV2Exchange.spec.ts index b6f3ccb..de107c1 100644 --- a/test/UniswapV2Exchange.spec.ts +++ b/test/UniswapV2Exchange.spec.ts @@ -12,7 +12,7 @@ const MINIMUM_LIQUIDITY = bigNumberify(10).pow(3) chai.use(solidity) const overrides = { - gasLimit: 1000000 + gasLimit: 9999999 } describe('UniswapV2Exchange', () => { @@ -68,7 +68,7 @@ describe('UniswapV2Exchange', () => { await token1.transfer(exchange.address, token1Amount) await exchange.mint(wallet.address, overrides) } - const testCases: BigNumber[][] = [ + const swapTestCases: BigNumber[][] = [ [1, 5, 10, '1662497915624478906'], [1, 10, 5, '453305446940074565'], @@ -78,15 +78,34 @@ describe('UniswapV2Exchange', () => { [1, 10, 10, '906610893880149131'], [1, 100, 100, '987158034397061298'], [1, 1000, 1000, '996006981039903216'] - ].map(a => a.map((n, i) => (i === 3 ? bigNumberify(n) : expandTo18Decimals(n as number)))) - testCases.forEach((testCase, i) => { + ].map(a => a.map(n => (typeof n === 'string' ? bigNumberify(n) : expandTo18Decimals(n)))) + swapTestCases.forEach((swapTestCase, i) => { it(`getInputPrice:${i}`, async () => { - await addLiquidity(testCase[1], testCase[2]) - await token0.transfer(exchange.address, testCase[0]) - await expect(exchange.swap(token0.address, testCase[3].add(1), wallet.address, overrides)).to.be.revertedWith( + const [swapAmount, token0Amount, token1Amount, expectedOutputAmount] = swapTestCase + await addLiquidity(token0Amount, token1Amount) + await token0.transfer(exchange.address, swapAmount) + await expect(exchange.swap(0, expectedOutputAmount.add(1), wallet.address, '0x', overrides)).to.be.revertedWith( 'UniswapV2: K' ) - await exchange.swap(token0.address, testCase[3], wallet.address, overrides) + await exchange.swap(0, expectedOutputAmount, wallet.address, '0x', overrides) + }) + }) + + const optimisticTestCases: BigNumber[][] = [ + ['997000000000000000', 5, 10, 1], // given amountIn, amountOut = floor(amountIn * .997) + ['997000000000000000', 10, 5, 1], + ['997000000000000000', 5, 5, 1], + [1, 5, 5, '1003009027081243732'] // given amountOut, amountIn = ceiling(amountOut / .997) + ].map(a => a.map(n => (typeof n === 'string' ? bigNumberify(n) : expandTo18Decimals(n)))) + optimisticTestCases.forEach((optimisticTestCase, i) => { + it(`optimistic:${i}`, async () => { + const [outputAmount, token0Amount, token1Amount, inputAmount] = optimisticTestCase + await addLiquidity(token0Amount, token1Amount) + await token0.transfer(exchange.address, inputAmount) + await expect(exchange.swap(outputAmount.add(1), 0, wallet.address, '0x', overrides)).to.be.revertedWith( + 'UniswapV2: K' + ) + await exchange.swap(outputAmount, 0, wallet.address, '0x', overrides) }) }) @@ -98,13 +117,13 @@ describe('UniswapV2Exchange', () => { const swapAmount = expandTo18Decimals(1) const expectedOutputAmount = bigNumberify('1662497915624478906') await token0.transfer(exchange.address, swapAmount) - await expect(exchange.swap(token0.address, expectedOutputAmount, wallet.address, overrides)) + await expect(exchange.swap(0, expectedOutputAmount, wallet.address, '0x', overrides)) .to.emit(token1, 'Transfer') .withArgs(exchange.address, wallet.address, expectedOutputAmount) .to.emit(exchange, 'Sync') .withArgs(token0Amount.add(swapAmount), token1Amount.sub(expectedOutputAmount)) .to.emit(exchange, 'Swap') - .withArgs(wallet.address, token0.address, swapAmount, expectedOutputAmount, wallet.address) + .withArgs(wallet.address, swapAmount, 0, 0, expectedOutputAmount, wallet.address) const reserves = await exchange.getReserves() expect(reserves[0]).to.eq(token0Amount.add(swapAmount)) @@ -125,13 +144,13 @@ describe('UniswapV2Exchange', () => { const swapAmount = expandTo18Decimals(1) const expectedOutputAmount = bigNumberify('453305446940074565') await token1.transfer(exchange.address, swapAmount) - await expect(exchange.swap(token1.address, expectedOutputAmount, wallet.address, overrides)) + await expect(exchange.swap(expectedOutputAmount, 0, wallet.address, '0x', overrides)) .to.emit(token0, 'Transfer') .withArgs(exchange.address, wallet.address, expectedOutputAmount) .to.emit(exchange, 'Sync') .withArgs(token0Amount.sub(expectedOutputAmount), token1Amount.add(swapAmount)) .to.emit(exchange, 'Swap') - .withArgs(wallet.address, token1.address, swapAmount, expectedOutputAmount, wallet.address) + .withArgs(wallet.address, 0, swapAmount, expectedOutputAmount, 0, wallet.address) const reserves = await exchange.getReserves() expect(reserves[0]).to.eq(token0Amount.sub(expectedOutputAmount)) @@ -157,7 +176,7 @@ describe('UniswapV2Exchange', () => { const expectedOutputAmount = bigNumberify('453305446940074565') await token0.transfer(exchange.address, swapAmount) await mineBlock(provider, (await provider.getBlock('latest')).timestamp + 1) - const gasCost = await exchange.estimate.swap(token0.address, expectedOutputAmount, wallet.address, overrides) + const gasCost = await exchange.estimate.swap(0, expectedOutputAmount, wallet.address, '0x', overrides) console.log(`Gas required for swap: ${gasCost}`) }) @@ -209,7 +228,7 @@ describe('UniswapV2Exchange', () => { await token0.transfer(exchange.address, swapAmount) await mineBlock(provider, blockTimestamp + 10) // swap to a new price eagerly instead of syncing - await exchange.swap(token0.address, expandTo18Decimals(1), wallet.address, overrides) // make the price nice + await exchange.swap(0, expandTo18Decimals(1), wallet.address, '0x', overrides) // make the price nice expect(await exchange.price0CumulativeLast()).to.eq(initialPrice[0].mul(10)) expect(await exchange.price1CumulativeLast()).to.eq(initialPrice[1].mul(10)) @@ -232,7 +251,7 @@ describe('UniswapV2Exchange', () => { const swapAmount = expandTo18Decimals(1) const expectedOutputAmount = bigNumberify('996006981039903216') await token1.transfer(exchange.address, swapAmount) - await exchange.swap(token1.address, expectedOutputAmount, wallet.address, overrides) + await exchange.swap(expectedOutputAmount, 0, wallet.address, '0x', overrides) const expectedLiquidity = expandTo18Decimals(1000) await exchange.transfer(exchange.address, expectedLiquidity.sub(MINIMUM_LIQUIDITY)) @@ -250,7 +269,7 @@ describe('UniswapV2Exchange', () => { const swapAmount = expandTo18Decimals(1) const expectedOutputAmount = bigNumberify('996006981039903216') await token1.transfer(exchange.address, swapAmount) - await exchange.swap(token1.address, expectedOutputAmount, wallet.address, overrides) + await exchange.swap(expectedOutputAmount, 0, wallet.address, '0x', overrides) const expectedLiquidity = expandTo18Decimals(1000) await exchange.transfer(exchange.address, expectedLiquidity.sub(MINIMUM_LIQUIDITY)) diff --git a/test/shared/fixtures.ts b/test/shared/fixtures.ts index 3f56797..697d5ac 100644 --- a/test/shared/fixtures.ts +++ b/test/shared/fixtures.ts @@ -12,8 +12,12 @@ interface FactoryFixture { factory: Contract } +const overrides = { + gasLimit: 9999999 +} + export async function factoryFixture(_: Web3Provider, [wallet]: Wallet[]): Promise { - const factory = await deployContract(wallet, UniswapV2Factory, [wallet.address]) + const factory = await deployContract(wallet, UniswapV2Factory, [wallet.address], overrides) return { factory } } @@ -26,10 +30,10 @@ interface ExchangeFixture extends FactoryFixture { export async function exchangeFixture(provider: Web3Provider, [wallet]: Wallet[]): Promise { const { factory } = await factoryFixture(provider, [wallet]) - const tokenA = await deployContract(wallet, ERC20, [expandTo18Decimals(10000)]) - const tokenB = await deployContract(wallet, ERC20, [expandTo18Decimals(10000)]) + const tokenA = await deployContract(wallet, ERC20, [expandTo18Decimals(10000)], overrides) + const tokenB = await deployContract(wallet, ERC20, [expandTo18Decimals(10000)], overrides) - await factory.createExchange(tokenA.address, tokenB.address) + await factory.createExchange(tokenA.address, tokenB.address, overrides) const exchangeAddress = await factory.getExchange(tokenA.address, tokenB.address) const exchange = new Contract(exchangeAddress, JSON.stringify(UniswapV2Exchange.abi), provider).connect(wallet)