From 8c0910c5ede7b69477ed1deb08f22572b51183a8 Mon Sep 17 00:00:00 2001 From: "Matt, Park" <45252226+mattverse@users.noreply.github.com> Date: Wed, 11 Jan 2023 15:49:18 +0900 Subject: [PATCH] feat(osmoutil/accum): Remove position Accumulator from state (#3961) * Delete position accum when 0 * Move position deletion logic to claimrewards * Update test --- osmoutils/accum/accum.go | 15 ++++++--- osmoutils/accum/accum_test.go | 62 ++++++++++++++++++++++++++++------- 2 files changed, 61 insertions(+), 16 deletions(-) diff --git a/osmoutils/accum/accum.go b/osmoutils/accum/accum.go index 2d162549885..5abcba64902 100644 --- a/osmoutils/accum/accum.go +++ b/osmoutils/accum/accum.go @@ -196,7 +196,7 @@ func (accum AccumulatorObject) RemoveFromPositionCustomAcc(name string, numShare // Ensure not removing more shares than exist if numSharesToRemove.GT(position.NumShares) { - return fmt.Errorf("Attempted to remove more shares (%s) than exist in the position (%s)", numSharesToRemove, position.NumShares) + return fmt.Errorf("Attempted to remove more shares (%s) than exist in the position (%s)", numSharesToRemove, position.NumShares) } // Save current number of shares and unclaimed rewards @@ -239,6 +239,10 @@ func (accum AccumulatorObject) UpdatePositionCustomAcc(name string, numShares sd return accum.AddToPositionCustomAcc(name, numShares, customAccumulatorValue) } +func (accum AccumulatorObject) deletePosition(name string) { + accum.store.Delete(formatPositionPrefixKey(accum.name, name)) +} + // GetPositionSize returns the number of shares the position corresponding to `addr` // in accumulator `accum` has, or an error if no position exists. func (accum AccumulatorObject) GetPositionSize(name string) (sdk.Dec, error) { @@ -273,9 +277,12 @@ func (accum AccumulatorObject) ClaimRewards(positionName string) (sdk.Coins, err // This is acceptable because we round in favour of the protocol. truncatedRewards, _ := totalRewards.TruncateDecimal() - // Create a completely new position, with no rewards - // TODO: remove the position from state entirely if numShares = zero - createNewPosition(accum, accum.value, positionName, position.NumShares, sdk.NewDecCoins(), position.Options) + // remove the position from state entirely if numShares = zero + if position.NumShares.Equal(sdk.ZeroDec()) { + accum.deletePosition(positionName) + } else { // else, create a completely new position, with no rewards + createNewPosition(accum, accum.value, positionName, position.NumShares, sdk.NewDecCoins(), position.Options) + } return truncatedRewards, nil } diff --git a/osmoutils/accum/accum_test.go b/osmoutils/accum/accum_test.go index 6c611e2562e..5fed39eb1b4 100644 --- a/osmoutils/accum/accum_test.go +++ b/osmoutils/accum/accum_test.go @@ -13,6 +13,7 @@ import ( "github.com/osmosis-labs/osmosis/osmoutils" accumPackage "github.com/osmosis-labs/osmosis/osmoutils/accum" + "github.com/osmosis-labs/osmosis/osmoutils/osmoassert" ) type AccumTestSuite struct { @@ -342,16 +343,23 @@ func (suite *AccumTestSuite) TestClaimRewards() { accumThreeRewards.SetValue(tripleDenomOneAndTwo) tests := map[string]struct { - accObject accumPackage.AccumulatorObject - name string - expectedResult sdk.Coins - expectError error + accObject accumPackage.AccumulatorObject + name string + expectedResult sdk.Coins + updateNumSharesToZero bool + expectError error }{ "claim at testAddressOne with no rewards - success": { accObject: accumNoRewards, name: testAddressOne, expectedResult: toCoins(emptyCoins), }, + "delete accum - claim at testAddressOne with no rewards - success": { + accObject: accumNoRewards, + name: testAddressOne, + updateNumSharesToZero: true, + expectedResult: toCoins(emptyCoins), + }, "claim at testAddressTwo with no rewards - success": { accObject: accumNoRewards, name: testAddressTwo, @@ -375,6 +383,14 @@ func (suite *AccumTestSuite) TestClaimRewards() { // denomTwo: (3 - 0) * 100 (accum diff * share count) = 300 expectedResult: toCoins(tripleDenomOneAndTwo.MulDec(positionOne.NumShares).Add(initialCoinDenomOne)), }, + "delete accum - claim at testAddressOne with multiple reward tokens and unclaimed rewards - success": { + accObject: accumThreeRewards, + name: testAddressOne, + updateNumSharesToZero: true, + // denomOne: (300.3 - 0) * 100 (accum diff * share count) + 100.1 (unclaimed rewards) = 30130.1 + // denomTwo: (3 - 0) * 100 (accum diff * share count) = 300 + expectedResult: toCoins(tripleDenomOneAndTwo.MulDec(positionOne.NumShares).Add(initialCoinDenomOne)), + }, "claim at testAddressTwo with multiple reward tokens and no unclaimed rewards - success": { accObject: accumThreeRewards, name: testAddressTwo, @@ -387,6 +403,12 @@ func (suite *AccumTestSuite) TestClaimRewards() { for name, tc := range tests { tc := tc suite.Run(name, func() { + if tc.updateNumSharesToZero { + positionSize, err := tc.accObject.GetPositionSize(tc.name) + suite.Require().NoError(err) + err = tc.accObject.UpdatePosition(tc.name, positionSize.Neg()) + suite.Require().NoError(err) + } // System under test. actualResult, err := tc.accObject.ClaimRewards(tc.name) @@ -399,14 +421,15 @@ func (suite *AccumTestSuite) TestClaimRewards() { } suite.Require().NoError(err) - suite.Require().Equal(tc.expectedResult, actualResult) - finalPosition := tc.accObject.GetPosition(tc.name) - suite.Require().NoError(err) + osmoassert.ConditionalPanic(suite.T(), tc.updateNumSharesToZero, func() { + finalPosition := tc.accObject.GetPosition(tc.name) + suite.Require().NoError(err) - // Unclaimed rewards are reset. - suite.Require().Equal(emptyCoins, finalPosition.UnclaimedRewards) + // Unclaimed rewards are reset. + suite.Require().Equal(emptyCoins, finalPosition.UnclaimedRewards) + }) }) } } @@ -1231,14 +1254,21 @@ func (suite *AccumTestSuite) TestUpdatePositionCustomAcc() { UnclaimedRewards: emptyCoins, }, }, - "custom acc value does not equal to acc; negative shares -> acts as RemoveFromPosition": { + "custom acc value does not equal to acc; remove same amount -> acts as RemoveFromPosition": { accObject: accObject, initialShares: positionTwo.NumShares, name: testAddressTwo, numShareUnits: positionTwo.NumShares.Neg(), // note: negative shares customAcc: accObject.GetValue().MulDec(sdk.NewDec(2)), + }, + "custom acc value does not equal to acc; remove diff amount -> acts as RemoveFromPosition": { + accObject: accObject, + initialShares: positionTwo.NumShares, + name: testAddressTwo, + numShareUnits: positionOne.NumShares.Neg(), // note: negative shares + customAcc: accObject.GetValue().MulDec(sdk.NewDec(2)), expectedPosition: accumPackage.Record{ - NumShares: sdk.ZeroDec(), // results in zero shares + NumShares: positionOne.NumShares, // results in 100 shares (200 - 100) InitAccumValue: accObject.GetValue().MulDec(sdk.NewDec(2)), UnclaimedRewards: emptyCoins, }, @@ -1264,6 +1294,11 @@ func (suite *AccumTestSuite) TestUpdatePositionCustomAcc() { for name, tc := range tests { tc := tc suite.Run(name, func() { + // make accumualtor based off of tc.accObject + accumPackage.MakeAccumulator(suite.store, testNameOne) + // manually update accumulator value + tc.accObject.UpdateAccumulator(initialCoinsDenomOne) + // Setup err := tc.accObject.NewPositionCustomAcc(tc.name, tc.initialShares, tc.accObject.GetValue(), nil) suite.Require().NoError(err) @@ -1278,8 +1313,11 @@ func (suite *AccumTestSuite) TestUpdatePositionCustomAcc() { } suite.Require().NoError(err) - // Assertions. + tc.accObject, err = accumPackage.GetAccumulator(suite.store, testNameOne) + suite.Require().NoError(err) + position := tc.accObject.GetPosition(tc.name) + // Assertions. suite.Require().Equal(tc.expectedPosition.NumShares, position.NumShares) suite.Require().Equal(tc.expectedPosition.InitAccumValue, position.InitAccumValue)