Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow gov module account to transfer any CL position #7768

Merged
merged 5 commits into from
Mar 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions x/concentrated-liquidity/position.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
sdkprefix "github.com/cosmos/cosmos-sdk/store/prefix"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/query"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"

"github.com/osmosis-labs/osmosis/osmomath"
"github.com/osmosis-labs/osmosis/osmoutils"
Expand Down Expand Up @@ -669,7 +670,7 @@ func (k Keeper) updateFullRangeLiquidityInPool(ctx sdk.Context, poolId uint64, l
// transferPositions transfers ownership of a set of positions from a sender to a recipient.
// It first checks if the provided position IDs are unique. If not, it returns a DuplicatePositionIdsError.
// 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.
// If the sender is not the owner (or the governance module account), 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.
Expand All @@ -684,14 +685,17 @@ func (k Keeper) transferPositions(ctx sdk.Context, positionIds []uint64, sender
return types.DuplicatePositionIdsError{PositionIds: positionIds}
}

// Check if the sender is the governance module account.
isGovModuleSender := sender.Equals(k.accountKeeper.GetModuleAccount(ctx, govtypes.ModuleName).GetAddress())

for _, positionId := range positionIds {
position, err := k.GetPosition(ctx, positionId)
if err != nil {
return err
}

// Check that the sender is the owner of the position.
if position.Address != sender.String() {
// If the sender is not the governance module, verify that the sender is the owner of the position.
if !isGovModuleSender && position.Address != sender.String() {
return types.PositionOwnerMismatchError{PositionOwner: position.Address, Sender: sender.String()}
}

Expand All @@ -705,22 +709,21 @@ func (k Keeper) transferPositions(ctx sdk.Context, positionIds []uint64, sender
}

// Collect any outstanding incentives and rewards for the position.
if _, err := k.collectSpreadRewards(ctx, sender, positionId); err != nil {
positionOwnerAddr := sdk.MustAccAddressFromBech32(position.Address)
if _, err := k.collectSpreadRewards(ctx, positionOwnerAddr, positionId); err != nil {
return err
}
if _, _, err := k.collectIncentives(ctx, sender, positionId); err != nil {
if _, _, err := k.collectIncentives(ctx, positionOwnerAddr, positionId); err != nil {
return err
}

// Delete the KVStore entries for the position.
err = k.deletePosition(ctx, positionId, sender, position.PoolId)
err = k.deletePosition(ctx, positionId, positionOwnerAddr, position.PoolId)
if err != nil {
return err
}

// There exists special logic branches we take if a position is the last position in a pool and it is withdrawn.
// It makes sense to accept the small annoyance of preventing a position from being transferred if it is the last position in a pool
// instead of considering all the edge cases that would arise if we allowed it.
// Check if transferring the last position in a pool.
anyPositionsRemainingInPool, err := k.HasAnyPositionForPool(ctx, position.PoolId)
if err != nil {
return err
Expand Down
16 changes: 15 additions & 1 deletion x/concentrated-liquidity/position_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/query"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"

"github.com/osmosis-labs/osmosis/osmomath"
"github.com/osmosis-labs/osmosis/osmoutils"
Expand Down Expand Up @@ -2279,6 +2280,7 @@ func (s *KeeperTestSuite) TestTransferPositions() {
positionsToTransfer []uint64
setupUnownedPosition bool
isLastPositionInPool bool
isGovAddress bool
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved

expectedError error
}{
Expand Down Expand Up @@ -2314,6 +2316,13 @@ func (s *KeeperTestSuite) TestTransferPositions() {
outOfRangePositions: []uint64{DefaultPositionId + 1, DefaultPositionId + 2},
positionsToTransfer: []uint64{DefaultPositionId, DefaultPositionId + 2},
},
"three position IDs, not an owner of one of them but caller is gov address": {
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved
inRangePositions: []uint64{DefaultPositionId, DefaultPositionId + 1},
outOfRangePositions: []uint64{DefaultPositionId + 2},
positionsToTransfer: []uint64{DefaultPositionId, DefaultPositionId + 1, DefaultPositionId + 2},
setupUnownedPosition: true,
isGovAddress: true,
},
"error: two position IDs, second ID does not exist": {
inRangePositions: []uint64{DefaultPositionId, DefaultPositionId + 1},
positionsToTransfer: []uint64{DefaultPositionId, DefaultPositionId + 3},
Expand Down Expand Up @@ -2387,8 +2396,13 @@ func (s *KeeperTestSuite) TestTransferPositions() {
// Account funds of new owner
preTransferNewOwnerFunds := s.App.BankKeeper.GetAllBalances(s.Ctx, newOwner)

transferCaller := oldOwner
if tc.isGovAddress {
transferCaller = s.App.AccountKeeper.GetModuleAddress(govtypes.ModuleName)
}

// System under test
err = s.App.ConcentratedLiquidityKeeper.TransferPositions(s.Ctx, tc.positionsToTransfer, oldOwner, newOwner)
err = s.App.ConcentratedLiquidityKeeper.TransferPositions(s.Ctx, tc.positionsToTransfer, transferCaller, newOwner)

if tc.expectedError != nil {
s.Require().Error(err)
Expand Down