[CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price #5541
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package math | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
|
|
||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
|
@@ -91,60 +92,44 @@ func TickToPrice(tickIndex int64) (price sdk.Dec, err error) { | |
| return price, nil | ||
| } | ||
|
|
||
| // RoundDownTickToSpacing rounds the tick index down to the nearest tick spacing if the tickIndex is in between authorized tick values | ||
| // Note that this is Euclidean modulus. | ||
| // The difference from default Go modulus is that Go default results | ||
| // in a negative remainder when the dividend is negative. | ||
| // Consider example tickIndex = -17, tickSpacing = 10 | ||
| // tickIndexModulus = tickIndex % tickSpacing = -7 | ||
| // tickIndexModulus = -7 + 10 = 3 | ||
| // tickIndex = -17 - 3 = -20 | ||
| func RoundDownTickToSpacing(tickIndex int64, tickSpacing int64) (int64, error) { | ||
| tickIndexModulus := tickIndex % tickSpacing | ||
| if tickIndexModulus < 0 { | ||
| tickIndexModulus += tickSpacing | ||
| } | ||
|
|
||
| if tickIndexModulus != 0 { | ||
| tickIndex = tickIndex - tickIndexModulus | ||
| } | ||
|
|
||
| // Defense-in-depth check to ensure that the tick index is within the authorized range | ||
| // Should never get here. | ||
| if tickIndex > types.MaxTick || tickIndex < types.MinTick { | ||
| return 0, types.TickIndexNotWithinBoundariesError{ActualTick: tickIndex, MinTick: types.MinTick, MaxTick: types.MaxTick} | ||
| } | ||
|
|
||
| return tickIndex, nil | ||
| } | ||
|
|
||
| // SqrtPriceToTickRoundDown converts the given sqrt price to its corresponding tick rounded down | ||
| // to the nearest tick spacing. | ||
| func SqrtPriceToTickRoundDownSpacing(sqrtPrice sdk.Dec, tickSpacing uint64) (int64, error) { | ||
| tickIndex, err := CalculateSqrtPriceToTick(sqrtPrice) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Seems like diff got cluttered when rebasing to main. This section might be easier to review in split view |
||
| tickIndex, err = RoundDownTickToSpacing(tickIndex, int64(tickSpacing)) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| return tickIndex, nil | ||
|
|
@@ -166,13 +151,9 @@ func powTenBigDec(exponent int64) osmomath.BigDec { | |
| return bigNegPowersOfTen[-exponent] | ||
| } | ||
|
|
||
| func CalculatePriceToTickDec(price sdk.Dec) (tickIndex sdk.Dec) { | ||
| if price.Equal(sdkOneDec) { | ||
| return sdk.ZeroDec() | ||
| } | ||
|
|
||
| // The approach here is to try determine which "geometric spacing" are we in. | ||
|
|
@@ -204,9 +185,73 @@ func CalculatePriceToTick(price sdk.Dec) (tickIndex int64) { | |
| ticksFilledByCurrentSpacing := priceInThisExponent.Quo(geoSpacing.additiveIncrementPerTick) | ||
| // we get the bucket index by: | ||
| // * taking the bucket index of the smallest price in this tick | ||
| // * adding to it the number of ticks filled by the current spacing | ||
| // We leave rounding of this to the caller | ||
| // (NOTE: You'd expect it to be number of ticks "completely" filled by the current spacing, | ||
| // which would be truncation. However price may have errors, hence it being callers job) | ||
| tickIndex = ticksFilledByCurrentSpacing.SDKDec() | ||
| tickIndex = tickIndex.Add(sdk.NewDec(geoSpacing.initialTick)) | ||
| return tickIndex | ||
| } | ||
|
|
||
| // CalculateSqrtPriceToTick takes in a square root and returns the corresponding tick index. | ||
| // This function does not take into consideration tick spacing. | ||
| func CalculateSqrtPriceToTick(sqrtPrice sdk.Dec) (tickIndex int64, err error) { | ||
| // SqrtPrice may have errors, so we take the tick obtained from the price | ||
| // and move it in a +/- 1 tick range based on the sqrt price those ticks would imply. | ||
| price := sqrtPrice.Mul(sqrtPrice) | ||
| tick := CalculatePriceToTickDec(price) | ||
| truncatedTick := tick.TruncateInt64() | ||
|
|
||
| // We have a candidate bucket index `t`. We discern here if: | ||
| // * sqrtPrice in [TickToSqrtPrice(t - 1), TickToSqrtPrice(t)) | ||
| // * sqrtPrice in [TickToSqrtPrice(t), TickToSqrtPrice(t + 1)) | ||
| // * sqrtPrice in [TickToSqrtPrice(t+1), TickToSqrtPrice(t + 2)) | ||
| // * sqrtPrice not in either. | ||
| // We handle boundary checks, by saying that if our candidate is the min tick, | ||
| // set the candidate to min tick + 1. | ||
| // If our candidate is at or above max tick - 1, set the candidate to max tick - 2. | ||
| // This is because to check tick t + 1, we need to go to t + 2, so to not go over | ||
| // max tick during these checks, we need to shift it down by 2. | ||
| // We check this at max tick - 1 instead of max tick, since we expect the output to | ||
| // have some error that can push us over the tick boundary. | ||
| outOfBounds := false | ||
| if truncatedTick <= types.MinTick { | ||
| truncatedTick = types.MinTick + 1 | ||
| outOfBounds = true | ||
| } else if truncatedTick >= types.MaxTick-1 { | ||
| truncatedTick = types.MaxTick - 2 | ||
| outOfBounds = true | ||
czarcas7ic marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+218
to
+224
There was a problem hiding this comment. I wonder if its simpler to just deal with these by checking the boundary cases in a separate function, on the sqrt price directly. (pre-squaring) There was a problem hiding this comment. Possibly, although my previous attempts at this led to constraining the "real" min tick and max tick by 1-2 ticks (which might be fine but leads to more cases that need to be checked/handled) If you see a way to abstract this logic without triggering this, would be great for us to do a follow up PR to clean this up! |
||
| } | ||
|
|
||
| _, sqrtPriceTmin1, errM1 := TickToSqrtPrice(truncatedTick - 1) | ||
| _, sqrtPriceT, errT := TickToSqrtPrice(truncatedTick) | ||
| _, sqrtPriceTplus1, errP1 := TickToSqrtPrice(truncatedTick + 1) | ||
| _, sqrtPriceTplus2, errP2 := TickToSqrtPrice(truncatedTick + 2) | ||
| if errM1 != nil || errT != nil || errP1 != nil || errP2 != nil { | ||
| return 0, errors.New("internal error in computing square roots within CalculateSqrtPriceToTick") | ||
| } | ||
|
|
||
| // We error if sqrtPriceT is above sqrtPriceTplus2 or below sqrtPriceTmin1. | ||
| // For cases where calculated tick does not fall on a limit (min/max tick), the upper end is exclusive. | ||
| // For cases where calculated tick falls on a limit, the upper end is inclusive, since the actual tick is | ||
| // already shifted and making it exclusive would make min/max tick impossible to reach by construction. | ||
| // We do this primary for code simplicity, as alternatives would require more branching and special cases. | ||
| if (!outOfBounds && sqrtPrice.GTE(sqrtPriceTplus2)) || sqrtPrice.LT(sqrtPriceTmin1) || sqrtPrice.GT(sqrtPriceTplus2) { | ||
| return 0, fmt.Errorf("sqrt price to tick could not find a satisfying tick index. Hit bounds: %v", outOfBounds) | ||
| } | ||
|
There was a problem hiding this comment. Not doubting this is correct, but its a bit hard to reason about. Consider breaking each of the three cases further in the comments, but not blocking (especially if its just me who is having trouble groking it) There was a problem hiding this comment. I am confused about the There was a problem hiding this comment. (I'd expect it to be There was a problem hiding this comment. This is unfortunately a slightly hacky way of special casing bound checks for |
||
|
|
||
| // We expect this case to only be hit when the original provided sqrt price is exactly equal to the max sqrt price. | ||
| if sqrtPrice.Equal(sqrtPriceTplus2) { | ||
| return truncatedTick + 2, nil | ||
| } | ||
|
|
||
| // The remaining cases handle shifting tick index by +/- 1. | ||
| if sqrtPrice.GTE(sqrtPriceTplus1) { | ||
| return truncatedTick + 1, nil | ||
| } | ||
| if sqrtPrice.GTE(sqrtPriceT) { | ||
| return truncatedTick, nil | ||
| } | ||
| return truncatedTick - 1, nil | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: these functions were moved to be high level test helpers to get them out of the state machine without breaking the many tests that rely on them to (correctly) generate expected values