From 3d35759a1c69edc9ca0e78e9b8a47671122a95b4 Mon Sep 17 00:00:00 2001 From: alpo Date: Thu, 14 Mar 2024 00:13:58 -0700 Subject: [PATCH 01/16] implement redepositing of forfeited incentives --- x/concentrated-liquidity/export_test.go | 8 ++- x/concentrated-liquidity/incentives.go | 60 +++++++++++---------- x/concentrated-liquidity/incentives_test.go | 44 +++++++++++---- x/concentrated-liquidity/invariant_test.go | 2 +- x/concentrated-liquidity/lp.go | 51 +++++++++++++++++- x/concentrated-liquidity/lp_test.go | 3 +- x/concentrated-liquidity/msg_server.go | 2 +- x/concentrated-liquidity/position.go | 4 +- x/concentrated-liquidity/position_test.go | 11 ++-- 9 files changed, 132 insertions(+), 53 deletions(-) diff --git a/x/concentrated-liquidity/export_test.go b/x/concentrated-liquidity/export_test.go index 1be7a8043c5..c56dee5c782 100644 --- a/x/concentrated-liquidity/export_test.go +++ b/x/concentrated-liquidity/export_test.go @@ -258,7 +258,7 @@ func (k Keeper) GetAllIncentiveRecordsForUptime(ctx sdk.Context, poolId uint64, return k.getAllIncentiveRecordsForUptime(ctx, poolId, minUptime) } -func (k Keeper) CollectIncentives(ctx sdk.Context, owner sdk.AccAddress, positionId uint64) (sdk.Coins, sdk.Coins, error) { +func (k Keeper) CollectIncentives(ctx sdk.Context, owner sdk.AccAddress, positionId uint64) (sdk.Coins, sdk.Coins, map[int]sdk.Coins, error) { return k.collectIncentives(ctx, owner, positionId) } @@ -270,7 +270,7 @@ func UpdateAccumAndClaimRewards(accum *accum.AccumulatorObject, positionKey stri return updateAccumAndClaimRewards(accum, positionKey, growthOutside) } -func (k Keeper) PrepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, error) { +func (k Keeper) PrepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, map[int]sdk.Coins, error) { return k.prepareClaimAllIncentivesForPosition(ctx, positionId) } @@ -355,3 +355,7 @@ func ComputeTotalIncentivesToEmit(timeElapsedSeconds osmomath.Dec, emissionRate func (k Keeper) GetIncentiveScalingFactorForPool(ctx sdk.Context, poolID uint64) (osmomath.Dec, error) { return k.getIncentiveScalingFactorForPool(ctx, poolID) } + +func ScaleDownIncentiveAmount(incentiveAmount osmomath.Int, scalingFactor osmomath.Dec) (scaledTotalEmittedAmount osmomath.Int) { + return scaleDownIncentiveAmount(incentiveAmount, scalingFactor) +} diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index 2b57752db08..567256992af 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -753,16 +753,16 @@ func moveRewardsToNewPositionAndDeleteOldAcc(accum *accum.AccumulatorObject, old // The parent function (collectIncentives) does the actual bank sends for both the collected and forfeited incentives. // // Returns error if the position/uptime accumulators don't exist, or if there is an issue that arises while claiming. -func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, error) { +func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, map[int]sdk.Coins, error) { // Retrieve the position with the given ID. position, err := k.GetPosition(ctx, positionId) if err != nil { - return sdk.Coins{}, sdk.Coins{}, err + return sdk.Coins{}, sdk.Coins{}, nil, err } err = k.UpdatePoolUptimeAccumulatorsToNow(ctx, position.PoolId) if err != nil { - return sdk.Coins{}, sdk.Coins{}, err + return sdk.Coins{}, sdk.Coins{}, nil, err } // Compute the age of the position. @@ -770,19 +770,19 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId // Should never happen, defense in depth. if positionAge < 0 { - return sdk.Coins{}, sdk.Coins{}, types.NegativeDurationError{Duration: positionAge} + return sdk.Coins{}, sdk.Coins{}, nil, types.NegativeDurationError{Duration: positionAge} } // Retrieve the uptime accumulators for the position's pool. uptimeAccumulators, err := k.GetUptimeAccumulators(ctx, position.PoolId) if err != nil { - return sdk.Coins{}, sdk.Coins{}, err + return sdk.Coins{}, sdk.Coins{}, nil, err } // Compute uptime growth outside of the range between lower tick and upper tick uptimeGrowthOutside, err := k.GetUptimeGrowthOutsideRange(ctx, position.PoolId, position.LowerTick, position.UpperTick) if err != nil { - return sdk.Coins{}, sdk.Coins{}, err + return sdk.Coins{}, sdk.Coins{}, nil, err } // Create a variable to hold the name of the position. @@ -796,10 +796,11 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId incentiveScalingFactor, err := k.getIncentiveScalingFactorForPool(ctx, position.PoolId) if err != nil { - return sdk.Coins{}, sdk.Coins{}, err + return sdk.Coins{}, sdk.Coins{}, nil, err } // Loop through each uptime accumulator for the pool. + scaledForfeitedIncentivesByUptime := make(map[int]sdk.Coins) for uptimeIndex, uptimeAccum := range uptimeAccumulators { // Check if the accumulator contains the position. // There should never be a case where you can have a position for 1 accumulator, and not the rest. @@ -809,7 +810,7 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId if hasPosition { collectedIncentivesForUptimeScaled, _, err := updateAccumAndClaimRewards(uptimeAccum, positionName, uptimeGrowthOutside[uptimeIndex]) if err != nil { - return sdk.Coins{}, sdk.Coins{}, err + return sdk.Coins{}, sdk.Coins{}, nil, err } // We scale the uptime per-unit of liquidity accumulator up to avoid truncation to zero. @@ -824,6 +825,13 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId } if positionAge < supportedUptimes[uptimeIndex] { + // We track forfeited incentives by uptime accumulator to allow for efficient redepositing. + // To avoid descaling and rescaling, we keep the forfeited incentives in scaled form. + // This is slightly unwieldy as it means we return a map of scaled coins, but doing it this way + // allows us to efficiently handle all cases related to forfeited incentives without recomputing + // expensive operations. + scaledForfeitedIncentivesByUptime[uptimeIndex] = collectedIncentivesForUptimeScaled + // If the age of the position is less than the current uptime we are iterating through, then the position's // incentives are forfeited to the community pool. The parent function does the actual bank send. forfeitedIncentivesForPosition = forfeitedIncentivesForPosition.Add(collectedIncentivesForUptime...) @@ -835,13 +843,15 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId } } - return collectedIncentivesForPosition, forfeitedIncentivesForPosition, nil + return collectedIncentivesForPosition, forfeitedIncentivesForPosition, scaledForfeitedIncentivesByUptime, nil } func (k Keeper) GetClaimableIncentives(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, error) { // Since this is a query, we don't want to modify the state and therefore use a cache context. cacheCtx, _ := ctx.CacheContext() - return k.prepareClaimAllIncentivesForPosition(cacheCtx, positionId) + // We omit the by-uptime forfeited incentives map as it is not needed for this query. + collectedIncentives, forfeitedIncentives, _, err := k.prepareClaimAllIncentivesForPosition(cacheCtx, positionId) + return collectedIncentives, forfeitedIncentives, err } // collectIncentives collects incentives for all uptime accumulators for the specified position id. @@ -850,49 +860,41 @@ func (k Keeper) GetClaimableIncentives(ctx sdk.Context, positionId uint64) (sdk. // Returns error if: // - position with the given id does not exist // - other internal database or math errors. -func (k Keeper) collectIncentives(ctx sdk.Context, sender sdk.AccAddress, positionId uint64) (sdk.Coins, sdk.Coins, error) { +func (k Keeper) collectIncentives(ctx sdk.Context, sender sdk.AccAddress, positionId uint64) (sdk.Coins, sdk.Coins, map[int]sdk.Coins, error) { // Retrieve the position with the given ID. position, err := k.GetPosition(ctx, positionId) if err != nil { - return sdk.Coins{}, sdk.Coins{}, err + return sdk.Coins{}, sdk.Coins{}, nil, err } if sender.String() != position.Address { - return sdk.Coins{}, sdk.Coins{}, types.NotPositionOwnerError{ + return sdk.Coins{}, sdk.Coins{}, nil, types.NotPositionOwnerError{ PositionId: positionId, Address: sender.String(), } } // Claim all incentives for the position. - collectedIncentivesForPosition, forfeitedIncentivesForPosition, err := k.prepareClaimAllIncentivesForPosition(ctx, position.PositionId) + collectedIncentivesForPosition, totalForfeitedIncentivesForPosition, scaledAmountForfeitedByUptime, err := k.prepareClaimAllIncentivesForPosition(ctx, position.PositionId) if err != nil { - return sdk.Coins{}, sdk.Coins{}, err + return sdk.Coins{}, sdk.Coins{}, nil, err } // If no incentives were collected, return an empty coin set. - if collectedIncentivesForPosition.IsZero() && forfeitedIncentivesForPosition.IsZero() { - return collectedIncentivesForPosition, forfeitedIncentivesForPosition, nil + if collectedIncentivesForPosition.IsZero() && totalForfeitedIncentivesForPosition.IsZero() { + return collectedIncentivesForPosition, totalForfeitedIncentivesForPosition, scaledAmountForfeitedByUptime, nil } // Send the collected incentives to the position's owner. pool, err := k.getPoolById(ctx, position.PoolId) if err != nil { - return sdk.Coins{}, sdk.Coins{}, err + return sdk.Coins{}, sdk.Coins{}, nil, err } // Send the collected incentives to the position's owner from the pool's address. if !collectedIncentivesForPosition.IsZero() { if err := k.bankKeeper.SendCoins(ctx, pool.GetIncentivesAddress(), sender, collectedIncentivesForPosition); err != nil { - return sdk.Coins{}, sdk.Coins{}, err - } - } - - // Send the forfeited incentives to the community pool from the pool's address. - if !forfeitedIncentivesForPosition.IsZero() { - err = k.communityPoolKeeper.FundCommunityPool(ctx, forfeitedIncentivesForPosition, pool.GetIncentivesAddress()) - if err != nil { - return sdk.Coins{}, sdk.Coins{}, err + return sdk.Coins{}, sdk.Coins{}, nil, err } } @@ -904,11 +906,11 @@ func (k Keeper) collectIncentives(ctx sdk.Context, sender sdk.AccAddress, positi sdk.NewAttribute(types.AttributeKeyPoolId, strconv.FormatUint(pool.GetId(), 10)), sdk.NewAttribute(types.AttributeKeyPositionId, strconv.FormatUint(positionId, 10)), sdk.NewAttribute(types.AttributeKeyTokensOut, collectedIncentivesForPosition.String()), - sdk.NewAttribute(types.AttributeKeyForfeitedTokens, forfeitedIncentivesForPosition.String()), + sdk.NewAttribute(types.AttributeKeyForfeitedTokens, totalForfeitedIncentivesForPosition.String()), ), }) - return collectedIncentivesForPosition, forfeitedIncentivesForPosition, nil + return collectedIncentivesForPosition, totalForfeitedIncentivesForPosition, scaledAmountForfeitedByUptime, nil } // createIncentive creates an incentive record in state for the given pool. diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index 9d4ee0e9917..046adf2bf25 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -2311,7 +2311,7 @@ func (s *KeeperTestSuite) TestQueryAndCollectIncentives() { s.Require().Equal(tc.expectedIncentivesClaimed, incentivesClaimedQuery) s.Require().Equal(tc.expectedForfeitedIncentives, incentivesForfeitedQuery) } - actualIncentivesClaimed, actualIncetivesForfeited, err := clKeeper.CollectIncentives(s.Ctx, ownerWithValidPosition, DefaultPositionId) + actualIncentivesClaimed, actualIncetivesForfeited, _, err := clKeeper.CollectIncentives(s.Ctx, ownerWithValidPosition, DefaultPositionId) // Assertions s.Require().Equal(incentivesClaimedQuery, actualIncentivesClaimed) @@ -2775,6 +2775,26 @@ func (s *KeeperTestSuite) TestUpdateAccumAndClaimRewards() { }) } +// checkForfeitedCoinsByUptime checks that the sum of forfeited coins by uptime matches the expected total forfeited coins. +// It adds up the Coins corresponding to each uptime in the map and asserts that the result is equal to the input totalForfeitedCoins. +func (s *KeeperTestSuite) checkForfeitedCoinsByUptime(totalForfeitedCoins sdk.Coins, scaledForfeitedCoinsByUptime map[int]sdk.Coins) { + forfeitedCoins := sdk.NewCoins() + // Iterate through the constant uptime indexes + for uptimeIndex := range types.SupportedUptimes { + // If there are forfeited coins for the current uptime, add them to the sum + if coins, ok := scaledForfeitedCoinsByUptime[uptimeIndex]; ok { + for _, coin := range coins { + // Scale down the actual forfeited coin amount + scaledDownAmount := cl.ScaleDownIncentiveAmount(coin.Amount, cl.PerUnitLiqScalingFactor) + forfeitedCoins = forfeitedCoins.Add(sdk.NewCoin(coin.Denom, scaledDownAmount)) + } + } + } + + // Use Equal for assertion to compare the totalForfeitedCoins with the scaled down summedCoins + s.Require().Equal(totalForfeitedCoins, forfeitedCoins, "Total forfeited coins do not match the sum of forfeited coins by uptime after scaling down") +} + // Note that the non-forfeit cases are thoroughly tested in `TestCollectIncentives` func (s *KeeperTestSuite) TestQueryAndClaimAllIncentives() { uptimeHelper := getExpectedUptimes() @@ -2905,7 +2925,7 @@ func (s *KeeperTestSuite) TestQueryAndClaimAllIncentives() { s.Require().Equal(initSenderBalances, newSenderBalances) s.Require().Equal(initPoolBalances, newPoolBalances) - amountClaimed, amountForfeited, err := s.Clk.PrepareClaimAllIncentivesForPosition(s.Ctx, tc.positionIdClaim) + amountClaimed, totalAmountForfeited, scaledAmountForfeitedByUptime, err := s.Clk.PrepareClaimAllIncentivesForPosition(s.Ctx, tc.positionIdClaim) // --- Assertions --- @@ -2914,7 +2934,8 @@ func (s *KeeperTestSuite) TestQueryAndClaimAllIncentives() { newPoolBalances = s.App.BankKeeper.GetAllBalances(s.Ctx, clPool.GetAddress()) s.Require().Equal(amountClaimedQuery, amountClaimed) - s.Require().Equal(amountForfeitedQuery, amountForfeited) + s.Require().Equal(amountForfeitedQuery, totalAmountForfeited) + s.checkForfeitedCoinsByUptime(totalAmountForfeited, scaledAmountForfeitedByUptime) if tc.expectedError != nil { s.Require().ErrorIs(err, tc.expectedError) @@ -2939,7 +2960,7 @@ func (s *KeeperTestSuite) TestQueryAndClaimAllIncentives() { expectedCoins = expectedCoins.Add(sdk.NormalizeCoins(growthInside)...) } s.Require().Equal(expectedCoins.String(), amountClaimed.String()) - s.Require().Equal(sdk.Coins{}, amountForfeited) + s.Require().Equal(sdk.Coins{}, totalAmountForfeited) } // Ensure balances have not been mutated @@ -3078,10 +3099,11 @@ func (s *KeeperTestSuite) TestPrepareClaimAllIncentivesForPosition() { } // System under test - collectedInc, forfeitedIncentives, err := s.Clk.PrepareClaimAllIncentivesForPosition(s.Ctx, positionOneData.ID) + collectedInc, totalForfeitedIncentives, scaledForfeitedIncentivesByUptime, err := s.Clk.PrepareClaimAllIncentivesForPosition(s.Ctx, positionOneData.ID) s.Require().NoError(err) s.Require().Equal(tc.expectedCoins.String(), collectedInc.String()) - s.Require().Equal(expectedForfeitedIncentives.String(), forfeitedIncentives.String()) + s.Require().Equal(expectedForfeitedIncentives.String(), totalForfeitedIncentives.String()) + s.checkForfeitedCoinsByUptime(totalForfeitedIncentives, scaledForfeitedIncentivesByUptime) // The difference accumulator value should have increased if we forfeited incentives by claiming. uptimeAccumsDiffPostClaim := sdk.NewDecCoins() @@ -3169,7 +3191,7 @@ func (s *KeeperTestSuite) TestFunctional_ClaimIncentives_LiquidityChange_Varying s.Ctx = s.Ctx.WithBlockTime(s.Ctx.BlockTime().Add(testFullChargeDuration)) // Claim incentives. - collected, _, err := s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, defaultAddress, positionOneData.ID) + collected, _, _, err := s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, defaultAddress, positionOneData.ID) s.Require().NoError(err) s.Require().Equal(expectedCoinsPerFullCharge.String(), collected.String()) @@ -3184,13 +3206,13 @@ func (s *KeeperTestSuite) TestFunctional_ClaimIncentives_LiquidityChange_Varying s.Ctx = s.Ctx.WithBlockTime(s.Ctx.BlockTime().Add(testFullChargeDuration)) // Claim for second position. Must only claim half of the original expected amount since now there are 2 positions. - collected, _, err = s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, defaultAddress, positionTwoData.ID) + collected, _, _, err = s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, defaultAddress, positionTwoData.ID) s.Require().NoError(err) s.Require().Equal(expectedHalfOfExpectedCoinsPerFullCharge.String(), collected.String()) // Claim for first position and observe that claims full expected charge for the period between 1st claim and 2nd position creation // and half of the full charge amount since the 2nd position was created. - collected, _, err = s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, defaultAddress, positionOneData.ID) + collected, _, _, err = s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, defaultAddress, positionOneData.ID) s.Require().NoError(err) // Note, adding one since both expected amounts already subtract one (-2 in total) s.Require().Equal(expectedCoinsPerFullCharge.Add(expectedHalfOfExpectedCoinsPerFullCharge.Add(oneUUSDCCoin)...).String(), collected.String()) @@ -3667,13 +3689,13 @@ func (s *KeeperTestSuite) TestIncentiveTruncation() { // The check below shows that the incentive is not claimed due to truncation s.Ctx = s.Ctx.WithBlockTime(s.Ctx.BlockTime().Add(time.Minute * 50)) - incentives, _, err := s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, s.TestAccs[0], positionData.ID) + incentives, _, _, err := s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, s.TestAccs[0], positionData.ID) s.Require().NoError(err) s.Require().True(incentives.IsZero()) // Incentives should now be claimed due to lack of truncation s.Ctx = s.Ctx.WithBlockTime(s.Ctx.BlockTime().Add(time.Hour * 6)) - incentives, _, err = s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, s.TestAccs[0], positionData.ID) + incentives, _, _, err = s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, s.TestAccs[0], positionData.ID) s.Require().NoError(err) s.Require().False(incentives.IsZero()) } diff --git a/x/concentrated-liquidity/invariant_test.go b/x/concentrated-liquidity/invariant_test.go index bea522948ce..a0202ffa5f0 100644 --- a/x/concentrated-liquidity/invariant_test.go +++ b/x/concentrated-liquidity/invariant_test.go @@ -92,7 +92,7 @@ func (s *KeeperTestSuite) assertTotalRewardsInvariant(expectedGlobalRewardValues // // Balancer full range incentives are also not factored in because they are claimed and sent to // gauge immediately upon distribution. - collectedIncentives, _, err := s.Clk.CollectIncentives(cachedCtx, owner, position.PositionId) + collectedIncentives, _, _, err := s.Clk.CollectIncentives(cachedCtx, owner, position.PositionId) s.Require().NoError(err) // Ensure position owner's balance was updated correctly diff --git a/x/concentrated-liquidity/lp.go b/x/concentrated-liquidity/lp.go index db4caa8980d..625824a9e85 100644 --- a/x/concentrated-liquidity/lp.go +++ b/x/concentrated-liquidity/lp.go @@ -268,7 +268,7 @@ func (k Keeper) WithdrawPosition(ctx sdk.Context, owner sdk.AccAddress, position return osmomath.Int{}, osmomath.Int{}, err } - _, _, err = k.collectIncentives(ctx, owner, positionId) + _, totalForefeitedIncentives, scaledForfeitedIncentivesByUptime, err := k.collectIncentives(ctx, owner, positionId) if err != nil { return osmomath.Int{}, osmomath.Int{}, err } @@ -289,6 +289,53 @@ func (k Keeper) WithdrawPosition(ctx sdk.Context, owner sdk.AccAddress, position return osmomath.Int{}, osmomath.Int{}, err } + // Re-fetch pool from state to check updated active liquidity. + pool, err = k.getPoolById(ctx, position.PoolId) + if err != nil { + return osmomath.Int{}, osmomath.Int{}, err + } + activeLiquidity := pool.GetLiquidity() + + // If pool has active liquidity on current tick, redeposit forfeited incentives into uptime accumulators. + // TODO: abstract this into a `redepositForfeitedIncentives` helper to declutter withdrawal logic + if activeLiquidity.GT(sdk.OneDec()) { + // Get uptime accums + uptimeAccums, err := k.GetUptimeAccumulators(ctx, position.PoolId) + if err != nil { + return osmomath.Int{}, osmomath.Int{}, err + } + + for uptimeIndex := range uptimeAccums { + // Get relevant uptime-level values + curUptimeForfeited := scaledForfeitedIncentivesByUptime[uptimeIndex] + if curUptimeForfeited.IsZero() { + continue + } + + // Loop through forfeited coins for current uptime + incentivesToAddToCurAccum := sdk.NewDecCoins() + for _, forfeitedCoin := range curUptimeForfeited { + // Calculate the amount to add to the accumulator by dividing the forfeited coin amount by the current uptime duration + forfeitedAmountPerLiquidity := forfeitedCoin.Amount.ToLegacyDec().QuoTruncate(activeLiquidity) + + // Create a DecCoin from the calculated amount + decCoinToAdd := sdk.NewDecCoinFromDec(forfeitedCoin.Denom, forfeitedAmountPerLiquidity) + + // Add the calculated DecCoin to the incentives to add to current accumulator + incentivesToAddToCurAccum = incentivesToAddToCurAccum.Add(decCoinToAdd) + } + + // Emit incentives to current uptime accumulator + uptimeAccums[uptimeIndex].AddToAccumulator(incentivesToAddToCurAccum) + } + } else { + // If no active liquidity, give the forfeited incentives to the sender. + err = k.bankKeeper.SendCoins(ctx, pool.GetIncentivesAddress(), owner, totalForefeitedIncentives) + if err != nil { + return osmomath.Int{}, osmomath.Int{}, err + } + } + // If the requested liquidity amount to withdraw is equal to the available liquidity, delete the position from state. // Ensure we collect any outstanding spread factors and incentives prior to deleting the position from state. This claiming // process also clears position records from spread factor and incentive accumulators. @@ -297,7 +344,7 @@ func (k Keeper) WithdrawPosition(ctx sdk.Context, owner sdk.AccAddress, position return osmomath.Int{}, osmomath.Int{}, err } - if _, _, err := k.collectIncentives(ctx, owner, positionId); err != nil { + if _, _, _, err := k.collectIncentives(ctx, owner, positionId); err != nil { return osmomath.Int{}, osmomath.Int{}, err } diff --git a/x/concentrated-liquidity/lp_test.go b/x/concentrated-liquidity/lp_test.go index 6bec56bab86..8d7af29ab6a 100644 --- a/x/concentrated-liquidity/lp_test.go +++ b/x/concentrated-liquidity/lp_test.go @@ -2,6 +2,7 @@ package concentrated_liquidity_test import ( "errors" + "fmt" "time" sdk "github.com/cosmos/cosmos-sdk/types" @@ -655,7 +656,7 @@ func (s *KeeperTestSuite) TestWithdrawPosition() { for _, coin := range expectedOwnerBalanceDelta { expected := expectedOwnerBalanceDelta.AmountOf(coin.Denom) actual := actualOwnerBalancerDelta.AmountOf(coin.Denom) - s.Require().True(expected.Equal(actual)) + s.Require().True(expected.Equal(actual), fmt.Sprintf("expected: %s, got: %s", expected.String(), actual.String())) } if tc.timeElapsed > 0 { diff --git a/x/concentrated-liquidity/msg_server.go b/x/concentrated-liquidity/msg_server.go index 32cbf74a612..9a4a95520da 100644 --- a/x/concentrated-liquidity/msg_server.go +++ b/x/concentrated-liquidity/msg_server.go @@ -148,7 +148,7 @@ func (server msgServer) CollectIncentives(goCtx context.Context, msg *types.MsgC totalCollectedIncentives := sdk.NewCoins() totalForefeitedIncentives := sdk.NewCoins() for _, positionId := range msg.PositionIds { - collectedIncentives, forfeitedIncentives, err := server.keeper.collectIncentives(ctx, sender, positionId) + collectedIncentives, forfeitedIncentives, _, err := server.keeper.collectIncentives(ctx, sender, positionId) if err != nil { return nil, err } diff --git a/x/concentrated-liquidity/position.go b/x/concentrated-liquidity/position.go index efc7abcf467..38fc5b1335d 100644 --- a/x/concentrated-liquidity/position.go +++ b/x/concentrated-liquidity/position.go @@ -708,7 +708,9 @@ func (k Keeper) transferPositions(ctx sdk.Context, positionIds []uint64, sender if _, err := k.collectSpreadRewards(ctx, sender, positionId); err != nil { return err } - if _, _, err := k.collectIncentives(ctx, sender, positionId); err != nil { + + // TODO: handle redepositing of forfeited incentives + if _, _, _, err := k.collectIncentives(ctx, sender, positionId); err != nil { return err } diff --git a/x/concentrated-liquidity/position_test.go b/x/concentrated-liquidity/position_test.go index 0a8d52eb1bc..d08374598ff 100644 --- a/x/concentrated-liquidity/position_test.go +++ b/x/concentrated-liquidity/position_test.go @@ -82,16 +82,17 @@ func (s *KeeperTestSuite) GetTotalAccruedRewardsByAccumulator(positionId uint64, // ExecuteAndValidateSuccessfulIncentiveClaim claims incentives for position Id and asserts its output is as expected. // It also asserts that no more incentives can be claimed for the position. -func (s *KeeperTestSuite) ExecuteAndValidateSuccessfulIncentiveClaim(positionId uint64, expectedRewards sdk.Coins, expectedForfeited sdk.Coins) { +func (s *KeeperTestSuite) ExecuteAndValidateSuccessfulIncentiveClaim(positionId uint64, expectedRewards sdk.Coins, expectedForfeited sdk.Coins, poolId uint64) { // Initial claim and assertion - claimedRewards, forfeitedRewards, err := s.Clk.PrepareClaimAllIncentivesForPosition(s.Ctx, positionId) + claimedRewards, totalForfeitedRewards, scaledForfeitedRewardsByUptime, err := s.Clk.PrepareClaimAllIncentivesForPosition(s.Ctx, positionId) s.Require().NoError(err) s.Require().Equal(expectedRewards, claimedRewards) - s.Require().Equal(expectedForfeited, forfeitedRewards) + s.Require().Equal(expectedForfeited, totalForfeitedRewards) + s.checkForfeitedCoinsByUptime(totalForfeitedRewards, scaledForfeitedRewardsByUptime) // Sanity check that cannot claim again. - claimedRewards, _, err = s.Clk.PrepareClaimAllIncentivesForPosition(s.Ctx, positionId) + claimedRewards, _, _, err = s.Clk.PrepareClaimAllIncentivesForPosition(s.Ctx, positionId) s.Require().NoError(err) s.Require().Equal(sdk.Coins(nil), claimedRewards) @@ -2468,7 +2469,7 @@ func (s *KeeperTestSuite) TestTransferPositions() { s.addUptimeGrowthInsideRange(s.Ctx, pool.GetId(), apptesting.DefaultLowerTick+1, DefaultLowerTick, DefaultUpperTick, expectedUptimes.hundredTokensMultiDenom) s.AddToSpreadRewardAccumulator(pool.GetId(), sdk.NewDecCoin(ETH, osmomath.NewInt(10))) for _, positionId := range tc.positionsToTransfer { - _, _, err := s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, newOwner, positionId) + _, _, _, err := s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, newOwner, positionId) s.Require().NoError(err) _, err = s.App.ConcentratedLiquidityKeeper.CollectSpreadRewards(s.Ctx, newOwner, positionId) s.Require().NoError(err) From 606d741a57b6bfa08a3167ec67d856b55d10ba8d Mon Sep 17 00:00:00 2001 From: alpo Date: Fri, 15 Mar 2024 00:02:34 -0700 Subject: [PATCH 02/16] fix broken tests to follow new logic --- x/concentrated-liquidity/incentives_test.go | 4 +++- x/concentrated-liquidity/lp_test.go | 16 +++++++++------- x/concentrated-liquidity/msg_server_test.go | 12 ++++++------ 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index 046adf2bf25..9611e30b47e 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -2341,7 +2341,9 @@ func (s *KeeperTestSuite) TestQueryAndCollectIncentives() { s.Require().Equal(tc.expectedForfeitedIncentives.String(), actualIncetivesForfeited.String()) // Ensure balances are updated by the correct amounts - s.Require().Equal(tc.expectedIncentivesClaimed.Add(tc.expectedForfeitedIncentives...).String(), (incentivesBalanceBeforeCollect.Sub(incentivesBalanceAfterCollect...)).String()) + // Note that we expect the forfeited incentives to remain in the pool incentives balance since they are + // redeposited, so we only expect the diff in incentives balance to be the amount successfully claimed. + s.Require().Equal(tc.expectedIncentivesClaimed.String(), (incentivesBalanceBeforeCollect.Sub(incentivesBalanceAfterCollect...)).String()) s.Require().Equal(tc.expectedIncentivesClaimed.String(), (ownerBalancerAfterCollect.Sub(ownerBalancerBeforeCollect...)).String()) }) } diff --git a/x/concentrated-liquidity/lp_test.go b/x/concentrated-liquidity/lp_test.go index 8d7af29ab6a..4790ebd3a69 100644 --- a/x/concentrated-liquidity/lp_test.go +++ b/x/concentrated-liquidity/lp_test.go @@ -6,7 +6,6 @@ import ( "time" sdk "github.com/cosmos/cosmos-sdk/types" - distributiontypes "github.com/cosmos/cosmos-sdk/x/distribution/types" "github.com/osmosis-labs/osmosis/osmomath" "github.com/osmosis-labs/osmosis/osmoutils" @@ -597,8 +596,6 @@ func (s *KeeperTestSuite) TestWithdrawPosition() { s.FundAcc(pool.GetSpreadRewardsAddress(), expectedSpreadRewardsClaimed) } - communityPoolBalanceBefore := s.App.BankKeeper.GetAllBalances(s.Ctx, s.App.AccountKeeper.GetModuleAddress(distributiontypes.ModuleName)) - // Set expected incentives and fund pool with appropriate amount expectedIncentivesClaimed = expectedIncentivesFromUptimeGrowth(defaultUptimeGrowth, liquidityCreated, tc.timeElapsed, defaultMultiplier) @@ -642,14 +639,19 @@ func (s *KeeperTestSuite) TestWithdrawPosition() { poolSpreadRewardBalanceAfterWithdraw := s.App.BankKeeper.GetAllBalances(s.Ctx, pool.GetSpreadRewardsAddress()) incentivesBalanceAfterWithdraw := s.App.BankKeeper.GetAllBalances(s.Ctx, pool.GetIncentivesAddress()) ownerBalancerAfterWithdraw := s.App.BankKeeper.GetAllBalances(s.Ctx, owner) - communityPoolBalanceAfter := s.App.BankKeeper.GetAllBalances(s.Ctx, s.App.AccountKeeper.GetModuleAddress(distributiontypes.ModuleName)) + + // If the position was the last in the pool, we expect it to receive full incentives since there + // is nobody to forfeit to + updatedPool, err := concentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId()) + s.Require().NoError(err) + if updatedPool.GetLiquidity().LTE(sdk.OneDec()) { + expectedIncentivesClaimed = expectedFullIncentivesFromAllUptimes + } // owner should only have tokens equivalent to the delta balance of the pool expectedOwnerBalanceDelta := expectedPoolBalanceDelta.Add(expectedIncentivesClaimed...).Add(expectedSpreadRewardsClaimed...) actualOwnerBalancerDelta := ownerBalancerAfterWithdraw.Sub(ownerBalancerBeforeWithdraw...) - - communityPoolBalanceDelta := communityPoolBalanceAfter.Sub(communityPoolBalanceBefore...) - actualIncentivesClaimed := incentivesBalanceBeforeWithdraw.Sub(incentivesBalanceAfterWithdraw...).Sub(communityPoolBalanceDelta...) + actualIncentivesClaimed := incentivesBalanceBeforeWithdraw.Sub(incentivesBalanceAfterWithdraw...) s.Require().Equal(expectedPoolBalanceDelta.String(), poolBalanceBeforeWithdraw.Sub(poolBalanceAfterWithdraw...).String()) s.Require().NotEmpty(expectedOwnerBalanceDelta) diff --git a/x/concentrated-liquidity/msg_server_test.go b/x/concentrated-liquidity/msg_server_test.go index 10d5b5b1a2d..f1df042da0d 100644 --- a/x/concentrated-liquidity/msg_server_test.go +++ b/x/concentrated-liquidity/msg_server_test.go @@ -357,7 +357,7 @@ func (s *KeeperTestSuite) TestCollectIncentives_Events() { numPositionsToCreate: 1, expectedTotalCollectIncentivesEvent: 1, expectedCollectIncentivesEvent: 1, - expectedMessageEvents: 2, // 1 for collect send, 1 for forfeit send + expectedMessageEvents: 1, // 1 for collect send }, "two position IDs": { upperTick: DefaultUpperTick, @@ -366,7 +366,7 @@ func (s *KeeperTestSuite) TestCollectIncentives_Events() { numPositionsToCreate: 2, expectedTotalCollectIncentivesEvent: 1, expectedCollectIncentivesEvent: 2, - expectedMessageEvents: 4, // 2 for collect send, 2 for forfeit send + expectedMessageEvents: 2, // 2 for collect send }, "three position IDs": { upperTick: DefaultUpperTick, @@ -375,7 +375,7 @@ func (s *KeeperTestSuite) TestCollectIncentives_Events() { numPositionsToCreate: 3, expectedTotalCollectIncentivesEvent: 1, expectedCollectIncentivesEvent: 3, - expectedMessageEvents: 6, // 3 for collect send, 3 for forfeit send + expectedMessageEvents: 3, // 3 for collect send }, "error: three position IDs - not an owner": { upperTick: DefaultUpperTick, @@ -572,7 +572,7 @@ func (s *KeeperTestSuite) TestTransferPositions_Events() { hasIncentivesToClaim: true, numPositionsToCreate: 1, expectedTransferPositionsEvent: 1, - expectedMessageEvents: 2, // 1 for collect incentives claim send, 1 for collect incentives forfeit send + expectedMessageEvents: 1, // 1 for collect incentives claim send }, "single position ID with claimable spread rewards": { positionIds: []uint64{DefaultPositionId}, @@ -587,7 +587,7 @@ func (s *KeeperTestSuite) TestTransferPositions_Events() { hasSpreadRewardsToClaim: true, numPositionsToCreate: 1, expectedTransferPositionsEvent: 1, - expectedMessageEvents: 3, // 1 for collect incentives claim send, 1 for collect incentives forfeit send, 1 for collect spread rewards claim send + expectedMessageEvents: 2, // 1 for collect incentives claim send, 1 for collect spread rewards claim send }, "two position IDs": { positionIds: []uint64{DefaultPositionId, DefaultPositionId + 1}, @@ -605,7 +605,7 @@ func (s *KeeperTestSuite) TestTransferPositions_Events() { hasSpreadRewardsToClaim: true, numPositionsToCreate: 3, expectedTransferPositionsEvent: 1, - expectedMessageEvents: 9, // 3 for collect incentives claim send, 3 for collect incentives forfeit send, 3 for collect spread rewards claim send + expectedMessageEvents: 6, // 3 for collect incentives claim send, 3 for collect spread rewards claim send }, "two position IDs, second ID does not exist": { positionIds: []uint64{DefaultPositionId, DefaultPositionId + 1}, From 3b3e3cfc78de56d25874c9dd2529eafcbdd37a33 Mon Sep 17 00:00:00 2001 From: alpo Date: Fri, 15 Mar 2024 11:22:07 -0700 Subject: [PATCH 03/16] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d90cf9c1ecc..df6edf4f5d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#7685](https://github.com/osmosis-labs/osmosis/pull/7685) Speedup CL actions by only marshalling for CL hooks if they will be used. * [#7503](https://github.com/osmosis-labs/osmosis/pull/7503) Add IBC wasm light clients module * [#7689](https://github.com/osmosis-labs/osmosis/pull/7689) Make CL price estimations not cause state writes (speed and gas improvements) +* [#7746](https://github.com/osmosis-labs/osmosis/pull/7746) Make forfeited incentives redeposit into the pool instead of sending to community pool ## v23.0.5 & v23.0.5-iavl-v1 From c3ea6dff5bb7c2c41e77bb0a8a1b395b9dd5537a Mon Sep 17 00:00:00 2001 From: alpo Date: Sun, 17 Mar 2024 16:55:34 -0700 Subject: [PATCH 04/16] convert tracker for forfeited incentives from map to slice --- x/concentrated-liquidity/export_test.go | 4 ++-- x/concentrated-liquidity/incentives.go | 6 +++--- x/concentrated-liquidity/incentives_test.go | 11 ++++++----- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/x/concentrated-liquidity/export_test.go b/x/concentrated-liquidity/export_test.go index c56dee5c782..b512486f964 100644 --- a/x/concentrated-liquidity/export_test.go +++ b/x/concentrated-liquidity/export_test.go @@ -258,7 +258,7 @@ func (k Keeper) GetAllIncentiveRecordsForUptime(ctx sdk.Context, poolId uint64, return k.getAllIncentiveRecordsForUptime(ctx, poolId, minUptime) } -func (k Keeper) CollectIncentives(ctx sdk.Context, owner sdk.AccAddress, positionId uint64) (sdk.Coins, sdk.Coins, map[int]sdk.Coins, error) { +func (k Keeper) CollectIncentives(ctx sdk.Context, owner sdk.AccAddress, positionId uint64) (sdk.Coins, sdk.Coins, []sdk.Coins, error) { return k.collectIncentives(ctx, owner, positionId) } @@ -270,7 +270,7 @@ func UpdateAccumAndClaimRewards(accum *accum.AccumulatorObject, positionKey stri return updateAccumAndClaimRewards(accum, positionKey, growthOutside) } -func (k Keeper) PrepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, map[int]sdk.Coins, error) { +func (k Keeper) PrepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, []sdk.Coins, error) { return k.prepareClaimAllIncentivesForPosition(ctx, positionId) } diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index 567256992af..15f7717c307 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -753,7 +753,7 @@ func moveRewardsToNewPositionAndDeleteOldAcc(accum *accum.AccumulatorObject, old // The parent function (collectIncentives) does the actual bank sends for both the collected and forfeited incentives. // // Returns error if the position/uptime accumulators don't exist, or if there is an issue that arises while claiming. -func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, map[int]sdk.Coins, error) { +func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, []sdk.Coins, error) { // Retrieve the position with the given ID. position, err := k.GetPosition(ctx, positionId) if err != nil { @@ -800,7 +800,7 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId } // Loop through each uptime accumulator for the pool. - scaledForfeitedIncentivesByUptime := make(map[int]sdk.Coins) + scaledForfeitedIncentivesByUptime := make([]sdk.Coins, len(types.SupportedUptimes)) for uptimeIndex, uptimeAccum := range uptimeAccumulators { // Check if the accumulator contains the position. // There should never be a case where you can have a position for 1 accumulator, and not the rest. @@ -860,7 +860,7 @@ func (k Keeper) GetClaimableIncentives(ctx sdk.Context, positionId uint64) (sdk. // Returns error if: // - position with the given id does not exist // - other internal database or math errors. -func (k Keeper) collectIncentives(ctx sdk.Context, sender sdk.AccAddress, positionId uint64) (sdk.Coins, sdk.Coins, map[int]sdk.Coins, error) { +func (k Keeper) collectIncentives(ctx sdk.Context, sender sdk.AccAddress, positionId uint64) (sdk.Coins, sdk.Coins, []sdk.Coins, error) { // Retrieve the position with the given ID. position, err := k.GetPosition(ctx, positionId) if err != nil { diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index 9611e30b47e..3723a7f1eab 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -2779,13 +2779,14 @@ func (s *KeeperTestSuite) TestUpdateAccumAndClaimRewards() { // checkForfeitedCoinsByUptime checks that the sum of forfeited coins by uptime matches the expected total forfeited coins. // It adds up the Coins corresponding to each uptime in the map and asserts that the result is equal to the input totalForfeitedCoins. -func (s *KeeperTestSuite) checkForfeitedCoinsByUptime(totalForfeitedCoins sdk.Coins, scaledForfeitedCoinsByUptime map[int]sdk.Coins) { +func (s *KeeperTestSuite) checkForfeitedCoinsByUptime(totalForfeitedCoins sdk.Coins, scaledForfeitedCoinsByUptime []sdk.Coins) { forfeitedCoins := sdk.NewCoins() - // Iterate through the constant uptime indexes + // Iterate through uptime indexes and add up the forfeited coins from each for uptimeIndex := range types.SupportedUptimes { - // If there are forfeited coins for the current uptime, add them to the sum - if coins, ok := scaledForfeitedCoinsByUptime[uptimeIndex]; ok { - for _, coin := range coins { + // Check if the slice at the current uptime index is not empty, then add the coins to the sum + forfeitedCurrentUptime := scaledForfeitedCoinsByUptime[uptimeIndex] + if !forfeitedCurrentUptime.IsZero() { + for _, coin := range forfeitedCurrentUptime { // Scale down the actual forfeited coin amount scaledDownAmount := cl.ScaleDownIncentiveAmount(coin.Amount, cl.PerUnitLiqScalingFactor) forfeitedCoins = forfeitedCoins.Add(sdk.NewCoin(coin.Denom, scaledDownAmount)) From 0e5839f081b1195ec76c07f517287b96f3d675c7 Mon Sep 17 00:00:00 2001 From: alpo Date: Mon, 18 Mar 2024 21:41:22 -0700 Subject: [PATCH 05/16] clean up comments --- x/concentrated-liquidity/incentives.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index 15f7717c307..ddbaab54a67 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -827,7 +827,7 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId if positionAge < supportedUptimes[uptimeIndex] { // We track forfeited incentives by uptime accumulator to allow for efficient redepositing. // To avoid descaling and rescaling, we keep the forfeited incentives in scaled form. - // This is slightly unwieldy as it means we return a map of scaled coins, but doing it this way + // This is slightly unwieldy as it means we return a slice of scaled coins, but doing it this way // allows us to efficiently handle all cases related to forfeited incentives without recomputing // expensive operations. scaledForfeitedIncentivesByUptime[uptimeIndex] = collectedIncentivesForUptimeScaled From 06d1535c71183e95317e30215cf8a7cccbc3ad26 Mon Sep 17 00:00:00 2001 From: alpo Date: Mon, 18 Mar 2024 21:59:42 -0700 Subject: [PATCH 06/16] move new lp logic into helper --- x/concentrated-liquidity/incentives.go | 49 +++++++++++++++++++++ x/concentrated-liquidity/incentives_test.go | 6 +++ x/concentrated-liquidity/lp.go | 45 +------------------ x/concentrated-liquidity/position.go | 1 - 4 files changed, 57 insertions(+), 44 deletions(-) diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index ddbaab54a67..f60d8fa4319 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -846,6 +846,55 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId return collectedIncentivesForPosition, forfeitedIncentivesForPosition, scaledForfeitedIncentivesByUptime, nil } +// redepositForfeitedIncentives redeposits forfeited incentives into uptime accumulators or sends them to the owner if there's no active liquidity. +func (k Keeper) redepositForfeitedIncentives(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, scaledForfeitedIncentivesByUptime []sdk.Coins, totalForefeitedIncentives sdk.Coins) error { + // Fetch pool from state to check active liquidity. + pool, err := k.getPoolById(ctx, poolId) + if err != nil { + return err + } + activeLiquidity := pool.GetLiquidity() + + // If pool has active liquidity on current tick, redeposit forfeited incentives into uptime accumulators. + if activeLiquidity.GT(sdk.OneDec()) { + uptimeAccums, err := k.GetUptimeAccumulators(ctx, poolId) + if err != nil { + return err + } + + for uptimeIndex := range uptimeAccums { + fmt.Println("scaledForfeitedIncentivesByUptime: ", scaledForfeitedIncentivesByUptime) + curUptimeForfeited := scaledForfeitedIncentivesByUptime[uptimeIndex] + if curUptimeForfeited.IsZero() { + continue + } + + incentivesToAddToCurAccum := sdk.NewDecCoins() + for _, forfeitedCoin := range curUptimeForfeited { + // Calculate the amount to add to the accumulator by dividing the forfeited coin amount by the current uptime duration + forfeitedAmountPerLiquidity := forfeitedCoin.Amount.ToLegacyDec().QuoTruncate(activeLiquidity) + + // Create a DecCoin from the calculated amount + decCoinToAdd := sdk.NewDecCoinFromDec(forfeitedCoin.Denom, forfeitedAmountPerLiquidity) + + // Add the calculated DecCoin to the incentives to add to current accumulator + incentivesToAddToCurAccum = incentivesToAddToCurAccum.Add(decCoinToAdd) + } + + // Emit incentives to current uptime accumulator + uptimeAccums[uptimeIndex].AddToAccumulator(incentivesToAddToCurAccum) + } + } else { + // If no active liquidity, give the forfeited incentives to the sender. + err := k.bankKeeper.SendCoins(ctx, pool.GetIncentivesAddress(), owner, totalForefeitedIncentives) + if err != nil { + return err + } + } + + return nil +} + func (k Keeper) GetClaimableIncentives(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, error) { // Since this is a query, we don't want to modify the state and therefore use a cache context. cacheCtx, _ := ctx.CacheContext() diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index 3723a7f1eab..4ba7615d64d 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -2780,6 +2780,12 @@ func (s *KeeperTestSuite) TestUpdateAccumAndClaimRewards() { // checkForfeitedCoinsByUptime checks that the sum of forfeited coins by uptime matches the expected total forfeited coins. // It adds up the Coins corresponding to each uptime in the map and asserts that the result is equal to the input totalForfeitedCoins. func (s *KeeperTestSuite) checkForfeitedCoinsByUptime(totalForfeitedCoins sdk.Coins, scaledForfeitedCoinsByUptime []sdk.Coins) { + // Exit early if scaledForfeitedCoinsByUptime is empty + if len(scaledForfeitedCoinsByUptime) == 0 { + s.Require().Equal(totalForfeitedCoins, sdk.NewCoins()) + return + } + forfeitedCoins := sdk.NewCoins() // Iterate through uptime indexes and add up the forfeited coins from each for uptimeIndex := range types.SupportedUptimes { diff --git a/x/concentrated-liquidity/lp.go b/x/concentrated-liquidity/lp.go index 625824a9e85..06abc87d58d 100644 --- a/x/concentrated-liquidity/lp.go +++ b/x/concentrated-liquidity/lp.go @@ -289,52 +289,11 @@ func (k Keeper) WithdrawPosition(ctx sdk.Context, owner sdk.AccAddress, position return osmomath.Int{}, osmomath.Int{}, err } - // Re-fetch pool from state to check updated active liquidity. - pool, err = k.getPoolById(ctx, position.PoolId) + // If the position has any forfeited incentives, re-deposit them into the pool. + err = k.redepositForfeitedIncentives(ctx, position.PoolId, owner, scaledForfeitedIncentivesByUptime, totalForefeitedIncentives) if err != nil { return osmomath.Int{}, osmomath.Int{}, err } - activeLiquidity := pool.GetLiquidity() - - // If pool has active liquidity on current tick, redeposit forfeited incentives into uptime accumulators. - // TODO: abstract this into a `redepositForfeitedIncentives` helper to declutter withdrawal logic - if activeLiquidity.GT(sdk.OneDec()) { - // Get uptime accums - uptimeAccums, err := k.GetUptimeAccumulators(ctx, position.PoolId) - if err != nil { - return osmomath.Int{}, osmomath.Int{}, err - } - - for uptimeIndex := range uptimeAccums { - // Get relevant uptime-level values - curUptimeForfeited := scaledForfeitedIncentivesByUptime[uptimeIndex] - if curUptimeForfeited.IsZero() { - continue - } - - // Loop through forfeited coins for current uptime - incentivesToAddToCurAccum := sdk.NewDecCoins() - for _, forfeitedCoin := range curUptimeForfeited { - // Calculate the amount to add to the accumulator by dividing the forfeited coin amount by the current uptime duration - forfeitedAmountPerLiquidity := forfeitedCoin.Amount.ToLegacyDec().QuoTruncate(activeLiquidity) - - // Create a DecCoin from the calculated amount - decCoinToAdd := sdk.NewDecCoinFromDec(forfeitedCoin.Denom, forfeitedAmountPerLiquidity) - - // Add the calculated DecCoin to the incentives to add to current accumulator - incentivesToAddToCurAccum = incentivesToAddToCurAccum.Add(decCoinToAdd) - } - - // Emit incentives to current uptime accumulator - uptimeAccums[uptimeIndex].AddToAccumulator(incentivesToAddToCurAccum) - } - } else { - // If no active liquidity, give the forfeited incentives to the sender. - err = k.bankKeeper.SendCoins(ctx, pool.GetIncentivesAddress(), owner, totalForefeitedIncentives) - if err != nil { - return osmomath.Int{}, osmomath.Int{}, err - } - } // If the requested liquidity amount to withdraw is equal to the available liquidity, delete the position from state. // Ensure we collect any outstanding spread factors and incentives prior to deleting the position from state. This claiming diff --git a/x/concentrated-liquidity/position.go b/x/concentrated-liquidity/position.go index 38fc5b1335d..468340ade24 100644 --- a/x/concentrated-liquidity/position.go +++ b/x/concentrated-liquidity/position.go @@ -709,7 +709,6 @@ func (k Keeper) transferPositions(ctx sdk.Context, positionIds []uint64, sender return err } - // TODO: handle redepositing of forfeited incentives if _, _, _, err := k.collectIncentives(ctx, sender, positionId); err != nil { return err } From 16022dd701d71a727108cc1995b7886a3e8b0d2b Mon Sep 17 00:00:00 2001 From: alpo Date: Mon, 18 Mar 2024 22:18:35 -0700 Subject: [PATCH 07/16] remove prints --- x/concentrated-liquidity/incentives.go | 1 - 1 file changed, 1 deletion(-) diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index f60d8fa4319..dbe0d368ce6 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -863,7 +863,6 @@ func (k Keeper) redepositForfeitedIncentives(ctx sdk.Context, poolId uint64, own } for uptimeIndex := range uptimeAccums { - fmt.Println("scaledForfeitedIncentivesByUptime: ", scaledForfeitedIncentivesByUptime) curUptimeForfeited := scaledForfeitedIncentivesByUptime[uptimeIndex] if curUptimeForfeited.IsZero() { continue From 3896fc7394b14494eedc8de1f64b39ca49406304 Mon Sep 17 00:00:00 2001 From: alpo Date: Mon, 18 Mar 2024 22:40:32 -0700 Subject: [PATCH 08/16] clean up helper logic --- x/concentrated-liquidity/incentives.go | 55 ++++++++++++++------------ 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index dbe0d368ce6..e31fc8685f7 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -855,40 +855,45 @@ func (k Keeper) redepositForfeitedIncentives(ctx sdk.Context, poolId uint64, own } activeLiquidity := pool.GetLiquidity() - // If pool has active liquidity on current tick, redeposit forfeited incentives into uptime accumulators. - if activeLiquidity.GT(sdk.OneDec()) { - uptimeAccums, err := k.GetUptimeAccumulators(ctx, poolId) + // If no active liquidity, give the forfeited incentives to the sender. + if activeLiquidity.LT(sdk.OneDec()) { + err := k.bankKeeper.SendCoins(ctx, pool.GetIncentivesAddress(), owner, totalForefeitedIncentives) if err != nil { return err } + return nil + } - for uptimeIndex := range uptimeAccums { - curUptimeForfeited := scaledForfeitedIncentivesByUptime[uptimeIndex] - if curUptimeForfeited.IsZero() { - continue - } + // If pool has active liquidity on current tick, redeposit forfeited incentives into uptime accumulators. + uptimeAccums, err := k.GetUptimeAccumulators(ctx, poolId) + if err != nil { + return err + } - incentivesToAddToCurAccum := sdk.NewDecCoins() - for _, forfeitedCoin := range curUptimeForfeited { - // Calculate the amount to add to the accumulator by dividing the forfeited coin amount by the current uptime duration - forfeitedAmountPerLiquidity := forfeitedCoin.Amount.ToLegacyDec().QuoTruncate(activeLiquidity) + // Loop through each uptime accumulator for the pool and redeposit forfeited incentives. + for uptimeIndex := range uptimeAccums { + curUptimeForfeited := scaledForfeitedIncentivesByUptime[uptimeIndex] + if curUptimeForfeited.IsZero() { + continue + } - // Create a DecCoin from the calculated amount - decCoinToAdd := sdk.NewDecCoinFromDec(forfeitedCoin.Denom, forfeitedAmountPerLiquidity) + // Note that this logic is a simplified version of the regular incentive distribution logic. + // It leans on the fact that the tracked forfeited incentives are already scaled appropriately + // so we do not need to run any additional computations beyond diving by the active liquidity. + incentivesToAddToCurAccum := sdk.NewDecCoins() + for _, forfeitedCoin := range curUptimeForfeited { + // Calculate the amount to add to the accumulator by dividing the forfeited coin amount by the current uptime duration + forfeitedAmountPerLiquidity := forfeitedCoin.Amount.ToLegacyDec().QuoTruncate(activeLiquidity) - // Add the calculated DecCoin to the incentives to add to current accumulator - incentivesToAddToCurAccum = incentivesToAddToCurAccum.Add(decCoinToAdd) - } + // Create a DecCoin from the calculated amount + decCoinToAdd := sdk.NewDecCoinFromDec(forfeitedCoin.Denom, forfeitedAmountPerLiquidity) - // Emit incentives to current uptime accumulator - uptimeAccums[uptimeIndex].AddToAccumulator(incentivesToAddToCurAccum) - } - } else { - // If no active liquidity, give the forfeited incentives to the sender. - err := k.bankKeeper.SendCoins(ctx, pool.GetIncentivesAddress(), owner, totalForefeitedIncentives) - if err != nil { - return err + // Add the calculated DecCoin to the incentives to add to current accumulator + incentivesToAddToCurAccum = incentivesToAddToCurAccum.Add(decCoinToAdd) } + + // Emit incentives to current uptime accumulator + uptimeAccums[uptimeIndex].AddToAccumulator(incentivesToAddToCurAccum) } return nil From 1202f7844911a5e7586b2adf8607832583bdaf67 Mon Sep 17 00:00:00 2001 From: alpo Date: Mon, 18 Mar 2024 22:45:40 -0700 Subject: [PATCH 09/16] clean up test comments --- x/concentrated-liquidity/incentives_test.go | 3 ++- x/concentrated-liquidity/lp_test.go | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index 760259c7bab..1a7df6ea801 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -2784,6 +2784,8 @@ func (s *KeeperTestSuite) checkForfeitedCoinsByUptime(totalForfeitedCoins sdk.Co forfeitedCoins := sdk.NewCoins() // Iterate through uptime indexes and add up the forfeited coins from each + // We unfortunately need to through each coin individually to properly scale down the amount + // (doing it in bulk leads to inconsistent rounding error) for uptimeIndex := range types.SupportedUptimes { // Check if the slice at the current uptime index is not empty, then add the coins to the sum forfeitedCurrentUptime := scaledForfeitedCoinsByUptime[uptimeIndex] @@ -2796,7 +2798,6 @@ func (s *KeeperTestSuite) checkForfeitedCoinsByUptime(totalForfeitedCoins sdk.Co } } - // Use Equal for assertion to compare the totalForfeitedCoins with the scaled down summedCoins s.Require().Equal(totalForfeitedCoins, forfeitedCoins, "Total forfeited coins do not match the sum of forfeited coins by uptime after scaling down") } diff --git a/x/concentrated-liquidity/lp_test.go b/x/concentrated-liquidity/lp_test.go index 4790ebd3a69..d779ac8fd2b 100644 --- a/x/concentrated-liquidity/lp_test.go +++ b/x/concentrated-liquidity/lp_test.go @@ -2,7 +2,6 @@ package concentrated_liquidity_test import ( "errors" - "fmt" "time" sdk "github.com/cosmos/cosmos-sdk/types" @@ -658,7 +657,7 @@ func (s *KeeperTestSuite) TestWithdrawPosition() { for _, coin := range expectedOwnerBalanceDelta { expected := expectedOwnerBalanceDelta.AmountOf(coin.Denom) actual := actualOwnerBalancerDelta.AmountOf(coin.Denom) - s.Require().True(expected.Equal(actual), fmt.Sprintf("expected: %s, got: %s", expected.String(), actual.String())) + s.Require().True(expected.Equal(actual)) } if tc.timeElapsed > 0 { From 83d2e92c60679c3e0f7ef944e42f4c041471e229 Mon Sep 17 00:00:00 2001 From: alpo Date: Tue, 19 Mar 2024 12:05:01 -0700 Subject: [PATCH 10/16] tests for new helper --- x/concentrated-liquidity/export_test.go | 4 + x/concentrated-liquidity/incentives.go | 4 + x/concentrated-liquidity/incentives_test.go | 136 ++++++++++++++++++++ x/concentrated-liquidity/types/errors.go | 9 ++ 4 files changed, 153 insertions(+) diff --git a/x/concentrated-liquidity/export_test.go b/x/concentrated-liquidity/export_test.go index af7099286c5..f2c1cf8120b 100644 --- a/x/concentrated-liquidity/export_test.go +++ b/x/concentrated-liquidity/export_test.go @@ -359,3 +359,7 @@ func (k Keeper) GetIncentiveScalingFactorForPool(ctx sdk.Context, poolID uint64) func ScaleDownIncentiveAmount(incentiveAmount osmomath.Int, scalingFactor osmomath.Dec) (scaledTotalEmittedAmount osmomath.Int) { return scaleDownIncentiveAmount(incentiveAmount, scalingFactor) } + +func (k Keeper) RedepositForfeitedIncentives(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, scaledForfeitedIncentivesByUptime []sdk.Coins, totalForefeitedIncentives sdk.Coins) error { + return k.redepositForfeitedIncentives(ctx, poolId, owner, scaledForfeitedIncentivesByUptime, totalForefeitedIncentives) +} diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index e31fc8685f7..b0f07094639 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -848,6 +848,10 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId // redepositForfeitedIncentives redeposits forfeited incentives into uptime accumulators or sends them to the owner if there's no active liquidity. func (k Keeper) redepositForfeitedIncentives(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, scaledForfeitedIncentivesByUptime []sdk.Coins, totalForefeitedIncentives sdk.Coins) error { + if len(scaledForfeitedIncentivesByUptime) != len(types.SupportedUptimes) { + return types.InvalidForfeitedIncentivesLengthError{ForfeitedIncentivesLength: len(scaledForfeitedIncentivesByUptime), ExpectedLength: len(types.SupportedUptimes)} + } + // Fetch pool from state to check active liquidity. pool, err := k.getPoolById(ctx, poolId) if err != nil { diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index 1a7df6ea801..a1a5b4af87a 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -3794,3 +3794,139 @@ func (s *KeeperTestSuite) scaleUptimeAccumulators(uptimeAccumulatorsToScale []sd return growthCopy } + +// assertUptimeAccumsEmpty asserts that the uptime accumulators for the given pool are empty. +func (s *KeeperTestSuite) assertUptimeAccumsEmpty(poolId uint64) { + uptimeAccums, err := s.App.ConcentratedLiquidityKeeper.GetUptimeAccumulators(s.Ctx, poolId) + s.Require().NoError(err) + + // Ensure uptime accums remain empty + for _, accum := range uptimeAccums { + s.Require().Equal(sdk.NewDecCoins(), sdk.NewDecCoins(accum.GetValue()...)) + } +} + +// TestRedepositForfeitedIncentives tests the redeposit of forfeited incentives into uptime accumulators. +// In the cases where the pool has active liquidity and the incentives need to be redeposited, +// it asserts that forfeitedIncentives / activeLiquidity was deposited into the accumulators. +// In the cases where the pool has no active liquidity, it asserts that the forfeited incentives were sent to the owner. +func (s *KeeperTestSuite) TestRedepositForfeitedIncentives() { + tests := map[string]struct { + setupPoolWithActiveLiquidity bool + forfeitedIncentives []sdk.Coins + expectedError error + }{ + "No forfeited incentives": { + setupPoolWithActiveLiquidity: true, + forfeitedIncentives: []sdk.Coins{sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, + }, + "With active liquidity - forfeited incentives redeposited": { + setupPoolWithActiveLiquidity: true, + forfeitedIncentives: []sdk.Coins{{sdk.NewCoin("foo", sdk.NewInt(12345))}, sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, + }, + "Multiple forfeited incentives redeposited": { + setupPoolWithActiveLiquidity: true, + forfeitedIncentives: []sdk.Coins{sdk.NewCoins(), {sdk.NewCoin("bar", sdk.NewInt(54321))}, sdk.NewCoins(), {sdk.NewCoin("foo", sdk.NewInt(12345))}}, + }, + "All slots filled with forfeited incentives": { + setupPoolWithActiveLiquidity: true, + forfeitedIncentives: []sdk.Coins{{sdk.NewCoin("foo", sdk.NewInt(10000))}, {sdk.NewCoin("bar", sdk.NewInt(20000))}, {sdk.NewCoin("baz", sdk.NewInt(30000))}, {sdk.NewCoin("qux", sdk.NewInt(40000))}}, + }, + "No active liquidity with no forfeited incentives": { + setupPoolWithActiveLiquidity: false, + forfeitedIncentives: []sdk.Coins{sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, + }, + "No active liquidity with forfeited incentives sent to owner": { + setupPoolWithActiveLiquidity: false, + forfeitedIncentives: []sdk.Coins{{sdk.NewCoin("foo", sdk.NewInt(10000))}, sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, + }, + "Incorrect forfeited incentives length": { + setupPoolWithActiveLiquidity: true, + forfeitedIncentives: []sdk.Coins{sdk.NewCoins()}, // Incorrect length, should be len(types.SupportedUptimes) + expectedError: types.InvalidForfeitedIncentivesLengthError{ForfeitedIncentivesLength: 1, ExpectedLength: len(types.SupportedUptimes)}, + }, + } + + for name, tc := range tests { + s.Run(name, func() { + s.SetupTest() + + // --- Setup --- + + // Setup pool + pool := s.PrepareConcentratedPool() + poolId := pool.GetId() + owner := s.TestAccs[0] + + if tc.setupPoolWithActiveLiquidity { + // Add position to ensure pool has active liquidity + s.SetupDefaultPosition(poolId) + } + + // Fund pool with forfeited incentives and track total + totalForfeitedIncentives := sdk.NewCoins() + for _, coins := range tc.forfeitedIncentives { + s.FundAcc(pool.GetIncentivesAddress(), coins) + totalForfeitedIncentives = totalForfeitedIncentives.Add(coins...) + } + + // Get balances before the operation to compare after + balancesBefore := s.App.BankKeeper.GetAllBalances(s.Ctx, owner) + + // --- System under test --- + + err := s.App.ConcentratedLiquidityKeeper.RedepositForfeitedIncentives(s.Ctx, poolId, owner, tc.forfeitedIncentives, totalForfeitedIncentives) + + // --- Assertions --- + + balancesAfter := s.App.BankKeeper.GetAllBalances(s.Ctx, owner) + balanceChange := balancesAfter.Sub(balancesBefore...) + + // If an error is expected, check if it matches the expected error + if tc.expectedError != nil { + s.Require().ErrorContains(err, tc.expectedError.Error()) + + // Check if the owner's balance did not change + s.Require().Equal(sdk.NewCoins(), sdk.NewCoins(balanceChange...)) + + // Assert that uptime accumulators remain empty + s.assertUptimeAccumsEmpty(poolId) + + return + } + + // If there is no active liquidity, the forfeited incentives should be sent to the owner + if !tc.setupPoolWithActiveLiquidity { + // Check if the owner received the forfeited incentives + s.Require().Equal(totalForfeitedIncentives, balanceChange) + + // Assert that uptime accumulators remain empty + s.assertUptimeAccumsEmpty(poolId) + + return + } + + // If there is active liquidity, the forfeited incentives should not + // be sent to the owner, but instead redeposited into the uptime accumulators. + s.Require().Equal(sdk.NewCoins(), sdk.NewCoins(balanceChange...)) + + // Refetch updated pool and accumulators + pool, err = s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, poolId) + s.Require().NoError(err) + activeLiquidity := pool.GetLiquidity() + uptimeAccums, err := s.App.ConcentratedLiquidityKeeper.GetUptimeAccumulators(s.Ctx, poolId) + s.Require().NoError(err) + + // Check if the forfeited incentives were redeposited into the uptime accumulators + for i, coins := range tc.forfeitedIncentives { + // Check that each accumulator has the correct value of scaledForfeitedIncentives / activeLiquidity + // Note that the function assumed the input slice is already scaled to avoid unnecessary recomputation. + for _, forfeitedCoin := range coins { + expectedAmount := forfeitedCoin.Amount.ToLegacyDec().QuoTruncate(activeLiquidity) + accumAmount := uptimeAccums[i].GetValue().AmountOf(forfeitedCoin.Denom) + s.Require().Equal(expectedAmount, accumAmount, "Forfeited incentive amount mismatch in uptime accumulator") + } + } + }) + } +} diff --git a/x/concentrated-liquidity/types/errors.go b/x/concentrated-liquidity/types/errors.go index c61a268551d..c464429f3f8 100644 --- a/x/concentrated-liquidity/types/errors.go +++ b/x/concentrated-liquidity/types/errors.go @@ -947,3 +947,12 @@ type IncentiveEmissionOvrflowError struct { func (e IncentiveEmissionOvrflowError) Error() string { return fmt.Sprintf("either too much time has passed since last pool update or the emission rate is too high, causing overflow: %s", e.PanicMessage) } + +type InvalidForfeitedIncentivesLengthError struct { + ForfeitedIncentivesLength int + ExpectedLength int +} + +func (e InvalidForfeitedIncentivesLengthError) Error() string { + return fmt.Sprintf("attempted to redeposit incorrectly constructed forfeited incentives slice. forfeited incentives must have an entry for each supported uptime. forfeit entries: %d, expected: %d", e.ForfeitedIncentivesLength, e.ExpectedLength) +} From 704882d2f47cb8661e5ad69ebf5b03e070473c33 Mon Sep 17 00:00:00 2001 From: alpo Date: Tue, 19 Mar 2024 12:05:34 -0700 Subject: [PATCH 11/16] further test cleanup --- x/concentrated-liquidity/incentives_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index a1a5b4af87a..c4a275d7b7d 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -3891,7 +3891,6 @@ func (s *KeeperTestSuite) TestRedepositForfeitedIncentives() { // Assert that uptime accumulators remain empty s.assertUptimeAccumsEmpty(poolId) - return } @@ -3902,7 +3901,6 @@ func (s *KeeperTestSuite) TestRedepositForfeitedIncentives() { // Assert that uptime accumulators remain empty s.assertUptimeAccumsEmpty(poolId) - return } @@ -3913,7 +3911,6 @@ func (s *KeeperTestSuite) TestRedepositForfeitedIncentives() { // Refetch updated pool and accumulators pool, err = s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, poolId) s.Require().NoError(err) - activeLiquidity := pool.GetLiquidity() uptimeAccums, err := s.App.ConcentratedLiquidityKeeper.GetUptimeAccumulators(s.Ctx, poolId) s.Require().NoError(err) @@ -3922,7 +3919,7 @@ func (s *KeeperTestSuite) TestRedepositForfeitedIncentives() { // Check that each accumulator has the correct value of scaledForfeitedIncentives / activeLiquidity // Note that the function assumed the input slice is already scaled to avoid unnecessary recomputation. for _, forfeitedCoin := range coins { - expectedAmount := forfeitedCoin.Amount.ToLegacyDec().QuoTruncate(activeLiquidity) + expectedAmount := forfeitedCoin.Amount.ToLegacyDec().QuoTruncate(pool.GetLiquidity()) accumAmount := uptimeAccums[i].GetValue().AmountOf(forfeitedCoin.Denom) s.Require().Equal(expectedAmount, accumAmount, "Forfeited incentive amount mismatch in uptime accumulator") } From a19e98dea66c9e7517562cb0dd1092cc6daf65cc Mon Sep 17 00:00:00 2001 From: alpo Date: Tue, 19 Mar 2024 12:28:31 -0700 Subject: [PATCH 12/16] simplify test helper logic --- x/concentrated-liquidity/incentives_test.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index c4a275d7b7d..655ddc56f58 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -2787,14 +2787,10 @@ func (s *KeeperTestSuite) checkForfeitedCoinsByUptime(totalForfeitedCoins sdk.Co // We unfortunately need to through each coin individually to properly scale down the amount // (doing it in bulk leads to inconsistent rounding error) for uptimeIndex := range types.SupportedUptimes { - // Check if the slice at the current uptime index is not empty, then add the coins to the sum - forfeitedCurrentUptime := scaledForfeitedCoinsByUptime[uptimeIndex] - if !forfeitedCurrentUptime.IsZero() { - for _, coin := range forfeitedCurrentUptime { - // Scale down the actual forfeited coin amount - scaledDownAmount := cl.ScaleDownIncentiveAmount(coin.Amount, cl.PerUnitLiqScalingFactor) - forfeitedCoins = forfeitedCoins.Add(sdk.NewCoin(coin.Denom, scaledDownAmount)) - } + for _, coin := range scaledForfeitedCoinsByUptime[uptimeIndex] { + // Scale down the actual forfeited coin amount + scaledDownAmount := cl.ScaleDownIncentiveAmount(coin.Amount, cl.PerUnitLiqScalingFactor) + forfeitedCoins = forfeitedCoins.Add(sdk.NewCoin(coin.Denom, scaledDownAmount)) } } From 73ab1d561dc4382f4bfbe30acbb37a5e1ec51729 Mon Sep 17 00:00:00 2001 From: alpo Date: Tue, 19 Mar 2024 12:51:05 -0700 Subject: [PATCH 13/16] fix tests to work with new supported uptimes --- x/concentrated-liquidity/incentives_test.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index b510fef20c3..4f6290e8596 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -3818,27 +3818,27 @@ func (s *KeeperTestSuite) TestRedepositForfeitedIncentives() { }{ "No forfeited incentives": { setupPoolWithActiveLiquidity: true, - forfeitedIncentives: []sdk.Coins{sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, + forfeitedIncentives: []sdk.Coins{sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, }, "With active liquidity - forfeited incentives redeposited": { setupPoolWithActiveLiquidity: true, - forfeitedIncentives: []sdk.Coins{{sdk.NewCoin("foo", sdk.NewInt(12345))}, sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, + forfeitedIncentives: []sdk.Coins{{sdk.NewCoin("foo", sdk.NewInt(12345))}, sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, }, "Multiple forfeited incentives redeposited": { setupPoolWithActiveLiquidity: true, - forfeitedIncentives: []sdk.Coins{sdk.NewCoins(), {sdk.NewCoin("bar", sdk.NewInt(54321))}, sdk.NewCoins(), {sdk.NewCoin("foo", sdk.NewInt(12345))}}, + forfeitedIncentives: []sdk.Coins{sdk.NewCoins(), {sdk.NewCoin("bar", sdk.NewInt(54321))}, sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), {sdk.NewCoin("foo", sdk.NewInt(12345))}}, }, "All slots filled with forfeited incentives": { setupPoolWithActiveLiquidity: true, - forfeitedIncentives: []sdk.Coins{{sdk.NewCoin("foo", sdk.NewInt(10000))}, {sdk.NewCoin("bar", sdk.NewInt(20000))}, {sdk.NewCoin("baz", sdk.NewInt(30000))}, {sdk.NewCoin("qux", sdk.NewInt(40000))}}, + forfeitedIncentives: []sdk.Coins{{sdk.NewCoin("foo", sdk.NewInt(10000))}, {sdk.NewCoin("bar", sdk.NewInt(20000))}, {sdk.NewCoin("baz", sdk.NewInt(30000))}, {sdk.NewCoin("qux", sdk.NewInt(40000))}, {sdk.NewCoin("quux", sdk.NewInt(50000))}, {sdk.NewCoin("corge", sdk.NewInt(60000))}}, }, "No active liquidity with no forfeited incentives": { setupPoolWithActiveLiquidity: false, - forfeitedIncentives: []sdk.Coins{sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, + forfeitedIncentives: []sdk.Coins{sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, }, "No active liquidity with forfeited incentives sent to owner": { setupPoolWithActiveLiquidity: false, - forfeitedIncentives: []sdk.Coins{{sdk.NewCoin("foo", sdk.NewInt(10000))}, sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, + forfeitedIncentives: []sdk.Coins{{sdk.NewCoin("foo", sdk.NewInt(10000))}, sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins(), sdk.NewCoins()}, }, "Incorrect forfeited incentives length": { setupPoolWithActiveLiquidity: true, @@ -3894,6 +3894,8 @@ func (s *KeeperTestSuite) TestRedepositForfeitedIncentives() { return } + s.Require().NoError(err) + // If there is no active liquidity, the forfeited incentives should be sent to the owner if !tc.setupPoolWithActiveLiquidity { // Check if the owner received the forfeited incentives @@ -3915,12 +3917,12 @@ func (s *KeeperTestSuite) TestRedepositForfeitedIncentives() { s.Require().NoError(err) // Check if the forfeited incentives were redeposited into the uptime accumulators - for i, coins := range tc.forfeitedIncentives { + for i, accum := range uptimeAccums { // Check that each accumulator has the correct value of scaledForfeitedIncentives / activeLiquidity // Note that the function assumed the input slice is already scaled to avoid unnecessary recomputation. - for _, forfeitedCoin := range coins { + for _, forfeitedCoin := range tc.forfeitedIncentives[i] { expectedAmount := forfeitedCoin.Amount.ToLegacyDec().QuoTruncate(pool.GetLiquidity()) - accumAmount := uptimeAccums[i].GetValue().AmountOf(forfeitedCoin.Denom) + accumAmount := accum.GetValue().AmountOf(forfeitedCoin.Denom) s.Require().Equal(expectedAmount, accumAmount, "Forfeited incentive amount mismatch in uptime accumulator") } } From 6e8f5cfa8fb948641523d263b94803dda92bf197 Mon Sep 17 00:00:00 2001 From: alpo Date: Tue, 19 Mar 2024 13:35:54 -0700 Subject: [PATCH 14/16] add thorough godoc for redepositing logic --- x/concentrated-liquidity/incentives.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index b0f07094639..39cf17b701e 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -828,8 +828,7 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId // We track forfeited incentives by uptime accumulator to allow for efficient redepositing. // To avoid descaling and rescaling, we keep the forfeited incentives in scaled form. // This is slightly unwieldy as it means we return a slice of scaled coins, but doing it this way - // allows us to efficiently handle all cases related to forfeited incentives without recomputing - // expensive operations. + // allows us to efficiently handle all cases related to forfeited incentives. scaledForfeitedIncentivesByUptime[uptimeIndex] = collectedIncentivesForUptimeScaled // If the age of the position is less than the current uptime we are iterating through, then the position's @@ -846,8 +845,19 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId return collectedIncentivesForPosition, forfeitedIncentivesForPosition, scaledForfeitedIncentivesByUptime, nil } -// redepositForfeitedIncentives redeposits forfeited incentives into uptime accumulators or sends them to the owner if there's no active liquidity. -func (k Keeper) redepositForfeitedIncentives(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, scaledForfeitedIncentivesByUptime []sdk.Coins, totalForefeitedIncentives sdk.Coins) error { +// redepositForfeitedIncentives handles logic for redepositing forfeited incentives for a given pool. +// Specifically, it implements the following flows: +// - If there is no remaining active liquidity, the forfeited incentives are sent back to the sender. +// - If there is active liquidity, the forfeited incentives are redeposited into the uptime accumulators. +// Since forfeits are already being tracked in "scaled form", we do not need to do any additional scaling +// and simply deposit amount / activeLiquidity into the uptime accumulators. +// +// Returns error if: +// * Pool with the given ID does not exist +// * Uptime accumulators for the pool cannot be retrieved +// * Forfeited incentives length does not match the supported uptimes (defense in depth, should never happen) +// * Bank send fails +func (k Keeper) redepositForfeitedIncentives(ctx sdk.Context, poolId uint64, sender sdk.AccAddress, scaledForfeitedIncentivesByUptime []sdk.Coins, totalForefeitedIncentives sdk.Coins) error { if len(scaledForfeitedIncentivesByUptime) != len(types.SupportedUptimes) { return types.InvalidForfeitedIncentivesLengthError{ForfeitedIncentivesLength: len(scaledForfeitedIncentivesByUptime), ExpectedLength: len(types.SupportedUptimes)} } @@ -861,7 +871,7 @@ func (k Keeper) redepositForfeitedIncentives(ctx sdk.Context, poolId uint64, own // If no active liquidity, give the forfeited incentives to the sender. if activeLiquidity.LT(sdk.OneDec()) { - err := k.bankKeeper.SendCoins(ctx, pool.GetIncentivesAddress(), owner, totalForefeitedIncentives) + err := k.bankKeeper.SendCoins(ctx, pool.GetIncentivesAddress(), sender, totalForefeitedIncentives) if err != nil { return err } From 89eac5111ee5561ef397a127411380c1a78098e9 Mon Sep 17 00:00:00 2001 From: Alpo <62043214+AlpinYukseloglu@users.noreply.github.com> Date: Tue, 19 Mar 2024 16:55:32 -0700 Subject: [PATCH 15/16] apply comment fixes from review Co-authored-by: Adam Tucker --- x/concentrated-liquidity/incentives.go | 2 +- x/concentrated-liquidity/incentives_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index 39cf17b701e..0a74756b7d9 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -893,7 +893,7 @@ func (k Keeper) redepositForfeitedIncentives(ctx sdk.Context, poolId uint64, sen // Note that this logic is a simplified version of the regular incentive distribution logic. // It leans on the fact that the tracked forfeited incentives are already scaled appropriately - // so we do not need to run any additional computations beyond diving by the active liquidity. + // so we do not need to run any additional computations beyond dividing by the active liquidity. incentivesToAddToCurAccum := sdk.NewDecCoins() for _, forfeitedCoin := range curUptimeForfeited { // Calculate the amount to add to the accumulator by dividing the forfeited coin amount by the current uptime duration diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index 4f6290e8596..5fb184aa14d 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -2788,7 +2788,7 @@ func (s *KeeperTestSuite) checkForfeitedCoinsByUptime(totalForfeitedCoins sdk.Co forfeitedCoins := sdk.NewCoins() // Iterate through uptime indexes and add up the forfeited coins from each - // We unfortunately need to through each coin individually to properly scale down the amount + // We unfortunately need to iterate through each coin individually to properly scale down the amount // (doing it in bulk leads to inconsistent rounding error) for uptimeIndex := range types.SupportedUptimes { for _, coin := range scaledForfeitedCoinsByUptime[uptimeIndex] { From 6d48912cb9aca7641421dba781e37d408dd8afdf Mon Sep 17 00:00:00 2001 From: alpo Date: Tue, 19 Mar 2024 17:20:06 -0700 Subject: [PATCH 16/16] further clean up comments --- x/concentrated-liquidity/incentives.go | 2 +- x/concentrated-liquidity/incentives_test.go | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/x/concentrated-liquidity/incentives.go b/x/concentrated-liquidity/incentives.go index 39cf17b701e..368778b351d 100644 --- a/x/concentrated-liquidity/incentives.go +++ b/x/concentrated-liquidity/incentives.go @@ -916,7 +916,7 @@ func (k Keeper) redepositForfeitedIncentives(ctx sdk.Context, poolId uint64, sen func (k Keeper) GetClaimableIncentives(ctx sdk.Context, positionId uint64) (sdk.Coins, sdk.Coins, error) { // Since this is a query, we don't want to modify the state and therefore use a cache context. cacheCtx, _ := ctx.CacheContext() - // We omit the by-uptime forfeited incentives map as it is not needed for this query. + // We omit the by-uptime forfeited incentives slice as it is not needed for this query. collectedIncentives, forfeitedIncentives, _, err := k.prepareClaimAllIncentivesForPosition(cacheCtx, positionId) return collectedIncentives, forfeitedIncentives, err } diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index 4f6290e8596..75a0e493766 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/x/concentrated-liquidity/incentives_test.go @@ -3851,8 +3851,6 @@ func (s *KeeperTestSuite) TestRedepositForfeitedIncentives() { s.Run(name, func() { s.SetupTest() - // --- Setup --- - // Setup pool pool := s.PrepareConcentratedPool() poolId := pool.GetId()