Add sqrtPriceX96 utility to uniswap integration

[kopy-kat]

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

[RichJamo]

@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]

Hey @RichJamo yeah that sounds good

[RichJamo]

@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!

[kopy-kat]

Sounds good.

1. Yeah I think this would make sense - ideally there could also be a helper for getting these prices.
2. I think added to the existing lib would be best: https://github.com/rhinestonewtf/modulekit/blob/main/src/integrations/uniswap/v3/Uniswap.sol

[RichJamo]

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!

[kopy-kat]

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?

[RichJamo]

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.

[kopy-kat]

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?

[RichJamo]

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.

[RichJamo]

Update: the call to the chainlink oracle works fine with the foundry forked node 🙌

[RichJamo]

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!

[kopy-kat]

Hey sorry for the lack of response - I've been OOO. I can see the PR and will take a look tmr