From 639178c1512bef4ae1817265c73d6d7ce1c434b6 Mon Sep 17 00:00:00 2001 From: mattverse Date: Fri, 1 Jul 2022 14:16:46 +0900 Subject: [PATCH 1/5] Add lock method refactor --- x/lockup/keeper/admin_keeper_test.go | 8 ++- x/lockup/keeper/lock.go | 74 ++++++++++------------------ x/lockup/keeper/lock_test.go | 59 +++++++++++----------- 3 files changed, 57 insertions(+), 84 deletions(-) diff --git a/x/lockup/keeper/admin_keeper_test.go b/x/lockup/keeper/admin_keeper_test.go index 9829c31635c..9d4d6a083bf 100644 --- a/x/lockup/keeper/admin_keeper_test.go +++ b/x/lockup/keeper/admin_keeper_test.go @@ -4,7 +4,6 @@ import ( "time" "github.com/osmosis-labs/osmosis/v7/x/lockup/keeper" - "github.com/osmosis-labs/osmosis/v7/x/lockup/types" sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -14,11 +13,10 @@ func (suite *KeeperTestSuite) TestRelock() { addr1 := sdk.AccAddress([]byte("addr1---------------")) coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} - lock := types.NewPeriodLock(1, addr1, time.Second, suite.Ctx.BlockTime().Add(time.Second), coins) // lock with balance suite.FundAcc(addr1, coins) - err := suite.App.LockupKeeper.Lock(suite.Ctx, lock) + lock, err := suite.App.LockupKeeper.CreateLock(suite.Ctx, addr1, coins, time.Second) suite.Require().NoError(err) // lock with balance with same id @@ -38,12 +36,12 @@ func (suite *KeeperTestSuite) BreakLock() { addr1 := sdk.AccAddress([]byte("addr1---------------")) coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} - lock := types.NewPeriodLock(1, addr1, time.Second, suite.Ctx.BlockTime().Add(time.Second), coins) // lock with balance suite.FundAcc(addr1, coins) - err := suite.App.LockupKeeper.Lock(suite.Ctx, lock) + lock, err := suite.App.LockupKeeper.CreateLock(suite.Ctx, addr1, coins, time.Second) + suite.Require().NoError(err) // break lock diff --git a/x/lockup/keeper/lock.go b/x/lockup/keeper/lock.go index 052a09e8905..62118ce067e 100644 --- a/x/lockup/keeper/lock.go +++ b/x/lockup/keeper/lock.go @@ -66,100 +66,76 @@ func (k Keeper) AddToExistingLock(ctx sdk.Context, owner sdk.AccAddress, coin sd // Tokens locked are sent and kept in the module account. // This method alters the lock state in store, thus we do a sanity check to ensure // lock owner matches the given owner. -func (k Keeper) AddTokensToLockByID(ctx sdk.Context, lockID uint64, owner sdk.AccAddress, coin sdk.Coin) (*types.PeriodLock, error) { +func (k Keeper) AddTokensToLockByID(ctx sdk.Context, lockID uint64, owner sdk.AccAddress, tokensToAdd sdk.Coin) (*types.PeriodLock, error) { lock, err := k.GetLockByID(ctx, lockID) + if err != nil { + return nil, err + } if lock.GetOwner() != owner.String() { return nil, types.ErrNotLockOwner } + lock.Coins = lock.Coins.Add(tokensToAdd) + err = k.lock(ctx, *lock, sdk.NewCoins(tokensToAdd)) if err != nil { return nil, err } - if err := k.bk.SendCoinsFromAccountToModule(ctx, lock.OwnerAddress(), types.ModuleName, sdk.NewCoins(coin)); err != nil { - return nil, err - } - err = k.addTokenToLock(ctx, lock, coin) - if err != nil { - return nil, err + for _, synthlock := range k.GetAllSyntheticLockupsByLockup(ctx, lock.ID) { + k.accumulationStore(ctx, synthlock.SynthDenom).Increase(accumulationKey(synthlock.Duration), tokensToAdd.Amount) } if k.hooks == nil { return lock, nil } + k.hooks.AfterAddTokensToLock(ctx, lock.OwnerAddress(), lock.GetID(), sdk.NewCoins(tokensToAdd)) - k.hooks.OnTokenLocked(ctx, lock.OwnerAddress(), lock.ID, sdk.Coins{coin}, lock.Duration, lock.EndTime) return lock, nil } -// addTokenToLock adds token to lock and modifies the state of the lock and the accumulation store. -func (k Keeper) addTokenToLock(ctx sdk.Context, lock *types.PeriodLock, coin sdk.Coin) error { - lock.Coins = lock.Coins.Add(coin) - - err := k.setLock(ctx, *lock) - if err != nil { - return err - } - - // modifications to accumulation store - k.accumulationStore(ctx, coin.Denom).Increase(accumulationKey(lock.Duration), coin.Amount) - - // CONTRACT: lock will have synthetic lock only if it has a single coin - // accumulation store for its synthetic denom is increased if exists. - lockedCoin, err := lock.SingleCoin() - if err == nil { - for _, synthlock := range k.GetAllSyntheticLockupsByLockup(ctx, lock.ID) { - k.accumulationStore(ctx, synthlock.SynthDenom).Increase(accumulationKey(synthlock.Duration), sdk.NewCoins(coin).AmountOf(lockedCoin.Denom)) - } - } - - k.hooks.AfterAddTokensToLock(ctx, lock.OwnerAddress(), lock.GetID(), sdk.NewCoins(coin)) - - return nil -} - // CreateLock creates a new lock with the specified duration for the owner. func (k Keeper) CreateLock(ctx sdk.Context, owner sdk.AccAddress, coins sdk.Coins, duration time.Duration) (types.PeriodLock, error) { ID := k.GetLastLockID(ctx) + 1 // unlock time is initially set without a value, gets set as unlock start time + duration // when unlocking starts. lock := types.NewPeriodLock(ID, owner, duration, time.Time{}, coins) - err := k.Lock(ctx, lock) + err := k.lock(ctx, lock, lock.Coins) + if err != nil { + return lock, err + } + + // add lock refs into not unlocking queue + err = k.addLockRefs(ctx, lock) if err != nil { return lock, err } + k.SetLastLockID(ctx, lock.ID) return lock, nil } -// Lock is a utility method to lock tokens into the module account. This method includes setting the -// lock within the state machine and increasing the value of accumulation store. -func (k Keeper) Lock(ctx sdk.Context, lock types.PeriodLock) error { +// lock is an internal utility to lock coins and set corresponding states. +// This is only called by either of the two possible entry points to lock tokens. +// 1. CreateLock +// 2. AddTokensToLockByID +func (k Keeper) lock(ctx sdk.Context, lock types.PeriodLock, tokensToLock sdk.Coins) error { owner, err := sdk.AccAddressFromBech32(lock.Owner) if err != nil { return err } - if err := k.bk.SendCoinsFromAccountToModule(ctx, owner, types.ModuleName, lock.Coins); err != nil { + if err := k.bk.SendCoinsFromAccountToModule(ctx, owner, types.ModuleName, tokensToLock); err != nil { return err } // store lock object into the store - store := ctx.KVStore(k.storeKey) - bz, err := proto.Marshal(&lock) - if err != nil { - return err - } - store.Set(lockStoreKey(lock.ID), bz) - - // add lock refs into not unlocking queue - err = k.addLockRefs(ctx, lock) + err = k.setLock(ctx, lock) if err != nil { return err } // add to accumulation store - for _, coin := range lock.Coins { + for _, coin := range tokensToLock { k.accumulationStore(ctx, coin.Denom).Increase(accumulationKey(lock.Duration), coin.Amount) } diff --git a/x/lockup/keeper/lock_test.go b/x/lockup/keeper/lock_test.go index 5afecbd678e..da3563fa88b 100644 --- a/x/lockup/keeper/lock_test.go +++ b/x/lockup/keeper/lock_test.go @@ -144,34 +144,34 @@ func (suite *KeeperTestSuite) TestUnlockPeriodLockByID() { suite.Require().Len(locks, 0) } -func (suite *KeeperTestSuite) TestLock() { - // test for coin locking - suite.SetupTest() - - addr1 := sdk.AccAddress([]byte("addr1---------------")) - coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} - lock := types.NewPeriodLock(1, addr1, time.Second, suite.Ctx.BlockTime().Add(time.Second), coins) - - // try lock without balance - err := suite.App.LockupKeeper.Lock(suite.Ctx, lock) - suite.Require().Error(err) - - // lock with balance - suite.FundAcc(addr1, coins) - err = suite.App.LockupKeeper.Lock(suite.Ctx, lock) - suite.Require().NoError(err) - - // lock with balance with same id - suite.FundAcc(addr1, coins) - err = suite.App.LockupKeeper.Lock(suite.Ctx, lock) - suite.Require().Error(err) - - // lock with balance with different id - lock = types.NewPeriodLock(2, addr1, time.Second, suite.Ctx.BlockTime().Add(time.Second), coins) - suite.FundAcc(addr1, coins) - err = suite.App.LockupKeeper.Lock(suite.Ctx, lock) - suite.Require().NoError(err) -} +// func (suite *KeeperTestSuite) TestLock() { +// // test for coin locking +// suite.SetupTest() + +// addr1 := sdk.AccAddress([]byte("addr1---------------")) +// coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} +// lock := types.NewPeriodLock(1, addr1, time.Second, suite.Ctx.BlockTime().Add(time.Second), coins) + +// // try lock without balance +// err := suite.App.LockupKeeper.Lock(suite.Ctx, lock) +// suite.Require().Error(err) + +// // lock with balance +// suite.FundAcc(addr1, coins) +// err = suite.App.LockupKeeper.Lock(suite.Ctx, lock) +// suite.Require().NoError(err) + +// // lock with balance with same id +// suite.FundAcc(addr1, coins) +// err = suite.App.LockupKeeper.Lock(suite.Ctx, lock) +// suite.Require().Error(err) + +// // lock with balance with different id +// lock = types.NewPeriodLock(2, addr1, time.Second, suite.Ctx.BlockTime().Add(time.Second), coins) +// suite.FundAcc(addr1, coins) +// err = suite.App.LockupKeeper.Lock(suite.Ctx, lock) +// suite.Require().NoError(err) +// } func (suite *KeeperTestSuite) TestUnlock() { // test for coin unlocking @@ -180,11 +180,10 @@ func (suite *KeeperTestSuite) TestUnlock() { addr1 := sdk.AccAddress([]byte("addr1---------------")) coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} - lock := types.NewPeriodLock(1, addr1, time.Second, time.Time{}, coins) // lock with balance suite.FundAcc(addr1, coins) - err := suite.App.LockupKeeper.Lock(suite.Ctx, lock) + lock, err := suite.App.LockupKeeper.CreateLock(suite.Ctx, addr1, coins, time.Second) suite.Require().NoError(err) // begin unlock with lock object From c9bfaebb338360ec52a8ae7b1b4ef73f10329922 Mon Sep 17 00:00:00 2001 From: mattverse Date: Fri, 1 Jul 2022 14:31:42 +0900 Subject: [PATCH 2/5] Delete duplciated testing --- x/lockup/keeper/lock_test.go | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/x/lockup/keeper/lock_test.go b/x/lockup/keeper/lock_test.go index da3563fa88b..fd416b4f66e 100644 --- a/x/lockup/keeper/lock_test.go +++ b/x/lockup/keeper/lock_test.go @@ -144,35 +144,6 @@ func (suite *KeeperTestSuite) TestUnlockPeriodLockByID() { suite.Require().Len(locks, 0) } -// func (suite *KeeperTestSuite) TestLock() { -// // test for coin locking -// suite.SetupTest() - -// addr1 := sdk.AccAddress([]byte("addr1---------------")) -// coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} -// lock := types.NewPeriodLock(1, addr1, time.Second, suite.Ctx.BlockTime().Add(time.Second), coins) - -// // try lock without balance -// err := suite.App.LockupKeeper.Lock(suite.Ctx, lock) -// suite.Require().Error(err) - -// // lock with balance -// suite.FundAcc(addr1, coins) -// err = suite.App.LockupKeeper.Lock(suite.Ctx, lock) -// suite.Require().NoError(err) - -// // lock with balance with same id -// suite.FundAcc(addr1, coins) -// err = suite.App.LockupKeeper.Lock(suite.Ctx, lock) -// suite.Require().Error(err) - -// // lock with balance with different id -// lock = types.NewPeriodLock(2, addr1, time.Second, suite.Ctx.BlockTime().Add(time.Second), coins) -// suite.FundAcc(addr1, coins) -// err = suite.App.LockupKeeper.Lock(suite.Ctx, lock) -// suite.Require().NoError(err) -// } - func (suite *KeeperTestSuite) TestUnlock() { // test for coin unlocking suite.SetupTest() From d0cfc3167e2483d76cb0e4b6d9926f4d85b0c00b Mon Sep 17 00:00:00 2001 From: "Matt, Park" <45252226+mattverse@users.noreply.github.com> Date: Mon, 4 Jul 2022 11:55:57 +0900 Subject: [PATCH 3/5] Update x/lockup/keeper/lock.go Co-authored-by: Aleksandr Bezobchuk --- x/lockup/keeper/lock.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/lockup/keeper/lock.go b/x/lockup/keeper/lock.go index 62118ce067e..3cd048eae44 100644 --- a/x/lockup/keeper/lock.go +++ b/x/lockup/keeper/lock.go @@ -89,6 +89,7 @@ func (k Keeper) AddTokensToLockByID(ctx sdk.Context, lockID uint64, owner sdk.Ac if k.hooks == nil { return lock, nil } + k.hooks.AfterAddTokensToLock(ctx, lock.OwnerAddress(), lock.GetID(), sdk.NewCoins(tokensToAdd)) return lock, nil From 64132438ea328789bd8ae71e24a8a1e832f5a8ee Mon Sep 17 00:00:00 2001 From: mattverse Date: Mon, 4 Jul 2022 12:47:01 +0900 Subject: [PATCH 4/5] Add tests implement feedback from code review --- x/lockup/keeper/lock.go | 2 ++ x/lockup/keeper/lock_test.go | 51 ++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/x/lockup/keeper/lock.go b/x/lockup/keeper/lock.go index 62118ce067e..d1e1848130d 100644 --- a/x/lockup/keeper/lock.go +++ b/x/lockup/keeper/lock.go @@ -95,6 +95,8 @@ func (k Keeper) AddTokensToLockByID(ctx sdk.Context, lockID uint64, owner sdk.Ac } // CreateLock creates a new lock with the specified duration for the owner. +// Returns an error in the following conditions: +// - account does not have enough balance func (k Keeper) CreateLock(ctx sdk.Context, owner sdk.AccAddress, coins sdk.Coins, duration time.Duration) (types.PeriodLock, error) { ID := k.GetLastLockID(ctx) + 1 // unlock time is initially set without a value, gets set as unlock start time + duration diff --git a/x/lockup/keeper/lock_test.go b/x/lockup/keeper/lock_test.go index fd416b4f66e..91f8b99cd0e 100644 --- a/x/lockup/keeper/lock_test.go +++ b/x/lockup/keeper/lock_test.go @@ -290,6 +290,57 @@ func (suite *KeeperTestSuite) TestLocksLongerThanDurationDenom() { suite.Require().Len(locks, 1) } +func (suite *KeeperTestSuite) TestCreateLock() { + suite.SetupTest() + + addr1 := sdk.AccAddress([]byte("addr1---------------")) + coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} + + // test locking without balance + _, err := suite.App.LockupKeeper.CreateLock(suite.Ctx, addr1, coins, time.Second) + suite.Require().Error(err) + + suite.FundAcc(addr1, coins) + + lock, err := suite.App.LockupKeeper.CreateLock(suite.Ctx, addr1, coins, time.Second) + suite.Require().NoError(err) + + // check new lock + suite.Require().Equal(coins, lock.Coins) + suite.Require().Equal(time.Second, lock.Duration) + suite.Require().Equal(time.Time{}, lock.EndTime) + suite.Require().Equal(uint64(1), lock.ID) + + lockID := suite.App.LockupKeeper.GetLastLockID(suite.Ctx) + suite.Require().Equal(uint64(1), lockID) + + // check accumulation store + accum := suite.App.LockupKeeper.GetPeriodLocksAccumulation(suite.Ctx, types.QueryCondition{ + LockQueryType: types.ByDuration, + Denom: "stake", + Duration: time.Second, + }) + suite.Require().Equal(accum.String(), "10") + + // create new lock + coins = sdk.Coins{sdk.NewInt64Coin("stake", 20)} + suite.FundAcc(addr1, coins) + + lock, err = suite.App.LockupKeeper.CreateLock(suite.Ctx, addr1, coins, time.Second) + suite.Require().NoError(err) + + lockID = suite.App.LockupKeeper.GetLastLockID(suite.Ctx) + suite.Require().Equal(uint64(2), lockID) + + // check accumulation store + accum = suite.App.LockupKeeper.GetPeriodLocksAccumulation(suite.Ctx, types.QueryCondition{ + LockQueryType: types.ByDuration, + Denom: "stake", + Duration: time.Second, + }) + suite.Require().Equal(accum.String(), "30") +} + func (suite *KeeperTestSuite) TestAddTokensToLock() { suite.SetupTest() From 73643bd6e568f34706bae13a46cbe2bb2f6229df Mon Sep 17 00:00:00 2001 From: mattverse Date: Mon, 4 Jul 2022 14:15:19 +0900 Subject: [PATCH 5/5] Add test cases --- x/lockup/keeper/export_test.go | 4 +++ x/lockup/keeper/lock_test.go | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/x/lockup/keeper/export_test.go b/x/lockup/keeper/export_test.go index 97dfe9579cf..7a0a28272e1 100644 --- a/x/lockup/keeper/export_test.go +++ b/x/lockup/keeper/export_test.go @@ -25,3 +25,7 @@ func (k Keeper) SyntheticCoins(coins sdk.Coins, suffix string) sdk.Coins { func (k Keeper) GetCoinsFromLocks(locks []types.PeriodLock) sdk.Coins { return k.getCoinsFromLocks(locks) } + +func (k Keeper) Lock(ctx sdk.Context, lock types.PeriodLock, tokensToLock sdk.Coins) error { + return k.lock(ctx, lock, tokensToLock) +} diff --git a/x/lockup/keeper/lock_test.go b/x/lockup/keeper/lock_test.go index 91f8b99cd0e..8ef1c9eaf23 100644 --- a/x/lockup/keeper/lock_test.go +++ b/x/lockup/keeper/lock_test.go @@ -339,6 +339,14 @@ func (suite *KeeperTestSuite) TestCreateLock() { Duration: time.Second, }) suite.Require().Equal(accum.String(), "30") + + // check balance + balance := suite.App.BankKeeper.GetBalance(suite.Ctx, addr1, "stake") + suite.Require().Equal(sdk.ZeroInt(), balance.Amount) + + acc := suite.App.AccountKeeper.GetModuleAccount(suite.Ctx, types.ModuleName) + balance = suite.App.BankKeeper.GetBalance(suite.Ctx, acc.GetAddress(), "stake") + suite.Require().Equal(sdk.NewInt(30), balance.Amount) } func (suite *KeeperTestSuite) TestAddTokensToLock() { @@ -402,6 +410,52 @@ func (suite *KeeperTestSuite) TestAddTokensToLock() { suite.Require().Error(err) } +func (suite *KeeperTestSuite) TestLock() { + suite.SetupTest() + + addr1 := sdk.AccAddress([]byte("addr1---------------")) + coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} + + lock := types.PeriodLock{ + ID: 1, + Owner: addr1.String(), + Duration: time.Second, + EndTime: time.Time{}, + Coins: coins, + } + + // test locking without balance + err := suite.App.LockupKeeper.Lock(suite.Ctx, lock, coins) + suite.Require().Error(err) + + // check accumulation store + accum := suite.App.LockupKeeper.GetPeriodLocksAccumulation(suite.Ctx, types.QueryCondition{ + LockQueryType: types.ByDuration, + Denom: "stake", + Duration: time.Second, + }) + suite.Require().Equal(accum.String(), "0") + + suite.FundAcc(addr1, coins) + err = suite.App.LockupKeeper.Lock(suite.Ctx, lock, coins) + suite.Require().NoError(err) + + // check accumulation store + accum = suite.App.LockupKeeper.GetPeriodLocksAccumulation(suite.Ctx, types.QueryCondition{ + LockQueryType: types.ByDuration, + Denom: "stake", + Duration: time.Second, + }) + suite.Require().Equal(accum.String(), "10") + + balance := suite.App.BankKeeper.GetBalance(suite.Ctx, addr1, "stake") + suite.Require().Equal(sdk.ZeroInt(), balance.Amount) + + acc := suite.App.AccountKeeper.GetModuleAccount(suite.Ctx, types.ModuleName) + balance = suite.App.BankKeeper.GetBalance(suite.Ctx, acc.GetAddress(), "stake") + suite.Require().Equal(sdk.NewInt(10), balance.Amount) +} + func (suite *KeeperTestSuite) AddTokensToLockForSynth() { suite.SetupTest()