Add unlocking lock id to BeginUnlocking response (backport #4489) (#4495

) Co-authored-by: Roman <roman@osmosis.team>
osmosis-labs · Mar 3, 2023 · 3ffb10f · 3ffb10f
1 parent 7a0f7ee
commit 3ffb10f
Show file tree

Hide file tree

Showing 12 changed files with 113 additions and 58 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -69,6 +69,7 @@ This release containts the following new modules:
 * [#3905](https://github.com/osmosis-labs/osmosis/pull/3905) Deprecate gamm queries `NumPools`, `EstimateSwapExactAmountIn` and `EstimateSwapExactAmountOut`.
 * [#3907](https://github.com/osmosis-labs/osmosis/pull/3907) Add `NumPools`, `EstimateSwapExactAmountIn` and `EstimateSwapExactAmountOut` query in poolmanager module to stargate whitelist.
 * [#3880](https://github.com/osmosis-labs/osmosis/pull/3880) Switch usage of proto-generated SwapAmountInRoute and SwapAmountOutRoute in x/gamm to import the structs from x/poolmanager module.
+ * [#4489](https://github.com/osmosis-labs/osmosis/pull/4489) Add unlockingLockId to BeginUnlocking response
 
 ### Bug Fix
 

diff --git a/proto/osmosis/lockup/tx.proto b/proto/osmosis/lockup/tx.proto
@@ -51,7 +51,10 @@ message MsgBeginUnlocking {
     (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"
   ];
 }
-message MsgBeginUnlockingResponse { bool success = 1; }
+message MsgBeginUnlockingResponse {
+  bool success = 1;
+  uint64 unlockingLockID = 2;
+}
 
 // MsgExtendLockup extends the existing lockup's duration.
 // The new duration is longer than the original.

diff --git a/x/lockup/keeper/iterator.go b/x/lockup/keeper/iterator.go
@@ -218,7 +218,7 @@ func (k Keeper) beginUnlockFromIterator(ctx sdk.Context, iterator db.Iterator) (
 
 	locks := k.getLocksFromIterator(ctx, iterator)
 	for _, lock := range locks {
-		err := k.BeginUnlock(ctx, lock.ID, nil)
+		_, err := k.BeginUnlock(ctx, lock.ID, nil)
 		if err != nil {
 			return locks, err
 		}

diff --git a/x/lockup/keeper/lock.go b/x/lockup/keeper/lock.go
@@ -161,19 +161,19 @@ func (k Keeper) lock(ctx sdk.Context, lock types.PeriodLock, tokensToLock sdk.Co
 
 // BeginUnlock is a utility to start unlocking coins from NotUnlocking queue.
 // Returns an error if the lock has a synthetic lock.
-func (k Keeper) BeginUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) error {
+func (k Keeper) BeginUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) (uint64, error) {
 	// prohibit BeginUnlock if synthetic locks are referring to this
 	// TODO: In the future, make synthetic locks only get partial restrictions on the main lock.
 	lock, err := k.GetLockByID(ctx, lockID)
 	if err != nil {
-		return err
+		return 0, err
 	}
 	if k.HasAnySyntheticLockups(ctx, lock.ID) {
-		return fmt.Errorf("cannot BeginUnlocking a lock with synthetic lockup")
+		return 0, fmt.Errorf("cannot BeginUnlocking a lock with synthetic lockup")
 	}
 
-	_, err = k.beginUnlock(ctx, *lock, coins)
-	return err
+	unlockingLock, err := k.beginUnlock(ctx, *lock, coins)
+	return unlockingLock, err
 }
 
 // BeginForceUnlock begins force unlock of the given lock.
@@ -356,7 +356,7 @@ func (k Keeper) ForceUnlock(ctx sdk.Context, lock types.PeriodLock) error {
 	}
 
 	if !lock.IsUnlocking() {
-		err := k.BeginUnlock(ctx, lock.ID, nil)
+		_, err := k.BeginUnlock(ctx, lock.ID, nil)
 		if err != nil {
 			return err
 		}

diff --git a/x/lockup/keeper/lock_test.go b/x/lockup/keeper/lock_test.go
@@ -97,7 +97,7 @@ func (suite *KeeperTestSuite) TestBeginForceUnlock() {
 				if tc.expectSameLockID {
 					suite.Require().Equal(lockID, lock.ID)
 				} else {
-					suite.Require().Equal(lockID, lock.ID + 1)
+					suite.Require().Equal(lockID, lock.ID+1)
 				}
 
 				// new or updated lock
@@ -139,6 +139,7 @@ func (suite *KeeperTestSuite) TestUnlock() {
 		passedTime                    time.Duration
 		expectedUnlockMaturedLockPass bool
 		balanceAfterUnlock            sdk.Coins
+		isPartial                     bool
 	}{
 		{
 			name:                          "normal unlocking case",
@@ -177,6 +178,7 @@ func (suite *KeeperTestSuite) TestUnlock() {
 			passedTime:                    time.Second,
 			expectedUnlockMaturedLockPass: true,
 			balanceAfterUnlock:            sdk.Coins{sdk.NewInt64Coin("stake", 5)},
+			isPartial:                     true,
 		},
 		{
 			name:                          "partial unlocking unknown tokens",
@@ -213,11 +215,15 @@ func (suite *KeeperTestSuite) TestUnlock() {
 		partialUnlocking := tc.unlockingCoins.IsAllLT(initialLockCoins) && tc.unlockingCoins != nil
 
 		// begin unlocking
-		err = lockupKeeper.BeginUnlock(ctx, lock.ID, tc.unlockingCoins)
+		unlockingLock, err := lockupKeeper.BeginUnlock(ctx, lock.ID, tc.unlockingCoins)
 
 		if tc.expectedBeginUnlockPass {
 			suite.Require().NoError(err)
 
+			if tc.isPartial {
+				suite.Require().Equal(unlockingLock, lock.ID+1)
+			}
+
 			// check unlocking coins. When a lock is a partial lock
 			// (i.e. tc.unlockingCoins is not nit and less than initialLockCoins),
 			// we only unlock the partial amount of tc.unlockingCoins

diff --git a/x/lockup/keeper/msg_server.go b/x/lockup/keeper/msg_server.go
@@ -91,14 +91,14 @@ func (server msgServer) BeginUnlocking(goCtx context.Context, msg *types.MsgBegi
 		return nil, sdkerrors.Wrap(types.ErrNotLockOwner, fmt.Sprintf("msg sender (%s) and lock owner (%s) does not match", msg.Owner, lock.Owner))
 	}
 
-	err = server.keeper.BeginUnlock(ctx, lock.ID, msg.Coins)
+	unlockingLock, err := server.keeper.BeginUnlock(ctx, lock.ID, msg.Coins)
 	if err != nil {
 		return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error())
 	}
 
 	// N.B. begin unlock event is emitted downstream in the keeper method.
 
-	return &types.MsgBeginUnlockingResponse{}, nil
+	return &types.MsgBeginUnlockingResponse{Success: true, UnlockingLockID: unlockingLock}, nil
 }
 
 // BeginUnlockingAll begins unlocking for all the locks that the account has by iterating all the not-unlocking locks the account holds.

diff --git a/x/lockup/keeper/msg_server_test.go b/x/lockup/keeper/msg_server_test.go
@@ -115,6 +115,7 @@ func (suite *KeeperTestSuite) TestMsgBeginUnlocking() {
 		name       string
 		param      param
 		expectPass bool
+		isPartial  bool
 	}{
 		{
 			name: "unlock full amount of tokens via begin unlock",
@@ -138,6 +139,7 @@ func (suite *KeeperTestSuite) TestMsgBeginUnlocking() {
 				duration:            time.Second,
 				coinsInOwnerAddress: sdk.Coins{sdk.NewInt64Coin("stake", 10)},
 			},
+			isPartial:  true,
 			expectPass: true,
 		},
 		{
@@ -163,6 +165,7 @@ func (suite *KeeperTestSuite) TestMsgBeginUnlocking() {
 				coinsInOwnerAddress: sdk.Coins{sdk.NewInt64Coin("stake", 10)},
 			},
 			expectPass: false,
+			isPartial:  true,
 		},
 	}
 
@@ -181,11 +184,17 @@ func (suite *KeeperTestSuite) TestMsgBeginUnlocking() {
 			suite.Require().NoError(err)
 		}
 
-		_, err = msgServer.BeginUnlocking(goCtx, types.NewMsgBeginUnlocking(test.param.lockOwner, resp.ID, test.param.coinsToUnlock))
+		unlockingResponse, err := msgServer.BeginUnlocking(goCtx, types.NewMsgBeginUnlocking(test.param.lockOwner, resp.ID, test.param.coinsToUnlock))
 
 		if test.expectPass {
 			suite.Require().NoError(err)
 			suite.AssertEventEmitted(suite.Ctx, types.TypeEvtBeginUnlock, 1)
+			suite.Require().True(unlockingResponse.Success)
+			if test.isPartial {
+				suite.Require().Equal(unlockingResponse.UnlockingLockID, resp.ID+1)
+			} else {
+				suite.Require().Equal(unlockingResponse.UnlockingLockID, resp.ID)
+			}
 		} else {
 			suite.Require().Error(err)
 			suite.AssertEventEmitted(suite.Ctx, types.TypeEvtBeginUnlock, 0)

diff --git a/x/lockup/types/tx.pb.go b/x/lockup/types/tx.pb.go
diff --git a/x/superfluid/keeper/edge_case_test.go b/x/superfluid/keeper/edge_case_test.go
@@ -136,7 +136,7 @@ func (suite *KeeperTestSuite) TestTryUnbondingSuperfluidLockupDirectly() {
 			_, _, locks := suite.setupSuperfluidDelegations(valAddrs, tc.superDelegations, denoms)
 
 			for _, lock := range locks {
-				err := suite.App.LockupKeeper.BeginUnlock(suite.Ctx, lock.ID, sdk.Coins{})
+				_, err := suite.App.LockupKeeper.BeginUnlock(suite.Ctx, lock.ID, sdk.Coins{})
 				suite.Require().Error(err)
 			}
 		})

diff --git a/x/superfluid/keeper/unpool_test.go b/x/superfluid/keeper/unpool_test.go
@@ -176,7 +176,7 @@ func (suite *KeeperTestSuite) TestUnpool() {
 				} else {
 					lock, err := lockupKeeper.GetLockByID(ctx, lockID)
 					suite.Require().NoError(err)
-					err = lockupKeeper.BeginUnlock(ctx, lockID, lock.Coins)
+					_, err = lockupKeeper.BeginUnlock(ctx, lockID, lock.Coins)
 					suite.Require().NoError(err)
 
 					// add time to current time to test lock end time

diff --git a/x/valset-pref/keeper_test.go b/x/valset-pref/keeper_test.go
@@ -246,7 +246,7 @@ func (suite *KeeperTestSuite) SetupLocks(delegator sdk.AccAddress) []lockuptypes
 	unbondingLocks, err := suite.App.LockupKeeper.CreateLock(suite.Ctx, delegator, osmoToLock, twoWeekDuration)
 	suite.Require().NoError(err)
 
-	err = suite.App.LockupKeeper.BeginUnlock(suite.Ctx, unbondingLocks.ID, nil)
+	_, err = suite.App.LockupKeeper.BeginUnlock(suite.Ctx, unbondingLocks.ID, nil)
 	suite.Require().NoError(err)
 
 	locks = append(locks, unbondingLocks)

diff --git a/x/valset-pref/types/expected_interfaces.go b/x/valset-pref/types/expected_interfaces.go
@@ -35,6 +35,6 @@ type LockupKeeper interface {
 	GetLockByID(ctx sdk.Context, lockID uint64) (*lockuptypes.PeriodLock, error)
 	GetAllSyntheticLockupsByLockup(ctx sdk.Context, lockID uint64) []lockuptypes.SyntheticLock
 	ForceUnlock(ctx sdk.Context, lock lockuptypes.PeriodLock) error
-	BeginUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) error
+	BeginUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) (uint64, error)
 	GetPeriodLocks(ctx sdk.Context) ([]lockuptypes.PeriodLock, error)
 }