fix/test/docs: negative interval accumulation with spread rewards

CHANGELOG.md

@@ -45,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### State Breaking
 
 * [#5532](https://github.com/osmosis-labs/osmosis/pull/5532) fix: Fix x/tokenfactory genesis import denoms reset x/bank existing denom metadata
+* [#5869](https://github.com/osmosis-labs/osmosis/pull/5869) fix negative interval accumulation with spread rewards
 * [#5883](https://github.com/osmosis-labs/osmosis/pull/5883) feat: Uninitialize empty ticks
 * [#5874](https://github.com/osmosis-labs/osmosis/pull/5874) Remove Partial Migration from superfluid migration to CL
 * [#5901](https://github.com/osmosis-labs/osmosis/pull/5901) Adding support for CW pools in ProtoRev

app/apptesting/test_suite.go

@@ -130,6 +130,18 @@ func (s *KeeperTestHelper) SetupTestForInitGenesis() {
 	s.hasUsedAbci = true
 }
 
+// RunTestCaseWithoutStateUpdates runs the testcase as a callback with the given name.
+// Does not persist any state changes. This is useful when test suite uses common state setup
+// but desures each test case to be run in isolation.
+func (s *KeeperTestHelper) RunTestCaseWithoutStateUpdates(name string, cb func(t *testing.T)) {
+	originalCtx := s.Ctx
+	s.Ctx, _ = s.Ctx.CacheContext()
+
+	s.T().Run(name, cb)
+
+	s.Ctx = originalCtx
+}
+
 func (s *KeeperTestHelper) SetEpochStartTime() {
 	epochsKeeper := s.App.EpochsKeeper
 

osmoutils/accum/accum.go

@@ -134,14 +134,10 @@ func (accum *AccumulatorObject) NewPosition(name string, numShareUnits sdk.Dec,
 // It sets the position's accumulator to the given value of intervalAccumulationPerShare.
 // This is useful for when the accumulation happens at a sub-range of the full accumulator
 // rewards range. For example, a concentrated liquidity narrow range position.
-// All intervalAccumulationPerShare DecCoin values must be non-negative.
+// intervalAccumulationPerShare DecCoin values are allowed to be negative.
 // The position is initialized with empty unclaimed rewards
 // If there is an existing position for the given address, it is overwritten.
 func (accum *AccumulatorObject) NewPositionIntervalAccumulation(name string, numShareUnits sdk.Dec, intervalAccumulationPerShare sdk.DecCoins, options *Options) error {
-	if intervalAccumulationPerShare.IsAnyNegative() {
-		return NegativeIntervalAccumulationPerShareError{intervalAccumulationPerShare}
-	}
-
 	if err := options.validate(); err != nil {
 		return err
 	}

osmoutils/accum/accum_test.go

@@ -344,12 +344,16 @@ func (suite *AccumTestSuite) TestNewPositionIntervalAccumulation() {
 			},
 			options: &emptyPositionOptions,
 		},
-		"negative acc value - error": {
+		"negative acc value - no error": {
 			accObject:                    defaultAccObject,
 			name:                         testAddressOne,
 			numShareUnits:                positionOne.NumShares,
 			intervalAccumulationPerShare: defaultAccObject.GetValue().MulDec(sdk.NewDec(-1)),
-			expectedError:                accumPackage.NegativeIntervalAccumulationPerShareError{defaultAccObject.GetValue().MulDec(sdk.NewDec(-1))},
+			expectedPosition: accumPackage.Record{
+				NumShares:             positionOne.NumShares,
+				AccumValuePerShare:    defaultAccObject.GetValue().MulDec(sdk.NewDec(-1)),
+				UnclaimedRewardsTotal: emptyCoins,
+			},
 		},
 	}
 

x/concentrated-liquidity/README.md

@@ -1353,6 +1353,78 @@ func (k Keeper) collectSpreadRewards(
 
 This returns the amount of spread rewards collected by the user.
 
+## Interval Accumulation
+
+As mentioned in the previous sections, to collect spread rewards,
+we utilize a rewards accumulator abstraction with an interval accumulation extension.
+
+The accumulator abstraction can be summarized by the following bullet points:
+- accumulator values are per-share
+- per-pool-global ever-increasing accumulator
+- each position takes a snapshot of the accumulator at the time of creation
+- assume t' represents the time of claiming rewards and t represents the time of position's snapshot of
+the global accumulator. Then, the total position's rewards are equal to
+`(global accumulator at time t' - position snapshot at time t) * number of shares
+
+Interval accumulation further extends this abstraction where positions only accumulate the rewards whenever they are active.
+That is, the current tick is within the position's range.
+
+To calculate that, we utilize tick accumulators. Each tick accumulator is updated whenever it is crossed. It can be thought
+of as having the snapshot of the global accumulator when going in the opposite direction of the last traversal. For example, assume that
+we are reasoning about the snapshot value of tick 100 when the current tick is 150. Then, we know that the value of the tick is 100
+contains rewards accrued before crossing tick 100 when going from the left to the right.
+
+Fundamentally, our base accumulator abstraction changes to the following:
+- accumulator values are per-share (still the same)
+- per-pool-global ever-increasing accumulator (still the same)
+- each position takes a snapshot of the rewards accumulated while the position is active at the time of its creation (as defined by tick accumulator snapshots) (changed)
+- assume t' represents the time of claiming rewards and t represents the time of the position's snapshot of the interval accumulation.
+Then, the total position's rewards are equal to `(global accumulator at time t' - interval accumulation outside at time t' - interval accumulation inside at time t) * number of shares
+Essentially, this gives us interval accumulation inside between times t and t' for each share of the position.
+
+By convention, we initialize the tick snapshot to either:
+a) 0 if the current tick is < tick
+b) global accumulator value if the current tick is >= tick
+
+This is done to ensure that the tick accumulator always contains the rewards accrued before crossing the tick from left to right.
+
+Since tick accumulator snapshots are initialized at different times, their comparison is not meaningful.
+
+By having tick snapshots at each edge of the position and a global value, we can compute how many rewards are accrued inside
+the position. It accounts for all rewards accrued between position creation and the time of claiming rewards whenever the following
+is true:
+`lower tick <= current tick < upper tick``
+
+For this reason, there are 3 ways to compute the rewards accrued inside the position:
+1. current tick is below the position's range (current tick < lower tick)
+- Then, we compute rewards as `lower tick snapshot - upper tick snapshot`
+2. current tick is within the position's range (lower tick <= current tick < upper tick)
+- Then, we compute rewards as `global accumulator - upper tick snapshot - lower tick snapshot`
+3. current tick is above the position's range (current tick >= upper tick)
+- Then, we compute rewards as `upper tick accumulator - lower tick accumulator`
+
+### Negative Interval Accumulation Edge Case Behavior
+
+Note, that if we initialize the lower tick after the upper tick is already initialized,
+for example, by another position, this might lead to negative accumulation inside
+the interval. This is only possible if the current tick is greater than the lower tick
+that is being initialized.
+
+The reason is that we initialize the lower tick accumulator to the global accumulator
+if the current tick >= tick. As a result, when subtracting the lower tick snapshot from the upper,
+the lower one is greater than or equal to the upper.
+
+This is not an issue because it gets canceled out by the "interval accumulation outside at time t'" that is added to
+the "interval accumulation inside at time t" before being subtracted from the global accumulator at the time of claiming.
+
+Perhaps even more importantly, as long as the _change_ in interval accumulation is tracked correctly, the initial value should not make a difference.
+
+Interestingly, this edge case should not be possible in the other direction. That is, we cannot get negative
+interval accumulation inside if the upper is initialized after the lower and the current tick is less than the upper tick.
+The reason is that if the current tick is less than the tick we initialize, the snapshot becomes 0 by convention.
+As a result, the subtraction from the global accumulator for computing interval accumulation never leads to a
+negative value.
+
 ## Swaps
 
 Swapping within a single tick works as the regular `xy = k` curve. For swaps

x/concentrated-liquidity/incentives.go

@@ -810,7 +810,8 @@ func updateAccumAndClaimRewards(accum *accum.AccumulatorObject, positionKey stri
 		// The position accumulator value must always equal to the growth inside at the time of last update.
 		// Since this is the time we update the accumulator, we must subtract the growth outside from the global accumulator value
 		// to get growth inside at the current block time.
-		currentGrowthInsideForPosition := accum.GetValue().Sub(growthOutside)
+		// Note: this is SafeSub because interval accumulation is allowed to be negative.
+		currentGrowthInsideForPosition, _ := accum.GetValue().SafeSub(growthOutside)
 		err := accum.SetPositionIntervalAccumulation(positionKey, currentGrowthInsideForPosition)
 		if err != nil {
 			return sdk.Coins{}, sdk.DecCoins{}, err

x/concentrated-liquidity/invariant_test.go

@@ -9,8 +9,13 @@ import (
 )
 
 type ExpectedGlobalRewardValues struct {
-	TotalSpreadRewards sdk.Coins
-	TotalIncentives    sdk.Coins
+	// By default, the global reward checks just ensure that rounding is done
+	// in the pools favor.
+	// The tolerance here ensures that it rounded in the pools favor by at
+	// _at most_ ExpectedAdditiveTolerance units.
+	ExpectedAdditiveTolerance sdk.Dec
+	TotalSpreadRewards        sdk.Coins
+	TotalIncentives           sdk.Coins
 }
 
 // assertGlobalInvariants asserts all available global invariants (i.e. invariants that should hold on all valid states).
@@ -102,12 +107,18 @@ func (s *KeeperTestSuite) assertTotalRewardsInvariant(expectedGlobalRewardValues
 		totalCollectedIncentives = totalCollectedIncentives.Add(collectedIncentives...)
 	}
 
+	additiveTolerance := sdk.Dec{}
+	if !expectedGlobalRewardValues.ExpectedAdditiveTolerance.IsNil() {
+		additiveTolerance = expectedGlobalRewardValues.ExpectedAdditiveTolerance
+	}
+
 	// For global invariant checks, we simply ensure that any rounding error was in the pool's favor.
 	// This is to allow for cases where we slightly overround, which would otherwise fail here.
 	// TODO: create ErrTolerance type that allows for additive OR multiplicative tolerance to allow for
 	// tightening this check further.
 	errTolerance := osmomath.ErrTolerance{
-		RoundingDir: osmomath.RoundDown,
+		AdditiveTolerance: additiveTolerance,
+		RoundingDir:       osmomath.RoundDown,
 	}
 
 	// Assert total collected spread rewards and incentives equal to expected