From 6235106bc129078f6afa4933b26158c492d49a7b Mon Sep 17 00:00:00 2001 From: alpo Date: Thu, 23 Jun 2022 01:59:51 -0500 Subject: [PATCH 1/9] add many clarifying comments and docs for logic that was confusing during audit --- x/gamm/keeper/genesis.go | 4 +++- x/gamm/keeper/multihop.go | 21 +++++++++++++++++++- x/gamm/keeper/pool.go | 2 +- x/gamm/keeper/swap.go | 4 ++++ x/gamm/pool-models/balancer/balancer_pool.go | 2 +- x/tokenfactory/keeper/admins_test.go | 8 ++------ x/tokenfactory/keeper/createdenom_test.go | 1 - x/tokenfactory/keeper/keeper_test.go | 4 ++-- 8 files changed, 33 insertions(+), 13 deletions(-) diff --git a/x/gamm/keeper/genesis.go b/x/gamm/keeper/genesis.go index eaac980d3ee..1a9040ab1a7 100644 --- a/x/gamm/keeper/genesis.go +++ b/x/gamm/keeper/genesis.go @@ -8,11 +8,13 @@ import ( ) // InitGenesis initializes the capability module's state from a provided genesis -// state. +// state, which includes the current live pools, global pool parameters (e.g. pool creation fee), next pool number etc. func (k Keeper) InitGenesis(ctx sdk.Context, genState types.GenesisState, unpacker codectypes.AnyUnpacker) { k.SetParams(ctx, genState.Params) k.SetNextPoolNumber(ctx, genState.NextPoolNumber) + // Sums up the liquidity in all genesis state pools to find the total liquidity across all pools + // Also adds each genesis state pool to the capability module's state liquidity := sdk.Coins{} for _, any := range genState.Pools { var pool types.PoolI diff --git a/x/gamm/keeper/multihop.go b/x/gamm/keeper/multihop.go index fa67fc2bb00..353442d7b76 100644 --- a/x/gamm/keeper/multihop.go +++ b/x/gamm/keeper/multihop.go @@ -17,15 +17,20 @@ func (k Keeper) MultihopSwapExactAmountIn( tokenOutMinAmount sdk.Int, ) (tokenOutAmount sdk.Int, err error) { for i, route := range routes { + // To prevent the multihop swap from being interrupted prematurely, we keep + // the minimum expected output at a very low number until the last pool _outMinAmount := sdk.NewInt(1) if len(routes)-1 == i { _outMinAmount = tokenOutMinAmount } + // Execute the expected swap on the current routed pool tokenOutAmount, err = k.SwapExactAmountIn(ctx, sender, route.PoolId, tokenIn, route.TokenOutDenom, _outMinAmount) if err != nil { return sdk.Int{}, err } + + // Chain output of current pool as the input for the next routed pool tokenIn = sdk.NewCoin(route.TokenOutDenom, tokenOutAmount) } return @@ -42,6 +47,7 @@ func (k Keeper) MultihopSwapExactAmountOut( tokenInMaxAmount sdk.Int, tokenOut sdk.Coin, ) (tokenInAmount sdk.Int, err error) { + // Determine what the estimated input would be for each pool along the multihop route insExpected, err := k.createMultihopExpectedSwapOuts(ctx, routes, tokenOut) if err != nil { return sdk.Int{}, err @@ -49,17 +55,26 @@ func (k Keeper) MultihopSwapExactAmountOut( 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, route := range routes { _tokenOut := tokenOut + + // If there is one pool left in the route, set the expected output of the current swap to the estimated input of the final pool if i != len(routes)-1 { _tokenOut = sdk.NewCoin(routes[i+1].TokenInDenom, insExpected[i+1]) } + // Execute the expected swap on the current routed pool _tokenInAmount, err := k.SwapExactAmountOut(ctx, sender, route.PoolId, route.TokenInDenom, insExpected[i], _tokenOut) if err != nil { return sdk.Int{}, err } + // Sets the final amount of tokens that need to be input into the first pool. Even though this is the final return value for the + // whole method and will not change after the first iteration, we still iterate through the rest of the pools to execute their respective + // swaps. if i == 0 { tokenInAmount = _tokenInAmount } @@ -68,7 +83,11 @@ func (k Keeper) MultihopSwapExactAmountOut( return tokenInAmount, nil } -// TODO: Document this function. +// createMultihopExpectedSwapOuts defines the output denom and output amount for the last pool in +// the route of pools the caller is intending to hop through in a fixed-output multihop tx. It estimates the input +// amount for this last pool and then chains that input as the output of the previous pool in the route, repeating +// until the first pool is reached. It returns an array of inputs, each of which correspond to a pool in the +// route of pools for the original multihop transaction. func (k Keeper) createMultihopExpectedSwapOuts(ctx sdk.Context, routes []types.SwapAmountOutRoute, tokenOut sdk.Coin) ([]sdk.Int, error) { insExpected := make([]sdk.Int, len(routes)) for i := len(routes) - 1; i >= 0; i-- { diff --git a/x/gamm/keeper/pool.go b/x/gamm/keeper/pool.go index 02b5c49115a..82d53be7b1a 100644 --- a/x/gamm/keeper/pool.go +++ b/x/gamm/keeper/pool.go @@ -43,7 +43,7 @@ func (k Keeper) GetPoolAndPoke(ctx sdk.Context, poolId uint64) (types.PoolI, err return pool, nil } -// Get pool, and check if the pool is active / allowed to be swapped against +// Get pool, and check if the pool is active (allowed to be swapped against) func (k Keeper) getPoolForSwap(ctx sdk.Context, poolId uint64) (types.PoolI, error) { pool, err := k.GetPoolAndPoke(ctx, poolId) if err != nil { diff --git a/x/gamm/keeper/swap.go b/x/gamm/keeper/swap.go index 49264c99c9b..ec3f6f71acb 100644 --- a/x/gamm/keeper/swap.go +++ b/x/gamm/keeper/swap.go @@ -49,6 +49,8 @@ func (k Keeper) swapExactAmountIn( } tokensIn := sdk.Coins{tokenIn} + // Executes the swap in the pool and stores the output. Updates pool assets but + // does not actually transfer any tokens to or from the pool. tokenOutCoin, err := pool.SwapOutAmtGivenIn(ctx, tokensIn, tokenOutDenom, swapFee) if err != nil { return sdk.Int{}, err @@ -64,6 +66,8 @@ func (k Keeper) swapExactAmountIn( return sdk.Int{}, sdkerrors.Wrapf(types.ErrLimitMinAmount, "%s token is lesser than min amount", tokenOutDenom) } + // Settles balances between the tx sender and the pool to match the swap that was executed earlier. + // Also emits swap event and updates related liquidity metrics if err := k.updatePoolForSwap(ctx, pool, sender, tokenIn, tokenOutCoin); err != nil { return sdk.Int{}, err } diff --git a/x/gamm/pool-models/balancer/balancer_pool.go b/x/gamm/pool-models/balancer/balancer_pool.go index c773533b665..9735d976086 100644 --- a/x/gamm/pool-models/balancer/balancer_pool.go +++ b/x/gamm/pool-models/balancer/balancer_pool.go @@ -388,7 +388,7 @@ func (pa *Pool) updateAllWeights(newWeights []PoolAsset) { } // PokePool checks to see if the pool's token weights need to be updated, and -// if so, does so. +// if so, does so. Currently doesn't do anything outside out LBPs. func (pa *Pool) PokePool(blockTime time.Time) { // check if pool weights didn't change poolWeightsChanging := pa.PoolParams.SmoothWeightChangeParams != nil diff --git a/x/tokenfactory/keeper/admins_test.go b/x/tokenfactory/keeper/admins_test.go index 944934b3216..25689c07887 100644 --- a/x/tokenfactory/keeper/admins_test.go +++ b/x/tokenfactory/keeper/admins_test.go @@ -71,9 +71,7 @@ func (suite *KeeperTestSuite) TestAdminMsgs() { // * Only the admin of a denom can mint tokens for it // * The admin of a denom can mint tokens for it func (suite *KeeperTestSuite) TestMintDenom() { - var ( - addr0bal = int64(0) - ) + addr0bal := int64(0) // Create a denom suite.CreateDefaultDenom() @@ -122,9 +120,7 @@ func (suite *KeeperTestSuite) TestMintDenom() { } func (suite *KeeperTestSuite) TestBurnDenom() { - var ( - addr0bal = int64(0) - ) + addr0bal := int64(0) // Create a denom. suite.CreateDefaultDenom() diff --git a/x/tokenfactory/keeper/createdenom_test.go b/x/tokenfactory/keeper/createdenom_test.go index e7809364012..77696ed7afb 100644 --- a/x/tokenfactory/keeper/createdenom_test.go +++ b/x/tokenfactory/keeper/createdenom_test.go @@ -74,7 +74,6 @@ func (suite *KeeperTestSuite) TestCreateDenom() { setup: func() { _, err := suite.msgServer.CreateDenom(sdk.WrapSDKContext(suite.Ctx), types.NewMsgCreateDenom(suite.TestAccs[0].String(), "bitcoin")) suite.Require().NoError(err) - }, subdenom: "bitcoin", valid: false, diff --git a/x/tokenfactory/keeper/keeper_test.go b/x/tokenfactory/keeper/keeper_test.go index 1737f3b5ef6..587fa7d030e 100644 --- a/x/tokenfactory/keeper/keeper_test.go +++ b/x/tokenfactory/keeper/keeper_test.go @@ -14,8 +14,8 @@ import ( type KeeperTestSuite struct { apptesting.KeeperTestHelper - queryClient types.QueryClient - msgServer types.MsgServer + queryClient types.QueryClient + msgServer types.MsgServer // defaultDenom is on the suite, as it depends on the creator test address. defaultDenom string } From cd728df6df7723d59d9b94bf179a9d1a1417c7ee Mon Sep 17 00:00:00 2001 From: alpo Date: Mon, 27 Jun 2022 09:44:17 -0500 Subject: [PATCH 2/9] update comments for clarity --- x/gamm/keeper/pool_service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/gamm/keeper/pool_service.go b/x/gamm/keeper/pool_service.go index 5247868a76e..f1d38ef18e2 100644 --- a/x/gamm/keeper/pool_service.go +++ b/x/gamm/keeper/pool_service.go @@ -220,7 +220,7 @@ func (k Keeper) JoinPoolNoSwap( return err } -// getMaximalNoSwapLPAmount returns the coins(lp liquidity) needed to get the specified amount of share of the pool. +// getMaximalNoSwapLPAmount returns the coins(lp liquidity) needed to get the specified amount of shares in the pool. // Steps to getting the needed lp liquidity coins needed for the share of the pools are // 1. calculate how much percent of the pool does given share account for(# of input shares / # of current total shares) // 2. since we know how much % of the pool we want, iterate through all pool liquidity to calculate how much coins we need for From da1f893b7eb4263ff6571d1ceca1f54ffc094fb0 Mon Sep 17 00:00:00 2001 From: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> Date: Thu, 30 Jun 2022 09:35:56 -0500 Subject: [PATCH 3/9] Update x/gamm/keeper/multihop.go Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com> --- x/gamm/keeper/multihop.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/gamm/keeper/multihop.go b/x/gamm/keeper/multihop.go index 353442d7b76..6c3f021ab90 100644 --- a/x/gamm/keeper/multihop.go +++ b/x/gamm/keeper/multihop.go @@ -86,7 +86,7 @@ func (k Keeper) MultihopSwapExactAmountOut( // createMultihopExpectedSwapOuts defines the output denom and output amount for the last pool in // the route of pools the caller is intending to hop through in a fixed-output multihop tx. It estimates the input // amount for this last pool and then chains that input as the output of the previous pool in the route, repeating -// until the first pool is reached. It returns an array of inputs, each of which correspond to a pool in the +// until the first pool is reached. It returns an array of inputs, each of which correspond to a pool ID in the // route of pools for the original multihop transaction. func (k Keeper) createMultihopExpectedSwapOuts(ctx sdk.Context, routes []types.SwapAmountOutRoute, tokenOut sdk.Coin) ([]sdk.Int, error) { insExpected := make([]sdk.Int, len(routes)) From 49ebe52728d9ce59d21755be98ee426105bce653 Mon Sep 17 00:00:00 2001 From: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> Date: Thu, 30 Jun 2022 09:36:46 -0500 Subject: [PATCH 4/9] Update x/gamm/keeper/genesis.go Co-authored-by: Aleksandr Bezobchuk --- x/gamm/keeper/genesis.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/gamm/keeper/genesis.go b/x/gamm/keeper/genesis.go index 1a9040ab1a7..7e02428c0fc 100644 --- a/x/gamm/keeper/genesis.go +++ b/x/gamm/keeper/genesis.go @@ -7,7 +7,7 @@ import ( "github.com/osmosis-labs/osmosis/v7/x/gamm/types" ) -// InitGenesis initializes the capability module's state from a provided genesis +// InitGenesis initializes the x/gamm module's state from a provided genesis // state, which includes the current live pools, global pool parameters (e.g. pool creation fee), next pool number etc. func (k Keeper) InitGenesis(ctx sdk.Context, genState types.GenesisState, unpacker codectypes.AnyUnpacker) { k.SetParams(ctx, genState.Params) From 9daa8a6959b6c67c07e50a8d940001d8f23c493d Mon Sep 17 00:00:00 2001 From: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> Date: Thu, 30 Jun 2022 09:37:09 -0500 Subject: [PATCH 5/9] Update x/gamm/keeper/genesis.go Co-authored-by: Aleksandr Bezobchuk --- x/gamm/keeper/genesis.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/gamm/keeper/genesis.go b/x/gamm/keeper/genesis.go index 7e02428c0fc..4b3f0a0d12e 100644 --- a/x/gamm/keeper/genesis.go +++ b/x/gamm/keeper/genesis.go @@ -13,7 +13,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState types.GenesisState, unpack k.SetParams(ctx, genState.Params) k.SetNextPoolNumber(ctx, genState.NextPoolNumber) - // Sums up the liquidity in all genesis state pools to find the total liquidity across all pools + // Sums up the liquidity in all genesis state pools to find the total liquidity across all pools. // Also adds each genesis state pool to the capability module's state liquidity := sdk.Coins{} for _, any := range genState.Pools { From 880e6efbcfb0beeeea6d5bddcd0289ec04b32305 Mon Sep 17 00:00:00 2001 From: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> Date: Thu, 30 Jun 2022 10:12:55 -0500 Subject: [PATCH 6/9] Update x/gamm/keeper/multihop.go Co-authored-by: Aleksandr Bezobchuk --- x/gamm/keeper/multihop.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x/gamm/keeper/multihop.go b/x/gamm/keeper/multihop.go index 6c3f021ab90..852cdd9cb7b 100644 --- a/x/gamm/keeper/multihop.go +++ b/x/gamm/keeper/multihop.go @@ -61,7 +61,8 @@ func (k Keeper) MultihopSwapExactAmountOut( for i, route := range routes { _tokenOut := tokenOut - // If there is one pool left in the route, set the expected output of the current swap to the estimated input of the final pool + // If there is one pool left in the route, set the expected output of the current swap + // to the estimated input of the final pool. if i != len(routes)-1 { _tokenOut = sdk.NewCoin(routes[i+1].TokenInDenom, insExpected[i+1]) } From 7f3af575871c5069da257d05ea2ce2dfc3645994 Mon Sep 17 00:00:00 2001 From: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> Date: Thu, 30 Jun 2022 10:13:29 -0500 Subject: [PATCH 7/9] Apply suggestions from code review Co-authored-by: Aleksandr Bezobchuk --- x/gamm/keeper/pool.go | 2 +- x/tokenfactory/keeper/admins_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/x/gamm/keeper/pool.go b/x/gamm/keeper/pool.go index 82d53be7b1a..357d96a293c 100644 --- a/x/gamm/keeper/pool.go +++ b/x/gamm/keeper/pool.go @@ -43,7 +43,7 @@ func (k Keeper) GetPoolAndPoke(ctx sdk.Context, poolId uint64) (types.PoolI, err return pool, nil } -// Get pool, and check if the pool is active (allowed to be swapped against) +// Get pool and check if the pool is active, i.e. allowed to be swapped against. func (k Keeper) getPoolForSwap(ctx sdk.Context, poolId uint64) (types.PoolI, error) { pool, err := k.GetPoolAndPoke(ctx, poolId) if err != nil { diff --git a/x/tokenfactory/keeper/admins_test.go b/x/tokenfactory/keeper/admins_test.go index 25689c07887..ceaaa794e20 100644 --- a/x/tokenfactory/keeper/admins_test.go +++ b/x/tokenfactory/keeper/admins_test.go @@ -71,7 +71,7 @@ func (suite *KeeperTestSuite) TestAdminMsgs() { // * Only the admin of a denom can mint tokens for it // * The admin of a denom can mint tokens for it func (suite *KeeperTestSuite) TestMintDenom() { - addr0bal := int64(0) + var addr0bal int64 // Create a denom suite.CreateDefaultDenom() @@ -120,7 +120,7 @@ func (suite *KeeperTestSuite) TestMintDenom() { } func (suite *KeeperTestSuite) TestBurnDenom() { - addr0bal := int64(0) + var addr0bal int64 // Create a denom. suite.CreateDefaultDenom() From efb115165716934d6620208f102d758937b6b5cc Mon Sep 17 00:00:00 2001 From: alpo Date: Thu, 30 Jun 2022 10:15:10 -0500 Subject: [PATCH 8/9] add more clarifying comments --- x/gamm/keeper/pool_service.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/gamm/keeper/pool_service.go b/x/gamm/keeper/pool_service.go index f1d38ef18e2..d8e82c3bc55 100644 --- a/x/gamm/keeper/pool_service.go +++ b/x/gamm/keeper/pool_service.go @@ -317,6 +317,7 @@ func (k Keeper) JoinSwapShareAmountOut( } tokenIn := sdk.NewCoins(sdk.NewCoin(tokenInDenom, tokenInAmount)) + // Not using generic JoinPool because we want to guarantee exact shares out extendedPool.IncreaseLiquidity(shareOutAmount, tokenIn) err = k.applyJoinPoolStateChange(ctx, pool, sender, shareOutAmount, tokenIn) From 2bd39f821226a4324401ce53e99f8a61ba7fdabd Mon Sep 17 00:00:00 2001 From: alpo Date: Thu, 30 Jun 2022 10:16:21 -0500 Subject: [PATCH 9/9] apply suggestions from review --- x/gamm/keeper/genesis.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/gamm/keeper/genesis.go b/x/gamm/keeper/genesis.go index 4b3f0a0d12e..bdccb8a4921 100644 --- a/x/gamm/keeper/genesis.go +++ b/x/gamm/keeper/genesis.go @@ -14,7 +14,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState types.GenesisState, unpack k.SetNextPoolNumber(ctx, genState.NextPoolNumber) // Sums up the liquidity in all genesis state pools to find the total liquidity across all pools. - // Also adds each genesis state pool to the capability module's state + // Also adds each genesis state pool to the x/gamm module's state liquidity := sdk.Coins{} for _, any := range genState.Pools { var pool types.PoolI