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

Update ControllerHelper pools to take a pool input from f/e to allow multi pool routing in future (pool, fee, etc) #294

Closed
aleone opened this issue Apr 6, 2022 · 10 comments · Fixed by #400
Assignees

Comments

@aleone
Copy link
Contributor

aleone commented Apr 6, 2022

If there is ever liquidity in different fee tier pools, this contract can not support trading other than the 0.3% pool, maybe should be included, or could be added / changed in future.

@alexisgauba
Copy link
Contributor

imo shld include ability to have other pools + fee tiers! I believe in OKRs there is an inititaive around building liquidity beyond the 0.3% sqth-eth pool

@aleone
Copy link
Contributor Author

aleone commented Apr 6, 2022

yeah, was what made me think about it being a priority!

@aleone
Copy link
Contributor Author

aleone commented Apr 6, 2022

easyish fix for multiple pools for 1 tx is f/e passes in params (but will need to do f/e routing) and not sure how f/e can support multiple pool transactions without something that can batch it or different smart contract architecture, with 2 liquid pools, most of the trade will ideally be routed through both of them

@alexisgauba
Copy link
Contributor

easyish fix for multiple pools for 1 tx is f/e passes in params (but will need to do f/e routing) and not sure how f/e can support multiple pool transactions without something that can batch it or different smart contract architecture, with 2 liquid pools, most of the trade will ideally be routed through both of them

how does uniswap autorouter do it / can we just use that?

@aleone
Copy link
Contributor Author

aleone commented Apr 6, 2022

IMO the the bigger issue is we do things other than just trading (lets say we can figure out the routing issue on the f/e, which is what uniswap does)

Say user wants to mint + sell 50/50 btwn 2 pools, I think we need to either call the function 2x with different pools for 50% size each (including mint/collateral amt, slippage, etc) or have a function that can trade on more than 1 pool (not feasible IMO)

Maybe something like multicall could do the formed, but I'm not sure it would work.

@alexisgauba
Copy link
Contributor

IMO the the bigger issue is we do things other than just trading (lets say we can figure out the routing issue on the f/e, which is what uniswap does)

Say user wants to mint + sell 50/50 btwn 2 pools, we need to either call the function 2x with different pools for 50% size each (including mint/collateral amt, slippage, etc) or have a function that can trade on more than 1 pool (not feasible IMO)

Maybe something like multicall could do the formed, but I'm not sure it would work.

ohh u mean support multiple pools for LPing? I think routing to LP in two pools is interesting but probably net necessary to be in scope because no users are doing it? Users seem to have a preferred pool thus far?

but it does seem like if there is liquidity across multiple pools we should be trading across multiple of them in buying + selling

@aleone
Copy link
Contributor Author

aleone commented Apr 6, 2022

No I mean if there is liquidity in 2 pools and someone wants to mint+sell (or do some other action that touches multiple pools, say buy back and burn) and the best price will be to split the selling btwn the 2 pools. Right now we can't do that in either old or new contracts.

Maybe something like this is possible, how uniswap does it: https://github.com/Uniswap/swap-router-contracts/blob/main/contracts/V3SwapRouter.sol

@alexisgauba
Copy link
Contributor

No I mean if there is liquidity in 2 pools and someone wants to mint+sell (or do some other action that touches multiple pools, say buy back and burn) and the best price will be to split the selling btwn the 2 pools. Right now we can't do that in either old or new contracts.

Maybe something like this is possible, how uniswap does it: https://github.com/Uniswap/swap-router-contracts/blob/main/contracts/V3SwapRouter.sol

ok gtocha yeah definitely think we shld be routing buys / sells between the pools as is best. Can we just call Uniswap's contract if they already have that logic?

@aleone
Copy link
Contributor Author

aleone commented Apr 13, 2022

@haythem96

  • Look into multicall solutions out there (we probably need to forward msg.value)
  • Look into call vs delegate call - cant have the multicall be the msg.sender (tx.origin has to be passed on)

@alexisgauba I think that if we can't get multicall working and transactions are only routed to 1 pool at a time, this change is not worth it as it is almost never optimal to route 100% to 1 pool. It would take a long time to implement a for type loop in the ControllerHelper that can take arbitrary number of transactions across arbitrary pools.

@haythemsellami
Copy link
Member

I think we can use Multicall implementation from OZ, and make the ControllerHelper inherit it to allow batching many ControllerHelper calls, assuming we modify the contract to allow adding the Uni pool address.
But I think will have to modify the OZ implementation to allow passing msg.value and thinking of how wrapping ETH will be affected as msg.value do not decrease Uniswap/v3-periphery#52

@aleone aleone changed the title Should we have the pool fee and wPowerPerp pool address hardcoded? Update ControllerHelper pools to take a pool input from f/e to allow multi pool routing in future (pool, fee, etc) Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants