fix: Add MigrationModuleManager to handle migration of upgrade module before other modules #16583

mmsqe · 2023-06-16T01:32:12Z

Description

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

CHANGELOG.md

types/module/module.go

alexanderbez · 2023-06-19T15:22:42Z

types/module/module.go

 	errorsmod "cosmossdk.io/errors"
-
+	storetypes "cosmossdk.io/store/types"
+	tmproto "github.com/cometbft/cometbft/proto/tendermint/types"


@tac0turtle is there any harm on depending on cometbft types in types/module?

ideally we dont do this, this seems like a bandaid that we will have to clean up later on. i think we should identify how to fix this problem without allowing modules to override consensus params.

We must be doing this within BeginBlock of Manager since the consensus params is accessed by other modules right after migration within the same block execution.

Yeah I guess there's no way around it it seems :/

We could perhaps return an any instead and then type cast where needed?

not sure if get params by consensusKeeper is better, but we need add module one by one (not to mention module outside sdk)

@tac0turtle May I ask if you have any better idea beside these two approaches?

I think the core issue here is the migration is "system" level functionalities, probably shouldn't happen in a module in the first place, it should be executed before all the module logics. is it possible to move the migration logic into baseapp somehow?

sorry for delay, the dependency is fine. agree with @yihuang. Maybe for this version of baseapp this is fine but for later on we can change it. Im not sure we have a clean alternative

100% agree 👍

types/module/module.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

types/module/module.go

alexanderbez · 2023-06-27T02:21:41Z

types/module/module.go

@@ -261,6 +262,11 @@ func (GenesisOnlyAppModule) EndBlock(sdk.Context) ([]abci.ValidatorUpdate, error
 	return []abci.ValidatorUpdate{}, nil
 }

