[CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price

x/concentrated-liquidity/invariant_test.go

@@ -144,10 +144,10 @@ func (s *KeeperTestSuite) assertWithdrawAllInvariant() {
 		totalWithdrawn = totalWithdrawn.Add(withdrawn...)
 	}
 
-	// We allow for an additive tolerance of 1 per position in the pool, since this is the maximum that can be truncated
-	// when collecting spread rewards.
+	// We allow for an additive tolerance of 2 per position in the pool, since we expect up to 1 to be lost to rounding on
+	// join and withdrawal each.
 	errTolerance := osmomath.ErrTolerance{
-		AdditiveTolerance: sdk.NewDec(int64(len(allPositions))),
+		AdditiveTolerance: sdk.NewDec(2 * int64(len(allPositions))),
 		RoundingDir:       osmomath.RoundDown,
 	}
 

x/concentrated-liquidity/keeper_test.go

@@ -50,10 +50,10 @@ var (
 	FullRangeLiquidityAmt                                = sdk.MustNewDecFromStr("70710678.118654752940000000")
 	DefaultTickSpacing                                   = uint64(100)
 	PoolCreationFee                                      = poolmanagertypes.DefaultParams().PoolCreationFee
-	DefaultExponentConsecutivePositionLowerTick, _       = math.PriceToTickRoundDown(sdk.NewDec(5500), DefaultTickSpacing)
-	DefaultExponentConsecutivePositionUpperTick, _       = math.PriceToTickRoundDown(sdk.NewDec(6250), DefaultTickSpacing)
-	DefaultExponentOverlappingPositionLowerTick, _       = math.PriceToTickRoundDown(sdk.NewDec(4000), DefaultTickSpacing)
-	DefaultExponentOverlappingPositionUpperTick, _       = math.PriceToTickRoundDown(sdk.NewDec(4999), DefaultTickSpacing)
+	DefaultExponentConsecutivePositionLowerTick, _       = math.PriceToTickRoundDownSpacing(sdk.NewDec(5500), DefaultTickSpacing)
+	DefaultExponentConsecutivePositionUpperTick, _       = math.PriceToTickRoundDownSpacing(sdk.NewDec(6250), DefaultTickSpacing)
+	DefaultExponentOverlappingPositionLowerTick, _       = math.PriceToTickRoundDownSpacing(sdk.NewDec(4000), DefaultTickSpacing)
+	DefaultExponentOverlappingPositionUpperTick, _       = math.PriceToTickRoundDownSpacing(sdk.NewDec(4999), DefaultTickSpacing)
 	BAR                                                  = "bar"
 	FOO                                                  = "foo"
 	InsufficientFundsError                               = fmt.Errorf("insufficient funds")

x/concentrated-liquidity/lp.go

@@ -66,13 +66,13 @@ func (k Keeper) createPosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddr
 	}
 
 	// Transform the provided ticks into their corresponding sqrtPrices.
-	priceLowerTick, priceUpperTick, sqrtPriceLowerTick, sqrtPriceUpperTick, err := math.TicksToSqrtPrice(lowerTick, upperTick)
+	_, _, sqrtPriceLowerTick, sqrtPriceUpperTick, err := math.TicksToSqrtPrice(lowerTick, upperTick)
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, 0, err
 	}
 
 	// If multiple ticks can represent the same spot price, ensure we are using the largest of those ticks.
-	lowerTick, upperTick, err = roundTickToCanonicalPriceTick(lowerTick, upperTick, priceLowerTick, priceUpperTick, pool.GetTickSpacing())
+	lowerTick, upperTick, err = roundTickToCanonicalPriceTick(lowerTick, upperTick, sqrtPriceLowerTick, sqrtPriceUpperTick, pool.GetTickSpacing())
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, 0, err
 	}
@@ -475,7 +475,7 @@ func (k Keeper) initializeInitialPositionForPool(ctx sdk.Context, pool types.Con
 	// Calculate the initial tick from the initial spot price
 	// We round down here so that the tick is rounded to
 	// the nearest possible value given the tick spacing.
-	initialTick, err := math.PriceToTickRoundDown(initialSpotPrice, pool.GetTickSpacing())
+	initialTick, err := math.SqrtPriceToTickRoundDownSpacing(initialCurSqrtPrice, pool.GetTickSpacing())
 	if err != nil {
 		return err
 	}

x/concentrated-liquidity/lp_test.go

@@ -1568,7 +1568,10 @@ func (s *KeeperTestSuite) TestInitializeInitialPositionForPool() {
 			amount1Desired:        sdk.NewInt(100_000_051),
 			tickSpacing:           1,
 			expectedCurrSqrtPrice: sqrt(100_000_051),
-			expectedTick:          72000001,
+			// We expect the returned tick to always be rounded down.
+			// In this case, tick 72000000 corresponds to 100_000_000,
+			// while 72000001 corresponds to 100_000_100.
+			expectedTick: 72000000,
 		},
 		"error: amount0Desired is zero": {
 			amount0Desired: sdk.ZeroInt(),

x/concentrated-liquidity/math/tick.go

@@ -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 PriceToTickRoundDown(price sdk.Dec, tickSpacing uint64) (int64, error) {
+func PriceToTickRoundDownSpacing(price sdk.Dec, tickSpacing uint64) (int64, error) {
 	tickIndex, err := PriceToTick(price)
 	if err != nil {
 		return 0, err
 	}
 
-	// Round 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
-	tickIndexModulus := tickIndex % int64(tickSpacing)
-	if tickIndexModulus < 0 {
-		tickIndexModulus += int64(tickSpacing)
+	tickIndex, err = RoundDownTickToSpacing(tickIndex, int64(tickSpacing))
+	if err != nil {
+		return 0, err
 	}
 
-	if tickIndexModulus != 0 {
-		tickIndex = tickIndex - tickIndexModulus
+	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
 	}
 
-	// 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}
+	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]
 }
 
-// CalculatePriceToTick takes in a price and returns the corresponding tick index.
-// This function does not take into consideration tick spacing.
-// NOTE: This is really returning a "Bucket index". Bucket index `b` corresponds to
-// all prices in range [TickToPrice(b), TickToPrice(b+1)).
-func CalculatePriceToTick(price sdk.Dec) (tickIndex int64) {
+func calculatePriceToTickDec(price sdk.Dec) (tickIndex sdk.Dec) {
 	if price.Equal(sdkOneDec) {
-		return 0
+		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 "completely" filled by the current spacing
-	// the latter is the truncation of the division above
-	// TODO: This should be rounding down?
-	tickIndex = geoSpacing.initialTick + ticksFilledByCurrentSpacing.SDKDec().RoundInt64()
+	// * 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()
+}
+
+// CalculatePriceToTick takes in a square root and returns the corresponding tick index.
+// 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)).
+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.
+	outOfBounds := false
+	if truncatedTick <= types.MinTick {
+		truncatedTick = types.MinTick + 1
+		outOfBounds = true
+	} else if truncatedTick >= types.MaxTick-1 {
+		truncatedTick = types.MaxTick - 2
+		outOfBounds = true
+	}
+
+	_, 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)
+	}
+
+	// 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
+}