Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ArrakisUniswapV3AmmAdapter #260

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

zishansami102
Copy link

The changes implement an adapter for the AmmModule which supports Arrakis Vaults holding UniswapV3 positions.

The goal is to be able to create and hold Uniswap's V3 Liquidity Provision tokens (LP tokens) inside of a Set Token.
There is a challenge caused by UniV3's LP tokens being ERC721s (NFTs). And since AmmModule.sol assumes amm pool tokens to follow IERC20, UniswapV3 Pools aren't directly integrable.

So are using Arrakis (Arrakis V1 Doc) that creates and holds Uniswap V3 Liquidity Position and issues fungible ERC20 tokens to anyone who adds more liquidity.

There are three main contracts being used in the implementation:

  1. Arrakis Facotry - ArrakisFactoryV1.sol
  2. Arrakis Vault - ArrakisVaultV1.sol
  3. Arrakis Router - GUniRouter.sol - Mainnet Deployment

Other Relevant Contracts - Link

Using Arrakis Factory, anyone can create new Arrakis Vaults (analogous to Token Pools) which hold UniswapV3 liquidity position for a pair of ERC20 tokens for a given price range bound (assuming there already exists a pool for the same pair of tokens in UniswapV3).

Once Vault is created, it works similar to UniswapV2 liquidity pools and can be passed to AmmModule contract as ammPool, which then will call ArrakisUniswapV3Adapter returning calldata with Arrakis Router, which handles adding and removing liquidity and minting/burning ERC20 Tokens to the caller (in our case it is SetToken).

The router in the ArrakisUniswapV3Adapter.sol is the address to the Arrakis Router contract which is passed in the constructor along with UniswapV3 Factory address. The overall approach is now very similar to how UniswapV2AmmAdapter is implemented with the following correlations:

  • Arrakis Facotry - UniswapV2 Factory
  • Arrakis Vault - UniswapV2 Liquidity Pools
  • Arrakis Router - UniswapV2 Router

Copy link
Contributor

@FlattestWhite FlattestWhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

address public immutable router;

// UniswapV3 factory contract
IUniswapV3Factory public immutable uniV3Factory;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove unused variable

Copy link
Contributor

@ckoopmann ckoopmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Mostly questions for my own understanding

"_minLiquidity is too high for input token limit"
);

target = router;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we using (i.e. encoding for) the addLiquidity method on the arrakis router instead of using the mint method on the vault directly. I'd expect the latter to incurr lower gas fees right ?

// Attempt to get the tokens of the provided pool
address token0;
address token1;
try IArrakisVaultV1(_pool).token0() returns (IERC20 _token0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try catch statements like these don't catch the case when _pool is not a smart contract, in which case the whole tx will still fail. If you want to avoid that you might want to add a codesize check for the _pool address.

See this so question that I posted regarding this topic:
https://ethereum.stackexchange.com/questions/128845/why-does-soliditys-try-catch-not-catch-call-to-non-contract-address-revertion


// Internal function string for adding liquidity
string internal constant ADD_LIQUIDITY =
"addLiquidity(address,uint256,uint256,uint256,uint256,address)";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking, we don't include addLiquidityEth interface in this adapter since we only allow providing liquidity with ERC20 and not native ETH correct ?

override
returns (address /*target*/, uint256 /*value*/, bytes memory /*data*/)
{
revert("Arrakis single asset addition is not supported");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be implemented using the rebalanceAndAddLiquidity method on the router ? Any specific reason why we didn't implement this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants