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 12 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 @@ -88,6 +88,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 @@ -158,7 +160,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
7 changes: 2 additions & 5 deletions runtime/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,12 @@ type AppInputs struct {
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?

Modules map[string]appmodule.AppModule
BaseAppOptions []BaseAppOption
}

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
Expand Down
42 changes: 22 additions & 20 deletions runtime/services/autocli.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,37 +17,39 @@ 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 {
// try to auto-discover options based on the last msg and query
// services registered for the module
cfg := &autocliConfigurator{}
mod.RegisterServices(cfg)
modOptions := &autocliv1.ModuleOptions{}
haveServices := false

if cfg.msgServer.serviceName != "" {
haveServices = true
modOptions.Tx = &autocliv1.ServiceCommandDescriptor{
Service: cfg.msgServer.serviceName,
if mod, ok := mod.(module.HasRegisterSerices); ok {
aaronc marked this conversation as resolved.
Show resolved Hide resolved
// try to auto-discover options based on the last msg and query
// services registered for the module
cfg := &autocliConfigurator{}
mod.RegisterServices(cfg)
modOptions := &autocliv1.ModuleOptions{}
haveServices := false

if cfg.msgServer.serviceName != "" {
haveServices = true
modOptions.Tx = &autocliv1.ServiceCommandDescriptor{
Service: cfg.msgServer.serviceName,
}
}
}

if cfg.queryServer.serviceName != "" {
haveServices = true
modOptions.Query = &autocliv1.ServiceCommandDescriptor{
Service: cfg.queryServer.serviceName,
if cfg.queryServer.serviceName != "" {
haveServices = true
modOptions.Query = &autocliv1.ServiceCommandDescriptor{
Service: cfg.queryServer.serviceName,
}
}
}

if haveServices {
moduleOptions[modName] = modOptions
if haveServices {
moduleOptions[modName] = modOptions
}
}
}
}
Expand Down
14 changes: 8 additions & 6 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.HasRegisterSerices); ok {
mod.RegisterServices(configurator)
}
}

// Initialize the chain
Expand Down Expand Up @@ -209,8 +211,6 @@ func TestInitGenesisOnMigration(t *testing.T) {
mockModule := mock.NewMockAppModule(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)
mockModule.EXPECT().ConsensusVersion().Times(1).Return(uint64(0))

app.ModuleManager.Modules["mock"] = mockModule

Expand Down Expand Up @@ -252,7 +252,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