From dd94e574c54524d74db18dfec594f8b4931c9c3d Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 19 Jul 2022 20:28:47 +0200 Subject: [PATCH] feat: Move AppModule.BeginBlock and AppModule.EndBlock to extension interfaces (backport #12603) (#12638) * feat: Move AppModule.BeginBlock and AppModule.EndBlock to extension interfaces (#12603) ## Description Most modules right now have a no-op for AppModule.BeginBlock and AppModule.EndBlock. We should move these methods off the AppModule interface so we have less deadcode, and instead move them to extension interfaces. 1. I added `BeginBlockAppModule` and `EndBlockAppModule` interface. 2. Remove the dead-code from modules that do no implement them 3. Add type casting in the the module code to use the new interface Closes: #12462 --- ### 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... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit b65f3fe070c57cf8841d25f5afe110f5fd951aa3) # Conflicts: # CHANGELOG.md # x/authz/module/module.go # x/group/module/module.go # x/nft/module/module.go # x/params/module.go # x/slashing/module.go # x/upgrade/module.go * remove conflicts * remove conflicts * remove unused modules Co-authored-by: Sishir Giri Co-authored-by: marbar3778 --- CHANGELOG.md | 3 +++ types/module/module.go | 51 +++++++++++++++++-------------------- types/module/module_test.go | 6 ++--- x/auth/module.go | 12 --------- x/auth/vesting/module.go | 12 +++------ x/authz/module/module.go | 5 +--- x/bank/module.go | 4 +-- x/distribution/module.go | 4 +-- x/evidence/module.go | 4 +-- x/feegrant/module/module.go | 24 ++++++++++++++++- x/gov/module.go | 8 +++--- x/mint/module.go | 4 +-- x/params/module.go | 7 ++--- x/slashing/module.go | 4 +-- x/upgrade/module.go | 6 ++--- 15 files changed, 76 insertions(+), 78 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3727681f471f..6998a88421a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,9 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] +### Features + +* (upgrade) [#12603](https://github.com/cosmos/cosmos-sdk/pull/12603) feat: Move AppModule.BeginBlock and AppModule.EndBlock to extension interfaces ### Bug Fixes * (x/mint) [#12384](https://github.com/cosmos/cosmos-sdk/pull/12384) Ensure `GoalBonded` must be positive when performing `x/mint` parameter validation. diff --git a/types/module/module.go b/types/module/module.go index f37621a23103..57c021cdd087 100644 --- a/types/module/module.go +++ b/types/module/module.go @@ -213,14 +213,23 @@ func (m *Manager) SetOrderExportGenesis(moduleNames ...string) { return !hasGenesis } - if _, hasABCIGenesis := module.(HasABCIGenesis); hasABCIGenesis { - return !hasABCIGenesis - } + // ConsensusVersion is a sequence number for state-breaking change of the + // module. It should be incremented on each consensus-breaking change + // introduced by the module. To avoid wrong/empty versions, the initial version + // should be set to 1. + ConsensusVersion() uint64 +} - _, hasGenesis := module.(HasGenesis) - return !hasGenesis - }) - m.OrderExportGenesis = moduleNames +// BeginBlockAppModule is an extension interface that contains information about the AppModule and BeginBlock. +type BeginBlockAppModule interface { + AppModule + BeginBlock(sdk.Context, abci.RequestBeginBlock) +} + +// EndBlockAppModule is an extension interface that contains information about the AppModule and EndBlock. +type EndBlockAppModule interface { + AppModule + EndBlock(sdk.Context, abci.RequestEndBlock) []abci.ValidatorUpdate } // SetOrderPreBlockers sets the order of set pre-blocker calls @@ -697,10 +706,9 @@ func (m *Manager) PreBlock(ctx sdk.Context) error { func (m *Manager) BeginBlock(ctx sdk.Context) (sdk.BeginBlock, error) { ctx = ctx.WithEventManager(sdk.NewEventManager()) for _, moduleName := range m.OrderBeginBlockers { - if module, ok := m.Modules[moduleName].(appmodule.HasBeginBlocker); ok { - if err := module.BeginBlock(ctx); err != nil { - return sdk.BeginBlock{}, err - } + module, ok := m.Modules[moduleName].(BeginBlockAppModule) + if ok { + module.BeginBlock(ctx, req) } } @@ -721,22 +729,11 @@ func (m *Manager) EndBlock(ctx sdk.Context, req abci.RequestEndBlock) abci.Respo validatorUpdates := []abci.ValidatorUpdate{} for _, moduleName := range m.OrderEndBlockers { - if module, ok := m.Modules[moduleName].(appmodule.HasEndBlocker); ok { - err := module.EndBlock(ctx) - if err != nil { - return sdk.EndBlock{}, err - } - } else if module, ok := m.Modules[moduleName].(HasABCIEndBlock); ok { - moduleValUpdates, err := module.EndBlock(ctx) - if err != nil { - return sdk.EndBlock{}, err - } - // use these validator updates if provided, the module manager assumes - // only one module will update the validator set - if len(moduleValUpdates) > 0 { - if len(validatorUpdates) > 0 { - return sdk.EndBlock{}, errors.New("validator EndBlock updates already set by a previous module") - } + module, ok := m.Modules[moduleName].(EndBlockAppModule) + if !ok { + continue + } + moduleValUpdates := module.EndBlock(ctx, req) validatorUpdates = append(validatorUpdates, moduleValUpdates...) } diff --git a/types/module/module_test.go b/types/module/module_test.go index a088ae8a4df8..3cddd73f7e5b 100644 --- a/types/module/module_test.go +++ b/types/module/module_test.go @@ -56,10 +56,8 @@ func TestAssertNoForgottenModules(t *testing.T) { mm.SetOrderExportGenesis("module1") }) - require.Equal(t, []string{"module1", "module3"}, mm.OrderEndBlockers) - require.PanicsWithValue(t, "all modules must be defined when setting SetOrderEndBlockers, missing: [module1]", func() { - mm.SetOrderEndBlockers("module3") - }) + // no-op + goam.RegisterInvariants(mockInvariantRegistry) } func TestManagerOrderSetters(t *testing.T) { diff --git a/x/auth/module.go b/x/auth/module.go index b959fcde5408..777070db663c 100644 --- a/x/auth/module.go +++ b/x/auth/module.go @@ -170,18 +170,6 @@ func (am AppModule) TxValidator(ctx context.Context, tx transaction.Tx) error { return fmt.Errorf("invalid tx type %T, expected sdk.Tx", tx) } - for _, validator := range validators { - if err := validator.ValidateTx(ctx, sdkTx); err != nil { - return err - } - } - - return nil -} - -// ConsensusVersion implements appmodule.HasConsensusVersion -func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion } - // AppModuleSimulation functions // GenerateGenesisState creates a randomized GenState of the auth module diff --git a/x/auth/vesting/module.go b/x/auth/vesting/module.go index aa0f84e2c06c..3873261641c6 100644 --- a/x/auth/vesting/module.go +++ b/x/auth/vesting/module.go @@ -54,15 +54,9 @@ func (am AppModule) InitGenesis(_ sdk.Context, _ codec.JSONCodec, _ json.RawMess return []abci.ValidatorUpdate{} } -// RegisterLegacyAminoCodec registers the module's types with the given codec. -func (AppModule) RegisterLegacyAminoCodec(registrar registry.AminoRegistrar) { - types.RegisterLegacyAminoCodec(registrar) -} - -// RegisterInterfaces registers the module's interfaces and implementations with -// the given interface registry. -func (AppModule) RegisterInterfaces(registrar registry.InterfaceRegistrar) { - types.RegisterInterfaces(registrar) +// ExportGenesis is always empty, as InitGenesis does nothing either. +func (am AppModule) ExportGenesis(_ sdk.Context, cdc codec.JSONCodec) json.RawMessage { + return am.DefaultGenesis(cdc) } // ConsensusVersion implements HasConsensusVersion. diff --git a/x/authz/module/module.go b/x/authz/module/module.go index d25ab35eba88..ef41e764d0e3 100644 --- a/x/authz/module/module.go +++ b/x/authz/module/module.go @@ -144,10 +144,7 @@ func (am AppModule) ExportGenesis(ctx context.Context) (json.RawMessage, error) // ConsensusVersion implements HasConsensusVersion. func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion } -// BeginBlock returns the begin blocker for the authz module. -func (am AppModule) BeginBlock(ctx context.Context) error { - return BeginBlocker(ctx, am.keeper) -} +// ____________________________________________________________________________ // AppModuleSimulation functions diff --git a/x/bank/module.go b/x/bank/module.go index 49aa2981b918..4c7eb908baed 100644 --- a/x/bank/module.go +++ b/x/bank/module.go @@ -151,8 +151,8 @@ func (am AppModule) ExportGenesis(ctx context.Context) (json.RawMessage, error) return am.cdc.MarshalJSON(gs) } -// ConsensusVersion implements HasConsensusVersion -func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion } +// ConsensusVersion implements AppModule/ConsensusVersion. +func (AppModule) ConsensusVersion() uint64 { return 2 } // AppModuleSimulation functions diff --git a/x/distribution/module.go b/x/distribution/module.go index b04590168eab..78ca5a673286 100644 --- a/x/distribution/module.go +++ b/x/distribution/module.go @@ -170,8 +170,8 @@ func (am AppModule) ExportGenesis(ctx context.Context) (json.RawMessage, error) func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion } // BeginBlock returns the begin blocker for the distribution module. -func (am AppModule) BeginBlock(ctx context.Context) error { - return am.keeper.BeginBlocker(ctx) +func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) { + BeginBlocker(ctx, req, am.keeper) } // AppModuleSimulation functions diff --git a/x/evidence/module.go b/x/evidence/module.go index 0327a83915ef..26e1923e95f8 100644 --- a/x/evidence/module.go +++ b/x/evidence/module.go @@ -137,8 +137,8 @@ func (am AppModule) ExportGenesis(ctx context.Context) (json.RawMessage, error) func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion } // BeginBlock executes all ABCI BeginBlock logic respective to the evidence module. -func (am AppModule) BeginBlock(ctx context.Context) error { - return am.keeper.BeginBlocker(ctx, am.cometService) +func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) { + BeginBlocker(ctx, req, am.keeper) } // AppModuleSimulation functions diff --git a/x/feegrant/module/module.go b/x/feegrant/module/module.go index 2e22fef937bc..90f714e45846 100644 --- a/x/feegrant/module/module.go +++ b/x/feegrant/module/module.go @@ -137,7 +137,29 @@ func (am AppModule) ExportGenesis(ctx context.Context) (json.RawMessage, error) return nil, err } - return am.cdc.MarshalJSON(gs) + return cdc.MustMarshalJSON(gs) +} + +// ConsensusVersion implements AppModule/ConsensusVersion. +func (AppModule) ConsensusVersion() uint64 { return 1 } + +// EndBlock returns the end blocker for the feegrant module. It returns no validator +// updates. +func (AppModule) EndBlock(_ sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate { + return []abci.ValidatorUpdate{} +} + +// AppModuleSimulation functions + +// GenerateGenesisState creates a randomized GenState of the feegrant module. +func (AppModule) GenerateGenesisState(simState *module.SimulationState) { + simulation.RandomizedGenState(simState) +} + +// ProposalContents returns all the feegrant content functions used to +// simulate governance proposals. +func (AppModule) ProposalContents(simState module.SimulationState) []simtypes.WeightedProposalContent { + return nil } // ConsensusVersion implements HasConsensusVersion diff --git a/x/gov/module.go b/x/gov/module.go index 8fc5f3061aab..eb596752e9a1 100644 --- a/x/gov/module.go +++ b/x/gov/module.go @@ -198,9 +198,11 @@ func (am AppModule) ExportGenesis(ctx context.Context) (json.RawMessage, error) // ConsensusVersion implements HasConsensusVersion func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion } -// EndBlock returns the end blocker for the gov module. -func (am AppModule) EndBlock(ctx context.Context) error { - return am.keeper.EndBlocker(ctx) +// EndBlock returns the end blocker for the gov module. It returns no validator +// updates. +func (am AppModule) EndBlock(ctx sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate { + EndBlocker(ctx, am.keeper) + return []abci.ValidatorUpdate{} } // AppModuleSimulation functions diff --git a/x/mint/module.go b/x/mint/module.go index 4cdad57dc56c..5400bca9a954 100644 --- a/x/mint/module.go +++ b/x/mint/module.go @@ -143,8 +143,8 @@ func (am AppModule) ExportGenesis(ctx context.Context) (json.RawMessage, error) func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion } // BeginBlock returns the begin blocker for the mint module. -func (am AppModule) BeginBlock(ctx context.Context) error { - return am.keeper.BeginBlocker(ctx) +func (am AppModule) BeginBlock(ctx sdk.Context, _ abci.RequestBeginBlock) { + BeginBlocker(ctx, am.keeper) } // AppModuleSimulation functions diff --git a/x/params/module.go b/x/params/module.go index cc531691e9cc..e742f3a98a63 100644 --- a/x/params/module.go +++ b/x/params/module.go @@ -76,8 +76,5 @@ func (am AppModule) RegisterServices(registrar grpc.ServiceRegistrar) error { return nil } -// RegisterStoreDecoder doesn't register any type. -func (AppModule) RegisterStoreDecoder(sdr simtypes.StoreDecoderRegistry) {} - -// ConsensusVersion implements HasConsensusVersion -func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion } +// ConsensusVersion implements AppModule/ConsensusVersion. +func (AppModule) ConsensusVersion() uint64 { return 1 } diff --git a/x/slashing/module.go b/x/slashing/module.go index b0836be79231..11b29a630398 100644 --- a/x/slashing/module.go +++ b/x/slashing/module.go @@ -172,8 +172,8 @@ func (am AppModule) ExportGenesis(ctx context.Context) (json.RawMessage, error) func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion } // BeginBlock returns the begin blocker for the slashing module. -func (am AppModule) BeginBlock(ctx context.Context) error { - return BeginBlocker(ctx, am.keeper, am.cometService) +func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) { + BeginBlocker(ctx, req, am.keeper) } // AppModuleSimulation functions diff --git a/x/upgrade/module.go b/x/upgrade/module.go index be391bce2f6f..018f3fb74a83 100644 --- a/x/upgrade/module.go +++ b/x/upgrade/module.go @@ -143,7 +143,7 @@ func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion } // PreBlock calls the upgrade module hooks // -// CONTRACT: this is called *before* all other modules' BeginBlock functions -func (am AppModule) PreBlock(ctx context.Context) error { - return am.keeper.PreBlocker(ctx) +// CONTRACT: this is registered in BeginBlocker *before* all other modules' BeginBlock functions +func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) { + BeginBlocker(am.keeper, ctx, req) }