feat(osmoutil/accum): Remove position Accumulator from state (#3961)

* Delete position accum when 0 * Move position deletion logic to claimrewards * Update test
osmosis-labs · Jan 11, 2023 · 8c0910c · 8c0910c
1 parent e58c3c1
commit 8c0910c
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 16 deletions.
diff --git 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
@@ -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)