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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
46421f1
allow gov module account to transfer any position
czarcas7ic Mar 18, 2024
e3c456a
add changlelog
czarcas7ic Mar 18, 2024
015a2c4
Update position_test.go
czarcas7ic Mar 19, 2024
c697ff5
Merge branch 'main' into adam/allow-transfer-of-positions-by-gov-modu…
czarcas7ic Mar 22, 2024
2f92e32
fix test
czarcas7ic Mar 22, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#7689](https://github.com/osmosis-labs/osmosis/pull/7689) Make CL price estimations not cause state writes (speed and gas improvements)
* [#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
* [#7768](https://github.com/osmosis-labs/osmosis/pull/7768) Allow governance module account to transfer any CL position

## v23.0.7-iavl-v1

Expand Down
21 changes: 12 additions & 9 deletions x/concentrated-liquidity/position.go
View file
Open in desktop
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
View file
Open in desktop
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