Skip to content

Commit

Permalink
[CL]: Fix tick rounding bug and implement direct conversion from tick…
Browse files Browse the repository at this point in the history
… <> sqrt price (#5541)

Closes: #5516

> **Note to reviewer:** the only real state machine change is in `tick.go` and heavily mirrors @ValarDragon's PR here: #5522 
> The rest of the changes are related to function renames/test refactors.

## What is the purpose of the change

This PR expands on #5522 and updates all price to tick conversions to use SqrtPriceToTick instead.

## Testing and Verifying

The new function is tested in `math/tick_test.go`

The original attack vector is also converted into an edge case test in `position_test.go`

## Documentation and Release Note

  - [ ] Does this pull request introduce a new feature or user-facing behavior changes?
  - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`?

Where is the change documented? 
  - [ ] Specification (`x/{module}/README.md`)
  - [ ] Osmosis documentation site
  - [ ] Code comments?
  - [ ] N/A
  • Loading branch information
AlpinYukseloglu authored Jun 19, 2023
1 parent 72f0cfc commit 2853245
Show file tree
Hide file tree
Showing 14 changed files with 362 additions and 120 deletions.
56 changes: 56 additions & 0 deletions app/apptesting/concentrated_liquidity.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package apptesting

import (
"fmt"
"time"

sdk "github.com/cosmos/cosmos-sdk/types"

clmath "github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/math"
clmodel "github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/model"
"github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/types"

Expand Down Expand Up @@ -141,3 +143,57 @@ func (s *KeeperTestHelper) SetupConcentratedLiquidityDenomsAndPoolCreation() {
defaultParams.AuthorizedQuoteDenoms = append(defaultParams.AuthorizedQuoteDenoms, ETH, USDC, BAR, BAZ, FOO, UOSMO, STAKE)
s.App.ConcentratedLiquidityKeeper.SetParams(s.Ctx, defaultParams)
}

// 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.
// 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 (s *KeeperTestHelper) PriceToTickRoundDownSpacing(price sdk.Dec, tickSpacing uint64) (int64, error) {
tickIndex, err := s.PriceToTick(price)
if err != nil {
return 0, err
}

tickIndex, err = clmath.RoundDownTickToSpacing(tickIndex, int64(tickSpacing))
if err != nil {
return 0, err
}

return tickIndex, nil
}

// 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 [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 (s *KeeperTestHelper) CalculatePriceToTick(price sdk.Dec) (tickIndex int64) {
// TODO: Make truncate, since this defines buckets as
// [TickToPrice(b - .5), TickToPrice(b+.5))
return clmath.CalculatePriceToTickDec(price).RoundInt64()
}

// PriceToTick takes a price and returns the corresponding tick index assuming
// tick spacing of 1.
func (s *KeeperTestHelper) PriceToTick(price sdk.Dec) (int64, error) {
if price.Equal(sdk.OneDec()) {
return 0, nil
}

if price.IsNegative() {
return 0, fmt.Errorf("price must be greater than zero")
}

if price.GT(types.MaxSpotPrice) || price.LT(types.MinSpotPrice) {
return 0, types.PriceBoundError{ProvidedPrice: price, MinSpotPrice: types.MinSpotPrice, MaxSpotPrice: types.MaxSpotPrice}
}

// Determine the tick that corresponds to the price
// This does not take into account the tickSpacing
tickIndex := s.CalculatePriceToTick(price)

return tickIndex, nil
}
12 changes: 10 additions & 2 deletions tests/cl-genesis-positions/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,20 @@ func ConvertSubgraphToOsmosisGenesis(positionCreatorAddresses []sdk.AccAddress,
continue
}

lowerTickOsmosis, err := math.PriceToTickRoundDown(lowerPrice, pool.GetTickSpacing())
sqrtPriceLower, err := lowerPrice.ApproxRoot(2)
if err != nil {
panic(err)
}
lowerTickOsmosis, err := math.SqrtPriceToTickRoundDownSpacing(sqrtPriceLower, pool.GetTickSpacing())
if err != nil {
panic(err)
}

upperTickOsmosis, err := math.PriceToTickRoundDown(upperPrice, pool.GetTickSpacing())
sqrtPriceUpper, err := upperPrice.ApproxRoot(2)
if err != nil {
panic(err)
}
upperTickOsmosis, err := math.SqrtPriceToTickRoundDownSpacing(sqrtPriceUpper, pool.GetTickSpacing())
if err != nil {
panic(err)
}
Expand Down
13 changes: 9 additions & 4 deletions x/concentrated-liquidity/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,15 @@ 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)
sqrt4000 = sdk.MustNewDecFromStr("63.245553203367586640")
sqrt4994 = sdk.MustNewDecFromStr("70.668238976219012614")
sqrt4999 = sdk.MustNewDecFromStr("70.703606697254136612")
sqrt5500 = sdk.MustNewDecFromStr("74.161984870956629487")
sqrt6250 = sdk.MustNewDecFromStr("79.056941504209483300")
DefaultExponentConsecutivePositionLowerTick, _ = math.SqrtPriceToTickRoundDownSpacing(sqrt5500, DefaultTickSpacing)
DefaultExponentConsecutivePositionUpperTick, _ = math.SqrtPriceToTickRoundDownSpacing(sqrt6250, DefaultTickSpacing)
DefaultExponentOverlappingPositionLowerTick, _ = math.SqrtPriceToTickRoundDownSpacing(sqrt4000, DefaultTickSpacing)
DefaultExponentOverlappingPositionUpperTick, _ = math.SqrtPriceToTickRoundDownSpacing(sqrt4999, DefaultTickSpacing)
BAR = "bar"
FOO = "foo"
InsufficientFundsError = fmt.Errorf("insufficient funds")
Expand Down
6 changes: 3 additions & 3 deletions x/concentrated-liquidity/lp.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
5 changes: 4 additions & 1 deletion x/concentrated-liquidity/lp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1706,7 +1706,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(),
Expand Down
147 changes: 96 additions & 51 deletions x/concentrated-liquidity/math/tick.go
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"
Expand Down Expand Up @@ -91,60 +92,44 @@ func TickToPrice(tickIndex int64) (price sdk.Dec, err error) {
return price, nil
}

// PriceToTick takes a price and returns the corresponding tick index assuming
// tick spacing of 1.
func PriceToTick(price sdk.Dec) (int64, error) {
if price.Equal(sdk.OneDec()) {
return 0, 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 price.IsNegative() {
return 0, fmt.Errorf("price must be greater than zero")
if tickIndexModulus != 0 {
tickIndex = tickIndex - tickIndexModulus
}

if price.GT(types.MaxSpotPrice) || price.LT(types.MinSpotPrice) {
return 0, types.PriceBoundError{ProvidedPrice: price, MinSpotPrice: types.MinSpotPrice, MaxSpotPrice: types.MaxSpotPrice}
// 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}
}

// Determine the tick that corresponds to the price
// This does not take into account the tickSpacing
tickIndex := CalculatePriceToTick(price)

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.
// 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) {
tickIndex, err := PriceToTick(price)
// 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
}

// 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)
}

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}
tickIndex, err = RoundDownTickToSpacing(tickIndex, int64(tickSpacing))
if err != nil {
return 0, err
}

return tickIndex, nil
Expand All @@ -166,13 +151,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.
Expand Down Expand Up @@ -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 "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
}

// 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
}

_, 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)) || (outOfBounds && sqrtPrice.GT(sqrtPriceTplus2)) || sqrtPrice.LT(sqrtPriceTmin1) {
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
}
Loading

0 comments on commit 2853245

Please sign in to comment.