From 99304f2b607f0323f9c7b025d2a4613473392d45 Mon Sep 17 00:00:00 2001 From: "Matt, Park" <45252226+mattverse@users.noreply.github.com> Date: Sat, 2 Jul 2022 05:11:11 +0900 Subject: [PATCH] x/lock: Fix `ExtendLockup` API (#1937) * Fix extendlock api * Apply suggestions from code review * Add changelog Co-authored-by: Dev Ojha Co-authored-by: Dev Ojha --- CHANGELOG.md | 1 + x/lockup/keeper/lock.go | 33 +++++++++++++++++++++------------ x/lockup/keeper/lock_test.go | 6 +++--- x/lockup/keeper/msg_server.go | 13 +++++++------ 4 files changed, 32 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 318d638e2d6..5af33dbbffe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 #### Golang API breaks +* [#1937](https://github.com/osmosis-labs/osmosis/pull/1937) Change `lockupKeeper.ExtendLock` to take in lockID instead of the direct lock struct. * [#1893](https://github.com/osmosis-labs/osmosis/pull/1893) Change `EpochsKeeper.SetEpochInfo` to `AddEpochInfo`, which has more safety checks with it. (Makes it suitable to be called within upgrades) * [#1671](https://github.com/osmosis-labs/osmosis/pull/1671) Remove methods that constitute AppModuleSimulation APIs for several modules' AppModules, which implemented no-ops * [#1671](https://github.com/osmosis-labs/osmosis/pull/1671) Add hourly epochs to `x/epochs` DefaultGenesis. diff --git a/x/lockup/keeper/lock.go b/x/lockup/keeper/lock.go index 052a09e8905..0331912dea8 100644 --- a/x/lockup/keeper/lock.go +++ b/x/lockup/keeper/lock.go @@ -374,7 +374,16 @@ func (k Keeper) unlockMaturedLockInternalLogic(ctx sdk.Context, lock types.Perio // 2. Locks that are unlokcing are not allowed to change duration. // 3. Locks that have synthetic lockup are not allowed to change. // 4. Provided duration should be greater than the original duration. -func (k Keeper) ExtendLockup(ctx sdk.Context, lock types.PeriodLock, newDuration time.Duration) error { +func (k Keeper) ExtendLockup(ctx sdk.Context, lockID uint64, owner sdk.AccAddress, newDuration time.Duration) error { + lock, err := k.GetLockByID(ctx, lockID) + if err != nil { + return err + } + + if lock.GetOwner() != owner.String() { + return types.ErrNotLockOwner + } + if lock.IsUnlocking() { return fmt.Errorf("cannot edit unlocking lockup for lock %d", lock.ID) } @@ -384,10 +393,15 @@ func (k Keeper) ExtendLockup(ctx sdk.Context, lock types.PeriodLock, newDuration return fmt.Errorf("cannot edit lockup with synthetic lock %d", lock.ID) } - oldLock := lock + // completely delete existing lock refs + err = k.deleteLockRefs(ctx, unlockingPrefix(lock.IsUnlocking()), *lock) + if err != nil { + return err + } + oldDuration := lock.GetDuration() if newDuration != 0 { - if newDuration <= lock.Duration { + if newDuration <= oldDuration { return fmt.Errorf("new duration should be greater than the original") } @@ -400,25 +414,20 @@ func (k Keeper) ExtendLockup(ctx sdk.Context, lock types.PeriodLock, newDuration lock.Duration = newDuration } - // update lockup - err := k.deleteLockRefs(ctx, unlockingPrefix(oldLock.IsUnlocking()), oldLock) - if err != nil { - return err - } - - err = k.addLockRefs(ctx, lock) + // add lock refs with the new duration + err = k.addLockRefs(ctx, *lock) if err != nil { return err } - err = k.setLock(ctx, lock) + err = k.setLock(ctx, *lock) if err != nil { return err } k.hooks.OnLockupExtend(ctx, lock.GetID(), - oldLock.GetDuration(), + oldDuration, lock.GetDuration(), ) diff --git a/x/lockup/keeper/lock_test.go b/x/lockup/keeper/lock_test.go index 5afecbd678e..5dde289529e 100644 --- a/x/lockup/keeper/lock_test.go +++ b/x/lockup/keeper/lock_test.go @@ -666,14 +666,14 @@ func (suite *KeeperTestSuite) TestEditLockup() { lock, _ := suite.App.LockupKeeper.GetLockByID(suite.Ctx, 1) // duration decrease should fail - err = suite.App.LockupKeeper.ExtendLockup(suite.Ctx, *lock, time.Second/2) + err = suite.App.LockupKeeper.ExtendLockup(suite.Ctx, lock.ID, addr, time.Second/2) suite.Require().Error(err) // extending lock with same duration should fail - err = suite.App.LockupKeeper.ExtendLockup(suite.Ctx, *lock, time.Second) + err = suite.App.LockupKeeper.ExtendLockup(suite.Ctx, lock.ID, addr, time.Second) suite.Require().Error(err) // duration increase should success - err = suite.App.LockupKeeper.ExtendLockup(suite.Ctx, *lock, time.Second*2) + err = suite.App.LockupKeeper.ExtendLockup(suite.Ctx, lock.ID, addr, time.Second*2) suite.Require().NoError(err) // check queries diff --git a/x/lockup/keeper/msg_server.go b/x/lockup/keeper/msg_server.go index 725570bad6c..ce123cff367 100644 --- a/x/lockup/keeper/msg_server.go +++ b/x/lockup/keeper/msg_server.go @@ -156,18 +156,19 @@ func createBeginUnlockEvent(lock *types.PeriodLock) sdk.Event { func (server msgServer) ExtendLockup(goCtx context.Context, msg *types.MsgExtendLockup) (*types.MsgExtendLockupResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) - lock, err := server.keeper.GetLockByID(ctx, msg.ID) + owner, err := sdk.AccAddressFromBech32(msg.Owner) if err != nil { - return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, err.Error()) + return nil, err } - if msg.Owner != lock.Owner { - return nil, sdkerrors.Wrapf(types.ErrNotLockOwner, fmt.Sprintf("msg sender (%s) and lock owner (%s) does not match", msg.Owner, lock.Owner)) + err = server.keeper.ExtendLockup(ctx, msg.ID, owner, msg.Duration) + if err != nil { + return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, err.Error()) } - err = server.keeper.ExtendLockup(ctx, *lock, msg.Duration) + lock, err := server.keeper.GetLockByID(ctx, msg.ID) if err != nil { - return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, err.Error()) + return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error()) } ctx.EventManager().EmitEvents(sdk.Events{