fix(CL): tick iterator bugs and tests

x/concentrated-liquidity/export_test.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/osmosis-labs/osmosis/osmoutils/accum"
 	"github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/model"
+	"github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/swapstrategy"
 	"github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/types"
 	poolmanagertypes "github.com/osmosis-labs/osmosis/v16/x/poolmanager/types"
 )
@@ -340,6 +341,12 @@ func (k Keeper) GetLargestSupportedUptimeDuration(ctx sdk.Context) time.Duration
 	return k.getLargestSupportedUptimeDuration(ctx)
 }
 
+func (k Keeper) SetupSwapStrategy(ctx sdk.Context, p types.ConcentratedPoolExtension,
+	spreadFactor sdk.Dec, tokenInDenom string,
+	priceLimit sdk.Dec) (strategy swapstrategy.SwapStrategy, sqrtPriceLimit sdk.Dec, err error) {
+	return k.setupSwapStrategy(ctx, p, spreadFactor, tokenInDenom, priceLimit)
+}
+
 func MoveRewardsToNewPositionAndDeleteOldAcc(ctx sdk.Context, accum accum.AccumulatorObject, oldPositionName, newPositionName string, growthOutside sdk.DecCoins) error {
 	return moveRewardsToNewPositionAndDeleteOldAcc(ctx, accum, oldPositionName, newPositionName, growthOutside)
 }

x/concentrated-liquidity/swaps.go

@@ -61,7 +61,7 @@ func newSwapState(specifiedAmount sdk.Int, p types.ConcentratedPoolExtension, st
 		amountSpecifiedRemaining: specifiedAmount.ToDec(),
 		amountCalculated:         sdk.ZeroDec(),
 		sqrtPrice:                p.GetCurrentSqrtPrice(),
-		tick:                     strategy.InitializeTickValue(p.GetCurrentTick()),
+		tick:                     p.GetCurrentTick(),
 		liquidity:                p.GetLiquidity(),
 		spreadRewardGrowthGlobal: sdk.ZeroDec(),
 	}
@@ -552,7 +552,7 @@ func (k Keeper) swapCrossTickLogic(ctx sdk.Context,
 	swapState.liquidity = newLiquidity
 
 	// Update the swapState's tick with the tick we retrieved liquidity from
-	swapState.tick = nextTick
+	swapState.tick = swapStrategy.UpdateTickAfterCrossing(nextTick)
 	return swapState, nil
 }
 

x/concentrated-liquidity/swaps_test.go

@@ -2891,3 +2891,256 @@ func (s *KeeperTestSuite) TestFunctionalSwaps() {
 	s.Require().Equal(0, multiplicativeTolerance.Compare(expectedTokenIn, totalTokenIn.Amount))
 	s.Require().Equal(0, multiplicativeTolerance.Compare(expectedTokenOut, totalTokenOut.Amount))
 }
