ADR 041: In-Place Store Migrations

[amaury1093]

Description

closes #8532

rendered

Please note that the implementation of this ADR has already started as part of #8345

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[anilcse]

🎉 this is a great addition. Thanks @AmauryM for the ADR.
Just small nits otherwise, looks great

[jgimeno]

Just a little idea, instead of per module increasing of protocolVersion that is somehow "arbitrary" wouldn't it be better if there was a fixed general version and the modules that update can subscribe their update to it?

In the end we only migrate when we change version, ex. 2.0.1 to 3, even minor versions too.

So for example staking can subscribe an update to version 2.0.1, but maybe wont subscribe to version 3 when it comes.

[gsora]

wouldn't it be better if there was a fixed general version and the modules that update can subscribe their update to it?

I wonder if we could use the upgrade plan name instead of version numbers, this way it would be even more clear from a administration perspective.

I agree on the subscription idea, it would be easier to maintain in the future.

[aaronc]

Just a little idea, instead of per module increasing of protocolVersion that is somehow "arbitrary" wouldn't it be better if there was a fixed general version and the modules that update can subscribe their update to it?

I don't really consider it arbitrary at all. When a module introduces a state machine breaking change it increments its version creating a mechanism for running automatic migrations.

I wonder if we could use the upgrade plan name instead of version numbers, this way it would be even more clear from a administration perspective.

The upgrade plan names live at a totally different level from these module versions. Each chain using modules has their own upgrade names, but modules need a way to track their own state machine version. That's all this is doing.

[jgimeno]

Thanks for clarification @aaronc, makes a lot of sense.

[aaronc]

Generally LGTM. Thanks @AmauryM !

[aaronc]

Let's explain that if there is no old ConsensusVersion, no migration is run but the version is set in the new migration map. i.e. if we add module foo to our chain at version 5, no migrations will be run but 5 will be saved as the current version after migrations are run.

[amaury1093]

Done. The upgradeKeeper.SetCurrentConsensusVersions() method to store the current module versions is run right after the upgrade handler.

[amaury1093]

This is a very good improvement, the only thing that I still feel not totally convinced is the pattern of adding a new method every time we want to add a migration to the keeper, couldn't we use more a composition pattern here? Not to make the keepers fat?

to make the keepers not so fat, I added in the latest commits the Migrator wrapper suggested by Robert. lmk if that's a better separation of concerns.

[alessio]

Concept ACK

[jgimeno]

Thanks @AmauryM @aaronc , I love the improvements. ACK!

[robert-zaremba]

Looks good.

[aaronc]

ACK 🎉 . Left a few minor comments. Feel free to merge when ready.