[CL] Changes from audit results(1/2)

osmomath/math_test.go

@@ -90,7 +90,7 @@ func TestPowApprox(t *testing.T) {
 
 	for i, tc := range testCases {
 		var actualResult sdk.Dec
-		ConditionalPanic(t, tc.base.Equal(sdk.ZeroDec()), func() {
+		ConditionalPanic(t, tc.base.IsZero(), func() {
 			fmt.Println(tc.base)
 			actualResult = PowApprox(tc.base, tc.exp, tc.powPrecision)
 			require.True(
@@ -154,7 +154,7 @@ func TestPow(t *testing.T) {
 
 	for i, tc := range testCases {
 		var actualResult sdk.Dec
-		ConditionalPanic(t, tc.base.Equal(sdk.ZeroDec()), func() {
+		ConditionalPanic(t, tc.base.IsZero(), func() {
 			actualResult = Pow(tc.base, tc.exp)
 			require.True(
 				t,

osmoutils/accum/accum.go

@@ -294,7 +294,7 @@ func (accum *AccumulatorObject) UpdatePosition(name string, numShares sdk.Dec) e
 // All intervalAccumulationPerShare DecCoin value must be non-negative. They must also be a superset of the
 // old accumulator value associated with the position.
 func (accum *AccumulatorObject) UpdatePositionIntervalAccumulation(name string, numShares sdk.Dec, intervalAccumulationPerShare sdk.DecCoins) error {
-	if numShares.Equal(sdk.ZeroDec()) {
+	if numShares.IsZero() {
 		return ZeroSharesError
 	}
 
@@ -422,7 +422,7 @@ func (accum AccumulatorObject) ClaimRewards(positionName string) (sdk.Coins, sdk
 	// This is acceptable because we round in favor of the protocol.
 	truncatedRewardsTotal, dust := totalRewards.TruncateDecimal()
 
-	if position.NumShares.Equal(sdk.ZeroDec()) {
+	if position.NumShares.IsZero() {
 		// remove the position from state entirely if numShares = zero
 		accum.deletePosition(positionName)
 	} else {

osmoutils/accum/accum_test.go

@@ -972,7 +972,7 @@ func (suite *AccumTestSuite) TestRemoveFromPosition() {
 				suite.Require().Equal(tc.startingUnclaimedRewards.Add(tc.expAccumDelta.MulDec(tc.startingNumShares)...), newPosition.UnclaimedRewardsTotal)
 
 				// Ensure address's position properly reflects new number of shares
-				if (tc.startingNumShares.Sub(tc.removedShares)).Equal(sdk.ZeroDec()) {
+				if (tc.startingNumShares.Sub(tc.removedShares)).IsZero() {
 					suite.Require().Equal(emptyDec, newPosition.NumShares)
 				} else {
 					suite.Require().Equal(tc.startingNumShares.Sub(tc.removedShares), newPosition.NumShares)
@@ -1163,7 +1163,7 @@ func (suite *AccumTestSuite) TestGetPositionSize() {
 			// Get position size from valid address (or from nonexistant if address does not exist)
 			positionSize, err := curAccum.GetPositionSize(positionName)
 
-			if tc.changedShares.GT(sdk.ZeroDec()) {
+			if tc.changedShares.IsPositive() {
 				accumPackage.InitOrUpdatePosition(curAccum, curAccum.GetValue(), positionName, tc.numShares.Add(tc.changedShares), sdk.NewDecCoins(), nil)
 			}
 
@@ -1613,14 +1613,14 @@ func (suite *AccumTestSuite) TestGetTotalShares() {
 
 		// Half the time, we add to the new position
 		addAmt := baseAmt.Mul(addShares)
-		if addAmt.GT(sdk.ZeroDec()) {
+		if addAmt.IsPositive() {
 			err = curAccum.AddToPosition(curAddr, addAmt)
 			suite.Require().NoError(err)
 		}
 
 		// Half the time, we remove one share from the position
 		amtToRemove := sdk.OneDec().Mul(removeShares)
-		if amtToRemove.GT(sdk.ZeroDec()) {
+		if amtToRemove.IsPositive() {
 			err = curAccum.RemoveFromPosition(curAddr, amtToRemove)
 			suite.Require().NoError(err)
 		}

x/concentrated-liquidity/export_test.go

@@ -118,10 +118,6 @@ func (k Keeper) CollectSpreadRewards(ctx sdk.Context, owner sdk.AccAddress, posi
 	return k.collectSpreadRewards(ctx, owner, positionId)
 }
 
-func (k Keeper) EnsurePositionOwner(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, positionId uint64) error {
-	return k.ensurePositionOwner(ctx, sender, poolId, positionId)
-}
-
 func (k Keeper) PrepareClaimableSpreadRewards(ctx sdk.Context, positionId uint64) (sdk.Coins, error) {
 	return k.prepareClaimableSpreadRewards(ctx, positionId)
 }

x/concentrated-liquidity/incentives.go

@@ -403,27 +403,24 @@ func (k Keeper) updateGivenPoolUptimeAccumulatorsToNow(ctx sdk.Context, pool typ
 
 	// We optimistically assume that all liquidity on the active tick qualifies and handle
 	// uptime-related checks in forfeiting logic.
+
+	// If there is no share to be incentivized for the current uptime accumulator, we leave it unchanged
 	qualifyingLiquidity := pool.GetLiquidity().Add(qualifyingBalancerShares)
+	if !qualifyingLiquidity.LT(sdk.OneDec()) {
+		for uptimeIndex := range uptimeAccums {
+			// Get relevant uptime-level values
+			curUptimeDuration := types.SupportedUptimes[uptimeIndex]
+			incentivesToAddToCurAccum, updatedPoolRecords, err := calcAccruedIncentivesForAccum(ctx, curUptimeDuration, qualifyingLiquidity, timeElapsedSec, poolIncentiveRecords)
+			if err != nil {
+				return err
+			}
 
-	for uptimeIndex := range uptimeAccums {
-		// Get relevant uptime-level values
-		curUptimeDuration := types.SupportedUptimes[uptimeIndex]
+			// Emit incentives to current uptime accumulator
+			uptimeAccums[uptimeIndex].AddToAccumulator(incentivesToAddToCurAccum)
 
-		// If there is no share to be incentivized for the current uptime accumulator, we leave it unchanged
-		if qualifyingLiquidity.LT(sdk.OneDec()) {
-			continue
+			// Update pool records (stored in state after loop)
+			poolIncentiveRecords = updatedPoolRecords
 		}
-
-		incentivesToAddToCurAccum, updatedPoolRecords, err := calcAccruedIncentivesForAccum(ctx, curUptimeDuration, qualifyingLiquidity, timeElapsedSec, poolIncentiveRecords)
-		if err != nil {
-			return err
-		}
-
-		// Emit incentives to current uptime accumulator
-		uptimeAccums[uptimeIndex].AddToAccumulator(incentivesToAddToCurAccum)
-
-		// Update pool records (stored in state after loop)
-		poolIncentiveRecords = updatedPoolRecords
 	}
 
 	// Update pool incentive records and LastLiquidityUpdate time in state to reflect emitted incentives
@@ -538,7 +535,7 @@ func (k Keeper) setIncentiveRecord(ctx sdk.Context, incentiveRecord types.Incent
 	// In all other cases, we update the record in state
 	if store.Has(key) && incentiveRecordBody.RemainingCoin.IsZero() {
 		store.Delete(key)
-	} else if incentiveRecordBody.RemainingCoin.Amount.GT(sdk.ZeroDec()) {
+	} else if incentiveRecordBody.RemainingCoin.Amount.IsPositive() {
 		osmoutils.MustSet(store, key, &incentiveRecordBody)
 	}
 
@@ -972,9 +969,11 @@ func (k Keeper) collectIncentives(ctx sdk.Context, sender sdk.AccAddress, positi
 		return sdk.Coins{}, sdk.Coins{}, err
 	}
 
-	err = k.ensurePositionOwner(ctx, sender, position.PoolId, positionId)
-	if err != nil {
-		return sdk.Coins{}, sdk.Coins{}, err
+	if sender.String() != position.Address {
+		return sdk.Coins{}, sdk.Coins{}, types.NotPositionOwnerError{
+			PositionId: positionId,
+			Address:    sender.String(),
+		}
 	}
 
 	// Claim all incentives for the position.

x/concentrated-liquidity/lp_test.go

@@ -663,9 +663,6 @@ func (s *KeeperTestSuite) TestWithdrawPosition() {
 				s.Require().ErrorAs(err, &types.PositionIdNotFoundError{PositionId: config.positionId})
 				s.Require().Equal(clmodel.Position{}, position)
 
-				err = concentratedLiquidityKeeper.EnsurePositionOwner(s.Ctx, owner, config.poolId, config.positionId)
-				s.Require().Error(err, types.NotPositionOwnerError{PositionId: config.positionId, Address: owner.String()})
-
 				// Since the positionLiquidity is deleted, retrieving it should return an error.
 				positionLiquidity, err := s.App.ConcentratedLiquidityKeeper.GetPositionLiquidity(s.Ctx, config.positionId)
 				s.Require().Error(err)

x/concentrated-liquidity/math/math.go

@@ -26,11 +26,11 @@ func Liquidity0(amount sdk.Int, sqrtPriceA, sqrtPriceB sdk.Dec) sdk.Dec {
 
 	product := sqrtPriceABigDec.Mul(sqrtPriceBBigDec)
 	diff := sqrtPriceBBigDec.Sub(sqrtPriceABigDec)
-	if diff.Equal(osmomath.ZeroDec()) {
+	if diff.IsZero() {
 		panic(fmt.Sprintf("liquidity0 diff is zero: sqrtPriceA %s sqrtPriceB %s", sqrtPriceA, sqrtPriceB))
 	}
 
-	return amountBigDec.Mul(product).Quo(diff).SDKDec()
+	return amountBigDec.MulMut(product).QuoMut(diff).SDKDec()
 }
 
 // Liquidity1 takes an amount of asset1 in the pool as well as the sqrtpCur and the nextPrice
@@ -49,11 +49,11 @@ func Liquidity1(amount sdk.Int, sqrtPriceA, sqrtPriceB sdk.Dec) sdk.Dec {
 	sqrtPriceBBigDec := osmomath.BigDecFromSDKDec(sqrtPriceB)
 
 	diff := sqrtPriceBBigDec.Sub(sqrtPriceABigDec)
-	if diff.Equal(osmomath.ZeroDec()) {
+	if diff.IsZero() {
 		panic(fmt.Sprintf("liquidity1 diff is zero: sqrtPriceA %s sqrtPriceB %s", sqrtPriceA, sqrtPriceB))
 	}
 
-	return amountBigDec.Quo(diff).SDKDec()
+	return amountBigDec.QuoMut(diff).SDKDec()
 }
 
 // CalcAmount0 takes the asset with the smaller liquidity in the pool as well as the sqrtpCur and the nextPrice and calculates the amount of asset 0
@@ -70,7 +70,7 @@ func CalcAmount0Delta(liq, sqrtPriceA, sqrtPriceB sdk.Dec, roundUp bool) sdk.Dec
 	// this is to prevent removing more from the pool than expected due to rounding
 	// example: we calculate 1000000.9999999 uusdc (~$1) amountIn and 2000000.999999 uosmo amountOut
 	// we would want the user to put in 1000001 uusdc rather than 1000000 uusdc to ensure we are charging enough for the amount they are removing
-	// additionally, without rounding, there exists cases where the swapState.amountSpecifiedRemaining.GT(sdk.ZeroDec()) for loop within
+	// additionally, without rounding, there exists cases where the swapState.amountSpecifiedRemaining.IsPositive() for loop within
 	// the CalcOut/In functions never actually reach zero due to dust that would have never gotten counted towards the amount (numbers after the 10^6 place)
 	if roundUp {
 		// Note that we do MulTruncate so that the denominator is smaller as this is
@@ -105,7 +105,7 @@ func CalcAmount1Delta(liq, sqrtPriceA, sqrtPriceB sdk.Dec, roundUp bool) sdk.Dec
 	// this is to prevent removing more from the pool than expected due to rounding
 	// example: we calculate 1000000.9999999 uusdc (~$1) amountIn and 2000000.999999 uosmo amountOut
 	// we would want the used to put in 1000001 uusdc rather than 1000000 uusdc to ensure we are charging enough for the amount they are removing
-	// additionally, without rounding, there exists cases where the swapState.amountSpecifiedRemaining.GT(sdk.ZeroDec()) for loop within
+	// additionally, without rounding, there exists cases where the swapState.amountSpecifiedRemaining.IsPositive() for loop within
 	// the CalcOut/In functions never actually reach zero due to dust that would have never gotten counted towards the amount (numbers after the 10^6 place)
 	if roundUp {
 		// Note that we do MulRoundUp so that the end result is larger as this is
@@ -128,12 +128,14 @@ func CalcAmount1Delta(liq, sqrtPriceA, sqrtPriceB sdk.Dec, roundUp bool) sdk.Dec
 // to avoid overpaying the amount out of the pool. Therefore, we round up.
 // sqrt_next = liq * sqrt_cur / (liq + token_in * sqrt_cur)
 func GetNextSqrtPriceFromAmount0InRoundingUp(sqrtPriceCurrent, liquidity, amountZeroRemainingIn sdk.Dec) (sqrtPriceNext sdk.Dec) {
-	if amountZeroRemainingIn.Equal(sdk.ZeroDec()) {
+	if amountZeroRemainingIn.IsZero() {
 		return sqrtPriceCurrent
 	}
 
 	product := amountZeroRemainingIn.Mul(sqrtPriceCurrent)
-	denominator := product.AddMut(liquidity)
+	// denominator = product + liquidity
+	denominator := product
+	denominator.AddMut(liquidity)
 	return liquidity.Mul(sqrtPriceCurrent).QuoRoundupMut(denominator)
 }
 
@@ -143,7 +145,7 @@ func GetNextSqrtPriceFromAmount0InRoundingUp(sqrtPriceCurrent, liquidity, amount
 // so that we get the desired output amount out. Therefore, we round up.
 // sqrt_next = liq * sqrt_cur / (liq - token_out * sqrt_cur)
 func GetNextSqrtPriceFromAmount0OutRoundingUp(sqrtPriceCurrent, liquidity, amountZeroRemainingOut sdk.Dec) (sqrtPriceNext sdk.Dec) {
-	if amountZeroRemainingOut.Equal(sdk.ZeroDec()) {
+	if amountZeroRemainingOut.IsZero() {
 		return sqrtPriceCurrent
 	}
 

x/concentrated-liquidity/model/pool_test.go

@@ -695,8 +695,8 @@ func (suite *ConcentratedPoolTestSuite) TestCalcActualAmounts() {
 				amt1Diff := actualAmount1.Sub(actualAmount1Neg.Neg())
 
 				// Difference is between 0 and 1 due to positive liquidity rounding up and negative liquidity performing math normally.
-				suite.Require().True(amt0Diff.GT(sdk.ZeroDec()) && amt0Diff.LT(sdk.OneDec()))
-				suite.Require().True(amt1Diff.GT(sdk.ZeroDec()) && amt1Diff.LT(sdk.OneDec()))
+				suite.Require().True(amt0Diff.IsPositive() && amt0Diff.LT(sdk.OneDec()))
+				suite.Require().True(amt1Diff.IsPositive() && amt1Diff.LT(sdk.OneDec()))
 			}
 		})
 	}

x/concentrated-liquidity/position.go

@@ -99,22 +99,6 @@ func (k Keeper) HasAnyPositionForPool(ctx sdk.Context, poolId uint64) (bool, err
 	return osmoutils.HasAnyAtPrefix(store, poolPositionKey, parse)
 }
 
-func (k Keeper) ensurePositionOwner(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, positionId uint64) error {
-	isOwner := k.isPositionOwner(ctx, sender, poolId, positionId)
-	if !isOwner {
-		return types.NotPositionOwnerError{
-			PositionId: positionId,
-			Address:    sender.String(),
-		}
-	}
-	return nil
-}
-
-// isPositionOwner returns true if the given positionId is owned by the given sender inside the given pool.
-func (k Keeper) isPositionOwner(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, positionId uint64) bool {
-	return ctx.KVStore(k.storeKey).Has(types.KeyAddressPoolIdPositionId(sender, poolId, positionId))
-}
-
 // GetAllPositionsForPoolId gets all the position for a specific poolId and store prefix.
 func (k Keeper) GetAllPositionIdsForPoolId(ctx sdk.Context, prefix []byte, poolId uint64) ([]uint64, error) {
 	store := ctx.KVStore(k.storeKey)

x/concentrated-liquidity/position_test.go

@@ -313,7 +313,7 @@ func (s *KeeperTestSuite) TestInitOrUpdatePosition() {
 				if test.positionExists {
 					// Track how much the current uptime accum has grown by
 					actualUptimeAccumDelta[uptimeIndex] = newUptimeAccumValues[uptimeIndex].Sub(initUptimeAccumValues[uptimeIndex])
-					if timeElapsedSec.GT(sdk.ZeroDec()) {
+					if timeElapsedSec.IsPositive() {
 						expectedGrowthCurAccum, _, err := cl.CalcAccruedIncentivesForAccum(s.Ctx, uptime, test.param.liquidityDelta, timeElapsedSec, expectedIncentiveRecords)
 						s.Require().NoError(err)
 						expectedUptimeAccumValueGrowth[uptimeIndex] = expectedGrowthCurAccum
@@ -410,103 +410,6 @@ type positionOwnershipTest struct {
 	poolId         uint64
 }
 
-func (s *KeeperTestSuite) runIsPositionOwnerTest(test positionOwnershipTest) {
-	s.SetupTest()
-	p := s.PrepareConcentratedPool()
-	for i, owner := range test.setupPositions {
-		err := s.App.ConcentratedLiquidityKeeper.InitOrUpdatePosition(s.Ctx, p.GetId(), owner, DefaultLowerTick, DefaultUpperTick, DefaultLiquidityAmt, DefaultJoinTime, uint64(i))
-		s.Require().NoError(err)
-	}
-	err := s.App.ConcentratedLiquidityKeeper.EnsurePositionOwner(s.Ctx, test.queryPositionOwner, test.poolId, test.queryPositionId)
-	if test.expPass {
-		s.Require().NoError(err)
-	} else {
-		s.Require().Error(err)
-	}
-
-}
-
-func (s *KeeperTestSuite) TestIsPositionOwnerMultiPosition() {
-	tenAddrOneAddr := []sdk.AccAddress{}
-	for i := 0; i < 10; i++ {
-		tenAddrOneAddr = append(tenAddrOneAddr, s.TestAccs[0])
-	}
-	tenAddrOneAddr = append(tenAddrOneAddr, s.TestAccs[1])
-	tests := map[string]positionOwnershipTest{
-		"prefix malleability (prior bug)": {
-			queryPositionOwner: s.TestAccs[1],
-			queryPositionId:    1, expPass: false,
-			setupPositions: tenAddrOneAddr,
-		},
-		"things work": {
-			queryPositionOwner: s.TestAccs[1],
-			queryPositionId:    10, expPass: true,
-			setupPositions: tenAddrOneAddr,
-		},
-	}
-	for name, test := range tests {
-		s.Run(name, func() {
-			test.poolId = 1
-			s.runIsPositionOwnerTest(test)
-		})
-	}
-}
-
-func (s *KeeperTestSuite) TestIsPositionOwner() {
-	actualOwner := s.TestAccs[0]
-	nonOwner := s.TestAccs[1]
-
-	tests := []struct {
-		name         string
-		ownerToQuery sdk.AccAddress
-		poolId       uint64
-		positionId   uint64
-		isOwner      bool
-	}{
-		{
-			name:         "Happy path",
-			ownerToQuery: actualOwner,
-			poolId:       1,
-			positionId:   DefaultPositionId,
-			isOwner:      true,
-		},
-		{
-			name:         "query non owner",
-			ownerToQuery: nonOwner,
-			poolId:       1,
-			positionId:   DefaultPositionId,
-			isOwner:      false,
-		},
-		{
-			name:         "different pool ID, not the owner",
-			ownerToQuery: actualOwner,
-			poolId:       2,
-			positionId:   DefaultPositionId,
-			isOwner:      false,
-		},
-		{
-			name:         "different position ID, not the owner",
-			ownerToQuery: actualOwner,
-			poolId:       1,
-			positionId:   DefaultPositionId + 1,
-			isOwner:      false,
-		},
-	}
-
-	for _, test := range tests {
-		s.Run(test.name, func() {
-			s.runIsPositionOwnerTest(positionOwnershipTest{
-				queryPositionOwner: test.ownerToQuery,
-				queryPositionId:    test.positionId,
-				expPass:            test.isOwner,
-				// positions 0 and 1 are owned by actualOwner
-				setupPositions: []sdk.AccAddress{actualOwner, actualOwner},
-				poolId:         test.poolId,
-			})
-		})
-	}
-}
-
 func (s *KeeperTestSuite) TestGetUserPositions() {
 	s.Setup()
 	defaultAddress := s.TestAccs[0]

x/concentrated-liquidity/simulation/sim_msgs.go

@@ -114,7 +114,7 @@ func RandMsgWithdrawPosition(k clkeeper.Keeper, sim *osmosimtypes.SimCtx, ctx sd
 	}
 
 	withdrawAmount := sim.RandomDecAmount(position.Liquidity)
-	if withdrawAmount.TruncateDec().LT(sdk.ZeroDec()) {
+	if withdrawAmount.TruncateDec().IsNegative() {
 		return nil, fmt.Errorf("Invalid withdraw amount")
 	}