feat: scaling factor for pool uptime accumulator to avoid truncation #7409

p0mvn · 2024-02-02T22:09:58Z

What is the purpose of the change

This is a solution to avoid truncation of the pool reward accumulator to zero when liquidity is large.

It focuses on defining the core of the scaling factor solution (task ref) and also implementing and testing logic for silently skipping incentive overflows (task ref).

Initial analysis of the approach was done here: https://hackmd.io/o3oqT8VhSPKAiqNl_mlxXQ

To be implemented next:

Pool migration

We should also analyze whether it makes sense to implement something similar for the spread rewards accumulator by inspecting the new telemetry: #7408

Testing and Verifying

Updated unit tests

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

x/concentrated-liquidity/incentives.go

x/concentrated-liquidity/keeper_test.go

x/concentrated-liquidity/incentives.go

czarcas7ic · 2024-02-06T02:24:17Z

x/concentrated-liquidity/incentives.go

+				if incentiveCoin.Amount.IsPositive() {
+					collectedIncentivesForUptime = append(collectedIncentivesForUptime, incentiveCoin)
+				}


Is this to handle the case is which, even after scaling, we still truncate to zero?

Yeah. It doesn't make sense to collect a bunch of zero coins so decided to simply skip them

czarcas7ic · 2024-02-06T02:29:07Z

x/concentrated-liquidity/incentives_test.go

