UniswapV2AMMAdapter #128
UniswapV2AMMAdapter #128
Conversation
|
|
||
| target = router; | ||
| value = 0; | ||
| data = abi.encodeWithSignature( |
There was a problem hiding this comment.
One note for adapter PRs is we like to link the relevant documentation in a PR comment so that it's easy to find the interface we're trying to call here.
I think additionally a little more descriptive naming could help here. I'm not sure what these amounts are.
There was a problem hiding this comment.
Added a comment to this PR with links to the docs. Also tried to make the variable names more descriptive.
|
Relevant parts of Uniswap V2 docs: Uniswap V2 Pools - Core Concepts |
| // determine how much actual tokens are supplied to the pool, therefore setting our | ||
| // amountAMin and amountBMin of the addLiquidity call to the expected amounts. | ||
| uint[] memory minTokensIn = new uint[](2); | ||
| minTokensIn[0] = liquidityExpectedFromSuppliedTokens.mul(reserves[0]).div(pair.totalSupply()); |
There was a problem hiding this comment.
Just so I understand...one token will have the same minTokensIn and maxTokensIn right?
There was a problem hiding this comment.
That is correct. If you look at the Router code, it will always use at least one of amountADesired or amountBDesired which are our maxTokensIn values.
https://github.com/Uniswap/uniswap-v2-periphery/blob/master/contracts/UniswapV2Router02.sol#L46-L57
| uint[] memory reserves = new uint[](2); | ||
| uint[] memory reservesOwnedByLiquidity = new uint[](2); | ||
| (reserves[0], reserves[1]) = _getReserves(pair, _components[0]); | ||
| reservesOwnedByLiquidity[0] = reserves[0].mul(_liquidity).div(pair.totalSupply()); |
There was a problem hiding this comment.
This isn't an issue here since we KNOW _liquidity and pair.totalSupply() are both 18 decimal tokens. But if we didn't have those guarantees (like say liquidity could be anything) then we would want to use our PreciseUnitMath library in order to make sure we kept precision.
There was a problem hiding this comment.
Thanks for the heads up here. This means we should also be safe in the Math.min() part above since tokens sent in and their reserves will also have the same units.
2 participants
These changes implement an adapter for the AmmModule which supports UniswapV2 and its clones.
The adapter supports adding/removing liquidity to/from Uniswap V2 pools via the addLiquidity/removeLiquidity AmmModule functions using the supplied amounts of both pool tokens.
It also supports adding/removing liquidity to/from the pool via addLiquiditySingleAsset/removeLiquiditySingleAsset by swapping part of the single supplied token to the other required token and then adding the liquidity with both tokens or redeeming the liquidity for both pool tokens and swapping one token for the other token to return the requested single token asset.
In order for this adapter to work, a change to the AmmModule was required in two places. In both the removeLiquidity and removeLiquiditySingleAsset functions, approval was not granted on the liquidity pool token (unlike the approvals granted to the pool tokens on the addLiquidity/addLiquiditySingleAsset functions). Without this approval, the adapter was unable to supply the liquidity tokens to the pool for redemption. Therefore, a change was made in both functions to approve the spending by the spender on the liquidity token. With this change, both removeLiquidity and removeLiquiditySingleAsset work as expected.
Example transactions on the Kovan network of all four methods triggered via the AmmModule on a test SetToken are shown below:
Add Liquidity: https://kovan.etherscan.io/tx/0x91f945663c9487f18ec7d1637b12c5823b126b5f3840c0467f11af908661d803
Remove Liquidity: https://kovan.etherscan.io/tx/0x9f46ea04be65eda6f22a7c7f8c1eb20ab38c565cb6f705a1d155efad3cb99a86
Add Liquidity Single Asset: https://kovan.etherscan.io/tx/0x50736626c30c429d8c650d1625d903153a7ff13c394a3b2aa0137dc251367d03
Remove Liquidity Single Asset: https://kovan.etherscan.io/tx/0x4b8e46f7de5a6331fa2763af7f51f4012e08af96ccce8380efbf1d7f26ce4ce1