[CL Message Audit] MsgSwapExactAmountOut (#5179)

* progress

* more checks

* checked logic

* moved all fmterror to errors

* added test

* cleanup

* cli tested

* resolved feedbacks

* resolved feedbacks

* fixed ci

* Update x/poolmanager/router_test.go

* Update x/poolmanager/router.go

---------

Co-authored-by: Roman <roman@osmosis.team>

x/concentrated-liquidity/client/cli/tx.go

@@ -47,9 +47,9 @@ func NewCreateConcentratedPoolCmd() (*osmocli.TxCliDesc, *clmodel.MsgCreateConce
 
 func NewCreatePositionCmd() (*osmocli.TxCliDesc, *types.MsgCreatePosition) {
 	return &osmocli.TxCliDesc{
-		Use:     "create-position [pool-id] [lower-tick] [upper-tick] [coins] [token-0-min-amount] [token-1-min-amount]",
+		Use:     "create-position [pool-id] [lower-tick] [upper-tick] [tokensProvided] [token-0-min-amount] [token-1-min-amount]",
 		Short:   "create or add to existing concentrated liquidity position",
-		Example: "create-position 1 \"[-69082]\" 69082 1000000000uosmo,10000000uion 0 0 --from val --chain-id osmosis-1",
+		Example: "create-position 1 \"[-69082]\" 69082 10000uosmo,10000uion 0 0 --from val --chain-id osmosis-1",
 	}, &types.MsgCreatePosition{}
 }
 

x/concentrated-liquidity/swaps.go

@@ -114,6 +114,8 @@ func (k Keeper) SwapExactAmountIn(
 	return tokenOutAmount, nil
 }
 
+// SwapExactAmountOut allows users to specify the output token amount they want to receive from a swap and get the exact
+// input token amount they need to provide based on the current pool prices and any applicable fees.
 func (k Keeper) SwapExactAmountOut(
 	ctx sdk.Context,
 	sender sdk.AccAddress,
@@ -138,6 +140,7 @@ func (k Keeper) SwapExactAmountOut(
 	zeroForOne := tokenOut.Denom == asset1
 
 	// change priceLimit based on which direction we are swapping
+	// if zeroForOne == true, use MinSpotPrice else use MaxSpotPrice
 	priceLimit := swapstrategy.GetPriceLimit(zeroForOne)
 	tokenIn, tokenOut, _, _, _, err := k.swapInAmtGivenOut(ctx, sender, pool, tokenOut, tokenInDenom, swapFee, priceLimit)
 	if err != nil {
@@ -153,7 +156,7 @@ func (k Keeper) SwapExactAmountOut(
 	return tokenInAmount, nil
 }
 
-// SwapOutAmtGivenIn is the internal mutative method for CalcOutAmtGivenIn. Utilizing CalcOutAmtGivenIn's output, this function applies the
+// swapOutAmtGivenIn is the internal mutative method for CalcOutAmtGivenIn. Utilizing CalcOutAmtGivenIn's output, this function applies the
 // new tick, liquidity, and sqrtPrice to the respective pool
 func (k Keeper) swapOutAmtGivenIn(
 	ctx sdk.Context,
@@ -182,6 +185,8 @@ func (k Keeper) swapOutAmtGivenIn(
 	return tokenIn, tokenOut, newCurrentTick, newLiquidity, newSqrtPrice, nil
 }
 
+// swapInAmtGivenOut is the internal mutative method for calcInAmtGivenOut. Utilizing calcInAmtGivenOut's output, this function applies the
+// new tick, liquidity, and sqrtPrice to the respective pool.
 func (k *Keeper) swapInAmtGivenOut(
 	ctx sdk.Context,
 	sender sdk.AccAddress,
@@ -491,7 +496,7 @@ func (k Keeper) computeInAmtGivenOut(
 	// take provided price limit and turn this into a sqrt price limit since formulas use sqrtPrice
 	sqrtPriceLimit, err := priceLimit.ApproxSqrt()
 	if err != nil {
-		return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, sdk.Dec{}, fmt.Errorf("issue calculating square root of price limit")
+		return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, sdk.Dec{}, types.SqrtRootCalculationError{SqrtPriceLimit: sqrtPriceLimit}
 	}
 
 	// set the swap strategy
@@ -542,13 +547,13 @@ func (k Keeper) computeInAmtGivenOut(
 		// if no ticks are initialized (no users have created liquidity positions) then we return an error
 		nextTick, ok := swapStrategy.NextInitializedTick(ctx, poolId, swapState.tick)
 		if !ok {
-			return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, sdk.Dec{}, fmt.Errorf("there are no more ticks initialized to fill the swap")
+			return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, sdk.Dec{}, types.InvalidTickError{}
 		}
 
 		// utilizing the next initialized tick, we find the corresponding nextPrice (the target price)
 		_, sqrtPriceNextTick, err := math.TickToSqrtPrice(nextTick)
 		if err != nil {
-			return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, sdk.Dec{}, fmt.Errorf("could not convert next tick (%v) to nextSqrtPrice", nextTick)
+			return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, sdk.Dec{}, types.TickToSqrtPriceConversionError{NextTick: nextTick}
 		}
 
 		sqrtPriceTarget := swapStrategy.GetSqrtTargetPrice(sqrtPriceNextTick)

x/concentrated-liquidity/types/errors.go

@@ -815,3 +815,19 @@ type RanOutOfTicksForPoolError struct {
 func (e RanOutOfTicksForPoolError) Error() string {
 	return fmt.Sprintf("ran out of ticks for pool (%d) during swap", e.PoolId)
 }
+
+type SqrtRootCalculationError struct {
+	SqrtPriceLimit sdk.Dec
+}
+
+func (e SqrtRootCalculationError) Error() string {
+	return fmt.Sprintf("issue calculating square root of price limit %s", e.SqrtPriceLimit)
+}
+
+type TickToSqrtPriceConversionError struct {
+	NextTick int64
+}
+
+func (e TickToSqrtPriceConversionError) Error() string {
+	return fmt.Sprintf("could not convert next tick  to nextSqrtPrice (%v)", e.NextTick)
+}

x/poolmanager/export_test.go

@@ -45,3 +45,20 @@ func (k Keeper) ValidateCreatedPool(ctx sdk.Context, poolId uint64, pool types.P
 func (k Keeper) IsOsmoRoutedMultihop(ctx sdk.Context, route types.MultihopRoute, inDenom, outDenom string) (isRouted bool) {
 	return k.isOsmoRoutedMultihop(ctx, route, inDenom, outDenom)
 }
+
+func (k Keeper) CreateMultihopExpectedSwapOuts(
+	ctx sdk.Context,
+	route []types.SwapAmountOutRoute,
+	tokenOut sdk.Coin,
+) ([]sdk.Int, error) {
+	return k.createMultihopExpectedSwapOuts(ctx, route, tokenOut)
+}
+
+func (k Keeper) CreateOsmoMultihopExpectedSwapOuts(
+	ctx sdk.Context,
+	route []types.SwapAmountOutRoute,
+	tokenOut sdk.Coin,
+	cumulativeRouteSwapFee, sumOfSwapFees sdk.Dec,
+) ([]sdk.Int, error) {
+	return k.createOsmoMultihopExpectedSwapOuts(ctx, route, tokenOut, cumulativeRouteSwapFee, sumOfSwapFees)
+}

x/poolmanager/router.go

@@ -282,17 +282,20 @@ func (k Keeper) MultihopEstimateOutGivenExactAmountIn(
 	return tokenOutAmount, err
 }
 
-// MultihopSwapExactAmountOut defines the output denom and output amount for the last pool.
-// Calculation starts by providing the tokenOutAmount of the final pool to calculate the required tokenInAmount
-// the calculated tokenInAmount is used as defined tokenOutAmount of the previous pool, calculating in reverse order of the swap
-// Transaction succeeds if the calculated tokenInAmount of the first pool is less than the defined tokenInMaxAmount defined.
+// RouteExactAmountOut processes a swap along the given route using the swap function corresponding
+// to poolID's pool type. This function is responsible for computing the optimal output amount
+// for a given input amount when swapping tokens, taking into account the current price of the
+// tokens in the pool and any slippage.
+// Transaction succeeds if the calculated tokenInAmount of the first pool is less than the defined
+// tokenInMaxAmount defined.
 func (k Keeper) RouteExactAmountOut(ctx sdk.Context,
 	sender sdk.AccAddress,
 	route []types.SwapAmountOutRoute,
 	tokenInMaxAmount sdk.Int,
 	tokenOut sdk.Coin,
 ) (tokenInAmount sdk.Int, err error) {
 	isMultiHopRouted, routeSwapFee, sumOfSwapFees := false, sdk.Dec{}, sdk.Dec{}
+	// Ensure that provided route is not empty and has valid denom format.
 	routeStep := types.SwapAmountOutRoutes(route)
 	if err := routeStep.Validate(); err != nil {
 		return sdk.Int{}, err
@@ -305,44 +308,44 @@ func (k Keeper) RouteExactAmountOut(ctx sdk.Context,
 		}
 	}()
 
-	// in this loop, we check if:
+	// In this loop (isOsmoRoutedMultihop), we check if:
 	// - the routeStep is of length 2
 	// - routeStep 1 and routeStep 2 don't trade via the same pool
 	// - routeStep 1 contains uosmo
 	// - both routeStep 1 and routeStep 2 are incentivized pools
+	//
 	// if all of the above is true, then we collect the additive and max fee between the two pools to later calculate the following:
-	// total_swap_fee = total_swap_fee = max(swapfee1, swapfee2)
+	// total_swap_fee =  max(swapfee1, swapfee2)
 	// fee_per_pool = total_swap_fee * ((pool_fee) / (swapfee1 + swapfee2))
-	if k.isOsmoRoutedMultihop(ctx, routeStep, route[0].TokenInDenom, tokenOut.Denom) {
-		isMultiHopRouted = true
-		routeSwapFee, sumOfSwapFees, err = k.getOsmoRoutedMultihopTotalSwapFee(ctx, routeStep)
-		if err != nil {
-			return sdk.Int{}, err
-		}
-	}
+	var insExpected []sdk.Int
+	isMultiHopRouted = k.isOsmoRoutedMultihop(ctx, routeStep, route[0].TokenInDenom, tokenOut.Denom)
 
 	// Determine what the estimated input would be for each pool along the multi-hop routeStep
 	// if we determined the routeStep is an osmo multi-hop and both route are incentivized,
 	// we utilize a separate function that calculates the discounted swap fees
-	var insExpected []sdk.Int
 	if isMultiHopRouted {
+		routeSwapFee, sumOfSwapFees, err = k.getOsmoRoutedMultihopTotalSwapFee(ctx, routeStep)
+		if err != nil {
+			return sdk.Int{}, err
+		}
 		insExpected, err = k.createOsmoMultihopExpectedSwapOuts(ctx, route, tokenOut, routeSwapFee, sumOfSwapFees)
 	} else {
 		insExpected, err = k.createMultihopExpectedSwapOuts(ctx, route, tokenOut)
 	}
+
 	if err != nil {
 		return sdk.Int{}, err
 	}
 	if len(insExpected) == 0 {
 		return sdk.Int{}, nil
 	}
-
 	insExpected[0] = tokenInMaxAmount
 
 	// Iterates through each routed pool and executes their respective swaps. Note that all of the work to get the return
 	// value of this method is done when we calculate insExpected – this for loop primarily serves to execute the actual
 	// swaps on each pool.
 	for i, routeStep := range route {
+		// Get underlying pool type corresponding to the pool ID at the current routeStep.
 		swapModule, err := k.GetPoolModule(ctx, routeStep.PoolId)
 		if err != nil {
 			return sdk.Int{}, err
@@ -364,10 +367,12 @@ func (k Keeper) RouteExactAmountOut(ctx sdk.Context,
 
 		// check if pool is active, if not error
 		if !pool.IsActive(ctx) {
-			return sdk.Int{}, fmt.Errorf("pool %d is not active", pool.GetId())
+			return sdk.Int{}, types.InactivePoolError{PoolId: pool.GetId()}
 		}
 
 		swapFee := pool.GetSwapFee(ctx)
+		// If we determined the routeStep is an osmo multi-hop and both route are incentivized,
+		// we modify the swap fee accordingly.
 		if isMultiHopRouted {
 			swapFee = routeSwapFee.Mul((swapFee.Quo(sumOfSwapFees)))
 		}
@@ -577,6 +582,7 @@ func (k Keeper) AllPools(
 	return sortedPools, nil
 }
 
+// IsOsmoRoutedMultihop determines if a multi-hop swap involves OSMO, as one of the intermediary tokens.
 func (k Keeper) isOsmoRoutedMultihop(ctx sdk.Context, route types.MultihopRoute, inDenom, outDenom string) (isRouted bool) {
 	if route.Length() != 2 {
 		return false
@@ -638,7 +644,6 @@ func (k Keeper) getOsmoRoutedMultihopTotalSwapFee(ctx sdk.Context, route types.M
 // amount for this last pool and then chains that input as the output of the previous pool in the routeStep, repeating
 // until the first pool is reached. It returns an array of inputs, each of which correspond to a pool ID in the
 // routeStep of pools for the original multihop transaction.
-// TODO: test this.
 func (k Keeper) createMultihopExpectedSwapOuts(
 	ctx sdk.Context,
 	route []types.SwapAmountOutRoute,
@@ -670,7 +675,7 @@ func (k Keeper) createMultihopExpectedSwapOuts(
 	return insExpected, nil
 }
 
-// createOsmoMultihopExpectedSwapOuts does the same as createMultihopExpectedSwapOuts, however discounts the swap fee
+// createOsmoMultihopExpectedSwapOuts does the same as createMultihopExpectedSwapOuts, however discounts the swap fee.
 func (k Keeper) createOsmoMultihopExpectedSwapOuts(
 	ctx sdk.Context,
 	route []types.SwapAmountOutRoute,
@@ -704,6 +709,7 @@ func (k Keeper) createOsmoMultihopExpectedSwapOuts(
 	return insExpected, nil
 }
 
+// GetTotalPoolLiquidity gets the total liquidity for a given poolId.
 func (k Keeper) GetTotalPoolLiquidity(ctx sdk.Context, poolId uint64) (sdk.Coins, error) {
 	swapModule, err := k.GetPoolModule(ctx, poolId)
 	if err != nil {

x/poolmanager/router_test.go

@@ -2442,3 +2442,143 @@ func (s *KeeperTestSuite) TestGetOsmoRoutedMultihopTotalSwapFee() {
 		})
 	}
 }
+
+func (suite *KeeperTestSuite) TestCreateMultihopExpectedSwapOuts() {
+	tests := map[string]struct {
+		route                  []types.SwapAmountOutRoute
+		tokenOut               sdk.Coin
+		balancerPoolCoins      []sdk.Coins
+		concentratedPoolDenoms [][]string
+		poolCoins              []sdk.Coins
+		cumulativeRouteSwapFee sdk.Dec
+		sumOfSwapFees          sdk.Dec
+
+		expectedSwapIns []sdk.Int
+		expectedError   bool
+	}{
+		"happy path: one route": {
+			route: []types.SwapAmountOutRoute{
+				{
+					PoolId:       1,
+					TokenInDenom: bar,
+				},
+			},
+			poolCoins: []sdk.Coins{sdk.NewCoins(sdk.NewCoin(foo, sdk.NewInt(100)), sdk.NewCoin(bar, sdk.NewInt(100)))},
+
+			tokenOut: sdk.NewCoin(foo, sdk.NewInt(10)),
+			// expectedSwapIns = (tokenOut * (poolTokenOutBalance / poolPostSwapOutBalance)).ceil()
+			// foo token = 10 * (100 / 90) ~ 12
+			expectedSwapIns: []sdk.Int{sdk.NewInt(12)},
+		},
+		"happy path: two route": {
+			route: []types.SwapAmountOutRoute{
+				{
+					PoolId:       1,
+					TokenInDenom: foo,
+				},
+				{
+					PoolId:       2,
+					TokenInDenom: bar,
+				},
+			},
+
+			poolCoins: []sdk.Coins{
+				sdk.NewCoins(sdk.NewCoin(foo, sdk.NewInt(100)), sdk.NewCoin(bar, sdk.NewInt(100))), // pool 1.
+				sdk.NewCoins(sdk.NewCoin(bar, sdk.NewInt(100)), sdk.NewCoin(baz, sdk.NewInt(100))), // pool 2.
+			},
+			tokenOut: sdk.NewCoin(baz, sdk.NewInt(10)),
+			// expectedSwapIns = (tokenOut * (poolTokenOutBalance / poolPostSwapOutBalance)).ceil()
+			// foo token = 10 * (100 / 90) ~ 12
+			// bar token = 12 * (100 / 88) ~ 14
+			expectedSwapIns: []sdk.Int{sdk.NewInt(14), sdk.NewInt(12)},
+		},
+		"happy path: one route with swap Fee": {
+			route: []types.SwapAmountOutRoute{
+				{
+					PoolId:       1,
+					TokenInDenom: bar,
+				},
+			},
+			poolCoins:              []sdk.Coins{sdk.NewCoins(sdk.NewCoin(uosmo, sdk.NewInt(100)), sdk.NewCoin(bar, sdk.NewInt(100)))},
+			cumulativeRouteSwapFee: sdk.NewDec(100),
+			sumOfSwapFees:          sdk.NewDec(500),
+
+			tokenOut:        sdk.NewCoin(uosmo, sdk.NewInt(10)),
+			expectedSwapIns: []sdk.Int{sdk.NewInt(12)},
+		},
+		"happy path: two route with swap Fee": {
+			route: []types.SwapAmountOutRoute{
+				{
+					PoolId:       1,
+					TokenInDenom: foo,
+				},
+				{
+					PoolId:       2,
+					TokenInDenom: bar,
+				},
+			},
+
+			poolCoins: []sdk.Coins{
+				sdk.NewCoins(sdk.NewCoin(foo, sdk.NewInt(100)), sdk.NewCoin(bar, sdk.NewInt(100))),   // pool 1.
+				sdk.NewCoins(sdk.NewCoin(bar, sdk.NewInt(100)), sdk.NewCoin(uosmo, sdk.NewInt(100))), // pool 2.
+			},
+			cumulativeRouteSwapFee: sdk.NewDec(100),
+			sumOfSwapFees:          sdk.NewDec(500),
+
+			tokenOut:        sdk.NewCoin(uosmo, sdk.NewInt(10)),
+			expectedSwapIns: []sdk.Int{sdk.NewInt(14), sdk.NewInt(12)},
+		},
+		"error: Invalid Pool": {
+			route: []types.SwapAmountOutRoute{
+				{
+					PoolId:       100,
+					TokenInDenom: foo,
+				},
+			},
+			poolCoins: []sdk.Coins{
+				sdk.NewCoins(sdk.NewCoin(foo, sdk.NewInt(100)), sdk.NewCoin(bar, sdk.NewInt(100))), // pool 1.
+			},
+			tokenOut:      sdk.NewCoin(baz, sdk.NewInt(10)),
+			expectedError: true,
+		},
+		"error: calculating in given out": {
+			route: []types.SwapAmountOutRoute{
+				{
+					PoolId:       1,
+					TokenInDenom: uosmo,
+				},
+			},
+
+			poolCoins: []sdk.Coins{
+				sdk.NewCoins(sdk.NewCoin(foo, sdk.NewInt(100)), sdk.NewCoin(bar, sdk.NewInt(100))), // pool 1.
+			},
+			tokenOut:        sdk.NewCoin(baz, sdk.NewInt(10)),
+			expectedSwapIns: []sdk.Int{},
+
+			expectedError: true,
+		},
+	}
+
+	for name, tc := range tests {
+		suite.Run(name, func() {
+			suite.SetupTest()
+
+			suite.createBalancerPoolsFromCoins(tc.poolCoins)
+
+			var actualSwapOuts []sdk.Int
+			var err error
+
+			if !tc.sumOfSwapFees.IsNil() && !tc.cumulativeRouteSwapFee.IsNil() {
+				actualSwapOuts, err = suite.App.PoolManagerKeeper.CreateOsmoMultihopExpectedSwapOuts(suite.Ctx, tc.route, tc.tokenOut, tc.cumulativeRouteSwapFee, tc.sumOfSwapFees)
+			} else {
+				actualSwapOuts, err = suite.App.PoolManagerKeeper.CreateMultihopExpectedSwapOuts(suite.Ctx, tc.route, tc.tokenOut)
+			}
+			if tc.expectedError {
+				suite.Require().Error(err)
+			} else {
+				suite.Require().NoError(err)
+				suite.Require().Equal(tc.expectedSwapIns, actualSwapOuts)
+			}
+		})
+	}
+}