From df8d3f26867e59b39a7c8cdeb14bf538d1cc59f6 Mon Sep 17 00:00:00 2001 From: Roman Date: Thu, 24 Aug 2023 12:34:11 +0200 Subject: [PATCH 1/6] fix balancer liquidity breaking incentives --- app/upgrades/v18/upgrades_test.go | 36 +++++++++++++++----- x/concentrated-liquidity/incentives.go | 46 ++++++++++++++------------ 2 files changed, 51 insertions(+), 31 deletions(-) diff --git a/app/upgrades/v18/upgrades_test.go b/app/upgrades/v18/upgrades_test.go index ad393b02314..984ca3135d0 100644 --- a/app/upgrades/v18/upgrades_test.go +++ b/app/upgrades/v18/upgrades_test.go @@ -44,34 +44,52 @@ func assertEqual(suite *UpgradeTestSuite, pre, post interface{}) { suite.Require().Equal(pre, post) } -func (suite *UpgradeTestSuite) TestUpgrade() { +func (s *UpgradeTestSuite) TestUpgrade() { // set up pools first to match v17 state(including linked cl pools) - suite.setupPoolsToMainnetState() + s.setupPoolsToMainnetState() // corrupt state to match mainnet state - suite.setupCorruptedState() + s.setupCorruptedState() // with the corrupted state, distribution used to panic in the `AfterEpochEnd` hook, // specifically from the one from incentives keeper. // This method ensures that with the corrupted state, we have the same state where // distribution would fail. - suite.ensurePreUpgradeDistributionPanics() + s.ensurePreUpgradeDistributionPanics() // upgrade software - suite.imitateUpgrade() - suite.App.BeginBlocker(suite.Ctx, abci.RequestBeginBlock{}) - suite.Ctx = suite.Ctx.WithBlockTime(suite.Ctx.BlockTime().Add(time.Hour * 24)) + s.imitateUpgrade() + s.App.BeginBlocker(s.Ctx, abci.RequestBeginBlock{}) + s.Ctx = s.Ctx.WithBlockTime(s.Ctx.BlockTime().Add(time.Hour * 24)) // after the accum values have been resetted correctly after upgrade, we expect the accumulator store to be initialized with the correct value, // which in our test case would be 10000(the amount that was locked) - valueAfterClear := suite.App.LockupKeeper.GetPeriodLocksAccumulation(suite.Ctx, lockuptypes.QueryCondition{ + valueAfterClear := s.App.LockupKeeper.GetPeriodLocksAccumulation(s.Ctx, lockuptypes.QueryCondition{ LockQueryType: lockuptypes.ByDuration, Denom: "gamm/pool/3", Duration: time.Hour * 24 * 14, }) valueAfterClear.Equal(sdk.NewInt(shareStaysLocked)) - suite.ensurePostUpgradeDistributionWorks() + s.ensurePostUpgradeDistributionWorks() + + // Check that can LP and swap into pool 3 with no usses + + migrationInfo, err := s.App.GAMMKeeper.GetAllMigrationInfo(s.Ctx) + s.Require().NoError(err) + + link := migrationInfo.BalancerToConcentratedPoolLinks[0] + s.Require().Equal(uint64(3), link.BalancerPoolId) + + clPoolId := link.ClPoolId + + pool, err := s.App.ConcentratedLiquidityKeeper.GetConcentratedPoolById(s.Ctx, clPoolId) + s.Require().NoError(err) + + lpTokens := sdk.NewCoins(sdk.NewCoin(pool.GetToken0(), sdk.NewInt(1_000_000)), sdk.NewCoin(pool.GetToken1(), sdk.NewInt(1_000_000))) + s.FundAcc(s.TestAccs[0], lpTokens) + _, err = s.App.ConcentratedLiquidityKeeper.CreateFullRangePosition(s.Ctx, clPoolId, s.TestAccs[0], lpTokens) + s.Require().NoError(err) } func (suite *UpgradeTestSuite) imitateUpgrade() { diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index c5868b9d868..9e63a9bf0ff 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -171,31 +171,33 @@ func (k Keeper) prepareBalancerPoolAsFullRange(ctx sdk.Context, clPoolId uint64, // relaxed in the future. // Note that we check denom compatibility later, and pool weights technically do not matter as they // are analogous to changing the spot price, which is handled by our lower bounding. - if len(balancerPoolLiquidity) != 2 { + if len(balancerPoolLiquidity) > 2 { return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} } - // We ensure that the asset ordering is correct when passing Balancer assets into the CL pool. - var asset0Amount, asset1Amount sdk.Int - if balancerPoolLiquidity[0].Denom == clPool.GetToken0() { - asset0Amount = balancerPoolLiquidity[0].Amount - asset1Amount = balancerPoolLiquidity[1].Amount - - // Ensure second denom matches (bal1 -> CL1) - if balancerPoolLiquidity[1].Denom != clPool.GetToken1() { - return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} - } - } else if balancerPoolLiquidity[0].Denom == clPool.GetToken1() { - asset0Amount = balancerPoolLiquidity[1].Amount - asset1Amount = balancerPoolLiquidity[0].Amount - - // Ensure second denom matches (bal1 -> CL0) - if balancerPoolLiquidity[1].Denom != clPool.GetToken0() { - return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} - } - } else { - return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} - } + asset0Amount := balancerPoolLiquidity.AmountOf(clPool.GetToken0()) + asset1Amount := balancerPoolLiquidity.AmountOf(clPool.GetToken1()) + + // // We ensure that the asset ordering is correct when passing Balancer assets into the CL pool. + // if balancerPoolLiquidity[0].Denom == clPool.GetToken0() { + // asset0Amount = balancerPoolLiquidity[0].Amount + // asset1Amount = balancerPoolLiquidity[1].Amount + + // // Ensure second denom matches (bal1 -> CL1) + // if balancerPoolLiquidity[1].Denom != clPool.GetToken1() { + // return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} + // } + // } else if balancerPoolLiquidity[0].Denom == clPool.GetToken1() { + // asset0Amount = balancerPoolLiquidity[1].Amount + // asset1Amount = balancerPoolLiquidity[0].Amount + + // // Ensure second denom matches (bal1 -> CL0) + // if balancerPoolLiquidity[1].Denom != clPool.GetToken0() { + // return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} + // } + // } else { + // return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} + // } // Calculate the amount of liquidity the Balancer amounts qualify in the CL pool. Note that since we use the CL spot price, this is // safe against prices drifting apart between the two pools (we take the lower bound on the qualifying liquidity in this case). From 2a2dee9a554aa899c179a7dc78bc4b87c580f97f Mon Sep 17 00:00:00 2001 From: Roman Date: Thu, 24 Aug 2023 12:35:04 +0200 Subject: [PATCH 2/6] clean up --- x/concentrated-liquidity/incentives.go | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index 9e63a9bf0ff..294db1f8f04 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -178,27 +178,6 @@ func (k Keeper) prepareBalancerPoolAsFullRange(ctx sdk.Context, clPoolId uint64, asset0Amount := balancerPoolLiquidity.AmountOf(clPool.GetToken0()) asset1Amount := balancerPoolLiquidity.AmountOf(clPool.GetToken1()) - // // We ensure that the asset ordering is correct when passing Balancer assets into the CL pool. - // if balancerPoolLiquidity[0].Denom == clPool.GetToken0() { - // asset0Amount = balancerPoolLiquidity[0].Amount - // asset1Amount = balancerPoolLiquidity[1].Amount - - // // Ensure second denom matches (bal1 -> CL1) - // if balancerPoolLiquidity[1].Denom != clPool.GetToken1() { - // return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} - // } - // } else if balancerPoolLiquidity[0].Denom == clPool.GetToken1() { - // asset0Amount = balancerPoolLiquidity[1].Amount - // asset1Amount = balancerPoolLiquidity[0].Amount - - // // Ensure second denom matches (bal1 -> CL0) - // if balancerPoolLiquidity[1].Denom != clPool.GetToken0() { - // return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} - // } - // } else { - // return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} - // } - // Calculate the amount of liquidity the Balancer amounts qualify in the CL pool. Note that since we use the CL spot price, this is // safe against prices drifting apart between the two pools (we take the lower bound on the qualifying liquidity in this case). // The `sqrtPriceLowerTick` and `sqrtPriceUpperTick` fields are set to the appropriate values for a full range position. From e7f07c9af485725ffdc4159877f0ee0b3690e13c Mon Sep 17 00:00:00 2001 From: Roman Date: Thu, 24 Aug 2023 12:36:01 +0200 Subject: [PATCH 3/6] comment --- x/concentrated-liquidity/incentives.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index 294db1f8f04..ea62ce0f9d4 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -171,6 +171,8 @@ func (k Keeper) prepareBalancerPoolAsFullRange(ctx sdk.Context, clPoolId uint64, // relaxed in the future. // Note that we check denom compatibility later, and pool weights technically do not matter as they // are analogous to changing the spot price, which is handled by our lower bounding. + // Note that due to low share ratio, the balancer token liquidity may be truncated to zero. + // Balancer liquidity may also upgrade in-full to CL. if len(balancerPoolLiquidity) > 2 { return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} } From 53fe97260f48676c385af6988a76727387f1db02 Mon Sep 17 00:00:00 2001 From: Nicolas Lara Date: Thu, 24 Aug 2023 12:51:55 +0200 Subject: [PATCH 4/6] added swap checks --- app/upgrades/v18/upgrades_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/upgrades/v18/upgrades_test.go b/app/upgrades/v18/upgrades_test.go index 984ca3135d0..a6cb29b4f3c 100644 --- a/app/upgrades/v18/upgrades_test.go +++ b/app/upgrades/v18/upgrades_test.go @@ -2,6 +2,7 @@ package v18_test import ( "fmt" + poolmanagertypes "github.com/osmosis-labs/osmosis/v17/x/poolmanager/types" "sort" "testing" "time" @@ -86,10 +87,17 @@ func (s *UpgradeTestSuite) TestUpgrade() { pool, err := s.App.ConcentratedLiquidityKeeper.GetConcentratedPoolById(s.Ctx, clPoolId) s.Require().NoError(err) + // LP lpTokens := sdk.NewCoins(sdk.NewCoin(pool.GetToken0(), sdk.NewInt(1_000_000)), sdk.NewCoin(pool.GetToken1(), sdk.NewInt(1_000_000))) s.FundAcc(s.TestAccs[0], lpTokens) _, err = s.App.ConcentratedLiquidityKeeper.CreateFullRangePosition(s.Ctx, clPoolId, s.TestAccs[0], lpTokens) s.Require().NoError(err) + + // Swap + toSwap := sdk.NewCoin(pool.GetToken0(), sdk.NewInt(100)) + _, err = s.App.ConcentratedLiquidityKeeper.SwapExactAmountIn(s.Ctx, s.TestAccs[0], pool.(poolmanagertypes.PoolI), toSwap, pool.GetToken1(), sdk.NewInt(1), sdk.ZeroDec()) + s.Require().NoError(err) + } func (suite *UpgradeTestSuite) imitateUpgrade() { From 8cb779a5e08bfc10b5d2ded5f7f28bb37117a86b Mon Sep 17 00:00:00 2001 From: Nicolas Lara Date: Thu, 24 Aug 2023 12:57:25 +0200 Subject: [PATCH 5/6] check that LPing fails before the upgrade --- app/upgrades/v18/upgrades_test.go | 33 ++++++++++++++++++------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/app/upgrades/v18/upgrades_test.go b/app/upgrades/v18/upgrades_test.go index a6cb29b4f3c..3fcba8ce562 100644 --- a/app/upgrades/v18/upgrades_test.go +++ b/app/upgrades/v18/upgrades_test.go @@ -58,6 +58,25 @@ func (s *UpgradeTestSuite) TestUpgrade() { // distribution would fail. s.ensurePreUpgradeDistributionPanics() + migrationInfo, err := s.App.GAMMKeeper.GetAllMigrationInfo(s.Ctx) + s.Require().NoError(err) + + link := migrationInfo.BalancerToConcentratedPoolLinks[0] + s.Require().Equal(uint64(3), link.BalancerPoolId) + + clPoolId := link.ClPoolId + + pool, err := s.App.ConcentratedLiquidityKeeper.GetConcentratedPoolById(s.Ctx, clPoolId) + s.Require().NoError(err) + + // LP Fails before the upgrade + lpTokens := sdk.NewCoins(sdk.NewCoin(pool.GetToken0(), sdk.NewInt(1_000_000)), sdk.NewCoin(pool.GetToken1(), sdk.NewInt(1_000_000))) + s.FundAcc(s.TestAccs[0], lpTokens) + // require a panic + s.Require().Panics(func() { + _, err = s.App.ConcentratedLiquidityKeeper.CreateFullRangePosition(s.Ctx, clPoolId, s.TestAccs[0], lpTokens) + }) + // upgrade software s.imitateUpgrade() s.App.BeginBlocker(s.Ctx, abci.RequestBeginBlock{}) @@ -75,21 +94,7 @@ func (s *UpgradeTestSuite) TestUpgrade() { s.ensurePostUpgradeDistributionWorks() // Check that can LP and swap into pool 3 with no usses - - migrationInfo, err := s.App.GAMMKeeper.GetAllMigrationInfo(s.Ctx) - s.Require().NoError(err) - - link := migrationInfo.BalancerToConcentratedPoolLinks[0] - s.Require().Equal(uint64(3), link.BalancerPoolId) - - clPoolId := link.ClPoolId - - pool, err := s.App.ConcentratedLiquidityKeeper.GetConcentratedPoolById(s.Ctx, clPoolId) - s.Require().NoError(err) - // LP - lpTokens := sdk.NewCoins(sdk.NewCoin(pool.GetToken0(), sdk.NewInt(1_000_000)), sdk.NewCoin(pool.GetToken1(), sdk.NewInt(1_000_000))) - s.FundAcc(s.TestAccs[0], lpTokens) _, err = s.App.ConcentratedLiquidityKeeper.CreateFullRangePosition(s.Ctx, clPoolId, s.TestAccs[0], lpTokens) s.Require().NoError(err) From 869cc55697539d0061d854d25010c271d930f0da Mon Sep 17 00:00:00 2001 From: Roman Date: Thu, 24 Aug 2023 13:27:19 +0200 Subject: [PATCH 6/6] fix test --- x/concentrated-liquidity/incentives.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index ea62ce0f9d4..9557adf75fb 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -177,8 +177,17 @@ func (k Keeper) prepareBalancerPoolAsFullRange(ctx sdk.Context, clPoolId uint64, return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} } - asset0Amount := balancerPoolLiquidity.AmountOf(clPool.GetToken0()) - asset1Amount := balancerPoolLiquidity.AmountOf(clPool.GetToken1()) + denom0 := clPool.GetToken0() + denom1 := clPool.GetToken1() + + // This check's purpose is to confirm that denoms are the same. + clCoins := totalBalancerPoolLiquidity.FilterDenoms([]string{denom0, denom1}) + if len(clCoins) != 2 { + return 0, sdk.ZeroDec(), types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: clPoolId, BalancerPoolId: canonicalBalancerPoolId, BalancerPoolLiquidity: balancerPoolLiquidity} + } + + asset0Amount := balancerPoolLiquidity.AmountOf(denom0) + asset1Amount := balancerPoolLiquidity.AmountOf(denom1) // Calculate the amount of liquidity the Balancer amounts qualify in the CL pool. Note that since we use the CL spot price, this is // safe against prices drifting apart between the two pools (we take the lower bound on the qualifying liquidity in this case).