From d26fc1ddbee7720e7bbaeaabb2f3b39c2df1a628 Mon Sep 17 00:00:00 2001 From: Sishir Giri Date: Fri, 19 May 2023 13:27:03 -0700 Subject: [PATCH] [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 --- x/concentrated-liquidity/client/cli/tx.go | 4 +- x/concentrated-liquidity/swaps.go | 13 +- x/concentrated-liquidity/types/errors.go | 16 +++ x/poolmanager/export_test.go | 17 +++ x/poolmanager/router.go | 42 ++++--- x/poolmanager/router_test.go | 140 ++++++++++++++++++++++ 6 files changed, 208 insertions(+), 24 deletions(-) diff --git a/x/concentrated-liquidity/client/cli/tx.go b/x/concentrated-liquidity/client/cli/tx.go index 8d432133301..d658d4a48d1 100644 --- a/x/concentrated-liquidity/client/cli/tx.go +++ b/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{} } diff --git a/x/concentrated-liquidity/swaps.go b/x/concentrated-liquidity/swaps.go index 1d702403ac8..0b1c3e663a1 100644 --- a/x/concentrated-liquidity/swaps.go +++ b/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) diff --git a/x/concentrated-liquidity/types/errors.go b/x/concentrated-liquidity/types/errors.go index 2a450cb5e0d..90e2d3dbfcf 100644 --- a/x/concentrated-liquidity/types/errors.go +++ b/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) +} diff --git a/x/poolmanager/export_test.go b/x/poolmanager/export_test.go index a88f76cfb0a..2aa856a9f79 100644 --- a/x/poolmanager/export_test.go +++ b/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) +} diff --git a/x/poolmanager/router.go b/x/poolmanager/router.go index c4987e4856f..baf882033ee 100644 --- a/x/poolmanager/router.go +++ b/x/poolmanager/router.go @@ -282,10 +282,12 @@ 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, @@ -293,6 +295,7 @@ func (k Keeper) RouteExactAmountOut(ctx sdk.Context, 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 { diff --git a/x/poolmanager/router_test.go b/x/poolmanager/router_test.go index 0a1d6b6586f..7955fa6d96b 100644 --- a/x/poolmanager/router_test.go +++ b/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) + } + }) + } +}