refactor/fix: swap step iteration limit (osmosis-labs#5647)

* repro infinite loop in swap logic * refactor/fix: swap step iteration limit * Update x/concentrated-liquidity/swaps_test.go Co-authored-by: Adam Tucker <[email protected]> * swap limit to 100 * clearer comments * Update x/concentrated-liquidity/swaps.go Co-authored-by: alpo <[email protected]> * Update x/concentrated-liquidity/types/errors.go Co-authored-by: alpo <[email protected]> --------- Co-authored-by: alpo <[email protected]> Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: alpo <[email protected]>
entrypoint-zone · Jun 30, 2023 · a593f6a · a593f6a
1 parent 0198e9d
commit a593f6a
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 8 deletions.
diff --git a/x/concentrated-liquidity/swaps.go b/x/concentrated-liquidity/swaps.go
@@ -60,6 +60,14 @@ type SwapState struct {
 	swapStrategy swapstrategy.SwapStrategy
 }
 
+// swapNoProgressLimit is the maximum number of iterations that can be performed
+// without progressing the swap state. If this limit is reached, the swap is
+// considered to have failed.
+// Note, the value is chosen arbitrarily.
+// From tests, there should be no reason for a swap to make more than 2 iterations without
+// progress. However, we leave a buffer of 1_000 to account for any unforeseen edge cases.
+const swapNoProgressLimit = 100
+
 func newSwapState(specifiedAmount sdk.Int, p types.ConcentratedPoolExtension, strategy swapstrategy.SwapStrategy) SwapState {
 	return SwapState{
 		amountSpecifiedRemaining:                 specifiedAmount.ToDec(),
@@ -314,6 +322,7 @@ func (k Keeper) computeOutAmtGivenIn(
 	defer nextInitTickIter.Close()
 
 	// Iterate and update swapState until we swap all tokenIn or we reach the specific sqrtPriceLimit
+	swapNoProgressIterationCount := 0
 	// TODO: for now, we check if amountSpecifiedRemaining is GT 0.0000001. This is because there are times when the remaining
 	// amount may be extremely small, and that small amount cannot generate and amountIn/amountOut and we are therefore left
 	// in an infinite loop.
@@ -383,6 +392,15 @@ func (k Keeper) computeOutAmtGivenIn(
 			}
 			swapState.tick = newTick
 		}
+
+		// If nothing was consumed from swapState.amountSpecifiedRemaining, we increment the swapNoProgressIterationCount.
+		// See definition of swapNoProgressLimit for more details.
+		if amountIn.IsZero() {
+			if swapNoProgressIterationCount >= swapNoProgressLimit {
+				return sdk.Coin{}, sdk.Coin{}, PoolUpdates{}, sdk.Dec{}, types.SwapNoProgressError{PoolId: poolId, UserProvidedCoin: tokenInMin}
+			}
+			swapNoProgressIterationCount++
+		}
 	}
 
 	// Add spread reward growth per share to the pool-global spread reward accumulator.
@@ -442,8 +460,10 @@ func (k Keeper) computeInAmtGivenOut(
 		return sdk.Coin{}, sdk.Coin{}, PoolUpdates{}, sdk.Dec{}, err
 	}
 
-	// TODO: This should be GT 0 but some instances have very small remainder
-	// need to look into fixing this
+	swapNoProgressIterationCount := 0
+	// TODO: for now, we check if amountSpecifiedRemaining is GT 10^-18. This is because there are times when the remaining
+	// amount may be extremely small, and that small amount cannot generate and amountIn/amountOut and we are therefore left
+	// in an infinite loop.
 	for swapState.amountSpecifiedRemaining.GT(smallestDec) && !swapState.sqrtPrice.Equal(sqrtPriceLimit) {
 		// log the sqrtPrice we start the iteration with
 		sqrtPriceStart := swapState.sqrtPrice
@@ -506,6 +526,15 @@ func (k Keeper) computeInAmtGivenOut(
 				return sdk.Coin{}, sdk.Coin{}, PoolUpdates{}, sdk.Dec{}, err
 			}
 		}
+
+		// If nothing was consumed from swapState.amountSpecifiedRemaining, we increment the swapNoProgressIterationCount.
+		// See definition of swapNoProgressLimit for more details.
+		if amountOut.IsZero() {
+			if swapNoProgressIterationCount >= swapNoProgressLimit {
+				return sdk.Coin{}, sdk.Coin{}, PoolUpdates{}, sdk.Dec{}, types.SwapNoProgressError{PoolId: poolId, UserProvidedCoin: desiredTokenOut}
+			}
+			swapNoProgressIterationCount++
+		}
 	}
 
 	// Add spread reward growth per share to the pool-global spread reward accumulator.

diff --git a/x/concentrated-liquidity/swaps_test.go b/x/concentrated-liquidity/swaps_test.go
@@ -3058,8 +3058,9 @@ func (s *KeeperTestSuite) TestFunctionalSwaps() {
 	s.Require().Equal(0, multiplicativeTolerance.Compare(expectedTokenOut, totalTokenOut.Amount))
 }
 
-// TestInfiniteSwapLoop demonstrates a case where an infinite loop can be triggered in swap logic.
-func (s *KeeperTestSuite) TestInfiniteSwapLoop() {
+// TestInfiniteSwapLoop_OutGivenIn demonstrates a case where an infinite loop can be triggered in swap logic if no
+// swap limit and other constraints are applied.
+func (s *KeeperTestSuite) TestInfiniteSwapLoop_OutGivenIn() {
 	s.SetupTest()
 	pool := s.PrepareConcentratedPool()
 
@@ -3077,10 +3078,9 @@ func (s *KeeperTestSuite) TestInfiniteSwapLoop() {
 	swapUSDCFunded := sdk.NewCoin(USDC, sdk.Int(sdk.MustNewDecFromStr("10000")))
 	s.FundAcc(swapAddress, sdk.NewCoins(swapEthFunded, swapUSDCFunded))
 	_, tokenOut, _, err := s.clk.SwapInAmtGivenOut(s.Ctx, swapAddress, pool, sdk.NewCoin(USDC, sdk.NewInt(10000)), ETH, pool.GetSpreadFactor(s.Ctx), sdk.ZeroDec())
-	fmt.Println("Token swapped out: ", tokenOut)
 
 	// Swap back in the amount that was swapped out to test the inverse relationship
-	// This line is commented out as it triggers an infinite loop.
-	// _, _, _, err = s.clk.SwapOutAmtGivenIn(s.Ctx, swapAddress, pool, tokenOut, ETH, pool.GetSpreadFactor(s.Ctx), sdk.ZeroDec())
-	s.Require().NoError(err)
+	_, _, _, err = s.clk.SwapOutAmtGivenIn(s.Ctx, swapAddress, pool, tokenOut, ETH, pool.GetSpreadFactor(s.Ctx), sdk.ZeroDec())
+	s.Require().Error(err)
+	s.Require().ErrorIs(err, types.SwapNoProgressError{PoolId: pool.GetId(), UserProvidedCoin: tokenOut})
 }
diff --git a/x/concentrated-liquidity/types/errors.go b/x/concentrated-liquidity/types/errors.go
@@ -840,3 +840,12 @@ type TickToSqrtPriceConversionError struct {
 func (e TickToSqrtPriceConversionError) Error() string {
 	return fmt.Sprintf("could not convert next tick  to nextSqrtPrice (%v)", e.NextTick)
 }
+
+type SwapNoProgressError struct {
+	PoolId           uint64
+	UserProvidedCoin sdk.Coin
+}
+
+func (e SwapNoProgressError) Error() string {
+	return fmt.Sprintf("ran out of iterations during swap. Possibly entered an infinite loop. Pool id (%d), user provided coin (%s)", e.PoolId, e.UserProvidedCoin)
+}