+			poolIncentiveRecords: []types.IncentiveRecord{withEmissionRate(incentiveRecordOne, oneE60Dec), incentiveRecordOne},
+
+			expectedResult: sdk.DecCoins{
+				// We only expect the first incentive record to qualify


We expect both to qualify, just the first is the only that should emit, right? I think the comment might just be wrong here.

Right except only the second one qualifies since the first one truncates.

Corrected here: 8a43a0b

czarcas7ic · 2024-02-06T02:30:33Z

x/concentrated-liquidity/incentives_test.go

@@ -2898,7 +2938,7 @@ func (s *KeeperTestSuite) TestQueryAndClaimAllIncentives() {
 					for _, growthInside := range tc.growthInside {
 						expectedCoins = expectedCoins.Add(sdk.NormalizeCoins(growthInside)...)
 					}
-					s.Require().Equal(expectedCoins, amountClaimed)
+					s.Require().Equal(expectedCoins.String(), amountClaimed.String())


clear indicator there was some debugging going on here 😂

czarcas7ic · 2024-02-06T02:32:37Z

x/concentrated-liquidity/incentives_test.go

+	// We multiply the current tick liqudity by this factor
+	// To bring the current tick liquidity to be at the border line of truncating.
+	// Then, we choose values on each side of the threshold to show that truncation is still possible
+	// but, with the scaling factor, it is less likely to occur due to more room for liquidity to grow.
+	currentTickLiquidityIncreaseFactor := osmomath.BigDecFromDec(cl.PerUnitLiqScalingFactor)


Are we able to write out somewhere what exactly these limits are? As in, are we just buying time and if a pool gets 100M liquidity we are back in the same boat, or is the liquidity unlikely (something like 1T for a single pool?)

The analysis is in the appendix of the hack md linked in the PR description.

In a sense, if liquidity growths infinitely, the limit will be hit again.

However, we have alerts in-place. Also, the buffer should be sufficient based on analysis.

Going higher with the scaling factor is also risky because, then, we would be running into overflows

czarcas7ic · 2024-02-06T02:33:22Z

x/concentrated-liquidity/incentives_test.go

@@ -3615,8 +3672,35 @@ func (s *KeeperTestSuite) TestIncentiveTruncation() {
 	s.Require().True(incentives.IsZero())

 	// Incentives should now be claimed due to lack of truncation
-	s.Ctx = s.Ctx.WithBlockTime(s.Ctx.BlockTime().Add(time.Minute * 51))
+	s.Ctx = s.Ctx.WithBlockTime(s.Ctx.BlockTime().Add(time.Hour * 6))


Why the change of this value?

The current tick liquidity and scaling factor value were changed so this threshold changes as well.

The goal is to show that with the increased scaling factor, we do not get a truncation within the expected range of increase

czarcas7ic

Really great work with this, super clear. IMO we should have at minimum 2 approvals on this prior to merge, 3 would be ideal but if we need to move fast I think 2 will suffice.

ValarDragon · 2024-02-06T16:55:21Z

x/concentrated-liquidity/incentives.go

 		// Total amount emitted = time elapsed * emission
-		totalEmittedAmount := timeElapsed.Mul(incentiveRecordBody.EmissionRate)
+		totalEmittedAmount := timeElapsed.MulTruncate(incentiveRecordBody.EmissionRate)


to future proof this a bit more, thoughts on adding comments in line here for the expected range of each term?

This is basically whats blocking approve for me rn (trying to rederive these)

Actual scale-up change LGTM

Attempted to address here:
9e10789

Let me know what you think

AlpinYukseloglu

Core scaling logic looks good to me, but had a clarifying question about how pools besides the ones being migrated are being handled

AlpinYukseloglu · 2024-02-07T05:14:54Z

app/apptesting/test_suite.go

@@ -696,3 +696,11 @@ func (s *KeeperTestHelper) SetupVolumeForPools(poolIDs []uint64, volumesForEachP
 func (s *KeeperTestHelper) IncreaseVolumeForPools(poolIDs []uint64, volumesForEachPool []osmomath.Int) {
 	s.SetupVolumeForPools(poolIDs, volumesForEachPool, map[uint64]osmomath.Int{})
 }
+
+// CompareDecCoinsSlice compares two slices of DecCoins
+func (s *KeeperTestHelper) CompareDecCoinsSlice(expected, actual []sdk.DecCoins) {


nit: something along the lines of RequireDecCoinsSlice seems more accurate

x/concentrated-liquidity/incentives.go

AlpinYukseloglu

LGTM, although I did not think deeply about implications of this change on existing state beyond our brief offline discussion, as it seems that is the focus #7416

…7409) * feat: scaling factor for pool uptime accumulator * changelog * updates * updates * handle overflows * Apply suggestions from code review * updates * Apply suggestions from code review * fix test and clean up * spelling * future proof multiplication overflow * clean up * comment * lint * rename

Closes: #XXX ## What is the purpose of the change This PR introduces incentive accumulator migration to address the truncation issue with pool 1423. In the v23 upgrade, we introduce a scaling factor to avoid truncating the per-unit-of-liquidity incentive accumulator. See: #7409 The goal is to have all the newly created pools utilize this accumulator. At the same time, we want to migrate the old pools that are currently affected (e.g. pool 1423). We also want to migrate only the affected pools to limit migration risk. To achieve all of the above, this PR: - Introduces a pool ID threshold snapshotted during the upgrade to state. After this threshold, all newly created pools utilize the scaling factor. - A hard-coded map of pool IDs that are below the threshold but migrated to the scaling factor logic In the core module logic, we dynamically select the scaling factor based on the pool ID to apply to the accumulator. Given successful migration, we will migrate the remaining pools in a future upgrade. ### Testnet Pool ID threshold is selected dynamically based on either mainnet or testnet state. For any environment other than `osmosis-1`, we avoid performing pool migration to avoid dealing with complexity. This is. fine as we don't care about other environments potentially truncating with older pools. ## Testing and Verifying - [ ] Test core helpers - [ ] Test upgrade handler - [ ] Test genesis ## 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

Closes: #XXX ## What is the purpose of the change This PR introduces incentive accumulator migration to address the truncation issue with pool 1423. In the v23 upgrade, we introduce a scaling factor to avoid truncating the per-unit-of-liquidity incentive accumulator. See: #7409 The goal is to have all the newly created pools utilize this accumulator. At the same time, we want to migrate the old pools that are currently affected (e.g. pool 1423). We also want to migrate only the affected pools to limit migration risk. To achieve all of the above, this PR: - Introduces a pool ID threshold snapshotted during the upgrade to state. After this threshold, all newly created pools utilize the scaling factor. - A hard-coded map of pool IDs that are below the threshold but migrated to the scaling factor logic In the core module logic, we dynamically select the scaling factor based on the pool ID to apply to the accumulator. Given successful migration, we will migrate the remaining pools in a future upgrade. ### Testnet Pool ID threshold is selected dynamically based on either mainnet or testnet state. For any environment other than `osmosis-1`, we avoid performing pool migration to avoid dealing with complexity. This is. fine as we don't care about other environments potentially truncating with older pools. ## Testing and Verifying - [ ] Test core helpers - [ ] Test upgrade handler - [ ] Test genesis ## 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 (cherry picked from commit b0d3ff9) # Conflicts: # go.mod # go.sum

feat: scaling factor for pool uptime accumulator

246e5f1

github-actions bot added C:app-wiring Changes to the app folder C:x/concentrated-liquidity labels Feb 2, 2024

p0mvn added the V:state/breaking State machine breaking PR label Feb 2, 2024

p0mvn changed the title ~~feat: scaling factor for pool uptime accumulator~~ feat: scaling factor for pool uptime accumulator to avoid truncation Feb 2, 2024

p0mvn added 5 commits February 2, 2024 18:48

changelog

99022a2

updates

213cec6

updates

04a91c6

Merge branch 'main' into roman/pool-incentive-scaling-factor

880e3d1

handle overflows

1367d0c

p0mvn commented Feb 5, 2024

View reviewed changes

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

p0mvn commented Feb 5, 2024

View reviewed changes

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

p0mvn commented Feb 5, 2024

View reviewed changes

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

p0mvn added 2 commits February 5, 2024 13:06

Apply suggestions from code review

748edd8

updates

3158f90

p0mvn commented Feb 5, 2024

View reviewed changes

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

p0mvn commented Feb 5, 2024

View reviewed changes

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

p0mvn added 3 commits February 5, 2024 13:10

Apply suggestions from code review

f76b9cb

fix test and clean up

f254a26

spelling

e36f4a1

p0mvn marked this pull request as ready for review February 5, 2024 20:24

p0mvn requested review from czarcas7ic and AlpinYukseloglu as code owners February 5, 2024 20:24

p0mvn mentioned this pull request Feb 6, 2024

feat: incentive accumulator migration #7416

Merged

9 tasks

czarcas7ic reviewed Feb 6, 2024

View reviewed changes

czarcas7ic approved these changes Feb 6, 2024

View reviewed changes

ValarDragon reviewed Feb 6, 2024

View reviewed changes

p0mvn added 3 commits February 6, 2024 14:27

future proof multiplication overflow

9e10789

clean up

de4b9ba

comment

8a43a0b

p0mvn requested review from ValarDragon and czarcas7ic February 6, 2024 21:49

lint

bab4173

AlpinYukseloglu reviewed Feb 7, 2024

View reviewed changes

AlpinYukseloglu approved these changes Feb 7, 2024

View reviewed changes

p0mvn added 2 commits February 7, 2024 09:26

Merge branch 'main' into roman/pool-incentive-scaling-factor

fea2d75

rename

5ac3e97

p0mvn merged commit 2661d4c into main Feb 7, 2024
1 check passed

p0mvn deleted the roman/pool-incentive-scaling-factor branch February 7, 2024 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: scaling factor for pool uptime accumulator to avoid truncation #7409

feat: scaling factor for pool uptime accumulator to avoid truncation #7409

p0mvn commented Feb 2, 2024 •

edited

Loading

czarcas7ic Feb 6, 2024

p0mvn Feb 6, 2024

czarcas7ic Feb 6, 2024

p0mvn Feb 6, 2024

czarcas7ic Feb 6, 2024

czarcas7ic Feb 6, 2024

p0mvn Feb 6, 2024 •

edited

Loading

czarcas7ic Feb 6, 2024

p0mvn Feb 6, 2024

czarcas7ic left a comment

ValarDragon Feb 6, 2024 •

edited

Loading

p0mvn Feb 6, 2024

AlpinYukseloglu left a comment

AlpinYukseloglu Feb 7, 2024

p0mvn Feb 7, 2024

AlpinYukseloglu left a comment

feat: scaling factor for pool uptime accumulator to avoid truncation #7409

feat: scaling factor for pool uptime accumulator to avoid truncation #7409

Conversation

p0mvn commented Feb 2, 2024 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

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 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

ValarDragon Feb 6, 2024 • edited Loading

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

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

p0mvn commented Feb 2, 2024 •

edited

Loading

p0mvn Feb 6, 2024 •

edited

Loading

ValarDragon Feb 6, 2024 •

edited

Loading