[CL]: Join-time tracker sumtree #3898

AlpinYukseloglu · 2022-12-31T11:13:10Z

Pulled from #3886

What is the purpose of the change

Note to reviewer: the majority of the file diff is updating wiring for sumtree usage (switching Int to Dec)

This PR implements a join-time tracker sumtree that dynamically updates as positions are changed in a given pool. This allows us to get the amount of liquidity that joined a pool before, at, or after a given join time, allowing us to incentivize uptime.

Once we finalize our process for reward calculation post-#3894 & decide on whether we want to support multiple uptime tiers per incentive token, we can add a second sum to our sumtree to track accumulated rewards.

Brief Changelog

Change sumtree to accumulate sdk.Dec instead of sdk.Int
Populate a sumtree tracking position join times for each pool every time a position is updated
Thoroughly fuzz test new sumtree logic

Testing and Verifying

All of the new logic is heavily tested in position_test.go (both by expanding existing tests & adding new robust fuzz tests)

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (no)
How is the feature or change documented? (not documented)

czarcas7ic

Nice work, have a few questions and then will happily review a second time (didn't feel I should approve on first look since this is my first time reviewing sumtree stuff)

czarcas7ic · 2022-12-31T18:44:28Z

x/concentrated-liquidity/incentives.go

+// Since we want to reset a position's JoinTime every time they update their liquidity, any
+// liquidity update is equivalent to removing the old JoinTime node from the liquidity tree
+// and replacing it with a new one. Thus, this function simply takes in the old and new liquidity
+// amounts and leaves liquidity delta calculations to the caller.


Side note: this will require the user to claim any outstanding rewards prior to replacing the node from the liquidity tree right?

czarcas7ic · 2022-12-31T18:48:50Z

x/concentrated-liquidity/position_test.go

+			// Update expectedPos time to current context's blocktime
+			if test.expectedPosition != nil {
+				test.expectedPosition.JoinTime = s.Ctx.BlockTime()
+			}
+


This got added but it appears nothing tests this logic

czarcas7ic · 2022-12-31T18:50:24Z

x/concentrated-liquidity/position_test.go

+	// We generate a new test account per position for robustness
+	testAccounts := osmoutils.CreateRandomAccounts(numPositions)


I feel like, for robustness, we should have accounts with multiple positions (this feels like the more complex logic imo)

czarcas7ic · 2022-12-31T18:52:25Z

x/concentrated-liquidity/position_test.go

+		// Generate random lower and upper ticks
+		lowerTick := (-1) * (rand.Int63() % tickLowerBound)
+		upperTick := rand.Int63() % tickUpperBound


I like what you did with this fuzz testing, but I think we might need to expand this to prevent every lower tick from being negative and every upper tick from being positive. Seems like we might be leaving some edge cases untested.

czarcas7ic · 2022-12-31T18:54:09Z

x/concentrated-liquidity/incentives.go

@@ -0,0 +1,83 @@
+package concentrated_liquidity


Are you planning to add tests for these?

p0mvn

Overall logic LGTM, nice work!

I'm wondering whether migration is justified. Is there a workaround to avoid changing Int -> Dec in the accumulator proto?

I still need to review position test updates. Also, would love to see tests added for logic in incentives.go

p0mvn · 2022-12-31T23:13:35Z

go.mod

@@ -3,6 +3,7 @@ module github.com/osmosis-labs/osmosis/v13
 go 1.18

 require (
+	cosmossdk.io/errors v1.0.0-beta.7


Why is this needed? We can import errors from our sdk fork? Might be an erroneous import

p0mvn · 2022-12-31T23:37:18Z

proto/osmosis/sumtree/v1beta1/tree.proto

@@ -11,7 +11,7 @@ message Node { repeated Child children = 1; }
 message Child {
  bytes index = 1;
  string accumulation = 2 [
-    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
+    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",


This is likely to require migrations for pre-existing lockup and incentives logic. Do we have a plan for this?

p0mvn · 2022-12-31T23:39:42Z

x/concentrated-liquidity/incentives.go

+	"github.com/osmosis-labs/osmosis/v13/x/concentrated-liquidity/types"
+)
+
+// Gets the current join-time sumtree for the passed in pool ID.


nit: for style consistency

ditto for other specs in this file

Suggested change

// Gets the current join-time sumtree for the passed in pool ID.

// getSumtree gets the current join-time sumtree for the passed in pool ID.

p0mvn · 2022-12-31T23:42:32Z

x/concentrated-liquidity/incentives.go

+
+// Gets the current join-time sumtree for the passed in pool ID.
+func (k Keeper) getSumtree(ctx sdk.Context, poolID uint64) sumtree.Tree {
+	return sumtree.NewTree(prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyAccumulationStore(poolID)), 10)


I'm guessing the choice of 10 for the branching factor was taken from other modules. I'm not sure how much it matters here but we might want to analyze it

p0mvn · 2022-12-31T23:48:06Z

x/concentrated-liquidity/types/keys.go

+func KeyJoinTime(joinTime time.Time) (res []byte) {
+	timeBz := sdk.FormatTimeBytes(joinTime)
+	timeBzL := len(timeBz)
+	prefixL := len(TimePrefix)


nit: can be stored as const

p0mvn · 2022-12-31T23:51:03Z

x/concentrated-liquidity/types/keys.go

+	// copy the prefix
+	copy(bz[:prefixL], TimePrefix)
+
+	// copy the encoded time bytes length


Why do we also need length?

p0mvn · 2023-01-01T00:00:55Z

x/concentrated-liquidity/incentives.go

+
+// Remove liquidity from `position`s join time without resetting position.JoinTime.
+// This is to allow for LPs to remove a portion of their liquidity without forfeiting their uptime
+// on their remaining liquidity.


Suggested change

// on their remaining liquidity.

// on their remaining liquidity.

// The given liquidity must be positive. Fails with an error otherwise.

p0mvn · 2023-01-01T00:01:43Z

x/concentrated-liquidity/position.go

@@ -57,6 +57,18 @@ func (k Keeper) initOrUpdatePosition(
 	// TODO: consider deleting position if liquidity becomes zero

 	k.setPosition(ctx, poolId, owner, lowerTick, upperTick, position)
+
+	// Update pool's liquidity tree to reflect new position
+	if liquidityDelta.GT(sdk.ZeroDec()) {


nit: we can store sdk.ZeroDec as a global variable to avoid underlying reallocations every time

Is there an IsPositive helper? It might be helpful here

AlpinYukseloglu · 2023-01-01T09:52:22Z

Thanks for the comments all – I recently realized that this sumtree logic needs another layer to track tick range for joins for it to be useful for incentives, so I'm converting this to a draft while I flesh out what the final version should look like

github-actions · 2023-01-16T00:02:12Z

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

czarcas7ic and others added 24 commits December 26, 2022 16:21

push initial fixes

aca63fa

modules.go

0eaf077

register AppModuleBasic

dc0b500

uncomment concentrated in swaprouter keeper

33322c9

remove create pool events from msg server

82273c8

add cl storekey to upgrade

66f02dd

rough skeleton with comments

c2e51a5

Merge branch 'roman/cl-merge-main-final' into alpo/cl-incentives

cb49876

add sumtree node docs

9db82d7

set up base liquidity tree usage in CL module

7028cf4

convert sumtree accumulation to sdk.Dec

e077dab

update lockup module to be compat with Dec sumtree

c603b2e

set up liquidity sumtrees for each CL pool and add basic sanity tests

db84c76

thorough fuzz testing for position JoinTime tree

59bd5a7

add support for removing liquidity without resetting join time

3d146cb

test cleanup for readability

b1e886d

Merge branch 'main' into alpo/cl-incentives

4c24521

fix conflicts

f4ab608

move keys to types/keys and make fn names more readable

377e49d

clean up comments

cadb102

implement initAccums and further clean up comments

5289f86

Merge main

7a76ff4

isolate sumtree logic and clean up comments

483d173

clean up comments

613b56f

github-actions bot added C:x/concentrated-liquidity C:x/lockup labels Dec 31, 2022

AlpinYukseloglu requested review from p0mvn, czarcas7ic and ValarDragon December 31, 2022 11:14

AlpinYukseloglu added the V:state/breaking State machine breaking PR label Dec 31, 2022

czarcas7ic reviewed Dec 31, 2022

View reviewed changes

p0mvn reviewed Jan 1, 2023

View reviewed changes

AlpinYukseloglu marked this pull request as draft January 1, 2023 09:50

p0mvn mentioned this pull request Jan 12, 2023

[Story][CL]: Incentives #3991

Closed

32 tasks

github-actions bot added the Stale label Jan 16, 2023

github-actions bot closed this Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CL]: Join-time tracker sumtree #3898

[CL]: Join-time tracker sumtree #3898

AlpinYukseloglu commented Dec 31, 2022 •

edited

Loading

czarcas7ic left a comment

czarcas7ic Dec 31, 2022

czarcas7ic Dec 31, 2022

czarcas7ic Dec 31, 2022

czarcas7ic Dec 31, 2022

czarcas7ic Dec 31, 2022

p0mvn left a comment

p0mvn Dec 31, 2022

p0mvn Dec 31, 2022

p0mvn Dec 31, 2022

p0mvn Dec 31, 2022

p0mvn Dec 31, 2022

p0mvn Dec 31, 2022

p0mvn Jan 1, 2023

p0mvn Jan 1, 2023

p0mvn Jan 1, 2023

AlpinYukseloglu commented Jan 1, 2023 •

edited

Loading

github-actions bot commented Jan 16, 2023

		// We generate a new test account per position for robustness
		testAccounts := osmoutils.CreateRandomAccounts(numPositions)

	// Gets the current join-time sumtree for the passed in pool ID.
	// getSumtree gets the current join-time sumtree for the passed in pool ID.

	// on their remaining liquidity.
	// on their remaining liquidity.
	// The given liquidity must be positive. Fails with an error otherwise.

[CL]: Join-time tracker sumtree #3898

[CL]: Join-time tracker sumtree #3898

Conversation

AlpinYukseloglu commented Dec 31, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

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

Choose a reason for hiding this comment

p0mvn 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

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 commented Jan 1, 2023 • edited Loading

github-actions bot commented Jan 16, 2023

AlpinYukseloglu commented Dec 31, 2022 •

edited

Loading

AlpinYukseloglu commented Jan 1, 2023 •

edited

Loading