From 46421f1d2f1ca8a33c4b5688d312aa013d3a92e2 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Mon, 18 Mar 2024 13:16:13 -0600 Subject: [PATCH 1/4] allow gov module account to transfer any position --- x/concentrated-liquidity/position.go | 21 ++++++++++++--------- x/concentrated-liquidity/position_test.go | 16 +++++++++++++++- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/x/concentrated-liquidity/position.go b/x/concentrated-liquidity/position.go index efc7abcf467..aaa786dc60e 100644 --- a/x/concentrated-liquidity/position.go +++ b/x/concentrated-liquidity/position.go @@ -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" @@ -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. @@ -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()} } @@ -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 diff --git a/x/concentrated-liquidity/position_test.go b/x/concentrated-liquidity/position_test.go index 0a8d52eb1bc..320f17e508c 100644 --- a/x/concentrated-liquidity/position_test.go +++ b/x/concentrated-liquidity/position_test.go @@ -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" @@ -2279,6 +2280,7 @@ func (s *KeeperTestSuite) TestTransferPositions() { positionsToTransfer []uint64 setupUnownedPosition bool isLastPositionInPool bool + isGovAddress bool expectedError error }{ @@ -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": { + 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}, @@ -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) From e3c456a1e3a08aef2f16abc93c7ad6f89df87d2e Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Mon, 18 Mar 2024 13:18:21 -0600 Subject: [PATCH 2/4] add changlelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0804b3c5d5e..cc823e3a373 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 * [#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 From 015a2c4e56b78ed81ebf1e7ccfac139edf1db2e0 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Tue, 19 Mar 2024 11:26:16 -0500 Subject: [PATCH 3/4] Update position_test.go --- x/concentrated-liquidity/position_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/concentrated-liquidity/position_test.go b/x/concentrated-liquidity/position_test.go index 320f17e508c..2e3180b1ae8 100644 --- a/x/concentrated-liquidity/position_test.go +++ b/x/concentrated-liquidity/position_test.go @@ -2316,7 +2316,7 @@ 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": { + "three position IDs, not an owner of any of them but caller is gov address": { inRangePositions: []uint64{DefaultPositionId, DefaultPositionId + 1}, outOfRangePositions: []uint64{DefaultPositionId + 2}, positionsToTransfer: []uint64{DefaultPositionId, DefaultPositionId + 1, DefaultPositionId + 2}, From 2f92e3231257757ba3504662d055e32689af376c Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Thu, 21 Mar 2024 21:30:39 -0600 Subject: [PATCH 4/4] fix test --- x/concentrated-liquidity/position_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/x/concentrated-liquidity/position_test.go b/x/concentrated-liquidity/position_test.go index 08d863eca28..df8af801bd7 100644 --- a/x/concentrated-liquidity/position_test.go +++ b/x/concentrated-liquidity/position_test.go @@ -2318,11 +2318,10 @@ func (s *KeeperTestSuite) TestTransferPositions() { positionsToTransfer: []uint64{DefaultPositionId, DefaultPositionId + 2}, }, "three position IDs, not an owner of any of them but caller is gov address": { - inRangePositions: []uint64{DefaultPositionId, DefaultPositionId + 1}, - outOfRangePositions: []uint64{DefaultPositionId + 2}, - positionsToTransfer: []uint64{DefaultPositionId, DefaultPositionId + 1, DefaultPositionId + 2}, - setupUnownedPosition: true, - isGovAddress: true, + inRangePositions: []uint64{DefaultPositionId, DefaultPositionId + 1}, + outOfRangePositions: []uint64{DefaultPositionId + 2}, + positionsToTransfer: []uint64{DefaultPositionId, DefaultPositionId + 1, DefaultPositionId + 2}, + isGovAddress: true, }, "error: two position IDs, second ID does not exist": { inRangePositions: []uint64{DefaultPositionId, DefaultPositionId + 1},