Fix v10.x state incompatability #2268
Conversation
| } | ||
|
|
||
| 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) |
There was a problem hiding this comment.
Why is OnTokenLocked hook called when AfterAddTokensToLock is already called above? Are both the hook calls necessary?
| err = suite.App.LockupKeeper.Lock(suite.Ctx, lock) | ||
| suite.Require().NoError(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
should we start writing test that checks gas at every intermediate steps of lock functions? Is that even possible?
There was a problem hiding this comment.
I really like this idea, want to use this for state compatibility testing :)
There was a problem hiding this comment.
No this seems excessive. Let's just fix the reason that this was an issue
There was a problem hiding this comment.
Im supposing its gonna take some time tho for this fix
There was a problem hiding this comment.
Depends on if we have agreement for this approach: cosmos/cosmos-sdk#12788
There was a problem hiding this comment.
agreeing on that & finishing tests in osmosis-labs/cosmos-sdk#304 << writing tooling for this lol.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := k.bk.SendCoinsFromAccountToModule(ctx, lock.OwnerAddress(), types.ModuleName, sdk.NewCoins(coin)); err != nil { |
There was a problem hiding this comment.
| if err := k.bk.SendCoinsFromAccountToModule(ctx, lock.OwnerAddress(), types.ModuleName, sdk.NewCoins(coin)); err != nil { | |
| if err := k.bk.SendCoinsFromAccountToModule(ctx, lock.OwnerAddress(), types.ModuleName, sdk.NewCoins(coin)); err != nil { |
| // 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 |
There was a problem hiding this comment.
| // CONTRACT: lock will have synthetic lock only if it has a single coin | |
| // CONTRACT: Lock will have synthetic lock only if it has a single coin |
There was a problem hiding this comment.
This is a revert, can make these changes happen in another PR if we really care
What is the purpose of the change
There was a State machine incompatability in v10.x, due to the ordering of gas measuring methods. cref:
These two PRs caused several gas-behavior changing logics, it just happens that the subtle case is what we hit on mainnet:
ExtendLockupAPI (backport #1937) #2030lockmethod (backport #1936) #2029They changed the order of operations in the normal case (which is what caused the out of gas bug. They also cause wholly different behavior on error situations.
Brief Changelog
Testing and Verifying
I reviewed all the commits in https://github.com/osmosis-labs/osmosis/tree/v10.x to look for any other state machine incompatabilities due to ordering. I didn't see any. We changed ordering of things in mint, but that is begin block logic, which doesn't have gas measurement concerns.
I've tested that this syncs through the offending block in v10.1.x that caused app hash mismatches.
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? yes