Add sqrtPriceX96 utility to uniswap integration
Closed this issue · 18 comments
Describe the feature you would like
Currently, the Uniswap integration helper allows easy swapping of tokens. However, to integration test this, you need to know the sqrtPriceX96 which is not straightforward to calculate. Adding a helper for this would simplify using this integration.
Additional context
No response
@kopy-kat We haven't met but Kurt suggested I take a look at the issues in this repo if I want to contribute. Are you happy for me to pick this one up and contribute an initial piece of work for your review? Thanks! Richard
@kopy-kat Great! Two quick questions /clarifications to start:
1 - I'm assuming this helper would be a function that would take as an input parameter the price ratio that one wants to set as the limit price (token0 price / token1 price), and returns the sqrtPriceX96 (square root, then multiply by 2^96) of that price ratio?
2 - I'm just wondering where would be the best place to put this function (and a possible sub function) - I'm thinking either inside src/test/utils/Vm.sol , or as a new .sol file in that same directory?
Thanks!
Sounds good.
- Yeah I think this would make sense - ideally there could also be a helper for getting these prices.
- I think added to the existing lib would be best: https://github.com/rhinestonewtf/modulekit/blob/main/src/integrations/uniswap/v3/Uniswap.sol
Cool.
I might be missing some of what you want to achieve here, so let me just check:
From what I understand, sqrtPriceX96 is an input param to the UniswapV3 swap function. It allows a caller to specify a limit price for the swap. If you set it to 0 then the param is basically inactive for the swap. This might be the best way to start testing the swap integration?
If we do want to make this param active and test it, then as you say we would need to pull in prices somehow (unless we just test it using a mock). It would also make sense to have helper functions to convert from a price ratio to sqrtPriceX96 (so that our test can pass in a price ratio as an input).
And perhaps at some point to optimise UX we might want to do something similar to the uniswapV3 front-end, where they give users the option to specify the limit at the market price, or +1%, +5%, +10% (or completely manually). (Doing this would also require pulling in prices)
I just want to check in to understand which of the above options we're aiming for. I've started implementing the conversion helpers, and also looking at pulling quotes in using getQuote, but this does start to add a little bit of complexity, so I just want to clarify first.
Thanks!
Yep, as you said, the lowest hanging fruit is using sqrtPriceX96 = 0. However, in some production-grade code it might be desirable to check that sqrtPriceX96 > 0 because this essentially means unlimited slippage. In this case, tests would need to actually calculate the limit.
Yes, it sounds like you're going for the approach of actually calculating it and the prices needed which was the goal of the issue. Does that all make sense?
Hey Konrad, yip, thanks it's definitely coming clearer. I still have a few more comments and questions to make sure I'm focussing on the right thing and that we've got a solid shared understanding. Let me put down my understanding of the swap function here, but please correct me if you think I'm missing the key point:
For swapExactInputSingle, the way I've specified slippage in the past is by specifying amountOutMin. In practice this has meant consulting an oracle (either a custom built TWAP oracle, or another type) to get the current price, and then setting amountOutMin to current price - slippage (e.g. slippage could be 1% of current price).
In the current implementation in the modulekit, amountOutMin is always zero, and sqrtPriceX96 is used to set the slippage limit, which is a new approach to me, but it makes sense. From what I'm reading it seems that best practice is to set both amountOutMin and sqrtPriceX96 to have maximum slippage protection, but for now I will move forward just using sqrtPriceX96 (it does seem a bit redundant to use both).
So then, to get back to the original intent of the issue, this means that we need helpers to get the current price, and to convert it to sqrtPriceX96.
There are two obvious means to do this:
1 - via an oracle, e.g. a chainlink oracle
2 - using the getQuote function on the uniswap Quoter contract
I will look to implement both of these, and then create a simple test to see how well they work in practice. Let me know if you have a strong preference for one or the other.
Just fyi while I'm looking at the details of the uniswap helper, some other issues that might need to be considered in future:
1 - We might want to set the deadline to block.timestamp + 300 (seconds) or some such to increase the chances of the swap succeeding. Alternatively we could make this something that the caller could set.
2 - We might want to build in the ability to set the fee according to the pool. Currently the fee is set at 3000, but some pools have different fees.
That makes sense, for approach 1 how would using an oracle work when using a foundry test environment? I assume that it should work if a network state is forked but not entirely sure on this.
I'm not entirely sure on the first of the final issues - the swap should just happen in the tx, right? The second issue makes sense to me - what do you think the best devX would be here?
I'll code up an oracle integration to test how it works in a foundry test with a forked node.
As for deadline - setting deadline to block.timestamp means that the swap must happen in the current block, or it gets reverted. A lot of the time this will be fine, but sometimes (e.g. if slippage is very tight) it can't be executed in the current block, and in that case setting a deadline of 3 or 5 minutes gives it longer. I read that the Uniswap FE sets the deadline to 20min, which sounded long to me.
In terms of both of these issues, my take is that the best devX is to give devs access to all of the input params that Uniswap makes available. Sure, it reduces simplicity, but it gives them the choice to design their apps the way that they want to, and they can always zero out or give default values to the params they don't want to use. That's my two cents.
Will let you know what I find with testing the oracle.
Update: the call to the chainlink oracle works fine with the foundry forked node 🙌
Hi Konrad, I've opened a pull request for this issue, but I can't see how to add you as a reviewer? (Apologies if I'm missing something obvious, but it's not showing the option to add a reviewer as I normally would).
The basic work on the helper is done, but I'd like to get your eyes on the code before going any further if that's possible.
Welcome any comments, particularly on (a) any code style or formatting issues that you can see, and moreover (b) on the way that I've built the helpers and the test, whether this is the right direction to be moving in, and finally (c) some thoughts from your side on next steps with this issue.
Thanks!
Hey sorry for the lack of response - I've been OOO. I can see the PR and will take a look tmr
Hey Konrad, hope you're well. Just checking in around this issue? Let me know if you had any issues getting it to run on your side. There was one new dependency and a .env file that had to provide an rpc url. Happy to make adjustments if needed.
Cheers.
Hey, yeah I'm still getting this issue. I think it's related to the dependencies (because there are two different versions of OZ). Can you reproduce the issue? I think the solution is removing the chainlink dependency and just adding the interface file used - lmk if that makes sense
I can completely remove the chainlink dependency for now, and remove the interface as well - the price from the price oracle is currently just there for demonstration purposes, it's not actually being used in the sqrtPriceX96 calculation and limit setting.
We can then come back to the inclusion of the price oracle price once we have this working?
Yeah that sounds good - lmk when I should try it out again
Cool - oracle call and chainlink dependency removed - test running smoothly on my side - the swap test takes a little longer than the others to complete due to the call to the forked node - let me know if any issues cropping up on your side🤞
Yep it works now thanks - will leave some comments on the pr