feat: uninitialize empty ticks #5883

czarcas7ic · 2023-07-25T00:40:39Z

Closes: #5867

What is the purpose of the change

We currently don't uninitialize a tick after it has been initialized. This causes extra unncecessary overhead when traversing ticks.

This PR uninitializes ticks after they have been drained. This is only done in withdrawPosition at the very end and cannot be done in initOrUpdateTick since fee/incentive info needs to be accessible when withdrawing.

NOTE:

This PR could also implement a traversal of existing CL pools and uninitialize empty ticks in the upgrade handler. If this is not done, then these ticks will forever be initialized. Its probably fine if this is left out, but we can talk about how we feel about including this.

Testing and Verifying

UpdatePosition, WithdrawPosition, and InitOrUpdateTick tests have been modified to check that empty ticks are removed from state, while partial withdraws do not remove the tick from state.

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

czarcas7ic · 2023-07-25T01:49:35Z

x/concentrated-liquidity/client/query_proto_wrap.go

+	if err != nil {
+		return nil, err
+	}
+


drive by change

czarcas7ic · 2023-07-25T01:51:39Z

x/concentrated-liquidity/export_test.go

@@ -98,7 +98,7 @@ func (k Keeper) ComputeInAmtGivenOut(
 	return k.computeInAmtGivenOut(ctx, desiredTokenOut, tokenInDenom, spreadFactor, priceLimit, poolId)
 }

