[CL Incentives] Remove ability to add to frozen positions #4508

stackman27 · 2023-03-06T05:20:50Z

What is the purpose of the change

Now that we key positions by joinTime and freezeDuration, it is no longer possible for people to add to an existing position.

Brief Changelog

-> users cannot update their existing position, instead we create new position

Testing and Verifying

remove unwanted tests

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

AlpinYukseloglu

Good work, left one comment about a logic-related issue

x/concentrated-liquidity/incentives.go

x/concentrated-liquidity/incentives_test.go

p0mvn · 2023-03-06T19:58:59Z

x/concentrated-liquidity/position.go

@@ -68,7 +68,7 @@ func (k Keeper) initOrUpdatePosition(

 	// TODO: consider deleting position if liquidity becomes zero

-	err = k.initOrUpdatePositionUptime(ctx, poolId, position, owner, lowerTick, upperTick, liquidityDelta, joinTime, freezeDuration)
+	err = k.initPositionUptime(ctx, poolId, position, owner, lowerTick, upperTick, liquidityDelta, joinTime, freezeDuration)


So we remove the ability to update the incentives accumulator but positions themselves still seem to be treated as if they are allowed to have liquidity increased for the same freeze duration. Shouldn't we disallow that as well?

i dont understand how can "position be allowed to have liquidity increase for same freeze duration", if there is any change in liquidity we update JoinTime thus creating new position, is that not the case?

x/concentrated-liquidity/incentives.go

czarcas7ic

Will finish review soon, just wanted to get these nits out there while I finished a few other tasks

x/concentrated-liquidity/incentives.go

x/concentrated-liquidity/incentives_test.go

x/concentrated-liquidity/position_test.go

p0mvn · 2023-03-08T02:09:41Z

x/concentrated-liquidity/lp_test.go

 			s.Require().True(hasPosition)

 			// Check position state
-			s.validatePositionUpdate(s.Ctx, tc.poolId, s.TestAccs[0], tc.lowerTick, tc.upperTick, defaultJoinTime, tc.freezeDuration, tc.liquidityAmount)
+			s.validatePositionUpdate(s.Ctx, tc.poolId, s.TestAccs[0], tc.lowerTick, tc.upperTick, DefaultJoinTime, tc.freezeDuration, tc.liquidityAmount)


I don't think we have assertions around uptime accumulators being updated the way we expect them to be - only on initialization. Please add tests

x/concentrated-liquidity/lp.go

stackman27 · 2023-03-09T00:49:36Z

x/concentrated-liquidity/lp.go

@@ -77,6 +77,13 @@ func (k Keeper) createPosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddr
 		}
 	}

+	if !k.hasFullPosition(ctx, poolId, owner, lowerTick, upperTick, joinTime, freezeDuration) {


@p0mvn not sure why we have to check if the position is frozen or not as you suggested?

talked to @AlpinYukseloglu about this and here's a quick tldr is

"currently we donot support 0sec uptime therefore positions are not frozen from 0sec-60sec (min supported uptime is 1min). I think in future we are planning on adding 0sec uptime so that all positions are frozen to begin with"

for the sake of this PR I added frozen check and made a note to remove it once we add 0sec supported uptime

AlpinYukseloglu

LGTM, good work. Left a few minor comments

AlpinYukseloglu · 2023-03-16T15:50:43Z

go.work.sum

We shouldn't be pushing go.work.sum changes (odd that this made it in since it's in .gitignore)

x/concentrated-liquidity/types/errors.go

x/concentrated-liquidity/position.go

p0mvn · 2023-03-16T20:01:12Z

x/concentrated-liquidity/lp.go

+	if !hasFullPosition {
+		err = k.initPositionUptime(ctx, poolId, owner, lowerTick, upperTick, liquidityDelta, joinTime, freezeDuration)
+		if err != nil {
+			return sdk.Int{}, sdk.Int{}, sdk.Dec{}, time.Time{}, err
+		}
+	}


Why do we not error in the else case of this if it should not be possible to have the position with the same key due to joinTime?

i didn't error for the <1min uptime case where one could modify position from 0sec to 60sec, but since we added 1nano second uptime, i'll add error

stackman27 · 2023-03-16T21:41:23Z

Thank you @AlpinYukseloglu and @roman for comments. Resolved all your feedbacks!

p0mvn · 2023-03-17T23:55:25Z

x/concentrated-liquidity/lp.go

