Remove repeated get in GetTotalShares (#5849)

* Make accumulator objects more consistently pointers throughout

* Update go mod

* Remove repeated get in GetTotalShares

* fix osmoutils version

* changelog update

CHANGELOG.md

@@ -57,6 +57,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * [#5534](https://github.com/osmosis-labs/osmosis/pull/5534) fix: fix the account number of x/tokenfactory module account
 * [#5750](https://github.com/osmosis-labs/osmosis/pull/5750) feat: add cli commmand for converting proto structs to proto marshalled bytes
+* [#5849] (https://github.com/osmosis-labs/osmosis/pull/5849) CL: Lower gas for leaving a position and withdrawing rewards
 * [#5893] (https://github.com/osmosis-labs/osmosis/pull/5893) Export createPosition method in CL so other modules can use it in testing
 
 ### Minor improvements & Bug Fixes

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-20230715164027-b45d8bd42434
+	github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230728163612-426afac90c44
 	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

@@ -950,8 +950,12 @@ github.com/osmosis-labs/go-mutesting v0.0.0-20221208041716-b43bcd97b3b3 h1:Ylmch
 github.com/osmosis-labs/go-mutesting v0.0.0-20221208041716-b43bcd97b3b3/go.mod h1:lV6KnqXYD/ayTe7310MHtM3I2q8Z6bBfMAi+bhwPYtI=
 github.com/osmosis-labs/osmosis/osmomath v0.0.3-dev.0.20230629191111-f375469de8b6 h1:Kmkx5Rh72+LB8AL6dc6fZA+IVR0INu0YIiMF2ScDhaQ=
 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-20230709024311-81c831b050de h1:W2lMduMgpNA5zheEIIialw08n1pWJ44Y4t2F924tpDU=
+github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230709024311-81c831b050de/go.mod h1:Pl8Nzx6O6ow/+aqfMoMSz4hX+zz6RrnDYsooptECGxM=
 github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230715164027-b45d8bd42434 h1:MXPrA3sDtqOHYUa9zl4HMGMW+IJwGMqUf6+Hl9nhrCA=
 github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230715164027-b45d8bd42434/go.mod h1:Pl8Nzx6O6ow/+aqfMoMSz4hX+zz6RrnDYsooptECGxM=
+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/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/accum/accum.go

@@ -344,12 +344,7 @@ func (accum *AccumulatorObject) DeletePosition(positionName string) (sdk.DecCoin
 	}
 
 	accum.store.Delete(FormatPositionPrefixKey(accum.name, positionName))
-
-	totalShares, err := accum.GetTotalShares()
-	if err != nil {
-		return sdk.DecCoins{}, err
-	}
-	accum.totalShares = totalShares.Sub(position.NumShares)
+	accum.totalShares.SubMut(position.NumShares)
 	err = setAccumulator(accum, accum.valuePerShare, accum.totalShares)
 	if err != nil {
 		return sdk.DecCoins{}, err
@@ -425,10 +420,8 @@ func (accum *AccumulatorObject) ClaimRewards(positionName string) (sdk.Coins, sd
 }
 
 // GetTotalShares returns the total number of shares in the accumulator
-func (accum AccumulatorObject) GetTotalShares() (sdk.Dec, error) {
-	// TODO: Make this not do an extra get.
-	accumPtr, err := GetAccumulator(accum.store, accum.name)
-	return accumPtr.totalShares, err
+func (accum AccumulatorObject) GetTotalShares() sdk.Dec {
+	return accum.totalShares
 }
 
 // AddToUnclaimedRewards adds the given amount of rewards to the unclaimed rewards

osmoutils/accum/accum_test.go

@@ -38,8 +38,7 @@ func (suite *AccumTestSuite) MakeAndGetAccumulator(name string) *accumPackage.Ac
 }
 
 func (suite *AccumTestSuite) TotalSharesCheck(accum *accumPackage.AccumulatorObject, expected sdk.Dec) {
-	shareCount, err := accum.GetTotalShares()
-	suite.Require().NoError(err)
+	shareCount := accum.GetTotalShares()
 	suite.Require().Equal(expected.String(), shareCount.String())
 }
 
@@ -232,45 +231,40 @@ func (suite *AccumTestSuite) TestNewPosition() {
 	// once at beginning so we can test duplicate positions
 	suite.SetupTest()
 
-	// Setup.
-	defaultAccObject := accumPackage.MakeTestAccumulator(suite.store, testNameOne, emptyCoins, emptyDec)
-
-	nonEmptyAccObject := accumPackage.MakeTestAccumulator(suite.store, testNameTwo, initialCoinsDenomOne, emptyDec)
-
 	tests := map[string]struct {
-		accObject        *accumPackage.AccumulatorObject
+		initialCoins     sdk.DecCoins
 		name             string
 		numShareUnits    sdk.Dec
 		options          *accumPackage.Options
 		expectedPosition accumPackage.Record
 	}{
 		"test address one - position created": {
-			accObject:        defaultAccObject,
+			initialCoins:     emptyCoins,
 			name:             testAddressOne,
 			numShareUnits:    positionOne.NumShares,
 			expectedPosition: positionOne,
 		},
 		"test address two (non-nil options) - position created": {
-			accObject:        defaultAccObject,
+			initialCoins:     emptyCoins,
 			name:             testAddressTwo,
 			numShareUnits:    positionTwo.NumShares,
 			expectedPosition: positionTwo,
 			options:          &emptyPositionOptions,
 		},
 		"test address one - position overwritten": {
-			accObject:        defaultAccObject,
+			initialCoins:     emptyCoins,
 			name:             testAddressOne,
 			numShareUnits:    positionOneV2.NumShares,
 			expectedPosition: positionOneV2,
 		},
 		"test address three - added": {
-			accObject:        defaultAccObject,
+			initialCoins:     emptyCoins,
 			name:             testAddressThree,
 			numShareUnits:    positionThree.NumShares,
 			expectedPosition: positionThree,
 		},
 		"test address one with non-empty accumulator - position created": {
-			accObject:        nonEmptyAccObject,
+			initialCoins:     initialCoinsDenomOne,
 			name:             testAddressOne,
 			numShareUnits:    positionOne.NumShares,
 			expectedPosition: withInitialAccumValue(positionOne, initialCoinsDenomOne),
@@ -280,24 +274,25 @@ func (suite *AccumTestSuite) TestNewPosition() {
 	for name, tc := range tests {
 		tc := tc
 		suite.Run(name, func() {
-			originalAccValue := tc.accObject.GetTotalShareField()
+			accObject := accumPackage.MakeTestAccumulator(suite.store, name, tc.initialCoins, emptyDec)
+			originalAccValue := accObject.GetTotalShareField()
 			expectedAccValue := originalAccValue.Add(tc.numShareUnits)
 
 			// System under test.
-			err := tc.accObject.NewPosition(tc.name, tc.numShareUnits, tc.options)
+			err := accObject.NewPosition(tc.name, tc.numShareUnits, tc.options)
 			suite.Require().NoError(err)
 
 			// Assertions.
-			position := tc.accObject.MustGetPosition(tc.name)
+			position := accObject.MustGetPosition(tc.name)
 
 			suite.Require().Equal(tc.expectedPosition.NumShares, position.NumShares)
 			suite.Require().Equal(tc.expectedPosition.AccumValuePerShare, position.AccumValuePerShare)
 			suite.Require().Equal(tc.expectedPosition.UnclaimedRewardsTotal, position.UnclaimedRewardsTotal)
 
 			// ensure receiver was mutated
-			suite.Require().Equal(expectedAccValue, tc.accObject.GetTotalShareField())
+			suite.Require().Equal(expectedAccValue, accObject.GetTotalShareField())
 			// ensure state was mutated
-			suite.TotalSharesCheck(tc.accObject, expectedAccValue)
+			suite.TotalSharesCheck(accObject, expectedAccValue)
 
 			if tc.options == nil {
 				suite.Require().Nil(position.Options)

tests/e2e/new_test.go

@@ -0,0 +1,12 @@
+package e2e
+
+import (
+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+type E2ETest struct {
+	name       string
+	fundCoins  sdk.Coins
+	logic      func(s *IntegrationTestSuite, t *E2ETest)
+	walletAddr sdk.AccAddress
+}

x/concentrated-liquidity/genesis.go

@@ -130,10 +130,7 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *genesis.GenesisState {
 			panic(err)
 		}
 
-		totalShares, err := accumObject.GetTotalShares()
-		if err != nil {
-			panic(err)
-		}
+		totalShares := accumObject.GetTotalShares()
 
 		spreadRewardAccumObject := genesis.AccumObject{
 			Name: types.KeySpreadRewardPoolAccumulator(poolI.GetId()),
@@ -156,10 +153,7 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *genesis.GenesisState {
 
 		incentivesAccumObject := make([]genesis.AccumObject, len(incentivesAccum))
 		for i, incentiveAccum := range incentivesAccum {
-			incentiveAccumTotalShares, err := incentiveAccum.GetTotalShares()
-			if err != nil {
-				panic(err)
-			}
+			incentiveAccumTotalShares := incentiveAccum.GetTotalShares()
 			genesisAccum := genesis.AccumObject{
 				Name: incentiveAccum.GetName(),
 				AccumContent: &accum.AccumulatorContent{

x/concentrated-liquidity/genesis_test.go

@@ -533,8 +533,7 @@ func (s *KeeperTestSuite) TestInitGenesis() {
 				s.Require().NoError(err)
 				for j, actualIncentiveAccum := range acutalIncentiveAccums {
 					expectedAccum := tc.genesis.PoolData[i].IncentivesAccumulators
-					actualTotalShares, err := actualIncentiveAccum.GetTotalShares()
-					s.Require().NoError(err)
+					actualTotalShares := actualIncentiveAccum.GetTotalShares()
 
 					s.Require().Equal(expectedAccum[j].GetName(), actualIncentiveAccum.GetName())
 					s.Require().Equal(expectedAccum[j].AccumContent.AccumValue, actualIncentiveAccum.GetValue())
@@ -580,8 +579,7 @@ func (s *KeeperTestSuite) TestInitGenesis() {
 			for i, accumObject := range spreadFactorAccums {
 				s.Require().Equal(spreadFactorAccums[i].GetValue(), tc.expectedspreadFactorAccumValues[i].AccumContent.AccumValue)
 
-				totalShares, err := accumObject.GetTotalShares()
-				s.Require().NoError(err)
+				totalShares := accumObject.GetTotalShares()
 				s.Require().Equal(totalShares, tc.expectedspreadFactorAccumValues[i].AccumContent.TotalShares)
 			}
 

x/concentrated-liquidity/incentives_test.go

@@ -3194,8 +3194,7 @@ func (s *KeeperTestSuite) TestPrepareClaimAllIncentivesForPosition() {
 				uptimeAccumulatorsPostClaim, err := s.clk.GetUptimeAccumulators(s.Ctx, pool.GetId())
 				s.Require().NoError(err)
 				for i, acc := range uptimeAccumulatorsPostClaim {
-					totalSharesAccum, err := acc.GetTotalShares()
-					s.Require().NoError(err)
+					totalSharesAccum := acc.GetTotalShares()
 
 					uptimeAccumsDiffPostClaim = append(uptimeAccumsDiffPostClaim, acc.GetValue().MulDec(totalSharesAccum).Sub(uptimeAccumulatorsPreClaim[i].GetValue().MulDec(totalSharesAccum))...)
 				}
@@ -3629,14 +3628,13 @@ func (s *KeeperTestSuite) TestPrepareBalancerPoolAsFullRange() {
 				s.Require().True(len(clPoolUptimeAccumulatorsFromState) > 0)
 				expectedShares := qualifyingShares.Add(initialLiquidity)
 				for uptimeIdx, uptimeAccum := range clPoolUptimeAccumulatorsFromState {
-					currAccumShares, err := uptimeAccum.GetTotalShares()
-					s.Require().NoError(err)
+					currAccumShares := uptimeAccum.GetTotalShares()
 
 					// Ensure each accum has the correct number of final shares
 					s.Require().Equal(expectedShares, currAccumShares)
 
 					// Also validate uptime accumulators passed in as parameter.
-					currAccumShares, err = uptimeAccums[uptimeIdx].GetTotalShares()
+					currAccumShares = uptimeAccums[uptimeIdx].GetTotalShares()
 					s.Require().Equal(expectedShares, currAccumShares)
 				}
 
@@ -3939,8 +3937,7 @@ func (s *KeeperTestSuite) TestClaimAndResetFullRangeBalancerPool() {
 
 					s.Require().True(len(clPoolUptimeAccumulatorsFromState) > 0)
 					for uptimeIdx, uptimeAccum := range clPoolUptimeAccumulatorsFromState {
-						currAccumShares, err := uptimeAccum.GetTotalShares()
-						s.Require().NoError(err)
+						currAccumShares := uptimeAccum.GetTotalShares()
 
 						// Since reversions for errors are done at a higher level of abstraction,
 						// we have to assume that any state updates that happened prior to the error
@@ -3957,7 +3954,7 @@ func (s *KeeperTestSuite) TestClaimAndResetFullRangeBalancerPool() {
 						s.Require().Equal(expectedLiquidity, currAccumShares)
 
 						// Also validate uptime accumulators passed in as parameter.
-						currAccumShares, err = uptimeAccums[uptimeIdx].GetTotalShares()
+						currAccumShares = uptimeAccums[uptimeIdx].GetTotalShares()
 						s.Require().Equal(expectedLiquidity, currAccumShares)
 					}
 
@@ -3990,8 +3987,7 @@ func (s *KeeperTestSuite) TestClaimAndResetFullRangeBalancerPool() {
 
 				s.Require().True(len(clPoolUptimeAccumulators) > 0)
 				for uptimeIndex, uptimeAccum := range clPoolUptimeAccumulators {
-					currAccumShares, err := uptimeAccum.GetTotalShares()
-					s.Require().NoError(err)
+					currAccumShares := uptimeAccum.GetTotalShares()
 
 					// Ensure each accum has been cleared of the balancer full range shares
 					balancerPositionName := string(types.KeyBalancerFullRange(clPoolId, balancerPoolId, uint64(uptimeIndex)))

x/concentrated-liquidity/spread_rewards.go

@@ -264,10 +264,7 @@ func (k Keeper) prepareClaimableSpreadRewards(ctx sdk.Context, positionId uint64
 			return nil, err
 		}
 
-		totalSharesRemaining, err := spreadRewardAccumulator.GetTotalShares()
-		if err != nil {
-			return nil, err
-		}
+		totalSharesRemaining := spreadRewardAccumulator.GetTotalShares()
 
 		// if there are no shares remaining, the dust is ignored. Otherwise, it is added back to the global accumulator.
 		// Total shares remaining can be zero if we claim in withdrawPosition for the last position in the pool.