-func (k Keeper) InitOrUpdateTick(ctx sdk.Context, poolId uint64, currentTick int64, tickIndex int64, liquidityIn sdk.Dec, upper bool) (err error) {
+func (k Keeper) InitOrUpdateTick(ctx sdk.Context, poolId uint64, currentTick int64, tickIndex int64, liquidityIn sdk.Dec, upper bool) (tickIsEmpty bool, err error) {


This is added as a return value so that we don't have to refetch the tick at the end of the withdraw method. It must be done at the end of the withdraw method, since if we delete the tick early, we will lose the fee/incentive info that is saved there that gets claimed after initOrUpdateTick is called in the WithdrawPosition method.

"if we delete the tick early, we will lose the fee/incentive info that is saved there that gets claimed after initOrUpdateTick is called in the WithdrawPosition method."

just curious:
when we withdrawPosition(entire position amount), we auto claim the left over fees/incentives and then delete tick right? if yes, then how could there be chance which we could lose fee/incentive that gets claimed after initOrUpdateTick is called

What the comment meant was, it would have been cleaner to delete the tick as soon as we know its empty, but some tick data is still needed when withdrawing to claim those rewards so we need to delete as the last step (as it is in this PR)

x/concentrated-liquidity/lp.go

stackman27

This LGTM just one question

Co-authored-by: Sishir Giri <[email protected]>

ValarDragon · 2023-07-28T15:42:23Z

x/concentrated-liquidity/lp.go

 // WARNING: this method may mutate the pool, make sure to refetch the pool after calling this method.
-func (k Keeper) UpdatePosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, lowerTick, upperTick int64, liquidityDelta sdk.Dec, joinTime time.Time, positionId uint64) (sdk.Int, sdk.Int, error) {
+func (k Keeper) UpdatePosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, lowerTick, upperTick int64, liquidityDelta sdk.Dec, joinTime time.Time, positionId uint64) (sdk.Int, sdk.Int, bool, bool, error) {


Why don't we just have this method deal with deleting the tick? Why pass it up?

@ValarDragon It needs to be passed up for the following reason:

We collect spread rewards when withdrawing an entire position

osmosis/x/concentrated-liquidity/lp.go

Lines 236 to 239 in 465399c

if requestedLiquidityAmountToWithdraw.Equal(position.Liquidity) {

if _, err := k.collectSpreadRewards(ctx, owner, positionId); err != nil {

return sdk.Int{}, sdk.Int{}, err

}

This calls prepareClaimableSpreadRewards

osmosis/x/concentrated-liquidity/spread_rewards.go

Lines 175 to 178 in 465399c

spreadRewardsClaimed, err := k.prepareClaimableSpreadRewards(ctx, positionId)

if err != nil {

return sdk.Coins{}, err

}

This calculates the spreadRewardGrowthOutside via the ticks

osmosis/x/concentrated-liquidity/spread_rewards.go

Lines 247 to 252 in 465399c

// Compute the spread reward growth outside of the range between the position's lower and upper ticks.

spreadRewardGrowthOutside, err := k.getSpreadRewardGrowthOutside(ctx, position.PoolId, position.LowerTick, position.UpperTick)

if err != nil {

return nil, err

}

If we delete tick info earlier, this will output an incorrect amount

* initial push uninitialize ticks * check tick fully removed * update CHANGELOG.md * add comments / clean up * Update x/concentrated-liquidity/lp.go Co-authored-by: Sishir Giri <[email protected]> --------- Co-authored-by: Sishir Giri <[email protected]>

initial push uninitialize ticks

b23c654

czarcas7ic added the V:state/breaking State machine breaking PR label Jul 25, 2023

github-actions bot added C:CLI C:x/superfluid C:x/concentrated-liquidity labels Jul 25, 2023

czarcas7ic added 2 commits July 24, 2023 20:19

check tick fully removed

1908fb7

update CHANGELOG.md

502e8b7

czarcas7ic commented Jul 25, 2023

View reviewed changes

add comments / clean up

3f9fbc0

czarcas7ic marked this pull request as ready for review July 25, 2023 02:44

czarcas7ic requested review from mattverse, stackman27 and p0mvn as code owners July 25, 2023 02:44

stackman27 reviewed Jul 25, 2023

View reviewed changes

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

stackman27 approved these changes Jul 25, 2023

View reviewed changes

Update x/concentrated-liquidity/lp.go

465399c

Co-authored-by: Sishir Giri <[email protected]>

ValarDragon reviewed Jul 28, 2023

View reviewed changes

Merge branch 'main' into adam/uninitialize-ticks

cba0dd8

ValarDragon approved these changes Jul 30, 2023

View reviewed changes

ValarDragon merged commit 092808f into main Jul 30, 2023

ValarDragon deleted the adam/uninitialize-ticks branch July 30, 2023 11:19

ValarDragon mentioned this pull request Jul 30, 2023

CL: Remove repeated tick info gets #5921

Open

github-actions bot mentioned this pull request Apr 1, 2024

Broken Links Detected #7903

Closed

github-actions bot mentioned this pull request May 1, 2024

Broken Links Detected #8183

Closed

github-actions bot mentioned this pull request May 15, 2024

Broken Links Detected #8269

Closed

github-actions bot mentioned this pull request Oct 15, 2024

Broken Links Detected notional-labs/osmosis#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: uninitialize empty ticks #5883

feat: uninitialize empty ticks #5883

czarcas7ic commented Jul 25, 2023 •

edited

Loading

czarcas7ic Jul 25, 2023

czarcas7ic Jul 25, 2023

stackman27 Jul 25, 2023

czarcas7ic Jul 25, 2023

stackman27 left a comment

ValarDragon Jul 28, 2023

czarcas7ic Jul 28, 2023

ValarDragon Jul 28, 2023

	if requestedLiquidityAmountToWithdraw.Equal(position.Liquidity) {
	if _, err := k.collectSpreadRewards(ctx, owner, positionId); err != nil {
	return sdk.Int{}, sdk.Int{}, err
	}

	spreadRewardsClaimed, err := k.prepareClaimableSpreadRewards(ctx, positionId)
	if err != nil {
	return sdk.Coins{}, err
	}

	// Compute the spread reward growth outside of the range between the position's lower and upper ticks.
	spreadRewardGrowthOutside, err := k.getSpreadRewardGrowthOutside(ctx, position.PoolId, position.LowerTick, position.UpperTick)
	if err != nil {
	return nil, err
	}

feat: uninitialize empty ticks #5883

feat: uninitialize empty ticks #5883

Conversation

czarcas7ic commented Jul 25, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

czarcas7ic Jul 25, 2023

Choose a reason for hiding this comment

czarcas7ic Jul 25, 2023

Choose a reason for hiding this comment

stackman27 Jul 25, 2023

Choose a reason for hiding this comment

czarcas7ic Jul 25, 2023

Choose a reason for hiding this comment

stackman27 left a comment

Choose a reason for hiding this comment

ValarDragon Jul 28, 2023

Choose a reason for hiding this comment

czarcas7ic Jul 28, 2023

Choose a reason for hiding this comment

ValarDragon Jul 28, 2023

Choose a reason for hiding this comment

czarcas7ic commented Jul 25, 2023 •

edited

Loading