From 2813b7334bca1c1a8dc73567c2e83eaefca1a6c3 Mon Sep 17 00:00:00 2001 From: t4sk Date: Wed, 18 Jan 2023 19:37:28 +0900 Subject: [PATCH 1/5] return lock id from BeginForceUnlock --- x/lockup/keeper/lock.go | 35 +++++++++++++++++--------- x/superfluid/keeper/stake.go | 7 +++++- x/superfluid/keeper/unpool.go | 2 +- x/superfluid/types/expected_keepers.go | 2 +- 4 files changed, 31 insertions(+), 15 deletions(-) diff --git a/x/lockup/keeper/lock.go b/x/lockup/keeper/lock.go index 48ae83172cd..30be8b94f9d 100644 --- a/x/lockup/keeper/lock.go +++ b/x/lockup/keeper/lock.go @@ -172,18 +172,29 @@ 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) + if err != nil { + return err + } + + return nil } // 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 { +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 +202,14 @@ 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 { +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 +218,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 +226,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 +250,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/superfluid/keeper/stake.go b/x/superfluid/keeper/stake.go index 9ee6bb42e85..8690c4c1e92 100644 --- a/x/superfluid/keeper/stake.go +++ b/x/superfluid/keeper/stake.go @@ -284,7 +284,12 @@ 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{}) + if err != nil { + return err + } + + return nil } // 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 74ab68fcb3f..e2dbe627908 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) From c46c23d0d98dd1b4c28b339dccdd317356a6b682 Mon Sep 17 00:00:00 2001 From: t4sk Date: Thu, 19 Jan 2023 09:29:25 +0900 Subject: [PATCH 2/5] refactor and comment to BeginForceUnlock --- x/lockup/keeper/lock.go | 10 ++++------ x/superfluid/keeper/stake.go | 6 +----- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/x/lockup/keeper/lock.go b/x/lockup/keeper/lock.go index 30be8b94f9d..dfe83d114c2 100644 --- a/x/lockup/keeper/lock.go +++ b/x/lockup/keeper/lock.go @@ -173,22 +173,19 @@ func (k Keeper) BeginUnlock(ctx sdk.Context, lockID uint64, coins sdk.Coins) err } _, err = k.beginUnlock(ctx, *lock, coins) - if err != nil { - return err - } - - return nil + 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. +// 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 0, err } - + lockID, err = k.beginUnlock(ctx, *lock, coins) if err != nil { return 0, err @@ -202,6 +199,7 @@ 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. +// 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) { diff --git a/x/superfluid/keeper/stake.go b/x/superfluid/keeper/stake.go index 8690c4c1e92..2d728a7611f 100644 --- a/x/superfluid/keeper/stake.go +++ b/x/superfluid/keeper/stake.go @@ -285,11 +285,7 @@ func (k Keeper) SuperfluidUnbondLock(ctx sdk.Context, underlyingLockId uint64, s return types.ErrBondingLockupNotSupported } _, err = k.lk.BeginForceUnlock(ctx, underlyingLockId, sdk.Coins{}) - if err != nil { - return err - } - - return nil + return err } // alreadySuperfluidStaking returns true if underlying lock used in superfluid staking. From 4346ff2b4f8d69c0da1a65924a5d4a2e75385e44 Mon Sep 17 00:00:00 2001 From: t4sk Date: Thu, 19 Jan 2023 10:09:10 +0900 Subject: [PATCH 3/5] unit test BeginForceUnlock --- x/lockup/keeper/lock_test.go | 46 ++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/x/lockup/keeper/lock_test.go b/x/lockup/keeper/lock_test.go index 24b2cdebbe1..40efe408bcd 100644 --- a/x/lockup/keeper/lock_test.go +++ b/x/lockup/keeper/lock_test.go @@ -45,6 +45,52 @@ func (suite *KeeperTestSuite) TestBeginUnlocking() { // test for all unlockable suite.Require().NotEqual(locks[0].IsUnlocking(), false) } +func (suite *KeeperTestSuite) TestBeginForceUnlock() { + 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---------------")) + coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} + suite.LockTokens(addr1, coins, time.Second) + + // check locks + locks, err = suite.App.LockupKeeper.GetPeriodLocks(suite.Ctx) + suite.Require().NoError(err) + + for _, lock := range locks { + suite.Require().Equal(lock.EndTime, time.Time{}) + suite.Require().Equal(lock.IsUnlocking(), false) + + // test force unlock partial amount + coins = sdk.Coins{} + for _, coin := range lock.Coins { + coins = append(coins, sdk.NewCoin(coin.Denom, coin.Amount.Sub(sdk.NewInt(1)))) + } + + lockID, err := suite.App.LockupKeeper.BeginForceUnlock(suite.Ctx, lock.ID, coins) + suite.Require().NoError(err) + suite.Require().NotEqual(lockID, lock.ID) + + newLock, err := suite.App.LockupKeeper.GetLockByID(suite.Ctx, lockID) + suite.Require().NoError(err) + suite.Require().True(newLock.IsUnlocking()) + + // test force unlock remainder + lockID, err = suite.App.LockupKeeper.BeginForceUnlock(suite.Ctx, lock.ID, sdk.Coins{}) + suite.Require().NoError(err) + suite.Require().Equal(lockID, lock.ID) + + updatedLock, err := suite.App.LockupKeeper.GetLockByID(suite.Ctx, lock.ID) + suite.Require().NoError(err) + suite.Require().True(updatedLock.IsUnlocking()) + } +} + func (suite *KeeperTestSuite) TestGetPeriodLocks() { suite.SetupTest() From 2271e24c40fc733bcb2dbb518acbe98ae60b52ef Mon Sep 17 00:00:00 2001 From: t4sk Date: Fri, 20 Jan 2023 09:26:22 +0900 Subject: [PATCH 4/5] table driven test BeginForceUnlock --- x/lockup/keeper/lock_test.go | 81 +++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 33 deletions(-) diff --git a/x/lockup/keeper/lock_test.go b/x/lockup/keeper/lock_test.go index 40efe408bcd..fc5835e63c1 100644 --- a/x/lockup/keeper/lock_test.go +++ b/x/lockup/keeper/lock_test.go @@ -46,48 +46,63 @@ func (suite *KeeperTestSuite) TestBeginUnlocking() { // test for all unlockable } func (suite *KeeperTestSuite) TestBeginForceUnlock() { - 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---------------")) + // coins to lock coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} - suite.LockTokens(addr1, coins, time.Second) - - // check locks - locks, err = suite.App.LockupKeeper.GetPeriodLocks(suite.Ctx) - suite.Require().NoError(err) - for _, lock := range locks { - suite.Require().Equal(lock.EndTime, time.Time{}) - suite.Require().Equal(lock.IsUnlocking(), false) + 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.Coins{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, + }, + } - // test force unlock partial amount - coins = sdk.Coins{} - for _, coin := range lock.Coins { - coins = append(coins, sdk.NewCoin(coin.Denom, coin.Amount.Sub(sdk.NewInt(1)))) - } + for _, tc := range testCases { + suite.SetupTest() - lockID, err := suite.App.LockupKeeper.BeginForceUnlock(suite.Ctx, lock.ID, coins) + // initial check + locks, err := suite.App.LockupKeeper.GetPeriodLocks(suite.Ctx) suite.Require().NoError(err) - suite.Require().NotEqual(lockID, lock.ID) + suite.Require().Len(locks, 0) - newLock, err := suite.App.LockupKeeper.GetLockByID(suite.Ctx, lockID) - suite.Require().NoError(err) - suite.Require().True(newLock.IsUnlocking()) + // lock coins + addr1 := sdk.AccAddress([]byte("addr1---------------")) + suite.LockTokens(addr1, tc.coins, time.Second) - // test force unlock remainder - lockID, err = suite.App.LockupKeeper.BeginForceUnlock(suite.Ctx, lock.ID, sdk.Coins{}) + // check locks + locks, err = suite.App.LockupKeeper.GetPeriodLocks(suite.Ctx) suite.Require().NoError(err) - suite.Require().Equal(lockID, lock.ID) - updatedLock, err := suite.App.LockupKeeper.GetLockByID(suite.Ctx, lock.ID) - suite.Require().NoError(err) - suite.Require().True(updatedLock.IsUnlocking()) + 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()) + } } } From 362196447b4bfd0e88006ad0a6628e6a94af8c8c Mon Sep 17 00:00:00 2001 From: t4sk Date: Fri, 20 Jan 2023 17:40:45 +0900 Subject: [PATCH 5/5] update unit test for BeginForceUnlock --- x/lockup/keeper/lock_test.go | 59 +++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/x/lockup/keeper/lock_test.go b/x/lockup/keeper/lock_test.go index fc5835e63c1..0b1ee206833 100644 --- a/x/lockup/keeper/lock_test.go +++ b/x/lockup/keeper/lock_test.go @@ -47,7 +47,7 @@ func (suite *KeeperTestSuite) TestBeginUnlocking() { // test for all unlockable func (suite *KeeperTestSuite) TestBeginForceUnlock() { // coins to lock - coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} + coins := sdk.NewCoins(sdk.NewInt64Coin("stake", 10)) testCases := []struct { name string @@ -58,7 +58,7 @@ func (suite *KeeperTestSuite) TestBeginForceUnlock() { { name: "new lock id is returned if the lock was split", coins: coins, - unlockCoins: sdk.Coins{sdk.NewInt64Coin("stake", 1)}, + unlockCoins: sdk.NewCoins(sdk.NewInt64Coin("stake", 1)), expectSameLockID: false, }, { @@ -70,39 +70,42 @@ func (suite *KeeperTestSuite) TestBeginForceUnlock() { } for _, tc := range testCases { - suite.SetupTest() + 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) + // 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) + // 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) + // 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) + 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) + 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) - } + 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()) - } + // new or updated lock + newLock, err := suite.App.LockupKeeper.GetLockByID(suite.Ctx, lockID) + suite.Require().NoError(err) + suite.Require().True(newLock.IsUnlocking()) + } + }) } }