Fix v10.x state incompatability (#2268)

* Revert "x/lock: Fix `ExtendLockup` API  (backport #1937) (#2030)"

This reverts commit bd32316c.

* Revert "Refactor `lock` method (backport #1936) (#2029)"

This reverts commit d733f648.

* Update changelog

* Update changelog

CHANGELOG.md

@@ -40,6 +40,18 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## v10.2.0
+
+In v10.1.0 a refactor did changed the ordering of certain function calls in a message execution.
+The data read and written to in state were the same, but re-ordered.
+This unknowingly caused gas_used incompatabilities, because you could out-of-gas error at any intermediate step, and end in a different gas_used quantity. cref: https://github.com/cosmos/cosmos-sdk/issues/12788
+
+The v10.2 line reverts the gas limit state incompatabilities introduced due to function ordering.
+Namely this reverts 
+
+* [#1937](https://github.com/osmosis-labs/osmosis/pull/1937) Change `lockupKeeper.ExtendLock` to take in lockID instead of the direct lock struct.
+* [#2030](https://github.com/osmosis-labs/osmosis/pull/2030) Rename lockup keeper `ResetAllLocks` to `InitializeAllLocks` and `ResetAllSyntheticLocks` to `InitializeAllSyntheticLocks`.
+
 ## v10.1.1
 
 #### Improvements

x/lockup/keeper/admin_keeper_test.go

@@ -4,6 +4,7 @@ import (
 	"time"
 
 	"github.com/osmosis-labs/osmosis/v10/x/lockup/keeper"
+	"github.com/osmosis-labs/osmosis/v10/x/lockup/types"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
@@ -13,10 +14,11 @@ 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)
-	lock, err := suite.App.LockupKeeper.CreateLock(suite.Ctx, addr1, coins, time.Second)
+	err := suite.App.LockupKeeper.Lock(suite.Ctx, lock)
 	suite.Require().NoError(err)
 
 	// lock with balance with same id
@@ -36,12 +38,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)
 
-	lock, err := suite.App.LockupKeeper.CreateLock(suite.Ctx, addr1, coins, time.Second)
-
+	err := suite.App.LockupKeeper.Lock(suite.Ctx, lock)
 	suite.Require().NoError(err)
 
 	// break lock

x/lockup/keeper/export_test.go

@@ -25,7 +25,3 @@ 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)
-}

x/lockup/keeper/lock.go

