fix/test/docs: negative interval accumulation with incentive rewards (#…

…5872)

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

* changelog

* updates

* go mod

* fix/test/docs: negative interval accumulation with incentive rewards

* changelog

* updates

* update go.mod

* test: validate negative interval accumulation does not panic when serialized and deserialized (#5873)

* update osmoutils

* go mod tidy

---------

Co-authored-by: Adam Tucker <[email protected]>

CHANGELOG.md

@@ -46,6 +46,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * [#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
+* [#5872](https://github.com/osmosis-labs/osmosis/pull/5872) fix negative interval accumulation with incentive 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

go.mod

@@ -21,7 +21,7 @@ require (
 	github.com/ory/dockertest/v3 v3.10.0
 	github.com/osmosis-labs/go-mutesting v0.0.0-20221208041716-b43bcd97b3b3
 	github.com/osmosis-labs/osmosis/osmomath v0.0.3-dev.0.20230629191111-f375469de8b6
-	github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230728163612-426afac90c44
+	github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230801224523-e85e9a9cf445
 	github.com/osmosis-labs/osmosis/x/epochs v0.0.0-20230328024000-175ec88e4304
 	github.com/osmosis-labs/osmosis/x/ibc-hooks v0.0.0-20230602130523-f9a94d8bbd10
 	github.com/pkg/errors v0.9.1

go.sum

@@ -952,6 +952,8 @@ github.com/osmosis-labs/osmosis/osmomath v0.0.3-dev.0.20230629191111-f375469de8b
 github.com/osmosis-labs/osmosis/osmomath v0.0.3-dev.0.20230629191111-f375469de8b6/go.mod h1:JTym95/bqrSnG5MPcXr1YDhv43JdCeo3p+iDbazoX68=
 github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230728163612-426afac90c44 h1:UOaBVxEMMv2FS1znU7kHBdtSeZQIjnmXL4r9r19XyBo=
 github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230728163612-426afac90c44/go.mod h1:Pl8Nzx6O6ow/+aqfMoMSz4hX+zz6RrnDYsooptECGxM=
+github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230801224523-e85e9a9cf445 h1:V942btb00oVXHox7hEN8FrPfEaMTiVuInM7Tr00eOMU=
+github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230801224523-e85e9a9cf445/go.mod h1:Pl8Nzx6O6ow/+aqfMoMSz4hX+zz6RrnDYsooptECGxM=
 github.com/osmosis-labs/osmosis/x/epochs v0.0.0-20230328024000-175ec88e4304 h1:RIrWLzIiZN5Xd2JOfSOtGZaf6V3qEQYg6EaDTAkMnCo=
 github.com/osmosis-labs/osmosis/x/epochs v0.0.0-20230328024000-175ec88e4304/go.mod h1:yPWoJTj5RKrXKUChAicp+G/4Ni/uVEpp27mi/FF/L9c=
 github.com/osmosis-labs/osmosis/x/ibc-hooks v0.0.0-20230602130523-f9a94d8bbd10 h1:XrES5AHZMZ/Y78boW35PTignkhN9h8VvJ1sP8EJDIu8=

osmoutils/coin_helper.go

@@ -45,6 +45,23 @@ func SubDecCoinArrays(decCoinsArrayA []sdk.DecCoins, decCoinsArrayB []sdk.DecCoi
 	return finalDecCoinArray, nil
 }
 
+// SafeSubDecCoinArrays subtracts the contents of the second param from the first (decCoinsArrayA - decCoinsArrayB)
+// Note that this takes in two _arrays_ of DecCoins, meaning that each term itself is of type DecCoins (i.e. an array of DecCoin).
+// Contrary to SubDecCoinArrays, this subtractions allows for negative result values.
+func SafeSubDecCoinArrays(decCoinsArrayA []sdk.DecCoins, decCoinsArrayB []sdk.DecCoins) ([]sdk.DecCoins, error) {
+	if len(decCoinsArrayA) != len(decCoinsArrayB) {
+		return []sdk.DecCoins{}, fmt.Errorf("DecCoin arrays must be of equal length to be subtracted")
+	}
+
+	finalDecCoinArray := []sdk.DecCoins{}
+	for i := range decCoinsArrayA {
+		subResult, _ := decCoinsArrayA[i].SafeSub(decCoinsArrayB[i])
+		finalDecCoinArray = append(finalDecCoinArray, subResult)
+	}
+
+	return finalDecCoinArray, nil
+}
+
 // AddDecCoinArrays adds the contents of the second param from the first (decCoinsArrayA + decCoinsArrayB)
 // Note that this takes in two _arrays_ of DecCoins, meaning that each term itself is of type DecCoins (i.e. an array of DecCoin).
 func AddDecCoinArrays(decCoinsArrayA []sdk.DecCoins, decCoinsArrayB []sdk.DecCoins) ([]sdk.DecCoins, error) {

osmoutils/coin_helper_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/osmosis-labs/osmosis/osmoutils"
+	"github.com/osmosis-labs/osmosis/osmoutils/osmoassert"
 )
 
 var (
@@ -32,6 +33,8 @@ func TestSubDecCoins(t *testing.T) {
 
 		expectedOutput []sdk.DecCoins
 		expectError    bool
+		// whether unsafe subtraction should panic
+		expectPanicUnsafe bool
 	}{
 		"[[100foo, 100bar], [100foo, 100bar]] - [[50foo, 50bar], [50foo, 100bar]]": {
 			firstInput:  []sdk.DecCoins{hundredEach, hundredEach},
@@ -69,19 +72,42 @@ func TestSubDecCoins(t *testing.T) {
 			expectedOutput: []sdk.DecCoins{},
 			expectError:    true,
 		},
+
+		"negative result": {
+			firstInput:  []sdk.DecCoins{fiftyEach},
+			secondInput: []sdk.DecCoins{hundredEach},
+
+			expectedOutput:    []sdk.DecCoins{{sdk.DecCoin{Denom: "bar", Amount: sdk.NewDec(-50)}, sdk.DecCoin{Denom: "foo", Amount: sdk.NewDec(-50)}}},
+			expectPanicUnsafe: true,
+		},
 	}
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			actualOutput, err := osmoutils.SubDecCoinArrays(tc.firstInput, tc.secondInput)
+
+			var (
+				actualOutput []sdk.DecCoins
+				err1         error
+			)
+			osmoassert.ConditionalPanic(t, tc.expectPanicUnsafe, func() {
+				actualOutput, err1 = osmoutils.SubDecCoinArrays(tc.firstInput, tc.secondInput)
+			})
+
+			actualOutputSafe, err2 := osmoutils.SafeSubDecCoinArrays(tc.firstInput, tc.secondInput)
 
 			if tc.expectError {
-				require.Error(t, err)
+				require.Error(t, err1)
+				require.Error(t, err2)
 				require.Equal(t, tc.expectedOutput, actualOutput)
+				require.Equal(t, tc.expectedOutput, actualOutputSafe)
 				return
 			}
 
-			require.NoError(t, err)
-			require.Equal(t, tc.expectedOutput, actualOutput)
+			require.NoError(t, err1)
+			require.NoError(t, err2)
+			if !tc.expectPanicUnsafe {
+				require.Equal(t, tc.expectedOutput, actualOutput)
+			}
+			require.Equal(t, tc.expectedOutput, actualOutputSafe)
 		})
 	}
 }

x/concentrated-liquidity/README.md

@@ -1355,6 +1355,9 @@ This returns the amount of spread rewards collected by the user.
 
 ## Interval Accumulation
 
+Section pre-face: interval accumulation for incentives functions
+similarly to the spread rewards. However, we focus on spread rewards only for brevity.
+
 As mentioned in the previous sections, to collect spread rewards,
 we utilize a rewards accumulator abstraction with an interval accumulation extension.
 
@@ -1405,6 +1408,8 @@ For this reason, there are 3 ways to compute the rewards accrued inside the posi
 
 ### Negative Interval Accumulation Edge Case Behavior
 
+Case 1: Initialize lower tick snapshot to be greater than upper tick snapshot when current tick > upper tick
+
 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
@@ -1425,6 +1430,15 @@ The reason is that if the current tick is less than the tick we initialize, the
 As a result, the subtraction from the global accumulator for computing interval accumulation never leads to a
 negative value.
 
+Case 2: Initialize lower tick snapshot to be zero while upper tick snapshot to be non-zero when current tick < lower tick
+
+Assume that initially current tick > upper tick and the upper tick gets initialized by some position.
+Then, its accumulator snapshot is set to the global accumulator. Now, assume that the current tick
+moves under the future position's lower tick. Then, the position gets initialized.
+
+As a result, the lower tick is set to 0, and interval accumulation is
+`lower tick snapshot - upper tick snapshot = 0 - positive value = negative value`
+
 ## Swaps
 
 Swapping within a single tick works as the regular `xy = k` curve. For swaps

x/concentrated-liquidity/incentives.go

@@ -671,18 +671,28 @@ func (k Keeper) GetUptimeGrowthInsideRange(ctx sdk.Context, poolId uint64, lower
 	upperTickUptimeValues := getUptimeTrackerValues(upperTickInfo.UptimeTrackers.List)
 	// If current tick is below range, we subtract uptime growth of upper tick from that of lower tick
 	if currentTick < lowerTick {
-		return osmoutils.SubDecCoinArrays(lowerTickUptimeValues, upperTickUptimeValues)
+		// Note: SafeSub with negative accumulation is possible if upper tick is initialized first
+		// while current tick > upper tick. Then, the current tick under the lower tick. The lower
+		// tick then gets initialized to zero.
+		// Therefore, we allow for negative result.
+		return osmoutils.SafeSubDecCoinArrays(lowerTickUptimeValues, upperTickUptimeValues)
 	} else if currentTick < upperTick {
 		// If current tick is within range, we subtract uptime growth of lower and upper tick from global growth
+		// Note: each individual tick snapshot never be greater than the global uptime accumulator.
+		// Therefore, we do not allow for negative result.
 		globalMinusUpper, err := osmoutils.SubDecCoinArrays(globalUptimeValues, upperTickUptimeValues)
 		if err != nil {
 			return []sdk.DecCoins{}, err
 		}
 
-		return osmoutils.SubDecCoinArrays(globalMinusUpper, lowerTickUptimeValues)
+		// Note: SafeSub with negative accumulation is possible if lower tick is initialized after upper tick
+		// and the current tick is between the two.
+		return osmoutils.SafeSubDecCoinArrays(globalMinusUpper, lowerTickUptimeValues)
 	} else {
 		// If current tick is above range, we subtract uptime growth of lower tick from that of upper tick
-		return osmoutils.SubDecCoinArrays(upperTickUptimeValues, lowerTickUptimeValues)
+		// Note: SafeSub with negative accumulation is possible if lower tick is initialized after upper tick
+		// and the current tick is above the two.
+		return osmoutils.SafeSubDecCoinArrays(upperTickUptimeValues, lowerTickUptimeValues)
 	}
 }
 

x/concentrated-liquidity/invariant_test.go

@@ -9,13 +9,10 @@ import (
 )
 
 type ExpectedGlobalRewardValues struct {
-	// 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
+	ExpectedAdditiveSpreadRewardTolerance sdk.Dec
+	ExpectedAdditiveIncentivesTolerance   sdk.Dec
+	TotalSpreadRewards                    sdk.Coins
+	TotalIncentives                       sdk.Coins
 }
 
 // assertGlobalInvariants asserts all available global invariants (i.e. invariants that should hold on all valid states).
@@ -107,23 +104,33 @@ func (s *KeeperTestSuite) assertTotalRewardsInvariant(expectedGlobalRewardValues
 		totalCollectedIncentives = totalCollectedIncentives.Add(collectedIncentives...)
 	}
 
-	additiveTolerance := sdk.Dec{}
-	if !expectedGlobalRewardValues.ExpectedAdditiveTolerance.IsNil() {
-		additiveTolerance = expectedGlobalRewardValues.ExpectedAdditiveTolerance
+	spreadRewardAdditiveTolerance := sdk.Dec{}
+	if !expectedGlobalRewardValues.ExpectedAdditiveSpreadRewardTolerance.IsNil() {
+		spreadRewardAdditiveTolerance = expectedGlobalRewardValues.ExpectedAdditiveSpreadRewardTolerance
 	}
 
-	// For global invariant checks, we simply ensure that any rounding error was in the pool's favor.
+	incentivesAdditiveTolerance := sdk.Dec{}
+	if !expectedGlobalRewardValues.ExpectedAdditiveIncentivesTolerance.IsNil() {
+		incentivesAdditiveTolerance = expectedGlobalRewardValues.ExpectedAdditiveSpreadRewardTolerance
+	}
+
+	// We ensure that any rounding error was in the pool's favor by rounding down.
 	// 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
+	// TODO: multiplicative tolerance to allow for
 	// tightening this check further.
-	errTolerance := osmomath.ErrTolerance{
-		AdditiveTolerance: additiveTolerance,
+	spreadRewardErrTolerance := osmomath.ErrTolerance{
+		AdditiveTolerance: spreadRewardAdditiveTolerance,
+		RoundingDir:       osmomath.RoundDown,
+	}
+
+	incentivesErrTolerance := osmomath.ErrTolerance{
+		AdditiveTolerance: incentivesAdditiveTolerance,
 		RoundingDir:       osmomath.RoundDown,
 	}
 
 	// Assert total collected spread rewards and incentives equal to expected
-	s.Require().True(errTolerance.EqualCoins(expectedTotalSpreadRewards, totalCollectedSpread), "expected spread rewards vs. collected: %s vs. %s", expectedTotalSpreadRewards, totalCollectedSpread)
-	s.Require().True(errTolerance.EqualCoins(expectedTotalIncentives, totalCollectedIncentives), "expected incentives vs. collected: %s vs. %s", expectedTotalIncentives, totalCollectedIncentives)
+	s.Require().True(spreadRewardErrTolerance.EqualCoins(expectedTotalSpreadRewards, totalCollectedSpread), "expected spread rewards vs. collected: %s vs. %s", expectedTotalSpreadRewards, totalCollectedSpread)
+	s.Require().True(incentivesErrTolerance.EqualCoins(expectedTotalIncentives, totalCollectedIncentives), "expected incentives vs. collected: %s vs. %s", expectedTotalIncentives, totalCollectedIncentives)
 
 	// Refetch total pool balances across all pools
 	remainingPositions, finalTotalPoolLiquidity, remainingTotalSpreadRewards, remainingTotalIncentives := s.getAllPositionsAndPoolBalances(cachedCtx)