[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
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
| 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" | ||
|
|
@@ -113,38 +114,64 @@ func PriceToTick(price sdk.Dec) (int64, error) { | |
| return tickIndex, 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 | ||
| } | ||
|
|
||
| // PriceToTickRoundDown takes a price and returns the corresponding tick index. | ||
| // If tickSpacing is provided, the tick index will be rounded down to the nearest multiple of tickSpacing. | ||
| // NOTE: This should not be called in the state machine. | ||
| // CONTRACT: tickSpacing must be smaller or equal to the max of 1 << 63 - 1. | ||
| // This is not a concern because we have authorized tick spacings that are smaller than this max, | ||
| // and we don't expect to ever require it to be this large. | ||
| func PriceToTickRoundDownSpacing(price sdk.Dec, tickSpacing uint64) (int64, error) { | ||
AlpinYukseloglu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| tickIndex, err := PriceToTick(price) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| tickIndex, err = RoundDownTickToSpacing(tickIndex, int64(tickSpacing)) | ||
| 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 |
||
| 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 | ||
| } | ||
|
|
||
| tickIndex, err = RoundDownTickToSpacing(tickIndex, int64(tickSpacing)) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| return tickIndex, nil | ||
|
|
@@ -166,13 +193,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 +227,85 @@ 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 | ||
| } | ||
|
|
||
| // CalculatePriceToTick takes in a price and returns the corresponding tick index. | ||
| // This function does not take into consideration tick spacing. | ||
| // NOTE: This should not be called in the state machine. | ||
| // NOTE: This is really returning a "Bucket index". Bucket index `b` corresponds to | ||
| // all prices in range [TickToSqrtPrice(b), TickToSqrtPrice(b+1)). | ||
| // We make an erroneous assumption here, that bucket index `b` corresponds to | ||
| // all prices in range [TickToPrice(b), TickToPrice(b+1)). | ||
| // This currently makes this function unsuitable for the state machine. | ||
| func CalculatePriceToTick(price sdk.Dec) (tickIndex int64) { | ||
| // TODO: Make truncate, since this defines buckets as | ||
| // [TickToPrice(b - .5), TickToPrice(b+.5)) | ||
| return calculatePriceToTickDec(price).RoundInt64() | ||
| } | ||
AlpinYukseloglu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // CalculatePriceToTick takes in a square root and returns the corresponding tick index. | ||
AlpinYukseloglu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // This function does not take into consideration tick spacing. | ||
| // NOTE: This is really returning a "Bucket index". Bucket index `b` corresponds to | ||
| // all sqrt prices in range [TickToSqrtPrice(b), TickToSqrtPrice(b+1)). | ||
AlpinYukseloglu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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 the max tick, set the candidate to max tick - 2. | ||
AlpinYukseloglu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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.
Driveby rename as the original name could be misinterpreted to refer to rounding behavior