fix: expect single synthetic lock per native lock ID (#5265)

* single synth lock per lock ID

* add changelog entry

* deprecate SyntheticLockupsByLockupID

* small nits

* add deprecated to underlying messages

* remove iterator

* add IsNil check

* add back iterator

* add back old CLI command

CHANGELOG.md

@@ -71,7 +71,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   * [#5187](https://github.com/osmosis-labs/osmosis/pull/5187) Expand `IncentivizedPools` query to include internally incentivized CL pools.
   * [#5239](https://github.com/osmosis-labs/osmosis/pull/5239) Implement `GetTotalPoolShares` public keeper function for GAMM.
   * [#5261](https://github.com/osmosis-labs/osmosis/pull/5261) Allows `UpdateFeeTokenProposal` to take in multiple fee tokens instead of just one.
-
+  * [#5265](https://github.com/osmosis-labs/osmosis/pull/5265) Ensure a lock cannot point to multiple synthetic locks. Deprecates `SyntheticLockupsByLockupID` in favor of `SyntheticLockupByLockupID`.
+
 ### API breaks
 
 * [#4336](https://github.com/osmosis-labs/osmosis/pull/4336) Move epochs module into its own go.mod

proto/osmosis/lockup/query.proto

@@ -88,13 +88,22 @@ service Query {
     option (google.api.http).get = "/osmosis/lockup/v1beta1/next_lock_id";
   }
 
-  // Returns synthetic lockups by native lockup id
+  // Returns synthetic lockup by native lockup id
+  // Deprecated: use SyntheticLockupByLockupID instead
   rpc SyntheticLockupsByLockupID(SyntheticLockupsByLockupIDRequest)
       returns (SyntheticLockupsByLockupIDResponse) {
+    option deprecated = true;
     option (google.api.http).get =
         "/osmosis/lockup/v1beta1/synthetic_lockups_by_lock_id/{lock_id}";
   }
 
+  // Returns synthetic lockup by native lockup id
+  rpc SyntheticLockupByLockupID(SyntheticLockupByLockupIDRequest)
+      returns (SyntheticLockupByLockupIDResponse) {
+    option (google.api.http).get =
+        "/osmosis/lockup/v1beta1/synthetic_lockup_by_lock_id/{lock_id}";
+  }
+
   // Returns account locked records with longer duration
   rpc AccountLockedLongerDuration(AccountLockedLongerDurationRequest)
       returns (AccountLockedLongerDurationResponse) {
@@ -247,11 +256,20 @@ message LockedResponse { PeriodLock lock = 1; };
 message NextLockIDRequest {};
 message NextLockIDResponse { uint64 lock_id = 1; };
 
-message SyntheticLockupsByLockupIDRequest { uint64 lock_id = 1; }
+message SyntheticLockupsByLockupIDRequest {
+  option deprecated = true;
+  uint64 lock_id = 1;
+}
 message SyntheticLockupsByLockupIDResponse {
+  option deprecated = true;
   repeated SyntheticLock synthetic_locks = 1 [ (gogoproto.nullable) = false ];
 }
 
+message SyntheticLockupByLockupIDRequest { uint64 lock_id = 1; }
+message SyntheticLockupByLockupIDResponse {
+  SyntheticLock synthetic_lock = 1 [ (gogoproto.nullable) = false ];
+}
+
 message AccountLockedLongerDurationRequest {
   string owner = 1 [ (gogoproto.moretags) = "yaml:\"owner\"" ];
   google.protobuf.Duration duration = 2 [

x/lockup/client/cli/query.go

@@ -43,6 +43,7 @@ func GetQueryCmd() *cobra.Command {
 		GetCmdAccountLockedLongerDurationDenom(),
 		GetCmdOutputLocksJson(),
 		GetCmdSyntheticLockupsByLockupID(),
+		GetCmdSyntheticLockupByLockupID(),
 		GetCmdAccountLockedDuration(),
 		GetCmdNextLockID(),
 		osmocli.GetParams[*types.QueryParamsRequest](
@@ -200,10 +201,19 @@ func GetCmdNextLockID() *cobra.Command {
 }
 
 // GetCmdSyntheticLockupsByLockupID returns synthetic lockups by lockup id.
+// nolint: staticcheck
 func GetCmdSyntheticLockupsByLockupID() *cobra.Command {
 	return osmocli.SimpleQueryCmd[*types.SyntheticLockupsByLockupIDRequest](
 		"synthetic-lockups-by-lock-id <id>",
-		"Query synthetic lockups by lockup id",
+		"Query synthetic lockups by lockup id (is deprecated for synthetic-lockup-by-lock-id)",
+		`{{.Short}}`, types.ModuleName, types.NewQueryClient)
+}
+
+// GetCmdSyntheticLockupByLockupID returns synthetic lockup by lockup id.
+func GetCmdSyntheticLockupByLockupID() *cobra.Command {
+	return osmocli.SimpleQueryCmd[*types.SyntheticLockupByLockupIDRequest](
+		"synthetic-lockup-by-lock-id <id>",
+		"Query synthetic lock by underlying lockup id",
 		`{{.Short}}`, types.ModuleName, types.NewQueryClient)
 }
 

x/lockup/keeper/grpc_query.go

@@ -178,14 +178,33 @@ func (q Querier) NextLockID(goCtx context.Context, req *types.NextLockIDRequest)
 }
 
 // SyntheticLockupsByLockupID returns synthetic lockups by native lockup id.
+// Deprecated: use SyntheticLockupByLockupID instead.
+// nolint: staticcheck
 func (q Querier) SyntheticLockupsByLockupID(goCtx context.Context, req *types.SyntheticLockupsByLockupIDRequest) (*types.SyntheticLockupsByLockupIDResponse, error) {
 	if req == nil {
 		return nil, status.Error(codes.InvalidArgument, "empty request")
 	}
 
 	ctx := sdk.UnwrapSDKContext(goCtx)
-	synthLocks := q.Keeper.GetAllSyntheticLockupsByLockup(ctx, req.LockId)
-	return &types.SyntheticLockupsByLockupIDResponse{SyntheticLocks: synthLocks}, nil
+	synthLock, err := q.Keeper.GetSyntheticLockupByUnderlyingLockId(ctx, req.LockId)
+	if err != nil {
+		return nil, err
+	}
+	return &types.SyntheticLockupsByLockupIDResponse{SyntheticLocks: []types.SyntheticLock{synthLock}}, nil
+}
+
+// SyntheticLockupByLockupID returns synthetic lockup by native lockup id.
+func (q Querier) SyntheticLockupByLockupID(goCtx context.Context, req *types.SyntheticLockupByLockupIDRequest) (*types.SyntheticLockupByLockupIDResponse, error) {
+	if req == nil {
+		return nil, status.Error(codes.InvalidArgument, "empty request")
+	}
+
+	ctx := sdk.UnwrapSDKContext(goCtx)
+	synthLock, err := q.Keeper.GetSyntheticLockupByUnderlyingLockId(ctx, req.LockId)
+	if err != nil {
+		return nil, err
+	}
+	return &types.SyntheticLockupByLockupIDResponse{SyntheticLock: synthLock}, nil
 }
 
 // AccountLockedLongerDuration returns locks of an account with duration longer than specified.

x/lockup/keeper/lock.go

@@ -105,9 +105,11 @@ func (k Keeper) AddTokensToLockByID(ctx sdk.Context, lockID uint64, owner sdk.Ac
 		return nil, err
 	}
 
-	for _, synthlock := range k.GetAllSyntheticLockupsByLockup(ctx, lock.ID) {
-		k.accumulationStore(ctx, synthlock.SynthDenom).Increase(accumulationKey(synthlock.Duration), tokensToAdd.Amount)
+	synthlock, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID)
+	if err != nil {
+		return nil, err
 	}
+	k.accumulationStore(ctx, synthlock.SynthDenom).Increase(accumulationKey(synthlock.Duration), tokensToAdd.Amount)
 
 	if k.hooks == nil {
 		return lock, nil
@@ -375,15 +377,20 @@ func (k Keeper) PartialForceUnlock(ctx sdk.Context, lock types.PeriodLock, coins
 // ForceUnlock ignores unlock duration and immediately unlocks the lock and refunds tokens to lock owner.
 func (k Keeper) ForceUnlock(ctx sdk.Context, lock types.PeriodLock) error {
 	// Steps:
-	// 1) Break associated synthetic locks. (Superfluid data)
+	// 1) Break associated synthetic lock. (Superfluid data)
 	// 2) If lock is bonded, move it to unlocking
 	// 3) Run logic to delete unlocking metadata, and send tokens to owner.
 
-	synthLocks := k.GetAllSyntheticLockupsByLockup(ctx, lock.ID)
-	err := k.DeleteAllSyntheticLocks(ctx, lock, synthLocks)
+	synthLock, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID)
 	if err != nil {
 		return err
 	}
+	if !synthLock.IsNil() {
+		err = k.DeleteSyntheticLockup(ctx, lock.ID, synthLock.SynthDenom)
+		if err != nil {
+			return err
+		}
+	}
 
 	if !lock.IsUnlocking() {
 		_, err := k.BeginUnlock(ctx, lock.ID, nil)
@@ -727,13 +734,14 @@ func (k Keeper) removeTokensFromLock(ctx sdk.Context, lock *types.PeriodLock, co
 	}
 
 	// increase synthetic lockup's accumulation store
-	synthLocks := k.GetAllSyntheticLockupsByLockup(ctx, lock.ID)
+	synthLock, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID)
+	if err != nil {
+		return err
+	}
 
 	// Note: since synthetic lockup deletion is using native lockup's coins to reduce accumulation store
 	// all the synthetic lockups' accumulation should be decreased
-	for _, synthlock := range synthLocks {
-		k.accumulationStore(ctx, synthlock.SynthDenom).Decrease(accumulationKey(synthlock.Duration), coins[0].Amount)
-	}
+	k.accumulationStore(ctx, synthLock.SynthDenom).Decrease(accumulationKey(synthLock.Duration), coins[0].Amount)
 	return nil
 }
 

x/lockup/keeper/lock_test.go

@@ -8,6 +8,7 @@ import (
 	cl "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity"
 	cltypes "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/types"
 	"github.com/osmosis-labs/osmosis/v15/x/lockup/types"
+	lockuptypes "github.com/osmosis-labs/osmosis/v15/x/lockup/types"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
@@ -852,11 +853,12 @@ func (s *KeeperTestSuite) AddTokensToLockForSynth() {
 		for i, synthlock := range s.App.LockupKeeper.GetAllSyntheticLockups(s.Ctx) {
 			s.Require().Equal(synthlock, synthlocks[i])
 		}
-		// by GetAllSyntheticLockupsByLockup
+		// by GetSyntheticLockupByUnderlyingLockId
 		for i := uint64(1); i <= 3; i++ {
-			for j, synthlockByLockup := range s.App.LockupKeeper.GetAllSyntheticLockupsByLockup(s.Ctx, i) {
-				s.Require().Equal(synthlockByLockup, synthlocks[(int(i)-1)*3+j])
-			}
+			synthlockByLockup, err := s.App.LockupKeeper.GetSyntheticLockupByUnderlyingLockId(s.Ctx, i)
+			s.Require().NoError(err)
+			s.Require().Equal(synthlockByLockup, synthlocks[(int(i)-1)*3+int(i)])
+
 		}
 		// by GetAllSyntheticLockupsByAddr
 		for i, synthlock := range s.App.LockupKeeper.GetAllSyntheticLockupsByAddr(s.Ctx, addr1) {
@@ -1313,9 +1315,10 @@ func (s *KeeperTestSuite) TestForceUnlock() {
 		s.Require().Equal(balances, coinsToLock)
 
 		// if it was superfluid delegated lock,
-		//  confirm that we don't have associated synth locks
-		synthLocks := s.App.LockupKeeper.GetAllSyntheticLockupsByLockup(s.Ctx, lock.ID)
-		s.Require().Equal(0, len(synthLocks))
+		// confirm that we don't have associated synth lock
+		synthLock, err := s.App.LockupKeeper.GetSyntheticLockupByUnderlyingLockId(s.Ctx, lock.ID)
+		s.Require().NoError(err)
+		s.Require().Equal((lockuptypes.SyntheticLock{}), synthLock)
 
 		// check if lock is deleted by checking trying to get lock ID
 		_, err = s.App.LockupKeeper.GetLockByID(s.Ctx, lock.ID)

x/lockup/keeper/msg_server.go

@@ -208,8 +208,11 @@ func (server msgServer) ForceUnlock(goCtx context.Context, msg *types.MsgForceUn
 	}
 
 	// check that given lock is not superfluid staked
-	synthLocks := server.keeper.GetAllSyntheticLockupsByLockup(ctx, lock.ID)
-	if len(synthLocks) > 0 {
+	synthLock, err := server.keeper.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID)
+	if err != nil {
+		return &types.MsgForceUnlockResponse{Success: false}, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, err.Error())
+	}
+	if !synthLock.IsNil() {
 		return &types.MsgForceUnlockResponse{Success: false}, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "superfluid delegation exists for lock")
 	}
 

x/lockup/keeper/synthetic_lock.go

@@ -36,8 +36,10 @@ func (k Keeper) GetSyntheticLockup(ctx sdk.Context, lockID uint64, synthdenom st
 	return &synthLock, err
 }
 
-// GetAllSyntheticLockupsByLockup gets all the synthetic lockup object of the original lockup.
-func (k Keeper) GetAllSyntheticLockupsByLockup(ctx sdk.Context, lockID uint64) []types.SyntheticLock {
+// Error is returned if:
+// - there are more than one synthetic lockup objects with the same underlying lock ID.
+// - there is no synthetic lockup object with the given underlying lock ID.
+func (k Keeper) GetSyntheticLockupByUnderlyingLockId(ctx sdk.Context, lockID uint64) (types.SyntheticLock, error) {
 	store := ctx.KVStore(k.storeKey)
 	iterator := sdk.KVStorePrefixIterator(store, combineKeys(types.KeyPrefixSyntheticLockup, sdk.Uint64ToBigEndian(lockID)))
 	defer iterator.Close()
@@ -47,19 +49,29 @@ func (k Keeper) GetAllSyntheticLockupsByLockup(ctx sdk.Context, lockID uint64) [
 		synthLock := types.SyntheticLock{}
 		err := proto.Unmarshal(iterator.Value(), &synthLock)
 		if err != nil {
-			panic(err)
+			return types.SyntheticLock{}, err
 		}
 		synthLocks = append(synthLocks, synthLock)
 	}
-	return synthLocks
+	if len(synthLocks) > 1 {
+		return types.SyntheticLock{}, fmt.Errorf("synthetic lockup with same lock id should not exist")
+	}
+	if len(synthLocks) == 0 {
+		return types.SyntheticLock{}, nil
+	}
+	return synthLocks[0], nil
 }
 
 // GetAllSyntheticLockupsByAddr gets all the synthetic lockups from all the locks owned by the given address.
 func (k Keeper) GetAllSyntheticLockupsByAddr(ctx sdk.Context, owner sdk.AccAddress) []types.SyntheticLock {
 	synthLocks := []types.SyntheticLock{}
 	locks := k.GetAccountPeriodLocks(ctx, owner)
 	for _, lock := range locks {
-		synthLocks = append(synthLocks, k.GetAllSyntheticLockupsByLockup(ctx, lock.ID)...)
+		synthLock, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lock.ID)
+		if err != nil {
+			panic(err)
+		}
+		synthLocks = append(synthLocks, synthLock)
 	}
 	return synthLocks
 }
@@ -96,8 +108,11 @@ func (k Keeper) CreateSyntheticLockup(ctx sdk.Context, lockID uint64, synthDenom
 	// There is no relationship between unbonding and bonding synthetic lockup, it's managed separately
 	// A separate accumulation store is incremented with the synth denom.
 
-	_, err := k.GetSyntheticLockup(ctx, lockID, synthDenom)
-	if err == nil {
+	synthLock, err := k.GetSyntheticLockupByUnderlyingLockId(ctx, lockID)
+	if err != nil {
+		return err
+	}
+	if !synthLock.IsNil() {
 		return types.ErrSyntheticLockupAlreadyExists
 	}
 
@@ -115,7 +130,7 @@ func (k Keeper) CreateSyntheticLockup(ctx sdk.Context, lockID uint64, synthDenom
 	}
 
 	// set synthetic lockup object
-	synthLock := types.SyntheticLock{
+	synthLock = types.SyntheticLock{
 		UnderlyingLockId: lockID,
 		SynthDenom:       synthDenom,
 		EndTime:          endTime,
@@ -141,21 +156,6 @@ func (k Keeper) CreateSyntheticLockup(ctx sdk.Context, lockID uint64, synthDenom
 	return nil
 }
 
-// DeleteAllSyntheticLocks iterates over given array of synthetic locks and deletes all individual synthetic locks.
-func (k Keeper) DeleteAllSyntheticLocks(ctx sdk.Context, lock types.PeriodLock, synthLocks []types.SyntheticLock) error {
-	if len(synthLocks) == 0 {
-		return nil
-	}
-
-	for _, synthLock := range synthLocks {
-		err := k.DeleteSyntheticLockup(ctx, lock.ID, synthLock.SynthDenom)
-		if err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 // DeleteSyntheticLockup delete synthetic lockup with lock id and synthdenom.
 // Synthetic lock has three relevant state entries.
 // - synthetic lock object itself