[CL Message Audit]: MsgUnlockAndMigrateSharesToFullRangeConcentratedP…

…osition [1/2] (#5178)

* initial push audit changes

* add back proto tag

* rename to validateGammLockForSuperfluidStaking

* remove unused errors

proto/osmosis/superfluid/tx.proto

@@ -150,8 +150,9 @@ message MsgUnlockAndMigrateSharesToFullRangeConcentratedPosition {
   ];
   // token_out_mins indicates minimum token to exit Balancer pool with.
   repeated cosmos.base.v1beta1.Coin token_out_mins = 4 [
-    (gogoproto.moretags) = "yaml:\"token_out_min_amounts\"",
-    (gogoproto.nullable) = false
+    (gogoproto.moretags) = "yaml:\"token_out_mins\"",
+    (gogoproto.nullable) = false,
+    (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"
   ];
 }
 

x/gamm/types/key.go

@@ -33,12 +33,11 @@ var (
 )
 
 func MustGetPoolIdFromShareDenom(denom string) uint64 {
-	numberStr := strings.TrimLeft(denom, GAMMTokenPrefix)
-	number, err := strconv.Atoi(numberStr)
+	number, err := GetPoolIdFromShareDenom(denom)
 	if err != nil {
 		panic(err)
 	}
-	return uint64(number)
+	return number
 }
 
 func GetPoolIdFromShareDenom(denom string) (uint64, error) {

x/superfluid/README.md

@@ -376,6 +376,41 @@ It then mints concentrated liquidity shares and locks them up for the
 staking duration. From there, the normal superfluid delegation logic
 is executed.
 
+### Create Full Range Position and Superfluid Delegate
+
+```{.go}
+type MsgUnlockAndMigrateSharesToFullRangeConcentratedPosition struct {
+ Sender string
+ LockId uint64
+ SharesToMigrate sdk.Coin
+ TokenOutMins sdk.Coins
+}
+```
+
+Upon completion, the following response is given:
+
+```{.go}
+type MsgUnlockAndMigrateSharesToFullRangeConcentratedPositionResponse struct {
+ Amount0 string
+ Amount1 string
+ LiquidityCreated sdk.Dec
+ JoinTime time.Time
+}
+```
+
+The message starts by determining which migration method to use.
+- If underlying lock is superfluid bonded
+  - `migrateSuperfluidBondedBalancerToConcentrated`
+- If underlying lock is superfluid unbonding
+  - `migrateSuperfluidUnbondingBalancerToConcentrated`
+- If underlying lock is not superfluid bonded (vanilla lock)
+  - `migrateNonSuperfluidLockBalancerToConcentrated`
+
+It then routes to that migration message, which will migrate the gamm lock from
+the previous state it was in, to the same state but in the concentrated pool. If
+the sharesToMigrate is zero, then the entire lock is migrated. If only a subset
+of the shares are migrated, then the remaining shares are left in the gamm pool.
+
 ## Epochs
 
 Overall Epoch sequence

x/superfluid/client/cli/tx.go

@@ -37,6 +37,7 @@ func GetTxCmd() *cobra.Command {
 	)
 	osmocli.AddTxCmd(cmd, NewCreateFullRangePositionAndSuperfluidDelegateCmd)
 	osmocli.AddTxCmd(cmd, NewAddToConcentratedLiquiditySuperfluidPositionCmd)
+	osmocli.AddTxCmd(cmd, NewUnlockAndMigrateSharesToFullRangeConcentratedPositionCmd)
 
 	return cmd
 }
@@ -414,3 +415,11 @@ func NewAddToConcentratedLiquiditySuperfluidPositionCmd() (*osmocli.TxCliDesc, *
 		Example: "add-to-superfluid-cl-position 10 1000000000uosmo 10000000uion",
 	}, &types.MsgAddToConcentratedLiquiditySuperfluidPosition{}
 }
+
+func NewUnlockAndMigrateSharesToFullRangeConcentratedPositionCmd() (*osmocli.TxCliDesc, *types.MsgUnlockAndMigrateSharesToFullRangeConcentratedPosition) {
+	return &osmocli.TxCliDesc{
+		Use:     "unlock-and-migrate-to-cl [lock-id] [shares-to-migrate] [token-out-mins]",
+		Short:   "unlock and migrate gamm shares to full range concentrated position",
+		Example: "unlock-and-migrate-cl 10 25000000000gamm/pool/2 1000000000uosmo,10000000uion",
+	}, &types.MsgUnlockAndMigrateSharesToFullRangeConcentratedPosition{}
+}

