-
Notifications
You must be signed in to change notification settings - Fork 608
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
refactor/test(CL): Stricter rounding behavior in CL math methods; unit tests at low price level #6369
Conversation
…t tests at low price level
// TODO (perf): consider better conversion helpers to minimize reallocations. | ||
amountBigDec := osmomath.BigDecFromDec(amount.ToLegacyDec()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewer:
tracking these here: #6370
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this pr can resolve this TODO #6409
// TODO (perf): consider Dec() function that does not reallocate | ||
return amountBigDec.MulMut(product).QuoMut(diff).Dec() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO (perf): consider Dec() function that does not reallocate | ||
return amountBigDec.QuoMut(diff).Dec() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but want to have discussion on the one comment below. I might just be missing something but it seems like we should be erroring on this case.
"low price range (with round up)": { | ||
liquidity: smallLiquidity, | ||
sqrtPA: sqrtANearMin, | ||
sqrtPB: sqrtBNearMin, | ||
roundUp: true, | ||
// from clmath decimal import * | ||
// calc_amount_one_delta(liq, sqrtPriceA, sqrtPriceB, False) | ||
// Actual result: 0.000000000000000000000000000103787163 | ||
// Gets rounded up to 1. Is this acceptable when the multiplicative difference is so large? | ||
amount1Expected: osmomath.MustNewBigDecFromStr("0.000000000000000000000000000103787163").Ceil(), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems problematic right? Shouldn't any calculation for amount1 being less than 1 just fail?
I think this might be okay since we only ever use round up to be in favor of the pool, but I think this would translate to larger slippage on the users end (but the slippage here is 1 unit so likely fine).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My recollection of digging into this during prelaunch internal reviews was that this was at least not a safety issue. We probably want to minimize rounding error in general but it's probably okay to overround to be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems problematic right? Shouldn't any calculation for amount1 being less than 1 just fail?
No, there are many instances where it is valid. E.g. no liquidity, adjacent ticks with little liquidity.
On another look, I also think this is fine since done only in pool's favor (controlled by the roundUp
flag)
I also reviewed Uniswap's logic and their rounding is even stricter where they take a ceiling of an intermediary calculation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice job. Had a clarifying question to get a better understanding of in which cases reordering is an improvement here, but regardless the implementation looks sound
"low price range (with round up)": { | ||
liquidity: smallLiquidity, | ||
sqrtPA: sqrtANearMin, | ||
sqrtPB: sqrtBNearMin, | ||
roundUp: true, | ||
// from clmath decimal import * | ||
// calc_amount_one_delta(liq, sqrtPriceA, sqrtPriceB, False) | ||
// Actual result: 0.000000000000000000000000000103787163 | ||
// Gets rounded up to 1. Is this acceptable when the multiplicative difference is so large? | ||
amount1Expected: osmomath.MustNewBigDecFromStr("0.000000000000000000000000000103787163").Ceil(), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My recollection of digging into this during prelaunch internal reviews was that this was at least not a safety issue. We probably want to minimize rounding error in general but it's probably okay to overround to be safe.
Closes: #6353
What is the purpose of the change
This effort was started with the goal of testing CL
math
at low price level to make sure that there are no unexpected error blow-ups or rounding behavior at the extended price level.While working on this, I noticed that there were small rounding direction issues in:
GetNextSqrtPriceFromAmount0InRoundingUp
. They ended up being more apparent at the lower min spot price range. As a result, the fix is added here.Additionally, in follow-up to #6352, I realized that dividing by a bigger sqrt price first and smaller second leads to a more accurate result in
CalcAmount0Delta
. This is consistent with how Uniswap does it. See here:https://github.com/Uniswap/v3-core/blob/412d9b236a1e75a98568d49b1aeb21e3a1430544/contracts/libraries/SqrtPriceMath.sol#L159-L172
sqrtRatioBX96
is the larger one. In our case, we were dividing by a small value first, ending up with a less precise result.Testing and Verifying
Documentation and Release Note
Unreleased
section ofCHANGELOG.md
?Where is the change documented?
x/{module}/README.md
)