SqrtPriceLimit computed from pool price/tick reading storage

[e1Ru1o]

100% price impact used as limit

[duncancmt]

It still seems to me that there ought to be a more efficient way of conditionally clamping the stopping price between its min and max values than what has been implemented here. But if you've thought about it and can't come up with anything better, then I'll leave it be.

Can you also triple check that you're rounding in the correct direction with all your fixed-point approximations? I'm extra paranoid about this now after Bunni and BalancerV2.

How does the gas compare for this technique vs the previous technique of passing in the limit in calldata? Make sure to account for the increased intrinsic gas costs of the additional nonzero bytes of calldata. At least from spot checking a couple of snapshots, this seems like a pretty severe gas pessimization, much more than I would've anticipated. Do you know why?

[duncancmt]

ditto

I wonder if this if/else tree can be more efficiently implemented. What does the reference do?

[e1Ru1o]

This is the closest I was looking at in the reference, which also have several branches

But will double check if there is anything better I am missing

[e1Ru1o]

they uses the above everytime they do math with sqrtPrices. The match in our case can only move to the upper/lower class as it is just multiplying/dividing for sqrt(2), so I tried to just check that to adjust the result

[e1Ru1o]

Match was improved and comments made explicit

[duncancmt]

I probably need to make 1 more pass over this before approval. How's the gas consumption looking?

[duncancmt]

@iamkunal9's AI agent caught some bugs. Please thoroughly go back over your code for MaverickV2 and make sure there are no more lingering issues

[duncancmt]

What happens when spacing is full-range? I believe we end up with delta = 0 and then no swaps are possible. Even in the absence of this edge case, I think that this logic is not correct. Please refer back to the MaverickV2 documentation and fix this.

[e1Ru1o]

I haven't found anything about full range liquidity in Maverick docs, but I can tell that delta = 0 doesn't prevent a swap from happening (tried it to double check) as the price can move without moving the tick, and that is what we use in Maverick, tick, not price. If I round up what we can guarantee is that the impact is at most 200%

[e1Ru1o]

I initially rounded up, but then wanted to enforce the at most 100% as everywhere else.

[e1Ru1o]

If you suspect it doesn't work due to something else please let me know

[duncancmt]

I still need to go over Dodo, Maverick, Ekubo, and UniV2

[duncancmt]

I'm not sure this comment is justified. Can you explain more?

[e1Ru1o]

🤔, it is kind of naive, the math was done to get to the price which exact 100% price impact, as this is exact math, so in floating math we will get to the exact price (==). But, as this is exact math, rounding down in the middle changes the inequality to less or equal (<=). That is why I said at most but we should not be able to reach the boundary

[duncancmt]

to the price which exact 100% price impact

are you sure it's exactly 100% price impact? you're using fixnums, which are not typically exact and demand attention to rounding direction. given that the direction that the price moves depends on zeroForOne, I would expect the correct computation to require that we round up in one direction and round down in the other. I would not expect that rounding down gives the correct <= bounds in both cases.

of course, given that we're working with fairly large fixnum bases here (Q96 and Q95), it's possible that the error introduced by inconsistent rounding either doesn't practically matter or literally does not matter even at the extremes of sqrtPriceX96. it would be good to call this out explicitly in this comment.

[duncancmt]

I have reviewed everything except your changes to the DodoV1 action. I am having some trouble with that one figuring out whether what you've written is exactly equivalent to the old code. Can you walk me through your argument about why it's the same?

[e1Ru1o]

Let me list what I discover and did in DodoV1:

1. There are 3 calculation functions, _RBelowSellQuoteToken, _RAboveSellQuoteToken, and _ROneSellQuoteToken
2. Each of them have the calculation of uint256 i = DecimalMath.divFloor(DecimalMath.ONE, state.oraclePrice);. Duplicated code!, that was moved to the main function dodoQuerySellQuoteToken
3. Two of them, _RAboveSellQuoteToken, and _ROneSellQuoteToken, are identical besides the second parameter passed to _SolveQuadraticFunctionForTrade which is also used in the return value. That is duplicated code, so they were merged.
4. DodoState was used for simplicity to pass the data from a function to the other. This along with QUOTE_BALANCE (Q in the code) being used only in one of the branches of dodoQuerySellQuoteToken made me decide to remove the struct to avoid allocating unnecessary memory and move the call to fast_QUOTE_BALANCE_ to the branch where it is relevant.
5. The branches with RStatus.ONE and RStatus.ABOVE_ONE were going to use the same function with just that parameter difference, so ternary was put in place to differentiate the parameter and the branches merged.
6. The last branch left (else in the original code) which was meant to be for RStatus.BELOW_ONE was mostly kept as it is, and only the contents of the _RBelowSellQuoteToken were inlined in favor of removing the function as it was the only place where it was used.

[immunefi-magnus]

🛡️ Immunefi PR Reviews

We noticed that your project isn't set up for automatic code reviews. If you'd like this PR reviewed by the Immunefi team, you can request it manually using the link below:

🔗 Send this PR in for review

Once submitted, we'll take care of assigning a reviewer and follow up here.