[CL Incentives]: Sync global uptime accumulators upon tick crossing #4399

AlpinYukseloglu · 2023-02-23T07:35:18Z

Closes: #4278

What is the purpose of the change

This PR implements uptime-related logic for tick crossing. Specifically, it "flips" the tick-level uptime accum values upon crossing.

Brief Changelog

Implement logic for flipping uptime trackers when a tick is crossed

Testing and Verifying

New logic is relatively well tested in tick_test.go (including cases for technically undefined behavior such as crossing into non-adjacent ticks)

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)

…/osmosis into adam/add-freeze-api

* single fee accum * lint

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

Co-authored-by: Roman <[email protected]>

stackman27

left few comments

x/concentrated-liquidity/tick_test.go

x/concentrated-liquidity/tick.go

x/concentrated-liquidity/incentives_test.go

p0mvn · 2023-02-25T00:07:37Z

x/concentrated-liquidity/tick.go

+	}
+
+	// Update global accums to now before uptime outside changes
+	if err := k.updateUptimeAccumulatorsToNow(ctx, poolId); err != nil {


Why do we need to update uptime accumulators as we cross ticks?

Shouldn't we be able to update them once per block?

My thinking was that the update logic for the target tick's uptime accumulators will involve the global accum values, so we need to ensure they are up to date. Are you suggesting this isn't necessary?

I do see a different issue though – we should be updating the accums prior to getting them instead of after, will fix.

So over a single swap, we might be crossing multiple ticks. From my understanding, we do not need to update the uptime accumulators every tick because that is idempotent within a swap. As a result, calling this update function as we cross every tick might be redundant.

Outside of the scope of this PR but - why do we also need to call updateUptimeAccumulatorsToNow in getTickInfo?

It feels that this update needs to happen at another level of abstraction or in another location

So over a single swap, we might be crossing multiple ticks. From my understanding, we do not need to update the uptime accumulators every tick because that is idempotent within a swap. As a result, calling this update function as we cross every tick might be redundant.

Good point – we can add a check so it only updates if LastUpdateTime < current time. Made an issue here (#4422) to not block this PR (and because it could break some downstream PRs)

Outside of the scope of this PR but - why do we also need to call updateUptimeAccumulatorsToNow in getTickInfo?

It feels that this update needs to happen at another level of abstraction or in another location

We update there simply because getInitialUptimeGrowthOutsidesForTick uses global accum values (so they need to be up to date). The level of abstraction does seem a little off, but the function does include accumulator init logic more broadly so it might be worth discussing a small refactor or rename

p0mvn · 2023-02-25T15:23:37Z

x/concentrated-liquidity/tick_test.go

 		additiveFee                  sdk.DecCoin
 		expectedLiquidityDelta       sdk.Dec
 		expectedTickFeeGrowthOutside sdk.DecCoins
 		expectedErr                  bool
 	}{
 		{
-			name:                         "Get tick info on existing pool and existing tick",
+			name:                    "Get tick info on existing pool and existing tick below current tick (nonzero uptime trackers)",


nit: let's remove "on existing pool" part from every test case name, please. It adds cognitive overhead while being unimportant context relative to other test case details because the pool existence check happens once at the top level

Agreed, done

p0mvn · 2023-02-25T15:28:36Z

x/concentrated-liquidity/tick_test.go

+			expectedTickFeeGrowthOutside: DefaultFeeAccumCoins.Add(defaultAdditiveFee),
+		},
+		{
+			name:                    "Get tick info on existing pool and existing tick below current tick (nonzero uptime trackers)",


Isn't it ("zero initial uptime") trackers. Should the test case name reflect that?

Hmm not sure I follow – the uptime trackers are nonzero in this test case no?

p0mvn · 2023-02-25T15:30:24Z

x/concentrated-liquidity/tick_test.go

+			expectedTickFeeGrowthOutside: DefaultFeeAccumCoins.Add(defaultAdditiveFee),
+		},
+		{
+			name:                         "Get tick info on existing pool and existing tick above current tick (nil uptime trackers)",


Does it make sense also having a test case where non-zero initial uptime trackers and above current tick?

This is already implicitly covered by case right below your comment, but I just added an explicit case for clarity anyway.

p0mvn

LGTM pending minor suggestions and the discussion on calling updateUptimeAccumulatorsToNow in crossTick and getTickInfo.

If that makes it easier for making progress, please feel free to merge this and move the discussion on Slack

czarcas7ic and others added 30 commits January 25, 2023 21:01

initial push

235653b

push

e4941f6

untracked files

a3ac7b7

Merge branch 'adam/add-freeze-api' of https://github.com/osmosis-labs…

e888677

…/osmosis into adam/add-freeze-api

add untracked files

61f5c76

Merge branch 'adam/add-freeze-api' of https://github.com/osmosis-labs…

afc350f

…/osmosis into adam/add-freeze-api

merge branches

085bdc1

Merge branch 'main' into adam/add-freeze-api

b7a2a70

add changes required for fees

2e4fd0f

lint

79017a3

fix test errors

8e68719

clean up

b221dcf

remove print lines

d2ede58

feat: single fee accum (#4116)

a515d5d

* single fee accum * lint

no longer need to get all positions for fee

838c2ab

incentive accum initialization

c75f163

lint

447967e

Update x/concentrated-liquidity/store_test.go

0adb801

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

add lower level tests and make uptime accum access private

17f47fe

lint

75e0c14

add gotests for helper fn

5f2e7a5

Merge branch 'main' into alpo/uptime-accums

433e215

Merge adam/add-freeze-api into alpo/uptime-accums

008379a

Update test to use existing variable

400ac3e

Co-authored-by: Roman <[email protected]>

create or update records upon position creation/update

a4fbf0b

add tests and clean up

c604659

sync position-related changes

1f75cea

add support for negative liquidity delta

982830e

add and test helper & set up tick initialization logic

768ec68

implement accumulator updates on tick crossing and clean up proto

c318fa0

AlpinYukseloglu added C:x/concentrated-liquidity F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board labels Feb 23, 2023

AlpinYukseloglu requested review from mattverse, czarcas7ic and p0mvn February 23, 2023 07:37

AlpinYukseloglu added 2 commits February 23, 2023 07:09

test and comment cleanup

1d208be

Merge branch 'alpo-cl-uptime-tick-init' into alpo-cl-uptime-cross

b080e81

stackman27 reviewed Feb 23, 2023

View reviewed changes

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

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

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

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

AlpinYukseloglu added 5 commits February 23, 2023 11:08

clean up test comments

bcf4abd

Merge branch 'alpo-cl-uptime-tick-init' into alpo-cl-uptime-cross

30aa0c5

fix test names

b0e9edb

Merge branch 'main' into alpo-cl-uptime-tick-init

a7d07f0

Merge branch 'alpo-cl-uptime-tick-init' into alpo-cl-uptime-cross

e610a05

czarcas7ic reviewed Feb 23, 2023

View reviewed changes

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

p0mvn reviewed Feb 25, 2023

View reviewed changes

update uptime accums before gettings them for tick crossing

a55414b

p0mvn reviewed Feb 25, 2023

View reviewed changes

p0mvn approved these changes Feb 25, 2023

View reviewed changes

p0mvn mentioned this pull request Feb 26, 2023

[CL Incentives] Add check to only sync uptime accums when necessary #4422

Closed

p0mvn added this to the CL 9 milestone Feb 28, 2023

lint

0a51d05

czarcas7ic assigned AlpinYukseloglu Mar 1, 2023

Base automatically changed from alpo-cl-uptime-tick-init to main March 2, 2023 02:30

AlpinYukseloglu added V:state/breaking State machine breaking PR and removed V:state/breaking State machine breaking PR labels Mar 2, 2023

clean up tests

86badc2

AlpinYukseloglu merged commit 981dcef into main Mar 3, 2023

AlpinYukseloglu deleted the alpo-cl-uptime-cross branch March 3, 2023 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CL Incentives]: Sync global uptime accumulators upon tick crossing #4399

[CL Incentives]: Sync global uptime accumulators upon tick crossing #4399

AlpinYukseloglu commented Feb 23, 2023

stackman27 left a comment

p0mvn Feb 25, 2023 •

edited

Loading

p0mvn Feb 25, 2023

AlpinYukseloglu Feb 25, 2023

p0mvn Feb 25, 2023

p0mvn Feb 25, 2023

AlpinYukseloglu Feb 25, 2023

AlpinYukseloglu Feb 25, 2023

p0mvn Feb 25, 2023 •

edited

Loading

AlpinYukseloglu Mar 3, 2023

p0mvn Feb 25, 2023

AlpinYukseloglu Mar 3, 2023

p0mvn Feb 25, 2023

AlpinYukseloglu Mar 3, 2023

p0mvn left a comment

[CL Incentives]: Sync global uptime accumulators upon tick crossing #4399

[CL Incentives]: Sync global uptime accumulators upon tick crossing #4399

Conversation

AlpinYukseloglu commented Feb 23, 2023

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

stackman27 left a comment

Choose a reason for hiding this comment

p0mvn Feb 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn Feb 25, 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

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

p0mvn Feb 25, 2023 •

edited

Loading

p0mvn Feb 25, 2023 •

edited

Loading