x/superfluid/keeper/concentrated_liquidity.go

@@ -69,7 +69,7 @@ func (k Keeper) addToConcentratedLiquiditySuperfluidPosition(ctx sdk.Context, ow
 
 	// Superfluid undelegate the superfluid delegated position.
 	// This deletes the connection between the lock and the intermediate account, deletes the synthetic lock, and burns the synthetic osmo.
-	intermediateAccount, err := k.SuperfluidUndelegateToConcentratedPosition(ctx, owner.String(), lockId)
+	intermediateAccount, err := k.superfluidUndelegateToConcentratedPosition(ctx, owner.String(), lockId)
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, 0, err
 	}

x/superfluid/keeper/export_test.go

@@ -7,6 +7,7 @@ import (
 
 	cltypes "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/types"
 	lockuptypes "github.com/osmosis-labs/osmosis/v15/x/lockup/types"
+	"github.com/osmosis-labs/osmosis/v15/x/superfluid/types"
 )
 
 var (
@@ -45,3 +46,15 @@ func (k Keeper) PrepareMigration(ctx sdk.Context, sender sdk.AccAddress, lockId
 func (k Keeper) AddToConcentratedLiquiditySuperfluidPosition(ctx sdk.Context, owner sdk.AccAddress, positionId uint64, amount0Added, amount1Added sdk.Int) (uint64, sdk.Int, sdk.Int, sdk.Dec, uint64, error) {
 	return k.addToConcentratedLiquiditySuperfluidPosition(ctx, owner, positionId, amount0Added, amount1Added)
 }
+
+func (k Keeper) SuperfluidUndelegateToConcentratedPosition(ctx sdk.Context, sender string, gammLockID uint64) (types.SuperfluidIntermediaryAccount, error) {
+	return k.superfluidUndelegateToConcentratedPosition(ctx, sender, gammLockID)
+}
+
+func (k Keeper) ValidateGammLockForSuperfluidStaking(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, lockId uint64) (*lockuptypes.PeriodLock, error) {
+	return k.validateGammLockForSuperfluidStaking(ctx, sender, poolId, lockId)
+}
+
+func (k Keeper) GetExistingLockRemainingDuration(ctx sdk.Context, lock *lockuptypes.PeriodLock) (time.Duration, error) {
+	return k.getExistingLockRemainingDuration(ctx, lock)
+}

x/superfluid/keeper/migrate.go

@@ -66,7 +66,7 @@ func (k Keeper) migrateSuperfluidBondedBalancerToConcentrated(ctx sdk.Context,
 
 	// Superfluid undelegate the superfluid delegated position.
 	// This deletes the connection between the lock and the intermediate account, deletes the synthetic lock, and burns the synthetic osmo.
-	intermediateAccount, err := k.SuperfluidUndelegateToConcentratedPosition(ctx, sender.String(), lockId)
+	intermediateAccount, err := k.superfluidUndelegateToConcentratedPosition(ctx, sender.String(), lockId)
 	if err != nil {
 		return 0, sdk.Int{}, sdk.Int{}, sdk.Dec{}, time.Time{}, 0, 0, err
 	}
@@ -286,13 +286,16 @@ func (k Keeper) prepareMigration(ctx sdk.Context, sender sdk.AccAddress, lockId
 	}
 
 	// Check that lockID corresponds to sender, and contains correct denomination of LP shares.
-	preMigrationLock, err = k.validateLockForUnpool(ctx, sender, poolIdLeaving, lockId)
+	preMigrationLock, err = k.validateGammLockForSuperfluidStaking(ctx, sender, poolIdLeaving, lockId)
 	if err != nil {
 		return 0, 0, nil, &lockuptypes.PeriodLock{}, 0, nil, false, false, err
 	}
 
 	// Before we break the lock, we must note the time remaining on the lock.
-	remainingLockTime = k.getExistingLockRemainingDuration(ctx, preMigrationLock)
+	remainingLockTime, err = k.getExistingLockRemainingDuration(ctx, preMigrationLock)
+	if err != nil {
+		return 0, 0, nil, &lockuptypes.PeriodLock{}, 0, nil, false, false, err
+	}
 
 	// Check if the lock has a corresponding synthetic lock.
 	// Synthetic lock existence implies that the lock is superfluid delegated or undelegating.

x/superfluid/keeper/msg_server.go

@@ -132,38 +132,6 @@ func (server msgServer) LockAndSuperfluidDelegate(goCtx context.Context, msg *ty
 	}, err
 }
 
-func (server msgServer) CreateFullRangePositionAndSuperfluidDelegate(goCtx context.Context, msg *types.MsgCreateFullRangePositionAndSuperfluidDelegate) (*types.MsgCreateFullRangePositionAndSuperfluidDelegateResponse, error) {
-	ctx := sdk.UnwrapSDKContext(goCtx)
-
-	address, err := sdk.AccAddressFromBech32(msg.Sender)
-	if err != nil {
-		return &types.MsgCreateFullRangePositionAndSuperfluidDelegateResponse{}, err
-	}
-	positionId, _, _, _, _, lockId, err := server.keeper.clk.CreateFullRangePositionLocked(ctx, msg.PoolId, address, msg.Coins, server.keeper.sk.GetParams(ctx).UnbondingTime)
-	if err != nil {
-		return &types.MsgCreateFullRangePositionAndSuperfluidDelegateResponse{}, err
-	}
-
-	superfluidDelegateMsg := types.MsgSuperfluidDelegate{
-		Sender:  msg.Sender,
-		LockId:  lockId,
-		ValAddr: msg.ValAddr,
-	}
-
-	_, err = server.SuperfluidDelegate(goCtx, &superfluidDelegateMsg)
-
-	if err != nil {
-		return &types.MsgCreateFullRangePositionAndSuperfluidDelegateResponse{}, err
-	}
-
-	events.EmitCreateFullRangePositionAndSuperfluidDelegateEvent(ctx, lockId, positionId, msg.ValAddr)
-
-	return &types.MsgCreateFullRangePositionAndSuperfluidDelegateResponse{
-		LockID:     lockId,
-		PositionID: positionId,
-	}, nil
-}
-
 func (server msgServer) UnPoolWhitelistedPool(goCtx context.Context, msg *types.MsgUnPoolWhitelistedPool) (*types.MsgUnPoolWhitelistedPoolResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
 
@@ -197,6 +165,38 @@ func (server msgServer) UnPoolWhitelistedPool(goCtx context.Context, msg *types.
 	return &types.MsgUnPoolWhitelistedPoolResponse{ExitedLockIds: allExitedLockIDs}, nil
 }
 
+func (server msgServer) CreateFullRangePositionAndSuperfluidDelegate(goCtx context.Context, msg *types.MsgCreateFullRangePositionAndSuperfluidDelegate) (*types.MsgCreateFullRangePositionAndSuperfluidDelegateResponse, error) {
+	ctx := sdk.UnwrapSDKContext(goCtx)
+
+	address, err := sdk.AccAddressFromBech32(msg.Sender)
+	if err != nil {
+		return &types.MsgCreateFullRangePositionAndSuperfluidDelegateResponse{}, err
+	}
+	positionId, _, _, _, _, lockId, err := server.keeper.clk.CreateFullRangePositionLocked(ctx, msg.PoolId, address, msg.Coins, server.keeper.sk.GetParams(ctx).UnbondingTime)
+	if err != nil {
+		return &types.MsgCreateFullRangePositionAndSuperfluidDelegateResponse{}, err
+	}
+
+	superfluidDelegateMsg := types.MsgSuperfluidDelegate{
+		Sender:  msg.Sender,
+		LockId:  lockId,
+		ValAddr: msg.ValAddr,
+	}
+
+	_, err = server.SuperfluidDelegate(goCtx, &superfluidDelegateMsg)
+
+	if err != nil {
+		return &types.MsgCreateFullRangePositionAndSuperfluidDelegateResponse{}, err
+	}
+
+	events.EmitCreateFullRangePositionAndSuperfluidDelegateEvent(ctx, lockId, positionId, msg.ValAddr)
+
+	return &types.MsgCreateFullRangePositionAndSuperfluidDelegateResponse{
+		LockID:     lockId,
+		PositionID: positionId,
+	}, nil
+}
+
 func (server msgServer) UnlockAndMigrateSharesToFullRangeConcentratedPosition(goCtx context.Context, msg *types.MsgUnlockAndMigrateSharesToFullRangeConcentratedPosition) (*types.MsgUnlockAndMigrateSharesToFullRangeConcentratedPositionResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
 

x/superfluid/keeper/msg_server_test.go

@@ -473,23 +473,14 @@ func (suite *KeeperTestSuite) TestMsgUnPoolWhitelistedPool_Event() {
 func (suite *KeeperTestSuite) TestUnlockAndMigrateSharesToFullRangeConcentratedPosition_Event() {
 	suite.SetupTest()
 
-	const (
-		token0Denom = "token0"
-	)
-
-	// Update authorized quote denoms with the quote denom relied on by the test
-	concentratedLiquidityParams := suite.App.ConcentratedLiquidityKeeper.GetParams(suite.Ctx)
-	concentratedLiquidityParams.AuthorizedQuoteDenoms = append(concentratedLiquidityParams.AuthorizedQuoteDenoms, token0Denom)
-	suite.App.ConcentratedLiquidityKeeper.SetParams(suite.Ctx, concentratedLiquidityParams)
-
 	msgServer := keeper.NewMsgServerImpl(suite.App.SuperfluidKeeper)
 	suite.FundAcc(suite.TestAccs[0], defaultAcctFunds)
 	fullRangeCoins := sdk.NewCoins(defaultPoolAssets[0].Token, defaultPoolAssets[1].Token)
 
 	// Set validators
 	valAddrs := suite.SetupValidators([]stakingtypes.BondStatus{stakingtypes.Bonded})
 
-	// Set balancer pool and make its respective gamm share an authorized superfluid asset
+	// Set balancer pool (foo and stake) and make its respective gamm share an authorized superfluid asset
 	msg := balancer.NewMsgCreateBalancerPool(suite.TestAccs[0], balancer.PoolParams{
 		SwapFee: sdk.NewDecWithPrec(1, 2),
 		ExitFee: sdk.NewDec(0),
@@ -505,7 +496,7 @@ func (suite *KeeperTestSuite) TestUnlockAndMigrateSharesToFullRangeConcentratedP
 	})
 	suite.Require().NoError(err)
 
-	// Set concentrated pool with the same denoms as the balancer pool
+	// Set concentrated pool with the same denoms as the balancer pool (foo and stake)
 	clPool := suite.PrepareCustomConcentratedPool(suite.TestAccs[0], defaultPoolAssets[0].Token.Denom, defaultPoolAssets[1].Token.Denom, 1, sdk.ZeroDec())
 
 	// Set migration link between the balancer and concentrated pool
@@ -529,7 +520,8 @@ func (suite *KeeperTestSuite) TestUnlockAndMigrateSharesToFullRangeConcentratedP
 	suite.Require().NoError(err)
 
 	// Execute UnlockAndMigrateSharesToFullRangeConcentratedPosition message
-	sender, _ := sdk.AccAddressFromBech32(locks[0].Owner)
+	sender, err := sdk.AccAddressFromBech32(locks[0].Owner)
+	suite.Require().NoError(err)
 	_, err = msgServer.UnlockAndMigrateSharesToFullRangeConcentratedPosition(sdk.WrapSDKContext(suite.Ctx),
 		types.NewMsgUnlockAndMigrateSharesToFullRangeConcentratedPosition(sender, locks[0].ID, locks[0].Coins[0]))
 	suite.Require().NoError(err)

x/superfluid/keeper/stake.go

@@ -242,7 +242,7 @@ func (k Keeper) SuperfluidDelegate(ctx sdk.Context, sender string, lockID uint64
 	return k.mintOsmoTokensAndDelegate(ctx, amount, acc)
 }
 
-// undelegateCommon is a helper function for SuperfluidUndelegate and SuperfluidUndelegateToConcentratedPosition.
+// undelegateCommon is a helper function for SuperfluidUndelegate and superfluidUndelegateToConcentratedPosition.
 // It performs the following tasks:
 // - checks that the lock is valid for superfluid staking
 // - gets the intermediary account associated with the lock id
@@ -300,10 +300,10 @@ func (k Keeper) SuperfluidUndelegate(ctx sdk.Context, sender string, lockID uint
 	return k.createSyntheticLockup(ctx, lockID, intermediaryAcc, unlockingStatus)
 }
 
-// SuperfluidUndelegateToConcentratedPosition starts undelegating superfluid delegated position for the given lock. It behaves similarly to SuperfluidUndelegate,
+// superfluidUndelegateToConcentratedPosition starts undelegating superfluid delegated position for the given lock. It behaves similarly to SuperfluidUndelegate,
 // however it does not create a new synthetic lockup representing the unstaking side. This is because at the time this function is called, the new concentrated liquidity side
 // lock has not yet been created. Once the new cl side lock is created, the synthetic lockup representing the unstaking side is created.
-func (k Keeper) SuperfluidUndelegateToConcentratedPosition(ctx sdk.Context, sender string, gammLockID uint64) (types.SuperfluidIntermediaryAccount, error) {
+func (k Keeper) superfluidUndelegateToConcentratedPosition(ctx sdk.Context, sender string, gammLockID uint64) (types.SuperfluidIntermediaryAccount, error) {
 	return k.undelegateCommon(ctx, sender, gammLockID)
 }
 

x/superfluid/keeper/unpool.go

@@ -33,14 +33,17 @@ func (k Keeper) UnpoolAllowedPools(ctx sdk.Context, sender sdk.AccAddress, poolI
 	// 2) Consistency check that lockID corresponds to sender, and contains correct LP shares.
 	// These are expected to be true by the caller, but good to double check
 	// TODO: Try to minimize dependence on lock here
-	lock, err := k.validateLockForUnpool(ctx, sender, poolId, lockId)
+	lock, err := k.validateGammLockForSuperfluidStaking(ctx, sender, poolId, lockId)
 	if err != nil {
 		return []uint64{}, err
 	}
 	gammSharesInLock := lock.Coins[0]
 
 	// 3) Get remaining duration on the lock. Handle if the lock was unbonding.
-	lockRemainingDuration := k.getExistingLockRemainingDuration(ctx, lock)
+	lockRemainingDuration, err := k.getExistingLockRemainingDuration(ctx, lock)
+	if err != nil {
+		return []uint64{}, err
+	}
 
 	// 4) If superfluid delegated, superfluid undelegate
 	err = k.unbondSuperfluidIfExists(ctx, sender, lockId)
@@ -99,39 +102,48 @@ func (k Keeper) checkUnpoolWhitelisted(ctx sdk.Context, poolId uint64) error {
 	return types.ErrPoolNotWhitelisted
 }
 
-// check if pool is whitelisted for unpool
-func (k Keeper) validateLockForUnpool(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, lockId uint64) (*lockuptypes.PeriodLock, error) {
+// validateGammLockForSuperfluidStaking checks if the provided lock:
+// 1) is owned by the provided sender
+// 2) contains only 1 coin
+// 3) contains the gamm LP shares associated with the provided poolId
+func (k Keeper) validateGammLockForSuperfluidStaking(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, lockId uint64) (*lockuptypes.PeriodLock, error) {
 	lock, err := k.lk.GetLockByID(ctx, lockId)
 	if err != nil {
-		return lock, err
+		return &lockuptypes.PeriodLock{}, err
 	}
 
-	// consistency check: validate lock owner
-	// However, we expect this to be guaranteed by caller though.
+	// Validate lock owner.
+	// We expect this to be guaranteed by caller, though.
 	if lock.Owner != sender.String() {
-		return lock, lockuptypes.ErrNotLockOwner
+		return &lockuptypes.PeriodLock{}, lockuptypes.ErrNotLockOwner
 	}
 
 	if lock.Coins.Len() != 1 {
-		return lock, types.ErrMultipleCoinsLockupNotSupported
+		return &lockuptypes.PeriodLock{}, types.ErrMultipleCoinsLockupNotSupported
 	}
 
 	gammShare := lock.Coins[0]
 	if gammShare.Denom != gammtypes.GetPoolShareDenom(poolId) {
-		return lock, types.ErrLockUnpoolNotAllowed
+		return &lockuptypes.PeriodLock{}, types.UnexpectedDenomError{ExpectedDenom: gammtypes.GetPoolShareDenom(poolId), ProvidedDenom: gammShare.Denom}
 	}
 
 	return lock, nil
 }
 
-func (k Keeper) getExistingLockRemainingDuration(ctx sdk.Context, lock *lockuptypes.PeriodLock) time.Duration {
+// getExistingLockRemainingDuration returns the time remaining until the lock is finished unlocking.
+// If the lock is not unlocking, then the duration field of the lock is returned.
+func (k Keeper) getExistingLockRemainingDuration(ctx sdk.Context, lock *lockuptypes.PeriodLock) (time.Duration, error) {
 	if lock.IsUnlocking() {
-		// lock is unlocking, so remaining duration equals lock.EndTime - ctx.BlockTime
+		// Lock is unlocking, so remaining duration equals lock.EndTime - ctx.BlockTime.
 		remainingDuration := lock.EndTime.Sub(ctx.BlockTime())
-		return remainingDuration
+		// Defense in depth, ensure the duration is not negative.
+		if remainingDuration < 0 {
+			return 0, types.NegativeDurationError{Duration: remainingDuration}
+		}
+		return remainingDuration, nil
 	}
-	// lock is bonded, thus the time it should take to unlock is lock.Duration
-	return lock.Duration
+	// Lock is not unlocking, thus the time it should take to unlock is the locks duration.
+	return lock.Duration, nil
 }
 
 // TODO: Review this in more depth