From edfa5b6458336ddc2903ca938cceaf656f3e93be Mon Sep 17 00:00:00 2001 From: alpo <62043214+AlpinYukseloglu@users.noreply.github.com> Date: Fri, 19 May 2023 20:16:27 -0700 Subject: [PATCH] [CL][bugfix] Update reward splitting logic to only use bonded balancer amounts (#5239) * implement/test necessary helper and implement/test bugfix for bonded share reward splitting * changelog for gamm changes * clean up tests and comments --- CHANGELOG.md | 1 + x/concentrated-liquidity/incentives.go | 29 ++++++++- x/concentrated-liquidity/incentives_test.go | 57 +++++++++++++++++- .../types/expected_keepers.go | 2 + x/gamm/keeper/pool.go | 10 ++++ x/gamm/keeper/pool_test.go | 60 +++++++++++++++++++ 6 files changed, 154 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7e0c3012f9..203f8b274c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#5129](https://github.com/osmosis-labs/osmosis/pull/5129) Relax twap record validation in init genesis to allow one of the spot prices to be non-zero when twap error is observed. * [#5165](https://github.com/osmosis-labs/osmosis/pull/5165) Improve error message when fully exiting from a pool. * [#5187](https://github.com/osmosis-labs/osmosis/pull/5187) Expand `IncentivizedPools` query to include internally incentivized CL pools. + * [#5239](https://github.com/osmosis-labs/osmosis/pull/5239) Implement `GetTotalPoolShares` public keeper function for GAMM. ### API breaks diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index e9b9bddcfb3..15a5111f42a 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -12,6 +12,7 @@ import ( "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/math" "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/model" "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/types" + gammtypes "github.com/osmosis-labs/osmosis/v15/x/gamm/types" ) // createUptimeAccumulators creates accumulator objects in store for each supported uptime for the given poolId. @@ -126,12 +127,36 @@ func (k Keeper) prepareBalancerPoolAsFullRange(ctx sdk.Context, clPoolId uint64) return 0, sdk.ZeroDec(), nil } - // Get Balancer pool liquidity - balancerPoolLiquidity, err := k.gammKeeper.GetTotalPoolLiquidity(ctx, canonicalBalancerPoolId) + // Get total balancer pool liquidity (denominated in pool coins) + totalBalancerPoolLiquidity, err := k.gammKeeper.GetTotalPoolLiquidity(ctx, canonicalBalancerPoolId) if err != nil { return 0, sdk.ZeroDec(), err } + // Get total balancer shares for Balancer pool + totalBalancerPoolShares, err := k.gammKeeper.GetTotalPoolShares(ctx, canonicalBalancerPoolId) + if err != nil { + return 0, sdk.ZeroDec(), err + } + + // Get total shares bonded on the longest lockup duration for Balancer pool + longestDuration, err := k.poolIncentivesKeeper.GetLongestLockableDuration(ctx) + if err != nil { + return 0, sdk.ZeroDec(), err + } + bondedShares := k.lockupKeeper.GetLockedDenom(ctx, gammtypes.GetPoolShareDenom(canonicalBalancerPoolId), longestDuration) + + // Calculate portion of Balancer pool shares that are bonded + bondedShareRatio := bondedShares.ToDec().Quo(totalBalancerPoolShares.ToDec()) + + // Calculate rough number of assets in Balancer pool that are bonded + balancerPoolLiquidity := sdk.NewCoins() + for _, liquidityToken := range totalBalancerPoolLiquidity { + // Rounding behavior is not critical here, but for simplicity we do bankers multiplication then truncate. + bondedLiquidityAmount := liquidityToken.Amount.ToDec().Mul(bondedShareRatio).TruncateInt() + balancerPoolLiquidity = balancerPoolLiquidity.Add(sdk.NewCoin(liquidityToken.Denom, bondedLiquidityAmount)) + } + // Validate Balancer pool liquidity. These properties should already be guaranteed by the caller, // but we check them anyway as an additional guardrail in case migration link validation is ever // relaxed in the future. diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index 506541a99a7..81ccff6093f 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -909,7 +909,13 @@ func (s *KeeperTestSuite) TestUpdateUptimeAccumulatorsToNow() { // If applicable, create and link a canonical balancer pool balancerPoolId := uint64(0) if tc.canonicalBalancerPoolAssets != nil { + // Create balancer pool and bond its shares balancerPoolId = s.PrepareCustomBalancerPool(tc.canonicalBalancerPoolAssets, defaultBalancerPoolParams) + longestDuration, err := s.App.PoolIncentivesKeeper.GetLongestLockableDuration(s.Ctx) + s.Require().NoError(err) + _, err = s.App.LockupKeeper.CreateLock(s.Ctx, s.TestAccs[0], sdk.NewCoins(sdk.NewCoin(gammtypes.GetPoolShareDenom(balancerPoolId), gammtypes.InitPoolSharesSupply)), longestDuration) + s.Require().NoError(err) + s.App.GAMMKeeper.OverwriteMigrationRecords(s.Ctx, gammtypes.MigrationRecords{ BalancerToConcentratedPoolLinks: []gammtypes.BalancerToConcentratedPoolLink{ @@ -3793,6 +3799,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { tests := map[string]struct { existingConcentratedLiquidity sdk.Coins balancerPoolAssets []balancer.PoolAsset + portionOfSharesBonded sdk.Dec noCanonicalBalancerPool bool noBalancerPoolWithID bool @@ -3806,6 +3813,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { // 100 existing shares and 100 shares added from balancer existingConcentratedLiquidity: defaultConcentratedAssets, balancerPoolAssets: defaultBalancerAssets, + portionOfSharesBonded: sdk.NewDec(1), }, "same spot price, different total share amount": { // 100 existing shares and 200 shares added from balancer @@ -3814,6 +3822,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { {Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(200))}, {Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(200))}, }, + portionOfSharesBonded: sdk.NewDec(1), }, "different spot price between balancer and CL pools (excess asset0)": { // 100 existing shares and 100 shares added from balancer. We expect only the even portion of @@ -3823,6 +3832,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { {Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(150))}, {Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(100))}, }, + portionOfSharesBonded: sdk.NewDec(1), }, "different spot price between balancer and CL pools (excess asset1)": { // 100 existing shares and 100 shares added from balancer. We expect only the even portion of @@ -3832,11 +3842,32 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { {Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(100))}, {Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(150))}, }, + portionOfSharesBonded: sdk.NewDec(1), + }, + "same spot price, different total share amount, only half bonded": { + // 100 existing shares and 200 shares added from balancer + existingConcentratedLiquidity: defaultConcentratedAssets, + balancerPoolAssets: []balancer.PoolAsset{ + {Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(200))}, + {Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(200))}, + }, + portionOfSharesBonded: sdk.MustNewDecFromStr("0.5"), + }, + "different spot price between balancer and CL pools (excess asset1), only partially bonded": { + // 100 existing shares and 100 shares added from balancer. We expect only the even portion of + // the Balancer pool to be joined, with the remaining 50bar not qualifying. + existingConcentratedLiquidity: defaultConcentratedAssets, + balancerPoolAssets: []balancer.PoolAsset{ + {Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(100))}, + {Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(150))}, + }, + portionOfSharesBonded: sdk.MustNewDecFromStr("0.1"), }, "no canonical balancer pool": { // 100 existing shares and 100 shares added from balancer existingConcentratedLiquidity: defaultConcentratedAssets, balancerPoolAssets: defaultBalancerAssets, + portionOfSharesBonded: sdk.NewDec(1), // Note that we expect this to fail quietly, as most CL pools will not have linked Balancer pools noCanonicalBalancerPool: true, @@ -3848,6 +3879,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { // 100 existing shares and 100 shares added from balancer existingConcentratedLiquidity: defaultConcentratedAssets, balancerPoolAssets: defaultBalancerAssets, + portionOfSharesBonded: sdk.NewDec(1), noBalancerPoolWithID: true, expectedError: gammtypes.PoolDoesNotExistError{PoolId: invalidPoolId}, @@ -3859,6 +3891,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { {Weight: sdk.NewInt(1), Token: sdk.NewCoin("invalid", sdk.NewInt(100))}, {Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(100))}, }, + portionOfSharesBonded: sdk.NewDec(1), invalidBalancerPoolLiquidity: true, expectedError: types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: 1, BalancerPoolId: 2, BalancerPoolLiquidity: sdk.NewCoins(sdk.NewCoin("invalid", sdk.NewInt(100)), sdk.NewCoin("bar", sdk.NewInt(100)))}, @@ -3870,6 +3903,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { {Weight: sdk.NewInt(1), Token: sdk.NewCoin("foo", sdk.NewInt(100))}, {Weight: sdk.NewInt(1), Token: sdk.NewCoin("invalid", sdk.NewInt(100))}, }, + portionOfSharesBonded: sdk.NewDec(1), invalidBalancerPoolLiquidity: true, expectedError: types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: 1, BalancerPoolId: 2, BalancerPoolLiquidity: sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(100)), sdk.NewCoin("invalid", sdk.NewInt(100)))}, @@ -3881,6 +3915,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { {Weight: sdk.NewInt(1), Token: sdk.NewCoin("invalid1", sdk.NewInt(100))}, {Weight: sdk.NewInt(1), Token: sdk.NewCoin("invalid2", sdk.NewInt(100))}, }, + portionOfSharesBonded: sdk.NewDec(1), invalidBalancerPoolLiquidity: true, expectedError: types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: 1, BalancerPoolId: 2, BalancerPoolLiquidity: sdk.NewCoins(sdk.NewCoin("invalid1", sdk.NewInt(100)), sdk.NewCoin("invalid2", sdk.NewInt(100)))}, @@ -3893,6 +3928,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { {Weight: sdk.NewInt(1), Token: sdk.NewCoin("bar", sdk.NewInt(100))}, {Weight: sdk.NewInt(1), Token: sdk.NewCoin("baz", sdk.NewInt(100))}, }, + portionOfSharesBonded: sdk.NewDec(1), invalidBalancerPoolLiquidity: true, expectedError: types.ErrInvalidBalancerPoolLiquidityError{ClPoolId: 1, BalancerPoolId: 2, BalancerPoolLiquidity: sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(100)), sdk.NewCoin("bar", sdk.NewInt(100)), sdk.NewCoin("baz", sdk.NewInt(100)))}, @@ -3901,6 +3937,7 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { // 100 existing shares and 100 shares added from balancer existingConcentratedLiquidity: defaultConcentratedAssets, balancerPoolAssets: defaultBalancerAssets, + portionOfSharesBonded: sdk.NewDec(1), invalidConcentratedPoolID: true, expectedError: types.PoolNotFoundError{PoolId: invalidPoolId + 1}, @@ -3918,6 +3955,14 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { // If a canonical balancer pool exists, we create it and link it with the CL pool balancerPoolId := s.PrepareCustomBalancerPool(tc.balancerPoolAssets, defaultBalancerPoolParams) + + // Bond the appropriate portion of total Balancer shares as defined by the current test case + longestDuration, err := s.App.PoolIncentivesKeeper.GetLongestLockableDuration(s.Ctx) + s.Require().NoError(err) + bondedShares := gammtypes.InitPoolSharesSupply.ToDec().Mul(tc.portionOfSharesBonded).TruncateInt() + _, err = s.App.LockupKeeper.CreateLock(s.Ctx, s.TestAccs[0], sdk.NewCoins(sdk.NewCoin(gammtypes.GetPoolShareDenom(balancerPoolId), bondedShares)), longestDuration) + s.Require().NoError(err) + if tc.noBalancerPoolWithID { balancerPoolId = invalidPoolId } else if tc.noCanonicalBalancerPool { @@ -3937,7 +3982,9 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() { // Calculate balancer share amount for full range updatedClPool, err := s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, clPool.GetId()) s.Require().NoError(err) - qualifyingSharesPreDiscount := math.GetLiquidityFromAmounts(updatedClPool.GetCurrentSqrtPrice(), types.MinSqrtPrice, types.MaxSqrtPrice, tc.balancerPoolAssets[1].Token.Amount, tc.balancerPoolAssets[0].Token.Amount) + asset0BalancerAmount := tc.balancerPoolAssets[0].Token.Amount.ToDec().Mul(tc.portionOfSharesBonded).TruncateInt() + asset1BalancerAmount := tc.balancerPoolAssets[1].Token.Amount.ToDec().Mul(tc.portionOfSharesBonded).TruncateInt() + qualifyingSharesPreDiscount := math.GetLiquidityFromAmounts(updatedClPool.GetCurrentSqrtPrice(), types.MinSqrtPrice, types.MaxSqrtPrice, asset1BalancerAmount, asset0BalancerAmount) qualifyingShares := (sdk.OneDec().Sub(types.DefaultBalancerSharesDiscount)).Mul(qualifyingSharesPreDiscount) clearOutQualifyingShares := tc.noBalancerPoolWithID || tc.invalidBalancerPoolLiquidity || tc.invalidConcentratedPoolID || tc.invalidBalancerPoolID || tc.noCanonicalBalancerPool @@ -4124,8 +4171,12 @@ func (s *KeeperTestSuite) TestClaimAndResetFullRangeBalancerPool() { // Note that the second return value here is the position ID, not an error. initialLiquidity, _ := s.SetupPosition(clPoolId, s.TestAccs[0], tc.existingConcentratedLiquidity, DefaultMinTick, DefaultMaxTick, s.Ctx.BlockTime()) - // Create balancer pool to be linked with CL pool in happy path cases + // Create and bond shares for balancer pool to be linked with CL pool in happy path cases balancerPoolId := s.PrepareCustomBalancerPool(tc.balancerPoolAssets, defaultBalancerPoolParams) + longestDuration, err := s.App.PoolIncentivesKeeper.GetLongestLockableDuration(s.Ctx) + s.Require().NoError(err) + _, err = s.App.LockupKeeper.CreateLock(s.Ctx, s.TestAccs[0], sdk.NewCoins(sdk.NewCoin(gammtypes.GetPoolShareDenom(balancerPoolId), gammtypes.InitPoolSharesSupply)), longestDuration) + s.Require().NoError(err) // Invalidate pool IDs if needed for error cases if tc.balancerPoolDoesNotExist { @@ -4165,7 +4216,7 @@ func (s *KeeperTestSuite) TestClaimAndResetFullRangeBalancerPool() { s.FundAcc(clPool.GetIncentivesAddress(), normalizedEmissions) } } - err := addToUptimeAccums(s.Ctx, clPool.GetId(), s.App.ConcentratedLiquidityKeeper, tc.uptimeGrowth) + err = addToUptimeAccums(s.Ctx, clPool.GetId(), s.App.ConcentratedLiquidityKeeper, tc.uptimeGrowth) s.Require().NoError(err) // --- System under test --- diff --git a/x/concentrated-liquidity/types/expected_keepers.go b/x/concentrated-liquidity/types/expected_keepers.go index 10191356a82..507f40fc1f4 100644 --- a/x/concentrated-liquidity/types/expected_keepers.go +++ b/x/concentrated-liquidity/types/expected_keepers.go @@ -38,6 +38,7 @@ type PoolManagerKeeper interface { type GAMMKeeper interface { GetTotalPoolLiquidity(ctx sdk.Context, poolId uint64) (sdk.Coins, error) GetLinkedBalancerPoolID(ctx sdk.Context, poolIdEntering uint64) (uint64, error) + GetTotalPoolShares(ctx sdk.Context, poolId uint64) (sdk.Int, error) } type PoolIncentivesKeeper interface { @@ -58,6 +59,7 @@ type LockupKeeper interface { ForceUnlock(ctx sdk.Context, lock lockuptypes.PeriodLock) error CreateLock(ctx sdk.Context, owner sdk.AccAddress, coins sdk.Coins, duration time.Duration) (lockuptypes.PeriodLock, error) SlashTokensFromLockByID(ctx sdk.Context, lockID uint64, coins sdk.Coins) (*lockuptypes.PeriodLock, error) + GetLockedDenom(ctx sdk.Context, denom string, duration time.Duration) sdk.Int } // CommunityPoolKeeper defines the contract needed to be fulfilled for distribution keeper. diff --git a/x/gamm/keeper/pool.go b/x/gamm/keeper/pool.go index 8be771719d4..c58e1e23ba0 100644 --- a/x/gamm/keeper/pool.go +++ b/x/gamm/keeper/pool.go @@ -298,6 +298,16 @@ func (k Keeper) GetTotalPoolLiquidity(ctx sdk.Context, poolId uint64) (sdk.Coins return pool.GetTotalPoolLiquidity(ctx), nil } +// GetTotalPoolShares returns the total number of pool shares for the given pool. +func (k Keeper) GetTotalPoolShares(ctx sdk.Context, poolId uint64) (sdk.Int, error) { + pool, err := k.GetCFMMPool(ctx, poolId) + if err != nil { + return sdk.Int{}, err + } + + return pool.GetTotalShares(), nil +} + // setStableSwapScalingFactors sets the stable swap scaling factors. // errors if the pool does not exist, the sender is not the scaling factor controller, or due to other // internal errors. diff --git a/x/gamm/keeper/pool_test.go b/x/gamm/keeper/pool_test.go index 961c6feb160..37de1be793b 100644 --- a/x/gamm/keeper/pool_test.go +++ b/x/gamm/keeper/pool_test.go @@ -517,3 +517,63 @@ func (s *KeeperTestSuite) TestSetStableSwapScalingFactors() { }) } } + +func (suite *KeeperTestSuite) TestGetTotalPoolShares() { + tests := map[string]struct { + sharesJoined sdk.Int + poolNotCreated bool + + expectedError error + }{ + "happy path: default balancer pool": { + sharesJoined: sdk.ZeroInt(), + }, + "Multiple LPs with shares exist": { + sharesJoined: types.OneShare, + }, + "error: pool does not exist": { + sharesJoined: sdk.ZeroInt(), + poolNotCreated: true, + expectedError: types.PoolDoesNotExistError{PoolId: uint64(0)}, + }, + } + + for name, tc := range tests { + suite.Run(name, func() { + suite.SetupTest() + gammKeeper := suite.App.GAMMKeeper + testAccount := suite.TestAccs[0] + + // --- Setup --- + + // Mint some assets to the accounts. + balancerPoolId := uint64(0) + if !tc.poolNotCreated { + balancerPoolId = suite.PrepareBalancerPool() + } + + sharesJoined := sdk.ZeroInt() + if !tc.sharesJoined.Equal(sdk.ZeroInt()) { + suite.FundAcc(testAccount, defaultAcctFunds) + _, sharesActualJoined, err := gammKeeper.JoinPoolNoSwap(suite.Ctx, testAccount, balancerPoolId, tc.sharesJoined, sdk.Coins{}) + suite.Require().NoError(err) + sharesJoined = sharesActualJoined + } + + // --- System under test --- + + totalShares, err := gammKeeper.GetTotalPoolShares(suite.Ctx, balancerPoolId) + + // --- Assertions --- + + if tc.expectedError != nil { + suite.Require().Error(err) + suite.Require().ErrorContains(err, tc.expectedError.Error()) + return + } + + suite.Require().NoError(err) + suite.Require().Equal(types.InitPoolSharesSupply.Add(sharesJoined), totalShares) + }) + } +}