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

refactor!: migrate to core appmodule.AppModule extension interfaces #13794

Merged
merged 25 commits into from
Nov 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
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
7 changes: 6 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#13781](https://github.com/cosmos/cosmos-sdk/pull/13781) Remove `client/keys.KeysCdc`.
* [#13803](https://github.com/cosmos/cosmos-sdk/pull/13803) Add an error log if iavl set operation failed.
* [#13802](https://github.com/cosmos/cosmos-sdk/pull/13802) Add --output-document flag to the export CLI command to allow writing genesis state to a file.
* [#13794](https://github.com/cosmos/cosmos-sdk/pull/13794) `types/module.Manager` now supports the
`cosmossdk.io/core/appmodule.AppModule` API via the new `NewManagerFromMap` constructor.

### State Machine Breaking

Expand Down Expand Up @@ -159,7 +161,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/gov) [#13160](https://github.com/cosmos/cosmos-sdk/pull/13160) Remove custom marshaling of proposl and voteoption.
* (types) [#13430](https://github.com/cosmos/cosmos-sdk/pull/13430) Remove unused code `ResponseCheckTx` and `ResponseDeliverTx`
* (store) [#13529](https://github.com/cosmos/cosmos-sdk/pull/13529) Add method `LatestVersion` to `MultiStore` interface, add method `SetQueryMultiStore` to baesapp to support alternative `MultiStore` implementation for query service.
* (pruning) [#13609]](https://github.com/cosmos/cosmos-sdk/pull/13609) Move pruning pacakge to be under store pacakge
* (pruning) [#13609]](https://github.com/cosmos/cosmos-sdk/pull/13609) Move pruning package to be under store package
* [#13794](https://github.com/cosmos/cosmos-sdk/pull/13794) Most methods on `types/module.AppModule` have been moved to
extension interfaces. `module.Manager.Modules` is now of type `map[string]interface{}` to support in parallel the new
`cosmossdk.io/core/appmodule.AppModule` API.

### CLI Breaking Changes

Expand Down
8 changes: 7 additions & 1 deletion UPGRADING.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@ is typically found in `RegisterAPIRoutes`.

### AppModule Interface

Remove `Querier`, `Route` and `LegacyQuerier` from the app module interface. This removes and fully deprecates all legacy queriers. All modules no longer support the REST API previously known as the LCD, and the `sdk.Msg#Route` method won't be used anymore.
Support for the `AppModule` `Querier`, `Route` and `LegacyQuerier` methods has been entirely removed from the `AppModule`
interface. This removes and fully deprecates all legacy queriers. All modules no longer support the REST API previously
known as the LCD, and the `sdk.Msg#Route` method won't be used anymore.

Most other existing `AppModule` methods have been moved to extension interfaces in preparation for the migration
to the `cosmossdk.io/core/appmodule` API in the next release. Most `AppModule` implementations should not be broken
by this change.

### SimApp

Expand Down
15 changes: 6 additions & 9 deletions docs/docs/building-modules/14-depinject.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,7 @@ All methods, structs and their fields must be public for `depinject`.
https://github.com/cosmos/cosmos-sdk/blob/main/x/group/module/module.go#L184-L192
```

2. `ProvideModuleBasic` is calls `WrapAppModuleBasic` for wrapping the module `AppModuleBasic`, so that it can be injected and used by the runtime module.

```go reference
https://github.com/cosmos/cosmos-sdk/blob/main/x/group/module/module.go#L194-L196
```

3. Define a struct that inherits `depinject.In` and define the module inputs (i.e. module dependencies):
2. Define a struct that inherits `depinject.In` and define the module inputs (i.e. module dependencies):
* `depinject` provides the right dependencies to the module.
* `depinject` also checks that all dependencies are provided.

Expand All @@ -99,19 +93,22 @@ All methods, structs and their fields must be public for `depinject`.
https://github.com/cosmos/cosmos-sdk/blob/main/x/group/module/module.go#L198-L208
```

4. Define the module outputs with a public struct that inherits `depinject.Out`:
3. Define the module outputs with a public struct that inherits `depinject.Out`:
The module outputs are the dependencies that the module provides to other modules. It is usually the module itself and its keeper.

```go reference
https://github.com/cosmos/cosmos-sdk/blob/main/x/group/module/module.go#L210-L215
```

5. Create a function named `ProvideModule` (as called in 1.) and use the inputs for instantitating the module outputs.
4. Create a function named `ProvideModule` (as called in 1.) and use the inputs for instantiating the module outputs.

```go reference
https://github.com/cosmos/cosmos-sdk/blob/main/x/group/module/module.go#L217-L227
```

The `ProvideModule` function should return an instance of `cosmossdk.io/core/appmodule.AppModule` which implements
one or more app module extension interfaces for initializing the module.

Following is the complete app wiring configuration for `group`:

```go reference
Expand Down
38 changes: 19 additions & 19 deletions runtime/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func (b BaseAppOption) IsManyPerContainerType() {}
func init() {
appmodule.Register(&runtimev1alpha1.Module{},
appmodule.Provide(
Provide,
ProvideApp,
ProvideKVStoreKey,
ProvideTransientStoreKey,
ProvideMemoryStoreKey,
Expand All @@ -38,7 +38,7 @@ func init() {
)
}

func Provide(moduleBasics map[string]AppModuleBasicWrapper) (
func ProvideApp() (
codectypes.InterfaceRegistry,
codec.Codec,
*codec.LegacyAmino,
Expand All @@ -49,13 +49,6 @@ func Provide(moduleBasics map[string]AppModuleBasicWrapper) (
interfaceRegistry := codectypes.NewInterfaceRegistry()
amino := codec.NewLegacyAmino()

// build codecs
basicManager := module.BasicManager{}
for name, wrapper := range moduleBasics {
basicManager[name] = wrapper
wrapper.RegisterInterfaces(interfaceRegistry)
wrapper.RegisterLegacyAminoCodec(amino)
}
std.RegisterInterfaces(interfaceRegistry)
std.RegisterLegacyAminoCodec(amino)

Expand All @@ -67,7 +60,7 @@ func Provide(moduleBasics map[string]AppModuleBasicWrapper) (
interfaceRegistry: interfaceRegistry,
cdc: cdc,
amino: amino,
basicManager: basicManager,
basicManager: module.BasicManager{},
msgServiceRouter: msgServiceRouter,
},
}
Expand All @@ -78,23 +71,30 @@ func Provide(moduleBasics map[string]AppModuleBasicWrapper) (
type AppInputs struct {
depinject.In

AppConfig *appv1alpha1.Config
Config *runtimev1alpha1.Module
AppBuilder *AppBuilder
Modules map[string]AppModuleWrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove AppModuleWrapper in runtime/wrappers.go, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I would even remove WrapAppModuleBasic and all ProvideModuleBasics. Does depinject need the AppModuleBasics? Maybe only provide the appmodule.AppModules?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed AppModuleWrapper. I'll look into removing WrapAppModuleBasic. It's needed for codec instantiation, but maybe I can find a workaround.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed WrapModuleBasic. A bunch of boilerplate gone now. Should I go ahead and make the rest of AppModuleBasic extension interfaces? In particular HasInterfaces and HasAmino?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think that's a good idea. Totally fine to do in a follow-up too, as this already has approvals. It's not breaking anyways, right?

BaseAppOptions []BaseAppOption
AppConfig *appv1alpha1.Config
Config *runtimev1alpha1.Module
AppBuilder *AppBuilder
Modules map[string]appmodule.AppModule
BaseAppOptions []BaseAppOption
InterfaceRegistry codectypes.InterfaceRegistry
LegacyAmino *codec.LegacyAmino
}

func SetupAppBuilder(inputs AppInputs) {
mm := &module.Manager{Modules: map[string]module.AppModule{}}
for name, wrapper := range inputs.Modules {
mm.Modules[name] = wrapper.AppModule
}
mm := module.NewManagerFromMap(inputs.Modules)
app := inputs.AppBuilder.app
app.baseAppOptions = inputs.BaseAppOptions
app.config = inputs.Config
app.ModuleManager = mm
app.appConfig = inputs.AppConfig

for name, mod := range inputs.Modules {
if basicMod, ok := mod.(module.AppModuleBasic); ok {
app.basicManager[name] = basicMod
basicMod.RegisterInterfaces(inputs.InterfaceRegistry)
basicMod.RegisterLegacyAminoCodec(inputs.LegacyAmino)
}
}
Comment on lines +91 to +97

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
}

func registerStoreKey(wrapper *AppBuilder, key storetypes.StoreKey) {
Expand Down
4 changes: 2 additions & 2 deletions runtime/services/autocli.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ type AutoCLIQueryService struct {
moduleOptions map[string]*autocliv1.ModuleOptions
}

func NewAutoCLIQueryService(appModules map[string]module.AppModule) *AutoCLIQueryService {
func NewAutoCLIQueryService(appModules map[string]interface{}) *AutoCLIQueryService {
moduleOptions := map[string]*autocliv1.ModuleOptions{}
for modName, mod := range appModules {
if autoCliMod, ok := mod.(interface {
AutoCLIOptions() *autocliv1.ModuleOptions
}); ok {
moduleOptions[modName] = autoCliMod.AutoCLIOptions()
} else {
} else if mod, ok := mod.(module.HasServices); ok {
// try to auto-discover options based on the last msg and query
// services registered for the module
cfg := &autocliConfigurator{}
Expand Down
33 changes: 0 additions & 33 deletions runtime/wrappers.go

This file was deleted.

1 change: 1 addition & 0 deletions scripts/mockgen.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ mockgen_cmd="mockgen"
$mockgen_cmd -source=client/account_retriever.go -package mock -destination testutil/mock/account_retriever.go
$mockgen_cmd -package mock -destination testutil/mock/tendermint_tm_db_DB.go github.com/tendermint/tm-db DB
$mockgen_cmd -source=types/module/module.go -package mock -destination testutil/mock/types_module_module.go
$mockgen_cmd -source=types/module/mock_appmodule_test.go -package mock -destination testutil/mock/types_mock_appmodule.go
$mockgen_cmd -source=types/invariant.go -package mock -destination testutil/mock/types_invariant.go
$mockgen_cmd -package mock -destination testutil/mock/grpc_server.go github.com/cosmos/gogoproto/grpc Server
$mockgen_cmd -package mock -destination testutil/mock/tendermint_tendermint_libs_log_DB.go github.com/tendermint/tendermint/libs/log Logger
Expand Down
14 changes: 9 additions & 5 deletions simapp/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,14 @@ func TestRunMigrations(t *testing.T) {
//
// The loop below is the same as calling `RegisterServices` on
// ModuleManager, except that we skip x/bank.
for _, module := range app.ModuleManager.Modules {
if module.Name() == banktypes.ModuleName {
for name, mod := range app.ModuleManager.Modules {
if name == banktypes.ModuleName {
continue
}

module.RegisterServices(configurator)
if mod, ok := mod.(module.HasServices); ok {
mod.RegisterServices(configurator)
}
}

// Initialize the chain
Expand Down Expand Up @@ -206,7 +208,7 @@ func TestInitGenesisOnMigration(t *testing.T) {
// adding during a migration.
mockCtrl := gomock.NewController(t)
t.Cleanup(mockCtrl.Finish)
mockModule := mock.NewMockAppModule(mockCtrl)
mockModule := mock.NewMockAppModuleWithAllExtensions(mockCtrl)
mockDefaultGenesis := json.RawMessage(`{"key": "value"}`)
mockModule.EXPECT().DefaultGenesis(gomock.Eq(app.appCodec)).Times(1).Return(mockDefaultGenesis)
mockModule.EXPECT().InitGenesis(gomock.Eq(ctx), gomock.Eq(app.appCodec), gomock.Eq(mockDefaultGenesis)).Times(1).Return(nil)
Expand Down Expand Up @@ -252,7 +254,9 @@ func TestUpgradeStateOnGenesis(t *testing.T) {
ctx := app.NewContext(false, tmproto.Header{})
vm := app.UpgradeKeeper.GetModuleVersionMap(ctx)
for v, i := range app.ModuleManager.Modules {
require.Equal(t, vm[v], i.ConsensusVersion())
if i, ok := i.(module.HasConsensusVersion); ok {
require.Equal(t, vm[v], i.ConsensusVersion())
}
}

require.NotNil(t, app.UpgradeKeeper.GetVersionSetter())
Expand Down
Loading