+// ConsensusParamsGetter is an interface to retrieve consensus parameters for a given context.
+type ConsensusParamsGetter interface {
+	GetConsensusParams(ctx sdk.Context) tmproto.ConsensusParams


what if we just return an any here and then type cast? Then we can avoid the CometBFT dependency.

since need use as cp := getter.GetConsensusParams(ctx).(tmproto.ConsensusParams), we can't use any for WithConsensusParams, or can we add a separate package to avoid import cometbft type directly

This reverts commit 591d7cf.

alexanderbez · 2023-07-17T16:59:07Z

types/module/interfaces/consensus_params.go

@@ -0,0 +1,12 @@
+package interfaces


I think this is OK @tac0turtle?

Why creating a new package?

it's to avoid directly import cometbft type, sth like allow B get A without import X:
A import X
B import A

This isn't used anymore right?

CHANGELOG.md

julienrbrt

Lgtm!

julienrbrt · 2023-08-11T07:50:10Z

Thanks for adding the godoc and tests!
One last thing to get it 100% would be to describe this new behavior in the docs:
somewhere here

cosmos-sdk/docs/docs/building-modules/01-module-manager.md

Line 269 in dd49f98

    
           * `BeginBlock(ctx context.Context) error`: At the beginning of each block, this function is called from [`BaseApp`](../core/00-baseapp.md#beginblock) and, in turn, calls the [`BeginBlock`](./05-beginblock-endblock.md) function of each modules implementing the `BeginBlockAppModule` interface, in the order defined in `OrderBeginBlockers`. It creates a child [context](../core/02-context.md) with an event manager to aggregate [events](../core/08-events.md) emitted from all modules.

.
Possibly as well here

cosmos-sdk/docs/docs/core/00-baseapp.md

Line 447 in dd49f98

#### BeginBlock

docs/docs/building-modules/01-module-manager.md

UPGRADING.md

docs/docs/building-modules/01-module-manager.md

Co-authored-by: Julien Robert <julien@rbrt.fr>

… before other modules (#16583) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> (cherry picked from commit 0c1f6fc)

… before other modules (backport #16583) (#17370)

… before other modules (#16583) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> (cherry picked from commit 0c1f6fc) # Conflicts: # CHANGELOG.md # UPGRADING.md # baseapp/baseapp.go # docs/docs/building-modules/01-module-manager.md # docs/docs/core/00-baseapp.md # testutil/mock/types_mock_appmodule.go # types/module/module.go # types/module/module_test.go

… before other modules (cosmos#16583) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> (cherry picked from commit 0c1f6fc)

…e module before other modules (backport #16583) (#17370)" This reverts commit 27f5db2.

… before other modules (cosmos#16583) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> (cherry picked from commit 0c1f6fc) # Conflicts: # CHANGELOG.md # UPGRADING.md # baseapp/baseapp.go # docs/docs/building-modules/01-module-manager.md # docs/docs/core/00-baseapp.md # testutil/mock/types_mock_appmodule.go # types/module/module.go # types/module/module_test.go

mmsqe added 6 commits June 16, 2023 09:29

add WithConsensusParamsGetter for manager

1e55c09

test WithConsensusParamsGetter

5d513a3

test continuously call BeginBlock

3fcd4d6

add doc

34d47d1

rm module name check

c21cff7

add change log

ea9e96a

mmsqe commented Jun 16, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mmsqe and others added 3 commits June 16, 2023 09:32

Update CHANGELOG.md

721fb8b

fix interface

3158f6a

fix lint

e54fc1a

mmsqe marked this pull request as ready for review June 16, 2023 02:15

mmsqe requested a review from a team as a code owner June 16, 2023 02:15

github-advanced-security bot found potential problems Jun 16, 2023

View reviewed changes

types/module/module.go Fixed Show fixed Hide fixed

tac0turtle assigned alexanderbez and tac0turtle Jun 16, 2023

alexanderbez reviewed Jun 19, 2023

View reviewed changes

mmsqe and others added 4 commits June 20, 2023 09:24

Apply suggestions from code review

81038ec

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Merge remote-tracking branch 'origin/main' into fix_cp

36a3944

cleanup doc

5e43e65

fix resolve

9c3a25b

github-advanced-security bot found potential problems Jun 20, 2023

View reviewed changes

types/module/module.go Fixed Show fixed Hide fixed

mmsqe added 3 commits June 26, 2023 09:15

Merge remote-tracking branch 'origin/main' into fix_cp

5bca160

fix lint

9f6994d

add ck

591d7cf

alexanderbez reviewed Jun 27, 2023

View reviewed changes

mmsqe added 3 commits June 27, 2023 10:53

Revert "add ck"

e3f80d6

This reverts commit 591d7cf.

add ConsensusParamsGetter interface

26e9624

Merge remote-tracking branch 'origin/main' into fix_cp

6e50d86

alexanderbez reviewed Jul 17, 2023

View reviewed changes

julienrbrt reviewed Jul 17, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mmsqe added 3 commits August 11, 2023 09:33

update doc

2b56da6

add test

95c6ed9

Merge remote-tracking branch 'origin/main' into fix_cp

08a9279

julienrbrt approved these changes Aug 11, 2023

View reviewed changes

julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Aug 11, 2023

update doc

6ceebbf

mmsqe commented Aug 11, 2023

View reviewed changes

docs/docs/building-modules/01-module-manager.md Outdated Show resolved Hide resolved

julienrbrt reviewed Aug 11, 2023

View reviewed changes

UPGRADING.md Outdated Show resolved Hide resolved

julienrbrt reviewed Aug 11, 2023

View reviewed changes

docs/docs/building-modules/01-module-manager.md Outdated Show resolved Hide resolved

Apply suggestions from code review

71ea378

Co-authored-by: Julien Robert <julien@rbrt.fr>

julienrbrt enabled auto-merge August 11, 2023 15:46

Merge branch 'main' into fix_cp

ff7f08d

julienrbrt added this pull request to the merge queue Aug 11, 2023

Merged via the queue into cosmos:main with commit 0c1f6fc Aug 11, 2023
43 of 45 checks passed

mergify bot mentioned this pull request Aug 11, 2023

fix: Add MigrationModuleManager to handle migration of upgrade module before other modules (backport #16583) #17370

Merged

mmsqe mentioned this pull request Aug 12, 2023

docs: update upgrade doc and changelog for MigrationModuleManager #17373

Merged

20 tasks

julienrbrt pushed a commit that referenced this pull request Aug 12, 2023

fix: Add MigrationModuleManager to handle migration of upgrade module…

27f5db2

… before other modules (backport #16583) (#17370)

julienrbrt mentioned this pull request Aug 14, 2023

refactor: move UpgradeModule interface to appmodule and fix mocks #17383

Merged

20 tasks

yihuang added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Aug 16, 2023

mergify bot mentioned this pull request Aug 16, 2023

fix: Add MigrationModuleManager to handle migration of upgrade module before other modules (backport #16583) #17398

Closed

julienrbrt removed the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Aug 16, 2023

This was referenced Aug 17, 2023

feat: introduce PreBlock #17421

Merged

fix: add missing refresh for finalizeBlockState.ctx #17449

Closed

julienrbrt added a commit that referenced this pull request Aug 18, 2023

Revert "fix: Add MigrationModuleManager to handle migration of upgrad…

8322b19

…e module before other modules (backport #16583) (#17370)" This reverts commit 27f5db2.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add MigrationModuleManager to handle migration of upgrade module before other modules #16583

fix: Add MigrationModuleManager to handle migration of upgrade module before other modules #16583

mmsqe commented Jun 16, 2023

alexanderbez Jun 19, 2023

tac0turtle Jun 26, 2023

mmsqe Jun 26, 2023

alexanderbez Jun 27, 2023

alexanderbez Jun 27, 2023

mmsqe Jun 27, 2023

mmsqe Aug 1, 2023

yihuang Aug 9, 2023 •

edited

Loading

tac0turtle Aug 9, 2023

alexanderbez Aug 9, 2023

alexanderbez Jun 27, 2023

mmsqe Jun 27, 2023 •

edited

Loading

alexanderbez Jul 17, 2023

julienrbrt Aug 9, 2023

mmsqe Aug 10, 2023

julienrbrt Aug 10, 2023

julienrbrt left a comment

julienrbrt commented Aug 11, 2023

fix: Add MigrationModuleManager to handle migration of upgrade module before other modules #16583

fix: Add MigrationModuleManager to handle migration of upgrade module before other modules #16583

Conversation

mmsqe commented Jun 16, 2023

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmsqe Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrbrt left a comment

Choose a reason for hiding this comment

julienrbrt commented Aug 11, 2023

yihuang Aug 9, 2023 •

edited

Loading

mmsqe Jun 27, 2023 •

edited

Loading