Skip to content

Commit

Permalink
fix: Implement retaining rewards for transfers (#7785) (#7802)
Browse files Browse the repository at this point in the history 
* 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

(cherry picked from commit d28ed22)

# Conflicts:
#	CHANGELOG.md
#	x/concentrated-liquidity/msg_server_test.go
#	x/concentrated-liquidity/position.go

Co-authored-by: Alpo <[email protected]>
Co-authored-by: PaddyMc <[email protected]>
  • Loading branch information
@mergify @AlpinYukseloglu @PaddyMc
3 people authored Mar 22, 2024
1 parent db38847 commit 887b313
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 29 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ 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
* [#7746](https://github.com/osmosis-labs/osmosis/pull/7746) Make forfeited incentives redeposit into the pool instead of sending to community pool

## v23.0.8-iavl-v1 & v23.0.8

Expand Down
10 changes: 5 additions & 5 deletions x/concentrated-liquidity/msg_server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}{
Expand All @@ -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},
Expand All @@ -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},
Expand Down
12 changes: 1 addition & 11 deletions x/concentrated-liquidity/position.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
29 changes: 16 additions & 13 deletions x/concentrated-liquidity/position_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 887b313

Please sign in to comment.