fix: Implement retaining rewards for transfers (#7785)

* implement retaining rewards for transfers

* add changelog

* clean up tests

* driveby test cleanup

* Revert "driveby test cleanup"

This reverts commit b1952b9.

* update transfer method spec

* fix conflicts

* lint

CHANGELOG.md

@@ -67,6 +67,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * [#7745](https://github.com/osmosis-labs/osmosis/pull/7745) Add gauge id query to stargate whitelist
 * [#7747](https://github.com/osmosis-labs/osmosis/pull/7747) Remove redundant call to incentive collection in CL position withdrawal logic
 * [#7746](https://github.com/osmosis-labs/osmosis/pull/7746) Make forfeited incentives redeposit into the pool instead of sending to community pool
+* [#7785](https://github.com/osmosis-labs/osmosis/pull/7785) Remove reward claiming during position transfers
 
 ## v23.0.8-iavl-v1 & v23.0.8
 

x/concentrated-liquidity/msg_server_test.go

@@ -558,7 +558,7 @@ func (s *KeeperTestSuite) TestTransferPositions_Events() {
 		hasIncentivesToClaim           bool
 		hasSpreadRewardsToClaim        bool
 		expectedTransferPositionsEvent int
-		expectedMessageEvents          int
+		expectedMessageEvents          int // We expect these to always be 0 because no additional events are being triggered
 		isLastPositionInPool           bool
 		expectedError                  error
 	}{
@@ -572,22 +572,22 @@ func (s *KeeperTestSuite) TestTransferPositions_Events() {
 			hasIncentivesToClaim:           true,
 			numPositionsToCreate:           1,
 			expectedTransferPositionsEvent: 1,
-			expectedMessageEvents:          1, // 1 for collect incentives claim send
+			expectedMessageEvents:          0,
 		},
 		"single position ID with claimable spread rewards": {
 			positionIds:                    []uint64{DefaultPositionId},
 			hasSpreadRewardsToClaim:        true,
 			numPositionsToCreate:           1,
 			expectedTransferPositionsEvent: 1,
-			expectedMessageEvents:          1, // 1 for collect spread rewards claim send
+			expectedMessageEvents:          0,
 		},
 		"single position ID with claimable incentives and spread rewards": {
 			positionIds:                    []uint64{DefaultPositionId},
 			hasIncentivesToClaim:           true,
 			hasSpreadRewardsToClaim:        true,
 			numPositionsToCreate:           1,
 			expectedTransferPositionsEvent: 1,
-			expectedMessageEvents:          2, // 1 for collect incentives claim send, 1 for collect spread rewards claim send
+			expectedMessageEvents:          0,
 		},
 		"two position IDs": {
 			positionIds:                    []uint64{DefaultPositionId, DefaultPositionId + 1},
@@ -605,7 +605,7 @@ func (s *KeeperTestSuite) TestTransferPositions_Events() {
 			hasSpreadRewardsToClaim:        true,
 			numPositionsToCreate:           3,
 			expectedTransferPositionsEvent: 1,
-			expectedMessageEvents:          6, // 3 for collect incentives claim send, 3 for collect spread rewards claim send
+			expectedMessageEvents:          0,
 		},
 		"two position IDs, second ID does not exist": {
 			positionIds:          []uint64{DefaultPositionId, DefaultPositionId + 1},

x/concentrated-liquidity/position.go

@@ -671,8 +671,7 @@ func (k Keeper) updateFullRangeLiquidityInPool(ctx sdk.Context, poolId uint64, l
 // For each position ID, it retrieves the corresponding position and checks if the sender is the owner of the position.
 // If the sender is not the owner, it returns an error.
 // It then checks if the position has an active underlying lock, and if so, returns an error.
-// It then collects any outstanding incentives and rewards for the position, deletes the KVStore entries for the position,
-// and restores the position under the recipient's account.
+// It then deletes the KVStore entries for the position, and restores the position under the recipient's account.
 // If any of these operations fail, it returns the corresponding error.
 // If all operations succeed, it returns nil.
 func (k Keeper) transferPositions(ctx sdk.Context, positionIds []uint64, sender sdk.AccAddress, recipient sdk.AccAddress) error {
@@ -704,15 +703,6 @@ func (k Keeper) transferPositions(ctx sdk.Context, positionIds []uint64, sender
 			return types.LockNotMatureError{PositionId: position.PositionId, LockId: lockId}
 		}
 
-		// Collect any outstanding incentives and rewards for the position.
-		if _, err := k.collectSpreadRewards(ctx, sender, positionId); err != nil {
-			return err
-		}
-
-		if _, _, _, err := k.collectIncentives(ctx, sender, positionId); err != nil {
-			return err
-		}
-
 		// Delete the KVStore entries for the position.
 		err = k.deletePosition(ctx, positionId, sender, position.PoolId)
 		if err != nil {

x/concentrated-liquidity/position_test.go

@@ -2441,31 +2441,34 @@ func (s *KeeperTestSuite) TestTransferPositions() {
 					s.Require().Equal(oldPosition, newPosition)
 				}
 
-				// Check that the incentives and spread rewards went to the old owner
+				// Check that the old owner's balance did not change due to the transfer
 				postTransferOriginalOwnerFunds := s.App.BankKeeper.GetAllBalances(s.Ctx, oldOwner)
-				expectedTransferOriginalOwnerFunds := preTransferOwnerFunds.Add(totalExpectedRewards...)
-				s.Require().Equal(expectedTransferOriginalOwnerFunds.String(), postTransferOriginalOwnerFunds.String())
+				s.Require().Equal(preTransferOwnerFunds.String(), postTransferOriginalOwnerFunds.String())
 
-				// Check that the new owner does not have any new funds
+				// Check that the new owner's balance did not change due to the transfer
 				postTransferNewOwnerFunds := s.App.BankKeeper.GetAllBalances(s.Ctx, newOwner)
 				s.Require().Equal(preTransferNewOwnerFunds, postTransferNewOwnerFunds)
 
-				// Test that claiming incentives/spread rewards with the new owner returns nothing
-				for _, positionId := range tc.positionsToTransfer {
-					fundsToClaim, fundsToForefeit, err := s.App.ConcentratedLiquidityKeeper.GetClaimableIncentives(s.Ctx, positionId)
+				// Claim rewards and ensure that previously accrued incentives and spread rewards go to the new owner
+				for _, positionID := range tc.positionsToTransfer {
+					_, err = s.App.ConcentratedLiquidityKeeper.CollectSpreadRewards(s.Ctx, newOwner, positionID)
 					s.Require().NoError(err)
-					s.Require().Equal(sdk.Coins{}, fundsToClaim)
-					s.Require().Equal(sdk.Coins{}, fundsToForefeit)
-
-					spreadRewards, err := s.App.ConcentratedLiquidityKeeper.GetClaimableSpreadRewards(s.Ctx, positionId)
+					_, _, _, err := s.App.ConcentratedLiquidityKeeper.CollectIncentives(s.Ctx, newOwner, positionID)
 					s.Require().NoError(err)
-					s.Require().Equal(sdk.Coins(nil), spreadRewards)
 				}
 
+				// Ensure all rewards went to the new owner
+				postClaimRewardsNewOwnerBalance := s.App.BankKeeper.GetAllBalances(s.Ctx, newOwner)
+				s.Require().Equal(totalExpectedRewards, postClaimRewardsNewOwnerBalance)
+
+				// Ensure no rewards went to the old owner
+				postClaimRewardsOldOwnerBalance := s.App.BankKeeper.GetAllBalances(s.Ctx, oldOwner)
+				s.Require().Equal(preTransferOwnerFunds.String(), postClaimRewardsOldOwnerBalance.String())
+
 				// Test that adding incentives/spread rewards and then claiming returns it to the new owner, and the old owner does not get anything
 				totalSpreadRewards := s.fundSpreadRewardsAddr(s.Ctx, pool.GetSpreadRewardsAddress(), tc.inRangePositions)
 				totalIncentives := s.fundIncentiveAddr(pool.GetIncentivesAddress(), tc.inRangePositions)
-				totalExpectedRewards := totalSpreadRewards.Add(totalIncentives...)
+				totalExpectedRewards := totalExpectedRewards.Add(totalSpreadRewards...).Add(totalIncentives...)
 				s.addUptimeGrowthInsideRange(s.Ctx, pool.GetId(), apptesting.DefaultLowerTick+1, DefaultLowerTick, DefaultUpperTick, expectedUptimes.hundredTokensMultiDenom)
 				s.AddToSpreadRewardAccumulator(pool.GetId(), sdk.NewDecCoin(ETH, osmomath.NewInt(10)))
 				for _, positionId := range tc.positionsToTransfer {