diff --git a/CHANGELOG.md b/CHANGELOG.md index 8542698c0f2..2b3c94f1fe3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/x/concentrated-liquidity/msg_server_test.go b/x/concentrated-liquidity/msg_server_test.go index f1df042da0d..c073669aa8c 100644 --- a/x/concentrated-liquidity/msg_server_test.go +++ b/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,14 +572,14 @@ 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}, @@ -587,7 +587,7 @@ func (s *KeeperTestSuite) TestTransferPositions_Events() { 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}, diff --git a/x/concentrated-liquidity/position.go b/x/concentrated-liquidity/position.go index 468340ade24..39a28529721 100644 --- a/x/concentrated-liquidity/position.go +++ b/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 { diff --git a/x/concentrated-liquidity/position_test.go b/x/concentrated-liquidity/position_test.go index d08374598ff..450babdf06f 100644 --- a/x/concentrated-liquidity/position_test.go +++ b/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 {