@@ -66,79 +66,100 @@ 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, tokensToAdd sdk.Coin) (*types.PeriodLock, error) {
+func (k Keeper) AddTokensToLockByID(ctx sdk.Context, lockID uint64, owner sdk.AccAddress, coin 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
+	}
 
-	for _, synthlock := range k.GetAllSyntheticLockupsByLockup(ctx, lock.ID) {
-		k.accumulationStore(ctx, synthlock.SynthDenom).Increase(accumulationKey(synthlock.Duration), tokensToAdd.Amount)
+	err = k.addTokenToLock(ctx, lock, coin)
+	if err != nil {
+		return nil, err
 	}
 
 	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.
-// 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
 	// when unlocking starts.
 	lock := types.NewPeriodLock(ID, owner, duration, time.Time{}, coins)
-	err := k.lock(ctx, lock, lock.Coins)
+	err := k.Lock(ctx, lock)
 	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 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 {
+// 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 {
 	owner, err := sdk.AccAddressFromBech32(lock.Owner)
 	if err != nil {
 		return err
 	}
-	if err := k.bk.SendCoinsFromAccountToModule(ctx, owner, types.ModuleName, tokensToLock); err != nil {
+	if err := k.bk.SendCoinsFromAccountToModule(ctx, owner, types.ModuleName, lock.Coins); err != nil {
 		return err
 	}
 
 	// store lock object into the store
-	err = k.setLock(ctx, lock)
+	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)
 	if err != nil {
 		return err
 	}
 
 	// add to accumulation store
-	for _, coin := range tokensToLock {
+	for _, coin := range lock.Coins {
 		k.accumulationStore(ctx, coin.Denom).Increase(accumulationKey(lock.Duration), coin.Amount)
 	}
 
@@ -353,16 +374,7 @@ 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, 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
-	}
-
+func (k Keeper) ExtendLockup(ctx sdk.Context, lock types.PeriodLock, newDuration time.Duration) error {
 	if lock.IsUnlocking() {
 		return fmt.Errorf("cannot edit unlocking lockup for lock %d", lock.ID)
 	}
@@ -372,15 +384,10 @@ func (k Keeper) ExtendLockup(ctx sdk.Context, lockID uint64, owner sdk.AccAddres
 		return fmt.Errorf("cannot edit lockup with synthetic lock %d", lock.ID)
 	}
 
-	// completely delete existing lock refs
-	err = k.deleteLockRefs(ctx, unlockingPrefix(lock.IsUnlocking()), *lock)
-	if err != nil {
-		return err
-	}
+	oldLock := lock
 
-	oldDuration := lock.GetDuration()
 	if newDuration != 0 {
-		if newDuration <= oldDuration {
+		if newDuration <= lock.Duration {
 			return fmt.Errorf("new duration should be greater than the original")
 		}
 
@@ -393,20 +400,25 @@ func (k Keeper) ExtendLockup(ctx sdk.Context, lockID uint64, owner sdk.AccAddres
 		lock.Duration = newDuration
 	}
 
-	// add lock refs with the new duration
-	err = k.addLockRefs(ctx, *lock)
+	// update lockup
+	err := k.deleteLockRefs(ctx, unlockingPrefix(oldLock.IsUnlocking()), oldLock)
 	if err != nil {
 		return err
 	}
 
-	err = k.setLock(ctx, *lock)
+	err = k.addLockRefs(ctx, lock)
+	if err != nil {
+		return err
+	}
+
+	err = k.setLock(ctx, lock)
 	if err != nil {
 		return err
 	}
 
 	k.hooks.OnLockupExtend(ctx,
 		lock.GetID(),
-		oldDuration,
+		oldLock.GetDuration(),
 		lock.GetDuration(),
 	)
 

x/lockup/keeper/lock_test.go

@@ -144,6 +144,35 @@ 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()
@@ -151,10 +180,11 @@ 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)
-	lock, err := suite.App.LockupKeeper.CreateLock(suite.Ctx, addr1, coins, time.Second)
+	err := suite.App.LockupKeeper.Lock(suite.Ctx, lock)
 	suite.Require().NoError(err)
 
 	// begin unlock with lock object
@@ -290,65 +320,6 @@ 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")
-
-	// 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() {
 	suite.SetupTest()
 
@@ -410,52 +381,6 @@ 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()
 
@@ -741,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.ID, addr, time.Second/2)
+	err = suite.App.LockupKeeper.ExtendLockup(suite.Ctx, *lock, time.Second/2)
 	suite.Require().Error(err)
 	// extending lock with same duration should fail
-	err = suite.App.LockupKeeper.ExtendLockup(suite.Ctx, lock.ID, addr, time.Second)
+	err = suite.App.LockupKeeper.ExtendLockup(suite.Ctx, *lock, time.Second)
 	suite.Require().Error(err)
 
 	// duration increase should success
-	err = suite.App.LockupKeeper.ExtendLockup(suite.Ctx, lock.ID, addr, time.Second*2)
+	err = suite.App.LockupKeeper.ExtendLockup(suite.Ctx, *lock, time.Second*2)
 	suite.Require().NoError(err)
 
 	// check queries

x/lockup/keeper/msg_server.go

@@ -155,19 +155,18 @@ func createBeginUnlockEvent(lock *types.PeriodLock) sdk.Event {
 func (server msgServer) ExtendLockup(goCtx context.Context, msg *types.MsgExtendLockup) (*types.MsgExtendLockupResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
 
-	owner, err := sdk.AccAddressFromBech32(msg.Owner)
+	lock, err := server.keeper.GetLockByID(ctx, msg.ID)
 	if err != nil {
-		return nil, err
+		return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, err.Error())
 	}
 
-	err = server.keeper.ExtendLockup(ctx, msg.ID, owner, msg.Duration)
-	if err != nil {
-		return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, err.Error())
+	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))
 	}
 
-	lock, err := server.keeper.GetLockByID(ctx, msg.ID)
+	err = server.keeper.ExtendLockup(ctx, *lock, msg.Duration)
 	if err != nil {
-		return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error())
+		return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, err.Error())
 	}
 
 	ctx.EventManager().EmitEvents(sdk.Events{