+
+// TestSwap_ZeroForOne_TickInitialization tests that ticks are initialized correctly when performing
+// 2 zero for one swaps in a row.
+// This test helped to identify a bug with tick initialization when performing 2 zero for one swaps in a row.
+//
+// The core of the bug: if we perform a swap and cross a tick, the subsequent swap in the same direction
+// would cross the same tick again and mistakenly kick in liquidity, completely invalidating pool state.
+// The reason is that by having a zero for one tick initialization at currentTick + 1, we end up getting
+// currentTick as nextTick and cross it twice. Once during the first swap and once during the second swap.
+//
+// This test is set up to reproduce the 2 subsequent swaps zero for one swaps in a row.
+//
+// It creates 2 positions:
+// - full range
+// - narrow range where
+//   - narrow range's lower tick is 1 tick spacing belowe current tick
+//   - narrow range's upper tick is 1 tick spacing above current tick
+//
+// It does the first swap that stops exactly after crossing the narrow position's lower tick.
+// Next, it continues with the second swap that we do not expect to cross the narrow position's
+// lower tick again.
+//
+// Once the second swap is completed we validated that the liquidity in the pool corresponds to
+// full range position only. If the tick were to be crossed twice, we would have mistakenly
+// subtracted liquidity net from the lower tick of the narrow position twice, making the
+// current liquidity be zero.
+//
+// Prior to adding this test and implementing a fix, the system would have panicked with:
+// "negative coin amount: -7070961745605321174329" at the end of the second swap. This stems
+// from the invalid liquidity of zero that is incorrectly computed due to crossing the tick twice.
+func (s *KeeperTestSuite) TestSwap_ZeroForOne_TickInitialization_TickSpacing100() {
+	s.Setup()
+
+	var (
+		tickSpacing = uint64(100)
+	)
+
+	pool := s.PrepareCustomConcentratedPool(s.TestAccs[0], ETH, USDC, tickSpacing, sdk.ZeroDec())
+	poolId := pool.GetId()
+
+	// Create a full range position
+	s.FundAcc(s.TestAccs[0], DefaultCoins)
+	_, _, _, liquidityFullRange, err := s.App.ConcentratedLiquidityKeeper.CreateFullRangePosition(s.Ctx, poolId, s.TestAccs[0], DefaultCoins)
+	s.Require().NoError(err)
+
+	// Refetch pool as the first position updated its state.
+	pool, err = s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId())
+	s.Require().NoError(err)
+
+	// Create narrow range position 1 tick spacing around the current tick
+	lowerTickOne := pool.GetCurrentTick() - int64(pool.GetTickSpacing())
+	upperTickOne := pool.GetCurrentTick() + int64(pool.GetTickSpacing())
+	s.FundAcc(s.TestAccs[0], DefaultCoins)
+	_, _, _, liquidityNarrowRangeOne, _, _, err := s.App.ConcentratedLiquidityKeeper.CreatePosition(s.Ctx, pool.GetId(), s.TestAccs[0], DefaultCoins, sdk.ZeroInt(), sdk.ZeroInt(), lowerTickOne, upperTickOne)
+	s.Require().NoError(err)
+
+	// Create narrow range position 2 tick spacings away from the current.
+	lowerTickTwo := pool.GetCurrentTick() - 2*int64(pool.GetTickSpacing())
+	upperTickTwo := pool.GetCurrentTick() + 2*int64(pool.GetTickSpacing())
+	s.FundAcc(s.TestAccs[0], DefaultCoins)
+	_, _, _, liquidityNarrowRangeTwo, _, _, err := s.App.ConcentratedLiquidityKeeper.CreatePosition(s.Ctx, pool.GetId(), s.TestAccs[0], DefaultCoins, sdk.ZeroInt(), sdk.ZeroInt(), lowerTickTwo, upperTickTwo)
+	s.Require().NoError(err)
+
+	// Refetch pool as the second position updated its state.
+	pool, err = s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, poolId)
+	s.Require().NoError(err)
+
+	// As a sanity check confirm that current liquidity corresponds
+	// to the sum of liquidities of the all positions.
+	s.Require().Equal(liquidityFullRange.Add(liquidityNarrowRangeOne.Add(liquidityNarrowRangeTwo)), pool.GetLiquidity())
+
+	// Compute the sqrt price corresponding to the lower tick
+	// of the narrow range position.
+	_, lowerSqrtPrice, err := math.TickToSqrtPrice(lowerTickOne)
+	s.Require().NoError(err)
+
+	// Check that narrow range position is considered in range
+	isNarrowInRange := pool.IsCurrentTickInRange(lowerTickOne, upperTickOne)
+	s.Require().True(isNarrowInRange)
+
+	var (
+		// This is the total amount necessary to cross the lower tick of narrow position.
+		// Note it is rounded up to ensure that the tick is crossed.
+		amountZeroInCrossTick = math.CalcAmount0Delta(pool.GetLiquidity(), lowerSqrtPrice, pool.GetCurrentSqrtPrice(), true).Ceil().TruncateInt()
+		tokenZeroInCrossTick  = sdk.NewCoin(pool.GetToken0(), amountZeroInCrossTick)
+	)
+
+	// Perform the swap that should cross the lower tick of the narrow range position.
+	s.FundAcc(s.TestAccs[0], sdk.NewCoins(tokenZeroInCrossTick))
+	_, err = s.App.ConcentratedLiquidityKeeper.SwapExactAmountIn(s.Ctx, s.TestAccs[0], pool, tokenZeroInCrossTick, pool.GetToken1(), sdk.ZeroInt(), sdk.ZeroDec())
+	s.Require().NoError(err)
+
+	// Refetch pool as the swap updated its state.
+	pool, err = s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId())
+	s.Require().NoError(err)
+
+	// Check that the tick was crossed
+	// Since our bound for "is position active" si:
+	// lowerTick <= currentTick < upperTick, we must make
+	// sure that the current tick is not equal to the lower tick.
+	s.Require().Equal(lowerTickOne-1, pool.GetCurrentTick())
+
+	// Since we expect the tick to be crossed, there must only be
+	// liquidity of the full range position.
+	s.Require().Equal(liquidityFullRange.Add(liquidityNarrowRangeTwo), pool.GetLiquidity())
+
+	// Check that narrow range position is considered out of range
+	isNarrowInRange = pool.IsCurrentTickInRange(lowerTickOne, upperTickOne)
+	s.Require().False(isNarrowInRange)
+
+	// Confirm that the next tick to be returned is correct for both zero for one and one for zero directions.
+	zeroForOneSwapStrategy, _, err := s.App.ConcentratedLiquidityKeeper.SetupSwapStrategy(s.Ctx, pool, sdk.ZeroDec(), pool.GetToken0(), types.MinSqrtPrice)
+	s.Require().NoError(err)
+
+	initializedTickValue := zeroForOneSwapStrategy.InitializeTickValue(pool.GetCurrentTick())
+
+	iter := zeroForOneSwapStrategy.InitializeNextTickIterator(s.Ctx, pool.GetId(), initializedTickValue)
+	s.Require().True(iter.Valid())
+
+	nextTick, err := types.TickIndexFromBytes(iter.Key())
+	s.Require().NoError(err)
+
+	s.Require().NoError(iter.Close())
+
+	s.Require().Equal(lowerTickTwo, nextTick)
+
+	// Therefore, if we swap a small amount again, we do not expect
+	// a tick to be crossed again.
+	smallAmount := sdk.NewCoin(pool.GetToken0(), sdk.NewInt(10))
+	s.FundAcc(s.TestAccs[0], sdk.NewCoins(smallAmount))
+	_, err = s.App.ConcentratedLiquidityKeeper.SwapExactAmountIn(s.Ctx, s.TestAccs[0], pool, smallAmount, pool.GetToken1(), sdk.ZeroInt(), sdk.ZeroDec())
+	s.Require().NoError(err)
+
+	// Refetch pool as the swap updated its state.
+	pool, err = s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId())
+	s.Require().NoError(err)
+
+	// Liquidity remains the same, confirming that we did not cross the same tick twice.
+	// Otherwise, if we were to cross it twice, the liquidity would be zero.
+	s.Require().Equal(liquidityFullRange.Add(liquidityNarrowRangeTwo), pool.GetLiquidity())
+
+	// Check that narrow range position is considered out of range
+	isNarrowInRange = pool.IsCurrentTickInRange(lowerTickOne, upperTickOne)
+	s.Require().False(isNarrowInRange)
+}
+
+func (s *KeeperTestSuite) TestSwap_ZeroForOne_TickInitialization_TickSpacing1() {
+	s.Setup()
+
+	var (
+		tickSpacing = uint64(1)
+	)
+
+	pool := s.PrepareCustomConcentratedPool(s.TestAccs[0], ETH, USDC, tickSpacing, sdk.ZeroDec())
+	poolId := pool.GetId()
+
+	// Create a full range position
+	s.FundAcc(s.TestAccs[0], DefaultCoins)
+	_, _, _, liquidityFullRange, err := s.App.ConcentratedLiquidityKeeper.CreateFullRangePosition(s.Ctx, poolId, s.TestAccs[0], DefaultCoins)
+	s.Require().NoError(err)
+
+	// Refetch pool as the first position updated its state.
+	pool, err = s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId())
+	s.Require().NoError(err)
+
+	// Create narrow range position 1 tick spacing around the current tick
+	lowerTick := pool.GetCurrentTick() - int64(pool.GetTickSpacing())
+	upperTick := pool.GetCurrentTick() + int64(pool.GetTickSpacing())
+	s.FundAcc(s.TestAccs[0], DefaultCoins)
+	_, _, _, liquidityNarrowRange, _, _, err := s.App.ConcentratedLiquidityKeeper.CreatePosition(s.Ctx, pool.GetId(), s.TestAccs[0], DefaultCoins, sdk.ZeroInt(), sdk.ZeroInt(), lowerTick, upperTick)
+	s.Require().NoError(err)
+
+	// Refetch pool as the second position updated its state.
+	pool, err = s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, poolId)
+	s.Require().NoError(err)
+
+	// As a sanity check confirm that current liquidity corresponds
+	// to the sum of liquidities of the two positions.
+	s.Require().Equal(liquidityFullRange.Add(liquidityNarrowRange), pool.GetLiquidity())
+
+	// Compute the sqrt price corresponding to the lower tick
+	// of the narrow range position.
+	_, lowerSqrtPrice, err := math.TickToSqrtPrice(lowerTick)
+	s.Require().NoError(err)
+
+	// Check that narrow range position is considered in range
+	isNarrowInRange := pool.IsCurrentTickInRange(lowerTick, upperTick)
+	s.Require().True(isNarrowInRange)
+
+	var (
+		// This is the total amount necessary to cross the lower tick of narrow position.
+		// Note it is rounded up to ensure that the tick is crossed.
+		amountZeroInCrossTick = math.CalcAmount0Delta(pool.GetLiquidity(), lowerSqrtPrice, pool.GetCurrentSqrtPrice(), true).Ceil().TruncateInt()
+		tokenZeroInCrossTick  = sdk.NewCoin(pool.GetToken0(), amountZeroInCrossTick)
+	)
+
+	// Perform the swap that should cross the lower tick of the narrow range position.
+	s.FundAcc(s.TestAccs[0], sdk.NewCoins(tokenZeroInCrossTick))
+	_, err = s.App.ConcentratedLiquidityKeeper.SwapExactAmountIn(s.Ctx, s.TestAccs[0], pool, tokenZeroInCrossTick, pool.GetToken1(), sdk.ZeroInt(), sdk.ZeroDec())
+	s.Require().NoError(err)
+
+	// Refetch pool as the swap updated its state.
+	pool, err = s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId())
+	s.Require().NoError(err)
+
+	// Check that the tick was crossed
+	// Since our bound for "is position active" si:
+	// lowerTick <= currentTick < upperTick, we must make
+	// sure that the current tick is not equal to the lower tick.
+	s.Require().Equal(lowerTick-1, pool.GetCurrentTick())
+
+	// Since we expect the tick to be crossed, there must only be
+	// liquidity of the full range position.
+	s.Require().Equal(liquidityFullRange, pool.GetLiquidity())
+
+	// Check that narrow range position is considered out of range
+	isNarrowInRange = pool.IsCurrentTickInRange(lowerTick, upperTick)
+	s.Require().False(isNarrowInRange)
+
+	// Confirm that the next tick to be returned is correct for both zero for one and one for zero directions.
+	zeroForOneSwapStrategy, _, err := s.App.ConcentratedLiquidityKeeper.SetupSwapStrategy(s.Ctx, pool, sdk.ZeroDec(), pool.GetToken0(), types.MinSqrtPrice)
+	s.Require().NoError(err)
+
+	initializedTickValue := zeroForOneSwapStrategy.InitializeTickValue(pool.GetCurrentTick())
+
+	iter := zeroForOneSwapStrategy.InitializeNextTickIterator(s.Ctx, pool.GetId(), initializedTickValue)
+	defer iter.Close()
+	s.Require().True(iter.Valid())
+
+	nextTick, err := types.TickIndexFromBytes(iter.Key())
+	s.Require().NoError(err)
+
+	s.Require().Equal(types.MinTick, nextTick)
+
+	// Therefore, if we swap a small amount again, we do not expect
+	// a tick to be crossed again.
+	smallAmount := sdk.NewCoin(pool.GetToken0(), sdk.NewInt(10))
+	s.FundAcc(s.TestAccs[0], sdk.NewCoins(smallAmount))
+	_, err = s.App.ConcentratedLiquidityKeeper.SwapExactAmountIn(s.Ctx, s.TestAccs[0], pool, smallAmount, pool.GetToken1(), sdk.ZeroInt(), sdk.ZeroDec())
+	s.Require().NoError(err)
+
+	// Refetch pool as the swap updated its state.
+	pool, err = s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId())
+	s.Require().NoError(err)
+
+	// Liquidity remains the same, confirming that we did not cross the same tick twice.
+	// Otherwise, if we were to cross it twice, the liquidity would be zero.
+	s.Require().Equal(liquidityFullRange, pool.GetLiquidity())
+
+	// Check that narrow range position is considered out of range
+	isNarrowInRange = pool.IsCurrentTickInRange(lowerTick, upperTick)
+	s.Require().False(isNarrowInRange)
+}

