[InternalReview][MsgFungifyChargedPositions] (#5091)

* [InternalReview][MsgFungifyChargedPositions]

* comment

* remove nit

* comment

* comment

* revert launch json

* lint

* fix ctx

* updates

* CLI fix

* revert me

* Revert "revert me"

This reverts commit 8acf2ce.

* remove debug event

* fix test

* fix e2e

* clean up comments

* updates

* updates

* message server

* fix test

* update README

* README update

* README

* tick spacing 100

* negative rewards

* comments

* comments

osmoutils/accum/accum.go

@@ -347,8 +347,9 @@ func (accum AccumulatorObject) deletePosition(positionName string) {
 	accum.store.Delete(FormatPositionPrefixKey(accum.name, positionName))
 }
 
-// GetPositionSize returns the number of shares the position corresponding to `addr`
-// in accumulator `accum` has, or an error if no position exists.
+// GetPositionSize returns the number of shares the position with the given
+// name has in the accumulator. Returns error if position does not exist
+// or if fails to retrieve position from state.
 func (accum AccumulatorObject) GetPositionSize(name string) (sdk.Dec, error) {
 	position, err := GetPosition(accum, name)
 	if err != nil {
@@ -422,8 +423,8 @@ func (accum AccumulatorObject) GetTotalShares() (sdk.Dec, error) {
 }
 
 // AddToUnclaimedRewards adds the given amount of rewards to the unclaimed rewards
-// for the given position. Returns error if no position exists for the given address.
-// Returns error if any database errors occur.
+// for the given position. Returns error if no position exists for the given position name.
+// Returns error if any database errors occur or if neggative rewards are provided.
 func (accum AccumulatorObject) AddToUnclaimedRewards(positionName string, rewards sdk.DecCoins) error {
 	position, err := GetPosition(accum, positionName)
 	if err != nil {

proto/osmosis/poolmanager/v1beta1/query.proto

@@ -152,9 +152,7 @@ message PoolResponse {
 }
 
 //=============================== AllPools
-message AllPoolsRequest {
-  uint64 pool_id = 1 [ (gogoproto.moretags) = "yaml:\"pool_id\"" ];
-}
+message AllPoolsRequest {}
 message AllPoolsResponse {
   repeated google.protobuf.Any pools = 1
       [ (cosmos_proto.accepts_interface) = "PoolI" ];

tests/e2e/configurer/chain/commands.go

@@ -72,7 +72,7 @@ func (n *NodeConfig) CollectFees(from, positionIds string) {
 func (n *NodeConfig) CreateConcentratedPool(from, denom1, denom2 string, tickSpacing uint64, swapFee string) (uint64, error) {
 	n.LogActionF("creating concentrated pool")
 
-	cmd := []string{"osmosisd", "tx", "concentratedliquidity", "create-concentrated-pool", denom1, denom2, fmt.Sprintf("%d", tickSpacing), swapFee, fmt.Sprintf("--from=%s", from)}
+	cmd := []string{"osmosisd", "tx", "concentratedliquidity", "create-pool", denom1, denom2, fmt.Sprintf("%d", tickSpacing), swapFee, fmt.Sprintf("--from=%s", from)}
 	_, _, err := n.containerManager.ExecTxCmd(n.t, n.chainId, n.Name, cmd)
 	if err != nil {
 		return 0, err

tests/e2e/e2e_test.go

@@ -1554,13 +1554,13 @@ func (s *IntegrationTestSuite) TestAConcentratedLiquidity_CanonicalPool_And_Para
 		s.T().Skip("Skipping v16 canonical pool creation test because upgrade is not enabled")
 	}
 
-	// Taken from: https://app.osmosis.zone/pool/674
 	expectedFee := sdk.MustNewDecFromStr("0.002")
 
 	chainA := s.configurer.GetChainConfig(0)
 	chainANode, err := chainA.GetDefaultNode()
 	s.Require().NoError(err)
 
+	// Taken from: https://app.osmosis.zone/pool/674
 	concentratedPoolId := chainANode.QueryConcentratedPooIdLinkFromCFMM(config.DaiOsmoPoolIdv16)
 
 	concentratedPool := s.updatedPool(chainANode, concentratedPoolId)

x/concentrated-liquidity/README.md

@@ -427,6 +427,36 @@ type MsgCollectFeesResponse struct {
 }
 ```
 
+### `MsgFungifyChargedPositions`
+
+This message allows fungifying the fully charged unlocked positions belonging to the same owner
+and located in the same tick range.
+MsgFungifyChargedPosition takes in a list of positionIds and combines them into a single position.
+It validates that all positions belong to the same owner, are in the same ticks and are fully charged.
+Fails if not. Otherwise, it creates a completely new position P. P's liquidity equals to the sum of all
+liquidities of positions given by positionIds. The uptime of the join time of the new position equals
+to current block time - max authorized uptime duration (to signify that it is fully charged).
+The previous positions are deleted from state. Prior to deleting, the rewards are claimed.
+The old position's unclaimed rewards are transferred to the new position.
+The new position ID is returned.
+
+```go
+type MsgFungifyChargedPositions struct {
+ PositionIds    []uint64
+ Sender         string
+}
+```
+
+- **Response**
+
+On successful response, the new position id is returned.
+
+```go
+type MsgFungifyChargedPositionsResponse struct {
+ NewPositionId uint64
+}
+```
+
 ## Relationship to Pool Manager Module
 
 ### Pool Creation

x/concentrated-liquidity/client/cli/tx.go

@@ -30,6 +30,7 @@ func NewTxCmd() *cobra.Command {
 	osmocli.AddTxCmd(txCmd, NewCollectFeesCmd)
 	osmocli.AddTxCmd(txCmd, NewCollectIncentivesCmd)
 	osmocli.AddTxCmd(txCmd, NewCreateIncentiveCmd)
+	osmocli.AddTxCmd(txCmd, NewFungifyChargedPositionsCmd)
 	return txCmd
 }
 
@@ -39,10 +40,10 @@ var poolIdFlagOverride = map[string]string{
 
 func NewCreateConcentratedPoolCmd() (*osmocli.TxCliDesc, *clmodel.MsgCreateConcentratedPool) {
 	return &osmocli.TxCliDesc{
-		Use:     "create-concentrated-pool [denom-0] [denom-1] [tick-spacing] [swap-fee]",
+		Use:     "create-pool [denom-0] [denom-1] [tick-spacing] [swap-fee]",
 		Short:   "create a concentrated liquidity pool with the given denom pair, tick spacing, and swap fee",
 		Long:    "denom-1 (the quote denom), tick spacing, and swap fees must all be authorized by the concentrated liquidity module",
-		Example: "create-concentrated-pool uion uosmo 1 0.01 --from val --chain-id osmosis-1",
+		Example: "create-pool uion uosmo 100 0.01 --from val --chain-id osmosis-1",
 	}, &clmodel.MsgCreateConcentratedPool{}
 }
 
@@ -88,14 +89,22 @@ func NewCollectIncentivesCmd() (*osmocli.TxCliDesc, *types.MsgCollectIncentives)
 
 func NewCreateIncentiveCmd() (*osmocli.TxCliDesc, *types.MsgCreateIncentive) {
 	return &osmocli.TxCliDesc{
-		Use:                 "create-incentive [incentive-denom] [incentive-amount] [emission-rate] [start-time] [min-uptime]",
+		Use:                 "create-incentive [incentive-coin] [emission-rate] [start-time] [min-uptime]",
 		Short:               "create an incentive record to emit incentives (per second) to a given pool",
-		Example:             "create-incentive uosmo 69082 0.02 2023-03-03 03:20:35.419543805 24h --pool-id 1 --from val --chain-id osmosis-1",
+		Example:             "create-incentive 69082uosmo 0.02 \"2023-03-03T03:20:35.419543805\" 24h --pool-id 1 --from val --chain-id osmosis-1 --fees 875uosmo",
 		CustomFlagOverrides: poolIdFlagOverride,
 		Flags:               osmocli.FlagDesc{RequiredFlags: []*flag.FlagSet{FlagSetJustPoolId()}},
 	}, &types.MsgCreateIncentive{}
 }
 
+func NewFungifyChargedPositionsCmd() (*osmocli.TxCliDesc, *types.MsgFungifyChargedPositions) {
+	return &osmocli.TxCliDesc{
+		Use:     "fungify-positions [position-ids]",
+		Short:   "Combine fully charged positions within the same range into a new single fully charged position",
+		Example: "fungify-positions 1,5,7 --from val --chain-id osmosis-1",
+	}, &types.MsgFungifyChargedPositions{}
+}
+
 // NewCmdCreateConcentratedLiquidityPoolProposal implements a command handler for create concentrated liquidity pool proposal
 func NewCmdCreateConcentratedLiquidityPoolProposal() *cobra.Command {
 	cmd := &cobra.Command{

x/concentrated-liquidity/export_test.go

@@ -16,11 +16,14 @@ const (
 )
 
 var (
-	EmptyCoins           = emptyCoins
-	HundredFooCoins      = sdk.NewDecCoin("foo", sdk.NewInt(100))
-	HundredBarCoins      = sdk.NewDecCoin("bar", sdk.NewInt(100))
-	TwoHundredFooCoins   = sdk.NewDecCoin("foo", sdk.NewInt(200))
-	TwoHundredBarCoins   = sdk.NewDecCoin("bar", sdk.NewInt(200))
+	EmptyCoins         = emptyCoins
+	HundredFooCoins    = sdk.NewDecCoin("foo", sdk.NewInt(100))
+	HundredBarCoins    = sdk.NewDecCoin("bar", sdk.NewInt(100))
+	TwoHundredFooCoins = sdk.NewDecCoin("foo", sdk.NewInt(200))
+	TwoHundredBarCoins = sdk.NewDecCoin("bar", sdk.NewInt(200))
+	// TODO: this is incorrect. Should be grabbing from
+	// authorized params instead. Must verify that all tests still make sense.
+	// https://github.com/osmosis-labs/osmosis/issues/5039
 	FullyChargedDuration = types.SupportedUptimes[len(types.SupportedUptimes)-1]
 )
 
@@ -145,8 +148,8 @@ func (k Keeper) CreateFeeAccumulator(ctx sdk.Context, poolId uint64) error {
 	return k.createFeeAccumulator(ctx, poolId)
 }
 
-func (k Keeper) InitOrUpdateFeeAccumulatorPosition(ctx sdk.Context, poolId uint64, lowerTick, upperTick int64, positionId uint64, liquidity sdk.Dec) error {
-	return k.initOrUpdateFeeAccumulatorPosition(ctx, poolId, lowerTick, upperTick, positionId, liquidity)
+func (k Keeper) InitOrUpdatePositionFeeAccumulator(ctx sdk.Context, poolId uint64, lowerTick, upperTick int64, positionId uint64, liquidity sdk.Dec) error {
+	return k.initOrUpdatePositionFeeAccumulator(ctx, poolId, lowerTick, upperTick, positionId, liquidity)
 }
 
 func (k Keeper) GetFeeGrowthOutside(ctx sdk.Context, poolId uint64, lowerTick, upperTick int64) (sdk.DecCoins, error) {
@@ -212,7 +215,7 @@ func CalcAccruedIncentivesForAccum(ctx sdk.Context, accumUptime time.Duration, q
 }
 
 func (k Keeper) UpdateUptimeAccumulatorsToNow(ctx sdk.Context, poolId uint64) error {
-	return k.updateUptimeAccumulatorsToNow(ctx, poolId)
+	return k.updatePoolUptimeAccumulatorsToNow(ctx, poolId)
 }
 
 func (k Keeper) SetIncentiveRecord(ctx sdk.Context, incentiveRecord types.IncentiveRecord) error {
@@ -227,12 +230,12 @@ func (k Keeper) GetInitialUptimeGrowthOutsidesForTick(ctx sdk.Context, poolId ui
 	return k.getInitialUptimeGrowthOutsidesForTick(ctx, poolId, tick)
 }
 
-func (k Keeper) GetAllIncentiveRecordsForUptime(ctx sdk.Context, poolId uint64, minUptime time.Duration) ([]types.IncentiveRecord, error) {
-	return k.getAllIncentiveRecordsForUptime(ctx, poolId, minUptime)
+func (k Keeper) InitOrUpdatePositionUptimeAccumulators(ctx sdk.Context, poolId uint64, position sdk.Dec, owner sdk.AccAddress, lowerTick, upperTick int64, liquidityDelta sdk.Dec, positionId uint64) error {
+	return k.initOrUpdatePositionUptimeAccumulators(ctx, poolId, position, owner, lowerTick, upperTick, liquidityDelta, positionId)
 }
 
-func (k Keeper) InitOrUpdatePositionUptime(ctx sdk.Context, poolId uint64, position sdk.Dec, owner sdk.AccAddress, lowerTick, upperTick int64, liquidityDelta sdk.Dec, joinTime time.Time, positionId uint64) error {
-	return k.initOrUpdatePositionUptime(ctx, poolId, position, owner, lowerTick, upperTick, liquidityDelta, joinTime, positionId)
+func (k Keeper) GetAllIncentiveRecordsForUptime(ctx sdk.Context, poolId uint64, minUptime time.Duration) ([]types.IncentiveRecord, error) {
+	return k.getAllIncentiveRecordsForUptime(ctx, poolId, minUptime)
 }
 
 func (k Keeper) CollectIncentives(ctx sdk.Context, owner sdk.AccAddress, positionId uint64) (sdk.Coins, sdk.Coins, error) {

x/concentrated-liquidity/fees.go

@@ -46,7 +46,7 @@ func (k Keeper) chargeFee(ctx sdk.Context, poolId uint64, feeUpdate sdk.DecCoin)
 	return nil
 }
 
-// initOrUpdateFeeAccumulatorPosition mutates the fee accumulator position by either creating or updating it
+// initOrUpdatePositionFeeAccumulator mutates the fee accumulator position by either creating or updating it
 // for the given pool id in the range specified by the given lower and upper ticks, position id and liquidityDelta.
 // If liquidityDelta is positive, it adds liquidity. If liquidityDelta is negative, it removes liquidity.
 // If this is a new position, liqudityDelta must be positive.
@@ -62,7 +62,7 @@ func (k Keeper) chargeFee(ctx sdk.Context, poolId uint64, feeUpdate sdk.DecCoin)
 // - fails to create a new position.
 // - fails to prepare the accumulator for update.
 // - fails to update the position's accumulator.
-func (k Keeper) initOrUpdateFeeAccumulatorPosition(ctx sdk.Context, poolId uint64, lowerTick, upperTick int64, positionId uint64, liquidityDelta sdk.Dec) error {
+func (k Keeper) initOrUpdatePositionFeeAccumulator(ctx sdk.Context, poolId uint64, lowerTick, upperTick int64, positionId uint64, liquidityDelta sdk.Dec) error {
 	// Get the fee accumulator for the position's pool.
 	feeAccumulator, err := k.GetFeeAccumulator(ctx, poolId)
 	if err != nil {
@@ -302,6 +302,7 @@ func calculateFeeGrowth(targetTick int64, feeGrowthOutside sdk.DecCoins, current
 // as we must set the position's accumulator value to the sum of
 // - the fee/uptime growth inside at position creation time (position.InitAccumValue)
 // - fee/uptime growth outside at the current block time (feeGrowthOutside/uptimeGrowthOutside)
+// CONTRACT: position accumulator value prior to this call is equal to the growth inside the position at the time of last update.
 func preparePositionAccumulator(accumulator accum.AccumulatorObject, positionKey string, growthOutside sdk.DecCoins) error {
 	position, err := accum.GetPosition(accumulator, positionKey)
 	if err != nil {

x/concentrated-liquidity/fees_test.go

@@ -83,7 +83,7 @@ func (s *KeeperTestSuite) TestCreateAndGetFeeAccumulator() {
 	}
 }
 
-func (s *KeeperTestSuite) TestInitOrUpdateFeeAccumulatorPosition() {
+func (s *KeeperTestSuite) TestInitOrUpdatePositionFeeAccumulator() {
 	// Setup is done once so that we test
 	// the relationship between test cases.
 	// For example, that positions with non-zero liquidity
@@ -137,42 +137,37 @@ func (s *KeeperTestSuite) TestInitOrUpdateFeeAccumulatorPosition() {
 		positionFields positionFields
 
 		expectedLiquidity sdk.Dec
-		expectedPass      bool
+		expectedError     error
 	}
 	tests := []initFeeAccumTest{
 		{
 			name:           "error: negative liquidity for the first position",
 			positionFields: withLiquidity(defaultPositionFields, DefaultLiquidityAmt.Neg()),
-			expectedPass:   false,
+			expectedError:  types.NonPositiveLiquidityForNewPositionError{LiquidityDelta: DefaultLiquidityAmt.Neg(), PositionId: DefaultPositionId},
 		},
 		{
 			name:              "first position",
 			positionFields:    defaultPositionFields,
 			expectedLiquidity: defaultPositionFields.liquidity,
-			expectedPass:      true,
 		},
 		{
 			name:              "second position",
 			positionFields:    withPositionId(withLowerTick(defaultPositionFields, DefaultLowerTick+1), DefaultPositionId+1),
 			expectedLiquidity: defaultPositionFields.liquidity,
-			expectedPass:      true,
 		},
 		{
 			name:              "adding to first position",
 			positionFields:    defaultPositionFields,
-			expectedPass:      true,
 			expectedLiquidity: defaultPositionFields.liquidity.MulInt64(2),
 		},
 		{
 			name:              "removing from first position",
 			positionFields:    withLiquidity(defaultPositionFields, defaultPositionFields.liquidity.Neg()),
-			expectedPass:      true,
 			expectedLiquidity: defaultPositionFields.liquidity,
 		},
 		{
 			name:              "adding to second position",
 			positionFields:    withPositionId(withLowerTick(defaultPositionFields, DefaultLowerTick+1), DefaultPositionId+1),
-			expectedPass:      true,
 			expectedLiquidity: defaultPositionFields.liquidity.MulInt64(2),
 		},
 		{
@@ -185,34 +180,31 @@ func (s *KeeperTestSuite) TestInitOrUpdateFeeAccumulatorPosition() {
 				DefaultPositionId,
 				DefaultLiquidityAmt,
 			},
-			expectedPass: false,
+			expectedError: accum.AccumDoesNotExistError{AccumName: types.KeyFeePoolAccumulator(defaultPoolId + 1)},
 		},
 		{
 			name:              "existing accumulator, different owner - different position",
 			positionFields:    withPositionId(withOwner(defaultPositionFields, s.TestAccs[1]), DefaultPositionId+2),
 			expectedLiquidity: defaultPositionFields.liquidity,
-			expectedPass:      true,
 		},
 		{
 			name:              "existing accumulator, different upper tick - different position",
 			positionFields:    withPositionId(withUpperTick(defaultPositionFields, DefaultUpperTick+1), DefaultPositionId+3),
 			expectedLiquidity: defaultPositionFields.liquidity,
-			expectedPass:      true,
 		},
 		{
 			name:              "existing accumulator, different lower tick - different position",
 			positionFields:    withPositionId(withLowerTick(defaultPositionFields, DefaultUpperTick+1), DefaultPositionId+4),
 			expectedLiquidity: defaultPositionFields.liquidity,
-			expectedPass:      true,
 		},
 	}
 
 	for _, tc := range tests {
 		tc := tc
 		s.Run(tc.name, func() {
 			// System under test
-			err := clKeeper.InitOrUpdateFeeAccumulatorPosition(s.Ctx, tc.positionFields.poolId, tc.positionFields.lowerTick, tc.positionFields.upperTick, tc.positionFields.positionId, tc.positionFields.liquidity)
-			if tc.expectedPass {
+			err := clKeeper.InitOrUpdatePositionFeeAccumulator(s.Ctx, tc.positionFields.poolId, tc.positionFields.lowerTick, tc.positionFields.upperTick, tc.positionFields.positionId, tc.positionFields.liquidity)
+			if tc.expectedError == nil {
 				s.Require().NoError(err)
 
 				// get fee accum and see if position size has been properly initialized
@@ -243,6 +235,7 @@ func (s *KeeperTestSuite) TestInitOrUpdateFeeAccumulatorPosition() {
 				s.Require().Equal(cl.EmptyCoins, positionRecord.UnclaimedRewards)
 			} else {
 				s.Require().Error(err)
+				s.Require().ErrorContains(err, tc.expectedError.Error())
 			}
 		})
 	}
@@ -1302,7 +1295,7 @@ func (s *KeeperTestSuite) TestInitOrUpdateFeeAccumulatorPosition_UpdatingPositio
 			s.Require().NoError(err)
 
 			// InitOrUpdateFeeAccumulatorPosition #1 lower tick to upper tick
-			err = s.App.ConcentratedLiquidityKeeper.InitOrUpdateFeeAccumulatorPosition(s.Ctx, poolId, DefaultLowerTick, DefaultUpperTick, DefaultPositionId, DefaultLiquidityAmt)
+			err = s.App.ConcentratedLiquidityKeeper.InitOrUpdatePositionFeeAccumulator(s.Ctx, poolId, DefaultLowerTick, DefaultUpperTick, DefaultPositionId, DefaultLiquidityAmt)
 			s.Require().NoError(err)
 
 			// Imaginary fee charge #2.
@@ -1311,7 +1304,7 @@ func (s *KeeperTestSuite) TestInitOrUpdateFeeAccumulatorPosition_UpdatingPositio
 			}
 
 			// InitOrUpdateFeeAccumulatorPosition # 2 lower tick to upper tick with a different position id.
-			err = s.App.ConcentratedLiquidityKeeper.InitOrUpdateFeeAccumulatorPosition(s.Ctx, poolId, DefaultLowerTick, DefaultUpperTick, DefaultPositionId+1, DefaultLiquidityAmt)
+			err = s.App.ConcentratedLiquidityKeeper.InitOrUpdatePositionFeeAccumulator(s.Ctx, poolId, DefaultLowerTick, DefaultUpperTick, DefaultPositionId+1, DefaultLiquidityAmt)
 			s.Require().NoError(err)
 
 			// Imaginary fee charge #3.
@@ -1320,7 +1313,7 @@ func (s *KeeperTestSuite) TestInitOrUpdateFeeAccumulatorPosition_UpdatingPositio
 			}
 
 			// InitOrUpdateFeeAccumulatorPosition # 3 lower tick to upper tick with the original position id.
-			err = s.App.ConcentratedLiquidityKeeper.InitOrUpdateFeeAccumulatorPosition(s.Ctx, poolId, DefaultLowerTick, DefaultUpperTick, DefaultPositionId, DefaultLiquidityAmt)
+			err = s.App.ConcentratedLiquidityKeeper.InitOrUpdatePositionFeeAccumulator(s.Ctx, poolId, DefaultLowerTick, DefaultUpperTick, DefaultPositionId, DefaultLiquidityAmt)
 			s.Require().NoError(err)
 
 			// Imaginary fee charge #4.