From 95d186d98ad877abecd4c9bad99a951f7c1cfca1 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 23 Jan 2023 09:06:19 +0100 Subject: [PATCH] Return lock id from BeginForceUnlock (#4059) (#4078) * return lock id from BeginForceUnlock * refactor and comment to BeginForceUnlock * unit test BeginForceUnlock * table driven test BeginForceUnlock * update unit test for BeginForceUnlock (cherry picked from commit 6ca0a56c26fd76344a469c541a7115dc330261a7) Co-authored-by: t4sk --- x/lockup/keeper/lock.go | 33 ++++++++----- x/lockup/keeper/lock_test.go | 64 ++++++++++++++++++++++++++ x/superfluid/keeper/stake.go | 3 +- x/superfluid/keeper/unpool.go | 2 +- x/superfluid/types/expected_keepers.go | 2 +- 5 files changed, 89 insertions(+), 15 deletions(-) diff --git a/x/lockup/keeper/lock.go b/x/lockup/keeper/lock.go index 48ae83172cd..dfe83d114c2 100644 --- a/x/lockup/keeper/lock.go +++ b/x/lockup/keeper/lock.go @@ -172,18 +172,26 @@ func (k Keeper) BeginUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) err return fmt.Errorf("cannot BeginUnlocking a lock with synthetic lockup") } - return k.beginUnlock(ctx, *lock, coins) + _, err = k.beginUnlock(ctx, *lock, coins) + return err } // BeginForceUnlock begins force unlock of the given lock. // This method should be called by the superfluid module ONLY, as it does not check whether // the lock has a synthetic lock or not before unlocking. -func (k Keeper) BeginForceUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) error { +// Returns lock id, new lock id if the lock was split, else same lock id. +func (k Keeper) BeginForceUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) (uint64, error) { lock, err := k.GetLockByID(ctx, lockID) if err != nil { - return err + return 0, err + } + + lockID, err = k.beginUnlock(ctx, *lock, coins) + if err != nil { + return 0, err } - return k.beginUnlock(ctx, *lock, coins) + + return lockID, nil } // beginUnlock unlocks specified tokens from the given lock. Existing lock refs @@ -191,14 +199,15 @@ func (k Keeper) BeginForceUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins // EndTime of the lock is set within this method. // Coins provided as the parameter does not require to have all the tokens in the lock, // as we allow partial unlockings of a lock. -func (k Keeper) beginUnlock(ctx sdk.Context, lock types.PeriodLock, coins sdk.Coins) error { +// Returns lock id, new lock id if the lock was split, else same lock id. +func (k Keeper) beginUnlock(ctx sdk.Context, lock types.PeriodLock, coins sdk.Coins) (uint64, error) { // sanity check if !coins.IsAllLTE(lock.Coins) { - return fmt.Errorf("requested amount to unlock exceeds locked tokens") + return 0, fmt.Errorf("requested amount to unlock exceeds locked tokens") } if lock.IsUnlocking() { - return fmt.Errorf("trying to unlock a lock that is already unlocking") + return 0, fmt.Errorf("trying to unlock a lock that is already unlocking") } // If the amount were unlocking is empty, or the entire coins amount, unlock the entire lock. @@ -207,7 +216,7 @@ func (k Keeper) beginUnlock(ctx sdk.Context, lock types.PeriodLock, coins sdk.Co if len(coins) != 0 && !coins.IsEqual(lock.Coins) { splitLock, err := k.splitLock(ctx, lock, coins, false) if err != nil { - return err + return 0, err } lock = splitLock } @@ -215,20 +224,20 @@ func (k Keeper) beginUnlock(ctx sdk.Context, lock types.PeriodLock, coins sdk.Co // remove existing lock refs from not unlocking queue err := k.deleteLockRefs(ctx, types.KeyPrefixNotUnlocking, lock) if err != nil { - return err + return 0, err } // store lock with the end time set to current block time + duration lock.EndTime = ctx.BlockTime().Add(lock.Duration) err = k.setLock(ctx, lock) if err != nil { - return err + return 0, err } // add lock refs into unlocking queue err = k.addLockRefs(ctx, lock) if err != nil { - return err + return 0, err } if k.hooks != nil { @@ -239,7 +248,7 @@ func (k Keeper) beginUnlock(ctx sdk.Context, lock types.PeriodLock, coins sdk.Co createBeginUnlockEvent(&lock), }) - return nil + return lock.ID, nil } func (k Keeper) clearKeysByPrefix(ctx sdk.Context, prefix []byte) { diff --git a/x/lockup/keeper/lock_test.go b/x/lockup/keeper/lock_test.go index 24b2cdebbe1..0b1ee206833 100644 --- a/x/lockup/keeper/lock_test.go +++ b/x/lockup/keeper/lock_test.go @@ -45,6 +45,70 @@ func (suite *KeeperTestSuite) TestBeginUnlocking() { // test for all unlockable suite.Require().NotEqual(locks[0].IsUnlocking(), false) } +func (suite *KeeperTestSuite) TestBeginForceUnlock() { + // coins to lock + coins := sdk.NewCoins(sdk.NewInt64Coin("stake", 10)) + + testCases := []struct { + name string + coins sdk.Coins + unlockCoins sdk.Coins + expectSameLockID bool + }{ + { + name: "new lock id is returned if the lock was split", + coins: coins, + unlockCoins: sdk.NewCoins(sdk.NewInt64Coin("stake", 1)), + expectSameLockID: false, + }, + { + name: "same lock id is returned if the lock was not split", + coins: coins, + unlockCoins: sdk.Coins{}, + expectSameLockID: true, + }, + } + + for _, tc := range testCases { + suite.Run(tc.name, func() { + suite.SetupTest() + + // initial check + locks, err := suite.App.LockupKeeper.GetPeriodLocks(suite.Ctx) + suite.Require().NoError(err) + suite.Require().Len(locks, 0) + + // lock coins + addr1 := sdk.AccAddress([]byte("addr1---------------")) + suite.LockTokens(addr1, tc.coins, time.Second) + + // check locks + locks, err = suite.App.LockupKeeper.GetPeriodLocks(suite.Ctx) + suite.Require().NoError(err) + suite.Require().True(len(locks) > 0) + + for _, lock := range locks { + suite.Require().Equal(lock.EndTime, time.Time{}) + suite.Require().Equal(lock.IsUnlocking(), false) + + lockID, err := suite.App.LockupKeeper.BeginForceUnlock(suite.Ctx, lock.ID, tc.unlockCoins) + suite.Require().NoError(err) + + if tc.expectSameLockID { + suite.Require().Equal(lockID, lock.ID) + } else { + suite.Require().Equal(lockID, lock.ID + 1) + } + + // new or updated lock + newLock, err := suite.App.LockupKeeper.GetLockByID(suite.Ctx, lockID) + suite.Require().NoError(err) + suite.Require().True(newLock.IsUnlocking()) + } + }) + } +} + func (suite *KeeperTestSuite) TestGetPeriodLocks() { suite.SetupTest() diff --git a/x/superfluid/keeper/stake.go b/x/superfluid/keeper/stake.go index 9ee6bb42e85..2d728a7611f 100644 --- a/x/superfluid/keeper/stake.go +++ b/x/superfluid/keeper/stake.go @@ -284,7 +284,8 @@ func (k Keeper) SuperfluidUnbondLock(ctx sdk.Context, underlyingLockId uint64, s if !synthLocks[0].IsUnlocking() { return types.ErrBondingLockupNotSupported } - return k.lk.BeginForceUnlock(ctx, underlyingLockId, sdk.Coins{}) + _, err = k.lk.BeginForceUnlock(ctx, underlyingLockId, sdk.Coins{}) + return err } // alreadySuperfluidStaking returns true if underlying lock used in superfluid staking. diff --git a/x/superfluid/keeper/unpool.go b/x/superfluid/keeper/unpool.go index 1fef122fcd2..627eb3769f6 100644 --- a/x/superfluid/keeper/unpool.go +++ b/x/superfluid/keeper/unpool.go @@ -77,7 +77,7 @@ func (k Keeper) UnpoolAllowedPools(ctx sdk.Context, sender sdk.AccAddress, poolI // 8) Begin unlocking every new lock for _, newLock := range newLocks { - err = k.lk.BeginForceUnlock(ctx, newLock.ID, newLock.Coins) + _, err = k.lk.BeginForceUnlock(ctx, newLock.ID, newLock.Coins) if err != nil { return []uint64{}, err } diff --git a/x/superfluid/types/expected_keepers.go b/x/superfluid/types/expected_keepers.go index 75285745227..4c453264721 100644 --- a/x/superfluid/types/expected_keepers.go +++ b/x/superfluid/types/expected_keepers.go @@ -24,7 +24,7 @@ type LockupKeeper interface { GetLockByID(ctx sdk.Context, lockID uint64) (*lockuptypes.PeriodLock, error) // Despite the name, BeginForceUnlock is really BeginUnlock // TODO: Fix this in future code update - BeginForceUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) error + BeginForceUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) (uint64, error) ForceUnlock(ctx sdk.Context, lock lockuptypes.PeriodLock) error CreateLock(ctx sdk.Context, owner sdk.AccAddress, coins sdk.Coins, duration time.Duration) (lockuptypes.PeriodLock, error)