refactor(CL): rounding edge case in zfo strategy and tests #6379
Conversation
| // This edge case deals with an edge case stemming from rounding. | ||
| // If the computed sqrtPriceNext is 1 BigDec ULP away from the sqrtPriceTarget AND | ||
| // amount zero in needed to reach target is 1 unit greater than the amount zero remaining after spread rewards, | ||
| // we deems this as a rounding error and make sqrtPriceNext equal to sqrtPriceTarget | ||
| // while consuming all among in actually remaining. | ||
| // | ||
| // Without this change, we reach an infinite loop swap condition where we try to swap until the target but fail to consume | ||
| // any amount in to advance the swap by only one ULP. | ||
| // | ||
| // Changing the rounding behavior causes issues in other parts of the system where we end up overconsuming the final amount in. | ||
| if sqrtPriceNext.Sub(sqrtPriceTarget).Equal(smallestBigDec) && amountZeroIn.Sub(amountZeroInRemainingLessSpreadReward).Equal(oneBigDec) { | ||
| sqrtPriceNext = sqrtPriceTarget | ||
| amountZeroIn = amountZeroInRemainingLessSpreadReward | ||
| } | ||
|
|
There was a problem hiding this comment.
Note to reviewer:
Fuzz tester repro that is failing without this change: https://github.com/osmosis-labs/osmosis/pull/6380/files#r1323366454
Must be ran from the linked branch with this change reverted
| // The code snippet below deals with an edge case stemming from rounding. | ||
| // If the computed sqrtPriceNext is 1 BigDec ULP away from the sqrtPriceTarget AND | ||
| // amount zero in needed to reach target is 1 unit greater than the amount zero remaining after spread rewards, | ||
| // we deem this as a rounding error and make sqrtPriceNext equal to sqrtPriceTarget | ||
| // while consuming all amount in actually remaining. | ||
| // | ||
| // Without this change, we reach an infinite loop swap condition where we try to swap until the target but fail to consume | ||
| // any amount in to advance by only 1 ULP. | ||
| // | ||
| // Changing the rounding behavior causes issues in other parts of the system where we end up overconsuming the final amount in. | ||
| if sqrtPriceNext.Sub(sqrtPriceTarget).Equal(smallestBigDec) && amountZeroIn.Sub(amountZeroInRemainingLessSpreadReward).Equal(oneBigDec) { | ||
| sqrtPriceNext = sqrtPriceTarget | ||
| amountZeroIn = amountZeroInRemainingLessSpreadReward | ||
| } | ||
|
|
There was a problem hiding this comment.
Is a similar situation possible in the InGivenOut direction?
There was a problem hiding this comment.
In given out did not experience this in the fuzzer.
Since rounding functions and logic are different between the two, a similar situation might be impossible for in given out.
Since this is more of a UX issue (some swaps failing due to infinite loops) and not a safety issue, I don't think it is worth a deep investigation
| @@ -77,6 +81,21 @@ func (s zeroForOneStrategy) ComputeSwapWithinBucketOutGivenIn(sqrtPriceCurrent, | |||
| sqrtPriceNext = math.GetNextSqrtPriceFromAmount0InRoundingUp(sqrtPriceCurrent, liquidityBigDec, amountZeroInRemainingLessSpreadReward) | |||
| } | |||
|
|
|||
| // The code snippet below deals with an edge case stemming from rounding. | |||
| // If the computed sqrtPriceNext is 1 BigDec ULP away from the sqrtPriceTarget AND | |||
There was a problem hiding this comment.
Is it 1 ULP or 1 smallestBigDec? Or are these synonymous?
There was a problem hiding this comment.
Synonymous in this context. BigDec and Dec ULPs are different.
This is what this stands for: https://en.wikipedia.org/wiki/Unit_in_the_last_place
| sqrtPriceCurrent: types.MinSqrtPriceBigDec, | ||
| sqrtPriceTarget: types.MinSqrtPriceV2, |
There was a problem hiding this comment.
To clarify, this doesn't necessarily need to happen at the minSqrtPrice target right? Just the diff between these two needs to be a smallestBigDec?
There was a problem hiding this comment.
Should be possible at other targets. However, the fuzzer was hitting this when swapping to min tick only. I don't have other repros
Closes: #XXX
What is the purpose of the change
Introduces a solution to the rounding edge case discovered by the fuzz test suite.
It seems to occur when swapping at a low price level. There were instances caught where our
amountInRemainingLessFeeis one unit smaller thanamountInnecessary to reach the target (next initialized tick). Additionally, thesqrtPriceNextends up being only 1 BigDec ULP above the target.The likey reason for such behavior is
QuoRoundUphere:osmosis/x/concentrated-liquidity/math/math.go
Line 131 in da738a2
However, if we remove this rounding behavior, we start getting issues elsewhere. Specifically, over-consuming the amount in.
The current direction is correct to round up in pool's favor.
However, it seems that in cases like the one encountered, it sometimes overrounds, breaking the swap progress.
Therefore, we detect this edge case and simply bump the
sqrtPriceNextby one ULP to reach target and consume the full amount in necessary to reach the next initialized tick.Testing and Verifying
This was caught by the fuzz test suite. Upon applying the change, the issue got resolved.
This issue can be reproducing by following the instructions here: https://github.com/osmosis-labs/osmosis/pull/6380/files#r1323366454
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)