Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 35 commits into from
Aug 11, 2023
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
1e55c09
add WithConsensusParamsGetter for manager
mmsqe Jun 15, 2023
5d513a3
test WithConsensusParamsGetter
mmsqe Jun 16, 2023
3fcd4d6
test continuously call BeginBlock
mmsqe Jun 16, 2023
34d47d1
add doc
mmsqe Jun 16, 2023
c21cff7
rm module name check
mmsqe Jun 16, 2023
ea9e96a
add change log
mmsqe Jun 16, 2023
721fb8b
Update CHANGELOG.md
mmsqe Jun 16, 2023
3158f6a
fix interface
mmsqe Jun 16, 2023
e54fc1a
fix lint
mmsqe Jun 16, 2023
81038ec
Apply suggestions from code review
mmsqe Jun 20, 2023
36a3944
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Jun 20, 2023
5e43e65
cleanup doc
mmsqe Jun 16, 2023
9c3a25b
fix resolve
mmsqe Jun 20, 2023
5bca160
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Jun 26, 2023
9f6994d
fix lint
mmsqe Jun 26, 2023
591d7cf
add ck
mmsqe Jun 27, 2023
e3f80d6
Revert "add ck"
mmsqe Jun 27, 2023
26e9624
add ConsensusParamsGetter interface
mmsqe Jun 27, 2023
6e50d86
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Jul 17, 2023
42fc450
fix doc
mmsqe Jul 18, 2023
59f4bef
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Aug 9, 2023
120cdae
handle upgrade 1st
mmsqe Aug 9, 2023
74746c9
Update x/upgrade/module.go
mmsqe Aug 10, 2023
7c812be
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Aug 10, 2023
bbf4eef
fix doc
mmsqe Aug 10, 2023
ca99e59
replace WithConsensusParamsGetter with MigrationModuleManager
mmsqe Aug 10, 2023
86bc9c0
fix test
mmsqe Aug 10, 2023
1a1b3ce
Apply suggestions from code review
mmsqe Aug 10, 2023
5400774
check nil
mmsqe Aug 11, 2023
2b56da6
update doc
mmsqe Aug 11, 2023
95c6ed9
add test
mmsqe Aug 11, 2023
08a9279
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Aug 11, 2023
6ceebbf
update doc
mmsqe Aug 11, 2023
71ea378
Apply suggestions from code review
mmsqe Aug 11, 2023
ff7f08d
Merge branch 'main' into fix_cp
julienrbrt Aug 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/group) [#17146](https://github.com/cosmos/cosmos-sdk/pull/17146) Rename x/group legacy ORM package's error codespace from "orm" to "legacy_orm", preventing collisions with ORM's error codespace "orm".
* (x/bank) [#17170](https://github.com/cosmos/cosmos-sdk/pull/17170) Avoid empty spendable error message on send coins.
* (x/distribution) [#17236](https://github.com/cosmos/cosmos-sdk/pull/17236) Using "validateCommunityTax" in "Params.ValidateBasic", preventing panic when field "CommunityTax" is nil.
* (migrations) [#16583](https://github.com/cosmos/cosmos-sdk/pull/16583) Add `WithConsensusParamsGetter` to allow other modules to access updated context with consensus parameters within the same block that executes migration.

### API Breaking Changes

Expand Down
2 changes: 1 addition & 1 deletion runtime/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func SetupAppBuilder(inputs AppInputs) {
app.config = inputs.Config
app.appConfig = inputs.AppConfig
app.logger = inputs.Logger
app.ModuleManager = module.NewManagerFromMap(inputs.Modules)
app.ModuleManager = module.NewManagerFromMap(inputs.Modules).WithConsensusParamsGetter(app)

for name, mod := range inputs.Modules {
if customBasicMod, ok := inputs.CustomModuleBasics[name]; ok {
Expand Down
37 changes: 37 additions & 0 deletions testutil/mock/types_mock_appmodule.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions types/module/interfaces/consensus_params.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package interfaces
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is OK @tac0turtle?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why creating a new package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used anymore right?


import (
tmproto "github.com/cometbft/cometbft/proto/tendermint/types"

sdk "github.com/cosmos/cosmos-sdk/types"
)

// ConsensusParamsGetter is an interface to retrieve consensus parameters for a given context.
type ConsensusParamsGetter interface {
GetConsensusParams(ctx sdk.Context) tmproto.ConsensusParams
}
41 changes: 38 additions & 3 deletions types/module/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import (
"github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/types/module/interfaces"
)

// AppModuleBasic is the standard form for basic non-dependant elements of an application module.
Expand All @@ -67,6 +68,11 @@ type HasName interface {
Name() string
}

// UpgradeModule is the extension interface that upgrade module
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be more descriptive on the godoc here of why is this needed.

type UpgradeModule interface {
IsUpgradeModule()
}

// HasGenesisBasics is the legacy interface for stateless genesis methods.
type HasGenesisBasics interface {
DefaultGenesis(codec.JSONCodec) json.RawMessage
Expand Down Expand Up @@ -276,6 +282,7 @@ type Manager struct {
OrderPrepareCheckStaters []string
OrderPrecommiters []string
OrderMigrations []string
consensusParamsGetter interfaces.ConsensusParamsGetter
}

// NewManager creates a new Manager object.
Expand Down Expand Up @@ -322,6 +329,12 @@ func NewManagerFromMap(moduleMap map[string]appmodule.AppModule) *Manager {
}
}

// WithConsensusParamsGetter sets ConsensusParamsGetter for Manager.
func (m *Manager) WithConsensusParamsGetter(g interfaces.ConsensusParamsGetter) *Manager {
m.consensusParamsGetter = g
return m
}

// SetOrderInitGenesis sets the order of init genesis calls
func (m *Manager) SetOrderInitGenesis(moduleNames ...string) {
m.assertNoForgottenModules("SetOrderInitGenesis", moduleNames, func(moduleName string) bool {
Expand Down Expand Up @@ -700,9 +713,31 @@ func (m *Manager) BeginBlock(ctx sdk.Context) (sdk.BeginBlock, error) {

for _, moduleName := range m.OrderBeginBlockers {
if module, ok := m.Modules[moduleName].(appmodule.HasBeginBlocker); ok {
err := module.BeginBlock(ctx)
if err != nil {
return sdk.BeginBlock{}, err
if _, ok := module.(UpgradeModule); ok {
if err := module.BeginBlock(ctx); err != nil {
Fixed Show fixed Hide fixed
return sdk.BeginBlock{}, err
}
if m.consensusParamsGetter != nil {
cp := ctx.ConsensusParams()
// Manager skips this step if Block is non-nil since upgrade module is expected to set this params
// and consensus parameters should not be overwritten.
if cp.Block == nil {
if cp = m.consensusParamsGetter.GetConsensusParams(ctx); cp.Block != nil {
Fixed Show fixed Hide fixed
ctx = ctx.WithConsensusParams(cp)
}
}
}
break
}
}
}

for _, moduleName := range m.OrderBeginBlockers {
if module, ok := m.Modules[moduleName].(appmodule.HasBeginBlocker); ok {
if _, ok := module.(UpgradeModule); !ok {
if err := module.BeginBlock(ctx); err != nil {

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call
return sdk.BeginBlock{}, err
}
}
}
}
Expand Down
65 changes: 65 additions & 0 deletions types/module/module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"errors"
"io"
"reflect"
"testing"

abci "github.com/cometbft/cometbft/abci/types"
Expand Down Expand Up @@ -491,6 +492,70 @@ func TestCoreAPIManager_BeginBlock(t *testing.T) {
require.EqualError(t, err, "some error")
}

type MockConsensusParamGetter struct {
ctrl *gomock.Controller
recorder *MockConsensusParamGetterRecorder
}

type MockConsensusParamGetterRecorder struct {
mock *MockConsensusParamGetter
}

func NewMockConsensusParamGetter(ctrl *gomock.Controller) *MockConsensusParamGetter {
mock := &MockConsensusParamGetter{ctrl: ctrl}
mock.recorder = &MockConsensusParamGetterRecorder{mock}
return mock
}

func (m *MockConsensusParamGetter) EXPECT() *MockConsensusParamGetterRecorder {
return m.recorder
}

func (m *MockConsensusParamGetter) GetConsensusParams(arg0 sdk.Context) cmtproto.ConsensusParams {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "GetConsensusParams", arg0)
ret0, _ := ret[0].(cmtproto.ConsensusParams)
return ret0
}

func (mr *MockConsensusParamGetterRecorder) GetConsensusParams(arg0 interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetConsensusParams", reflect.TypeOf((*MockConsensusParamGetter)(nil).GetConsensusParams), arg0)
}

func TestManager_BeginBlock_WithConsensusParamsGetter(t *testing.T) {
mockCtrl := gomock.NewController(t)
t.Cleanup(mockCtrl.Finish)

mockUpgradeModule := mock.NewMockUpgradeModule(mockCtrl)
mockParamsGetter := NewMockConsensusParamGetter(mockCtrl)

// Create a Manager with the mock consensusParamsGetter and an upgrade module
m := module.NewManagerFromMap(map[string]appmodule.AppModule{
"upgrade": mockUpgradeModule,
}).WithConsensusParamsGetter(mockParamsGetter)

// Create a context with a nil consensus params object and an empty event manager
ctx := sdk.Context{}

mockUpgradeModule.EXPECT().BeginBlock(gomock.Any()).Times(1).Return(nil)
mockParamsGetter.EXPECT().GetConsensusParams(gomock.Any()).Times(1).Return(cmtproto.ConsensusParams{
Block: &cmtproto.BlockParams{MaxBytes: 1000},
})
_, err := m.BeginBlock(ctx)
require.NoError(t, err)

// Create a context with a consensus params object and an empty event manager
ctx = sdk.Context{}.WithConsensusParams(cmtproto.ConsensusParams{
Block: &cmtproto.BlockParams{MaxBytes: 1000},
})

mockUpgradeModule.EXPECT().BeginBlock(gomock.Any()).Times(1).Return(nil)
mockParamsGetter.EXPECT().GetConsensusParams(gomock.Any()).Times(0)
_, err = m.BeginBlock(ctx)
require.NoError(t, err)
}

func TestCoreAPIManager_EndBlock(t *testing.T) {
mockCtrl := gomock.NewController(t)
t.Cleanup(mockCtrl.Finish)
Expand Down
3 changes: 3 additions & 0 deletions x/upgrade/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@ func (am AppModule) BeginBlock(ctx context.Context) error {
return BeginBlocker(ctx, am.keeper)
}

// IsUpgradeModule implements the appmodule.AppModule interface.
mmsqe marked this conversation as resolved.
Show resolved Hide resolved
func (am AppModuleBasic) IsUpgradeModule() {}

//
// App Wiring Setup
//
Expand Down