x/concentrated-liquidity/swapstrategy/one_for_zero.go

@@ -152,9 +152,9 @@ func (s oneForZeroStrategy) ComputeSwapStepInGivenOut(sqrtPriceCurrent, sqrtPric
 //
 // oneForZeroStrategy assumes moving to the right of the current square root price.
 // As a result, we use forward iterator to seek to the next tick index relative to the currentTickIndex.
-// Since start key of the forward iterator is inclusive, we search directly from the tickIndex
+// Since start key of the forward iterator is inclusive, we search directly from the currentTickIndex
 // forwards in increasing lexicographic order until a tick greater than currentTickIndex is found.
-// Returns an invalid iterator if tickIndex is not in the store.
+// Returns an invalid iterator if no tick greater than currentTickIndex is found in the store.
 // Panics if fails to parse tick index from bytes.
 // The caller is responsible for closing the iterator on success.
 func (s oneForZeroStrategy) InitializeNextTickIterator(ctx sdk.Context, poolId uint64, currentTickIndex int64) dbm.Iterator {
@@ -209,6 +209,17 @@ func (s oneForZeroStrategy) SetLiquidityDeltaSign(deltaLiquidity sdk.Dec) sdk.De
 	return deltaLiquidity
 }
 
+// UpdateTickAfterCrossing updates the next tick after crossing
+// to satisfy our "position in-range" invariant which is:
+// lower tick <= current tick < upper tick.
+// When crossing a tick in one for zero direction, we move
+// right on the range. As a result, we end up crossing the upper tick
+// that is exclusive. Therefore, we leave the next tick as is since
+// it is already excluded from the current range.
+func (s oneForZeroStrategy) UpdateTickAfterCrossing(nextTick int64) int64 {
+	return nextTick
+}
+
 // ValidateSqrtPrice validates the given square root price
 // relative to the current square root price on one side of the bound
 // and the min/max sqrt price on the other side.

x/concentrated-liquidity/swapstrategy/swap_strategy.go

@@ -67,6 +67,18 @@ type SwapStrategy interface {
 	// going up. As a result, the sign depends on the direction we are moving.
 	// See oneForZeroStrategy or zeroForOneStrategy for implementation details.
 	SetLiquidityDeltaSign(liquidityDelta sdk.Dec) sdk.Dec
+	// UpdateTickAfterCrossing updates the next tick after crossing
+	// to satisfy our "position in-range" invariant which is:
+	// lower tick <= current tick < upper tick
+	// When crossing a tick in zero for one direction, we move
+	// left on the range. As a result, we end up crossing the lower tick
+	// that is inclusive. Therefore, we must decrease the next tick
+	// by 1 additional unit so that it falls under the current range.
+	// When crossing a tick in one for zero direction, we move
+	// right on the range. As a result, we end up crossing the upper tick
+	// that is exclusive. Therefore, we leave the next tick as is since
+	// it is already excluded from the current range.
+	UpdateTickAfterCrossing(nextTick int64) (updatedNextTick int64)
 	// ValidateSqrtPrice validates the given square root price
 	// relative to the current square root price on one side of the bound
 	// and the min/max sqrt price on the other side.

x/concentrated-liquidity/swapstrategy/swap_strategy_test.go

@@ -78,8 +78,7 @@ func (suite *StrategyTestSuite) runTickIteratorTest(strategy swapstrategy.SwapSt
 	currentTick := pool.GetCurrentTick()
 	suite.Require().Equal(int64(0), currentTick)
 
-	tickIndex := strategy.InitializeTickValue(currentTick)
-	iter := strategy.InitializeNextTickIterator(suite.Ctx, defaultPoolId, tickIndex)
+	iter := strategy.InitializeNextTickIterator(suite.Ctx, defaultPoolId, currentTick)
 	defer iter.Close()
 
 	suite.Require().Equal(tc.expectIsValid, iter.Valid())