From 8bb2ab19e7a53eb4c2d940e99ae3ba55bacb2d6a Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Fri, 28 Jul 2023 20:07:59 +0200 Subject: [PATCH] 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 | 1 + go.mod | 2 +- go.sum | 4 +++ osmoutils/accum/accum.go | 13 ++------- osmoutils/accum/accum_test.go | 31 +++++++++------------ tests/e2e/new_test.go | 12 ++++++++ x/concentrated-liquidity/genesis.go | 10 ++----- x/concentrated-liquidity/genesis_test.go | 6 ++-- x/concentrated-liquidity/incentives_test.go | 16 ++++------- x/concentrated-liquidity/spread_rewards.go | 5 +--- 10 files changed, 45 insertions(+), 55 deletions(-) create mode 100644 tests/e2e/new_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 8daf6f260b5..0e15e14e8a7 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/go.mod b/go.mod index bf767f08bb8..9a593c13e35 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 9916fbf6bdd..a9808298f6a 100644 --- a/go.sum +++ b/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= diff --git a/osmoutils/accum/accum.go b/osmoutils/accum/accum.go index 0b58daa6ba7..58ac9875ea2 100644 --- a/osmoutils/accum/accum.go +++ b/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 diff --git a/osmoutils/accum/accum_test.go b/osmoutils/accum/accum_test.go index ecee59f3815..febae3fe2c5 100644 --- a/osmoutils/accum/accum_test.go +++ b/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) diff --git a/tests/e2e/new_test.go b/tests/e2e/new_test.go new file mode 100644 index 00000000000..a48c35e7d7e --- /dev/null +++ b/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 +} diff --git a/x/concentrated-liquidity/genesis.go b/x/concentrated-liquidity/genesis.go index fa985f27756..f88b0d05ef5 100644 --- a/x/concentrated-liquidity/genesis.go +++ b/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{ diff --git a/x/concentrated-liquidity/genesis_test.go b/x/concentrated-liquidity/genesis_test.go index fe1127e9a26..2ead17b34f6 100644 --- a/x/concentrated-liquidity/genesis_test.go +++ b/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) } diff --git a/x/concentrated-liquidity/incentives_test.go b/x/concentrated-liquidity/incentives_test.go index 6d74a2d3596..d7134260dd8 100644 --- a/x/concentrated-liquidity/incentives_test.go +++ b/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))) diff --git a/x/concentrated-liquidity/spread_rewards.go b/x/concentrated-liquidity/spread_rewards.go index 539b82bc3ba..bc002437282 100644 --- a/x/concentrated-liquidity/spread_rewards.go +++ b/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.