[CL Incentives] Allow early withdrawals for frozen positions #4525
Conversation
…/osmosis into adam/add-freeze-api
…/osmosis into adam/add-freeze-api
* single fee accum * lint
Co-authored-by: Sishir Giri <[email protected]>
Co-authored-by: Roman <[email protected]>
|
|
||
| if tc.expectError != nil { | ||
| s.Require().Error(err) | ||
| s.Require().ErrorIs(err, tc.expectError) |
There was a problem hiding this comment.
I don't believe we have any error cases we are testing here, can we:
- Verify that there aren't any error cases we need to check here and
- If we add error cases, change this to ErrorContains. Otherwise, remove.
I believe there are likely error cases we want to test on ClaimAllIncentivesForPosition though
There was a problem hiding this comment.
Since this function was extracted from CollectIncentives, there are thorough tests there for all the relevant cases on ClaimAllIncentivesForPosition, but I agree that at least having sanity error checks is good even if duplicates.
Adding rn
| if tc.forfeitIncentives { | ||
| newUptimeAccumValues, err := clKeeper.GetUptimeAccumulatorValues(s.Ctx, validPoolId) | ||
| s.Require().NoError(err) | ||
|
|
||
| // Subtract the initial accum values to get the delta | ||
| uptimeAccumDeltaValues, err := osmoutils.SubDecCoinArrays(newUptimeAccumValues, initUptimeAccumValues) | ||
| s.Require().NoError(err) | ||
|
|
||
| // Convert DecCoins to Coins by truncation for comparison | ||
| normalizedUptimeAccumDelta := sdk.NewCoins() | ||
| for _, uptimeAccumDelta := range uptimeAccumDeltaValues { | ||
| normalizedUptimeAccumDelta = normalizedUptimeAccumDelta.Add(sdk.NormalizeCoins(uptimeAccumDelta)...) | ||
| } | ||
|
|
||
| s.Require().Equal(normalizedUptimeAccumDelta, amountClaimed) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Something that would make me feel extra comfortable with this is that we check bank balances before and after claim, to ensure that the rewards in fact did not make it to the account.
Co-authored-by: Roman <[email protected]>
Co-authored-by: Roman <[email protected]>
…smosis into alpo-uptime-claim
* draft implementation * begin testing * add more test cases * add error catching test cases * implement createIncentive message * cli and codec * add check and related tests * add further tests and lint * add test case for existing incentive records
AlpinYukseloglu
changed the base branch from
alpo-uptime-claim
to
alpo-create-incentive
| // claimAllIncentivesForPosition claims and returns all the incentives for a given position. | ||
| // It takes in a `forfeitIncentives` boolean to indicate whether the accrued incentives should be forfeited, in which case it | ||
| // redeposits the accrued rewards back into the accumulator as additional rewards for other participants. |
There was a problem hiding this comment.
One minor test case I think would be good to see is that, if we have three positions (one 20%, one 50%, and one 30%) that get created at the same time, if the 20% forfeits rewards that is is properly distributed between the 50% and 30% proportionally. Not at all blocking, just something I thought would be good to verify.
x/concentrated-liquidity/lp.go
Outdated
| // If the position is still frozen, claim and forfeit any accrued incentives for the position. | ||
| isPositionFrozen := joinTime.Add(position.FreezeDuration).After(ctx.BlockTime()) | ||
| if isPositionFrozen { | ||
| if !requestedLiquidityAmountToWithdraw.Equal(position.Liquidity) { | ||
| return sdk.Int{}, sdk.Int{}, fmt.Errorf("If withdrawing from frozen position, must withdraw all liquidity.") | ||
| } | ||
|
|
||
| _, err := k.claimAllIncentivesForPosition(ctx, poolId, owner, position, lowerTick, upperTick, true) | ||
| if err != nil { | ||
| return sdk.Int{}, sdk.Int{}, err | ||
| } |
There was a problem hiding this comment.
Unsure how I feel about this implicit (if I withdraw everything then I give away all my incentives). Feels like it should be an explicit param.
Closes: #4456, #4514
What is the purpose of the change
This PR implements early position exits for frozen positions. It allows users to exit even before their
freezeDurationis over if they are willing to forfeit the incentives they have accrued.Brief Changelog
claimAllIncentivesForPosition, that claims and returns all the accrued incentives for a given position. This helper takes in aforfeitIncentivesparameter which, when true, redeposits the claimed incentives into the appropriate accumulators right after claiming. If it is false, the rewards are claimed but no balance transfers are processed (this is to keep the helper useful in standard claiming logic while avoiding duplicate code)Testing and Verifying
incentives_test.goandlp_test.gocollectIncentivesandcollectFees, the cases included are mainly sanity checks and cases for new functionality e.g. forfeiting rewards.Documentation and Release Note
Unreleasedsection inCHANGELOG.md? (no)