+			return sdk.Int{}, sdk.Int{}, sdk.Dec{}, time.Time{}, err
+		}
+	} else {
+		return sdk.Int{}, sdk.Int{}, sdk.Dec{}, time.Time{}, types.PositionNotFoundError{PoolId: poolId, LowerTick: lowerTick, UpperTick: upperTick, JoinTime: joinTime, FreezeDuration: freezeDuration}


Is there a test covering this?

Also, this should not be a PositionNotFoundError. We should have a new one for attempting to add to an existing position

yup changed to PositionAlreadyExist Error and is covered here (inside incentives_test)
: https://github.com/osmosis-labs/osmosis/pull/4508/files#diff-91569dd5b56ddbfc9450f57de47bc13061e1daceaba154cab15b635d98912d19R1716-R1773

The linked test seems to cover another function.

We should have a direct test for createPosition in lp_test.go for this. I think the commented-out test that was removed can be converted to cover this

converted commented out test to cover this
https://github.com/osmosis-labs/osmosis/pull/4508/files#diff-8a43021241fc488af68dd6cdd7c762a85476f3aa8e44248784e9265c9ef0bbdfR799

AlpinYukseloglu · 2023-03-25T02:07:12Z

x/concentrated-liquidity/lp.go

@@ -130,7 +140,7 @@ func (k Keeper) withdrawPosition(ctx sdk.Context, owner sdk.AccAddress, position
 	}

 	// If the position is still frozen, claim and forfeit any accrued incentives for the position.
-	isPositionFrozen := position.JoinTime.Add(position.FreezeDuration).After(ctx.BlockTime())
+	isPositionFrozen := k.IsPositionStillFrozen(ctx, position.JoinTime, position.FreezeDuration)


Now that we're removing freezeDuration, we should be able to remove this check and just call claimAllIncentivesForPosition. It might make sense to rebase this PR to the latest CL PR that has these changes.

AlpinYukseloglu · 2023-03-26T22:02:02Z

Merging this understanding that it will cause some minor conflicts in #4724 to not block this small change on that large PR

stackman27 requested review from AlpinYukseloglu and p0mvn March 6, 2023 05:20

github-actions bot added the C:x/concentrated-liquidity label Mar 6, 2023

stackman27 requested a review from czarcas7ic March 6, 2023 05:22

stackman27 added V:state/compatible/backport State machine compatible PR, should be backported V:state/breaking State machine breaking PR and removed V:state/compatible/backport State machine compatible PR, should be backported labels Mar 6, 2023

AlpinYukseloglu reviewed Mar 6, 2023

View reviewed changes

x/concentrated-liquidity/incentives.go Show resolved Hide resolved

p0mvn reviewed Mar 6, 2023

View reviewed changes

stackman27 marked this pull request as draft March 7, 2023 07:24

stackman27 marked this pull request as ready for review March 7, 2023 19:13

stackman27 requested review from p0mvn and AlpinYukseloglu March 7, 2023 19:27

stackman27 commented Mar 7, 2023

View reviewed changes

x/concentrated-liquidity/incentives.go Show resolved Hide resolved

czarcas7ic reviewed Mar 8, 2023

View reviewed changes

x/concentrated-liquidity/incentives.go Outdated Show resolved Hide resolved

x/concentrated-liquidity/incentives.go Show resolved Hide resolved

x/concentrated-liquidity/incentives.go Show resolved Hide resolved

p0mvn reviewed Mar 8, 2023

View reviewed changes

stackman27 force-pushed the sis/cl-inc-remove-update branch from dd72895 to d692411 Compare March 9, 2023 00:31

stackman27 commented Mar 9, 2023

View reviewed changes

stackman27 requested review from czarcas7ic and p0mvn March 9, 2023 00:50

stackman27 force-pushed the sis/cl-inc-remove-update branch 8 times, most recently from 742ead5 to 9de86de Compare March 16, 2023 07:21

AlpinYukseloglu approved these changes Mar 16, 2023

View reviewed changes

AlpinYukseloglu reviewed Mar 16, 2023

View reviewed changes

x/concentrated-liquidity/position.go Show resolved Hide resolved

p0mvn reviewed Mar 16, 2023

View reviewed changes

stackman27 force-pushed the sis/cl-inc-remove-update branch 3 times, most recently from 55214cd to 6af866c Compare March 17, 2023 21:50

stackman27 requested a review from p0mvn March 17, 2023 21:52

p0mvn reviewed Mar 17, 2023

View reviewed changes

stackman27 force-pushed the sis/cl-inc-remove-update branch from efb0ba7 to f60e6b5 Compare March 18, 2023 00:03

stackman27 requested a review from p0mvn March 18, 2023 00:03

stackman27 force-pushed the sis/cl-inc-remove-update branch 3 times, most recently from caa2d36 to 40dc177 Compare March 24, 2023 22:26

stackman27 added 2 commits March 24, 2023 15:32

readded has position logic

78abc1a

rebased

2488f07

stackman27 force-pushed the sis/cl-inc-remove-update branch from b0d4419 to 2488f07 Compare March 25, 2023 00:26

Delete go.work.sum

7c72ddd

AlpinYukseloglu reviewed Mar 25, 2023

View reviewed changes

AlpinYukseloglu merged commit c20a2b1 into main Mar 26, 2023

AlpinYukseloglu deleted the sis/cl-inc-remove-update branch March 26, 2023 22:02

stackman27 mentioned this pull request Mar 27, 2023

[CL Incentives] Remove ability to add to frozen positions #4482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CL Incentives] Remove ability to add to frozen positions #4508

[CL Incentives] Remove ability to add to frozen positions #4508

stackman27 commented Mar 6, 2023

AlpinYukseloglu left a comment

p0mvn Mar 6, 2023

stackman27 Mar 6, 2023 •

edited

Loading

czarcas7ic left a comment

p0mvn Mar 8, 2023

stackman27 Mar 10, 2023

stackman27 Mar 9, 2023

stackman27 Mar 10, 2023

AlpinYukseloglu left a comment

AlpinYukseloglu Mar 16, 2023

p0mvn Mar 16, 2023

stackman27 Mar 16, 2023

stackman27 commented Mar 16, 2023 •

edited

Loading

p0mvn Mar 17, 2023

stackman27 Mar 18, 2023 •

edited

Loading

p0mvn Mar 19, 2023

stackman27 Mar 21, 2023

AlpinYukseloglu Mar 25, 2023

AlpinYukseloglu commented Mar 26, 2023

[CL Incentives] Remove ability to add to frozen positions #4508

[CL Incentives] Remove ability to add to frozen positions #4508

Conversation

stackman27 commented Mar 6, 2023

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 commented Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

stackman27 Mar 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlpinYukseloglu commented Mar 26, 2023

stackman27 Mar 6, 2023 •

edited

Loading

stackman27 commented Mar 16, 2023 •

edited

Loading

stackman27 Mar 18, 2023 •

edited

Loading