Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Implement redepositing of forfeited incentives #7746

Merged
merged 20 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#7689](https://github.com/osmosis-labs/osmosis/pull/7689) Make CL price estimations not cause state writes (speed and gas improvements)
* [#7745](https://github.com/osmosis-labs/osmosis/pull/7745) Add gauge id query to stargate whitelist
* [#7747](https://github.com/osmosis-labs/osmosis/pull/7747) Remove redundant call to incentive collection in CL position withdrawal logic
* [#7746](https://github.com/osmosis-labs/osmosis/pull/7746) Make forfeited incentives redeposit into the pool instead of sending to community pool

## v23.0.7-iavl-v1

Expand Down
12 changes: 10 additions & 2 deletions x/concentrated-liquidity/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, []sdk.Coins, error) {
return k.collectIncentives(ctx, owner, positionId)
}

Expand All @@ -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, []sdk.Coins, error) {
return k.prepareClaimAllIncentivesForPosition(ctx, positionId)
}

Expand Down Expand Up @@ -355,3 +355,11 @@ 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)
}
AlpinYukseloglu marked this conversation as resolved.
Show resolved Hide resolved

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)
}
127 changes: 98 additions & 29 deletions x/concentrated-liquidity/incentives.go
Original file line number Diff line number Diff line change
Expand Up @@ -753,36 +753,36 @@ 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, []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.
positionAge := ctx.BlockTime().Sub(position.JoinTime)

// 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.
Expand All @@ -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([]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.
Expand All @@ -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.
Expand All @@ -824,6 +825,12 @@ 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 slice of scaled coins, but doing it this way
// allows us to efficiently handle all cases related to forfeited incentives.
scaledForfeitedIncentivesByUptime[uptimeIndex] = collectedIncentivesForUptimeScaled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, at first I was trying to understand why the Coins array but this makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah also we'd need to track forfeits separately regardless to know which incentives were forfeited from which accumulators, so something of this form would be necessary regardless


// 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...)
Expand All @@ -835,13 +842,83 @@ func (k Keeper) prepareClaimAllIncentivesForPosition(ctx sdk.Context, positionId
}
}

return collectedIncentivesForPosition, forfeitedIncentivesForPosition, nil
return collectedIncentivesForPosition, forfeitedIncentivesForPosition, scaledForfeitedIncentivesByUptime, nil
}

// 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)}
}

// Fetch pool from state to check active liquidity.
pool, err := k.getPoolById(ctx, poolId)
if err != nil {
return err
}
AlpinYukseloglu marked this conversation as resolved.
Show resolved Hide resolved
activeLiquidity := pool.GetLiquidity()

// If no active liquidity, give the forfeited incentives to the sender.
if activeLiquidity.LT(sdk.OneDec()) {
err := k.bankKeeper.SendCoins(ctx, pool.GetIncentivesAddress(), sender, totalForefeitedIncentives)
if err != nil {
return err
}
return nil
}
Comment on lines +872 to +879
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably ignore this comment but, why not just send to the community pool? Just thinking in the event of unexpectedly being able to trigger this as a bug I would rather funds get sent to the community pool instead of the exploiter.

Copy link
Contributor Author

@AlpinYukseloglu AlpinYukseloglu Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This basically makes it so the logic is "do uptime incentives if possible, but fall back to status quo incentives if not". It feels like this is the most appropriate approach, especially since this logic will be affecting external incentives as well (who probably would prefer incentivizing active MMs over sending to community pool)

Also in the case where there's a bug in related logic, imo having the pool fall back to regular incentives seems more appropriate than bricking all incentives and sending them to community pool


// 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
}

// Loop through each uptime accumulator for the pool and redeposit forfeited incentives.
for uptimeIndex := range uptimeAccums {
curUptimeForfeited := scaledForfeitedIncentivesByUptime[uptimeIndex]
if curUptimeForfeited.IsZero() {
continue
}

// 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 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
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)
}

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()
return k.prepareClaimAllIncentivesForPosition(cacheCtx, positionId)
// 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
}

// collectIncentives collects incentives for all uptime accumulators for the specified position id.
Expand All @@ -850,49 +927,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, []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
}
}

Expand All @@ -904,11 +973,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.
Expand Down
Loading