Simplified app.go wiring

[aaronc]

ADR 033 describes a mechanism for inter-module communication based off of protobuf service code generation. One aspect of ADR 033 that has been discussed but isn’t fully specified is how this approach affects app.go module/keeper wiring which is actually one of the biggest goals and potential benefits of this approach.

The high-level goals I have in mind for ADR 033 and app.go are:

1. Making app.go wiring as minimal as possible so that a) it is easier to spin-up apps in module tests and b) properly wiring up an app is simpler and less error prone
2. Make inter-module wiring purely declarative so that it could potentially be represented in a config file and eventually as on-chain params

Goals

I want to briefly describe a few goals I have for app wiring.

Goal 1: Make app wiring much simpler

The key benefits I see for this are:

• app.go would be much shorter simpler and less error prone
• it would be much easier to spin up “simapp”-like integration test fixtures

Easier integration test fixtures are a particularly big pain point for me having attempted to develop modules outside of an app a few times in the past and having needed to basically completely replicate “simapp” to do any real end to end testing.

Goal 2: Make app wiring completely declarative

The idea here is that basically the entire module wiring configuration could basically be serialized to and from JSON/protobuf.

The hasn’t been talked about much but it could be potentially quite valuable for zones because it makes basically the entire app configuration mutable by governance. This could allow zones to spin up binaries with experimental modules that aren’t enabled initially but that can be switched on with a param change vote. Also, this basically reduces the surface area for parameters not configurable by governance to zero.

One particular case I’m holding in mind for the send deny-list issue that needed to be patched in app.go recently.

Example declarative config:

{
  "modules":[
    {
      "@type":"/cosmos.auth.module.v1.Module"
    },
    {
      "@type":"/cosmos.bank.module.v2.Module",
      "some_param":"value"
    }
  ]

Module Design

In order to accomplish this I want to propose the following rough Module interface redesign.

We start with a basic Module interface that kind of replaces AppModuleBasic which has a function NewAppModule which returns an instance of AppModule:

type MyModule struct {
...
}

type MyAppModule struct {
  MyModule
  ...
}

func (m MyModule) NewAppModule(moduleKey ModuleKey, cdc Codec)  AppModule {
  bankMsgClient := bank.NewMsgClient(moduleKey)
  authQueryClient := auth.NewQueryClient(moduleKey)
  return MyAppModule{m, moduleKey, cdc, bankMsgClient, authQueryClient, ...}
}

Using ADR 033, modules can talk to other modules using the ModuleKey to construct MsgClient and QueryClient instances.

Dependency Injection Option

To make the construction of MsgClient and QueryClient instances more ergonomic and fool-proof, we could consider an optional dependency injection approach that would look something like this:

func (m MyModule) NewAppModule(moduleKey ModuleKey, cdc Codec, bankMsgClient bank.MsgClient, authQueryClient auth.QueryClient)  AppModule {
  return MyAppModule{m, moduleKey, cdc, bankMsgClient, authQueryClient, ...}
}

This would eliminate the need to have a manual “require dependencies” step as described in ADR 033 which developers may forget and force the dependency resolution problem into the framework.

---

There are still many other issues that would need to be worked out to make this happen such as how do we wire AnteHandlers, plugins, and all sorts of other config parameters, but I want to get the discussion started.

I want to also be clear that in terms of priorities, I strongly believe we should work towards Goal 1 incrementally and first, before trying to make the config fully declarative which may even be deemed a long-term nice to have.

[hxrts]

it could be potentially quite valuable for zones because it makes basically the entire app configuration mutable by governance

Really cool, we might even be able to add new modules via rolling upgrades using this feature.

[blushi]

Dependency Injection Option

To make the construction of MsgClient and QueryClient instances more ergonomic and fool-proof, we could consider an optional dependency injection approach that would look something like this:

func (m MyModule) NewAppModule(moduleKey ModuleKey, cdc Codec, bankMsgClient bank.MsgClient, authQueryClient auth.QueryClient)  AppModule {
  return MyAppModule{m, moduleKey, cdc, bankMsgClient, authQueryClient, ...}
}

This would eliminate the need to have a manual “require dependencies” step as described in ADR 033 which developers may forget and force the dependency resolution problem into the framework.

I've been thinking about this dependency approach in terms of usage within the group module:

1. We would need to provide a long list of msg clients as params, although that could be within a struct, right?
2. When executing messages as part of proposals, how would you map the messages to the appropriate client method? Or we would just use the module key directly as initially implemented?

[robert-zaremba]

Internally, in past months we were discussing this issue, so it's worth to link the Regen issue where were experimenting with the refactore: regen-network/regen-ledger#301

Few highlights:

• allow order configuration for Begin and EndBlocker
• make the wiring up as declarative as possible.

[fadeev]

This looks like a great initiative 👍

Representing https://github.com/tendermint/starport as one of the users of Cosmos SDK, there are two important requirements from our side.

• Being able to import newly scaffolded modules in app.go without placeholders. For example, starport scaffold module foo creates all the files in x/foo and also correctly imports foo in app.go. Right now we always scaffold a module and try importing it in app.go, but it's not possible if required placeholders are not there.
• Being able to import third-party modules. Imagine we have a simple module registry with a single namespace. Then, we would be able to do starport module import liquidity. This command should be able to correctly import the liquidity module in app.go similarly to the previous command.

Also, since a chain needs to keep a list of imported modules, may I suggest using config.yml for this (instead of a JSON). Starport currently uses this file to keep configuration options, but there is no reason why other tools cannot use the same file (if only for the module list). It does have an added advantage of being less verbose than JSON.

config.yml:

modules:
  - type: "/cosmos.auth.module.v1.Module"
  - type: "/cosmos.bank.module.v2.Module"
    params:
      key: value

[fadeev]

Sure. So, right now for us to import, say, CosmWasm, we need to insert a bunch of lines in app.go. The way we do it right now is we have placeholder comments that we replace with lines of code. Like this,

func getGovProposalHandlers() []govclient.ProposalHandler {
	var govProposalHandlers []govclient.ProposalHandler
	// this line is used by starport scaffolding # stargate/app/govProposalHandlers
	govProposalHandlers = append(govProposalHandlers,
		paramsclient.ProposalHandler,
		distrclient.ProposalHandler,
		upgradeclient.ProposalHandler,
		upgradeclient.CancelProposalHandler,
		// this line is used by starport scaffolding # stargate/app/govProposalHandler
	)
	return govProposalHandlers
}

This is fragile and it's the main reason why we're not adding module importing capabilities to Starport.

[robert-zaremba]

The wire approach I'm testing should solve this issue. However you will need to restructure the code - everything must come by parameter or object self reference. No globals. Which is a good way to structure a program.

[robert-zaremba]

@fadeev - we have a working group for improving building app and wiring modules. Would you like to join it and help us validate the concepts? If so, let me know your Discord handle.

[fadeev]

@robert-zaremba The call is tomorrow, right? I'm Denis Fadeev | Tendermint Inc#4963 on Discord.

[aaronc]

Yep, you're on the invite @fadeev

[aaronc]

So I am generally starting to think of a new strategy here that allows us to deploy improvements to the current app.go UX without doing a lot of refactoring of existing modules. I still think many of the existing modules need to evolve and be refactored/redesigned but I don't think we can wait to have every module implemented ADR 033 before we can improve app.go.

Basically, by creating the right kid of dependency injection (DI) container, I think we can enable a declarative config like I've described above with the current module set mostly unmodified. I have a proof of concept PR I've been working on (#9398), with a partially working DI container that I think will work fine for our current keepers/AppModule's and even allow for some (optional) declarative Ocaps-like security. Some example config JSON is here: https://github.com/cosmos/cosmos-sdk/blob/b0a93479520730d576e94e11716d4f9b603c6a31/simapp2/app_config.json.

I might do a bit more work on this approach and show a minimal example where we could wire up the existing x/auth and x/bank like this (WIP here: #9442).

If we go with this approach, I'm also thinking that maybe some of the bigger refactors we're thinking of (ex. #9336 and #9066) might be best framed as proper v2s of the current modules. There are a lot of things I know we all want to do with the current SDK before we pin it down at call it v1.0. But I wonder if a better approach might be to pin down most of what we currently have as v1 - maybe with a best effort house-keeping type audit - and then start working on v2 sooner rather than later.

[aaronc]

I also consider the global bech32 prefixes a blocker (#7448).

[robert-zaremba]

@hxrts IAVL / SMT are unrelated to the proto definitions.

[robert-zaremba]

@aaronc I think removing bech32 prefixes won't change proto definitions.

[aaronc]

So I am mostly talking about SDK v1.0. I think proto files should move to v1 ASAP.

[robert-zaremba]

Great. The issue I created is for proto 👍

[robert-zaremba]

Store as a keeper dependency

We are considering a different way to provide a store. Instead of accessing a store through a context and module key: ctx.KVStore(ModuleStoreKey), we are exploring a way to set a store as a dependency for keepers. This way:

• keeper won't be able to directly access store of other keepers
• will remove area of errors, where we make a mistake with module keys
• will remove the need of ModuleStoreKey in the keeper code
• increase OCAPs
• encapsulate the code and improve testability: some keepers can have a mock store, and keeper under test can have real store.

[aaronc]

Can you say a little bit more about what this would look like concretely?

[robert-zaremba]

As per our discussion, the idea is that a keeper shouldn't know how to configure a store. Instead it should have a provider:

type StoreProvider func(std.Context) KVStore

func NewKeeper(storeProvider StoreProvider, ...) {...}

func (k Keeper) DoSomething() {
    store := keeper.storeProvider(ctx)
    ....
}

[aaronc]

I just want to give an update on my latest thinking to see if anyone has any feedback. My latest proof of concept is in #9529.

Approach

• small app.go
• declarative app.yaml
• an example Module proto file
• Module implementation. all of the Module.Provide* methods receive dependencies from the dependency injection container and provide dependencies back into the container for other modules

This uses Uber's dig dependency injection container under the hood.

Benefits

This will allow us to create a very simple app.go with the existing modules we have in the SDK without any bigger refactorings. We simply need to add the Module config object for each module with its Provide* methods and then this will work.

This means that it's likely we can deliver this relatively quickly. It's not even API breaking and could theoretically be backported to 0.42 or 0.43.

Possible Drawbacks

The main possible drawbacks to this approach are:

• learning curve - users will need to understand how to properly write dependency injection Provide* methods and learn about dig's annotations
• hard to know if you have all dependencies statically. But, all dependency resolution is done as soon as the app starts and dig provides reasonable error messages

Plan

If we stakeholders feel that this is a valuable improvement and that the developer UX for this is reasonable, I would like to move this forward relatively quickly (i.e. over the summer) and starting refining all the details. Let me know what you think.

[alexanderbez]

Would you be able to setup a live walkthrough and demonstrate how this works and how to build off of it? I think the idea is neat!

[aaronc]

I'd like to do a walkthrough in the architecture call next Friday if people are interested. We also have a work group Thursday mornings if you'd like to join @alexanderbez.

[alexanderbez]

Great! I'll listen in at the architecture call 👍

[ruhatch]

This looks like a great improvement over the existing app.go.

I really like the structure of the new module.go files and how they force you to clearly map inputs and outputs of the module in one place, rather than spread across various files and invoke in different parts of the app.go.

Do you think it will be easy to spin up testing frameworks with this as well?

[aaronc]

Do you think it will be easy to spin up testing frameworks with this as well?

That's the idea. I'm thinking of something similar to the Fixture setup in regen-ledger.

[fadeev]

From the developer experience perspective there are two things we are most concerned about:

• Programmatically adding modules to a chain without placeholders
• Programmatically adding features to a chain without placeholders

In the latest presented design the way modules will be handled is by:

• Having a module registry that maps names to URLs and metadata
• YAML file inside the source code that lists modules
• Metadata about the module inside the module's source code
• Starport commands for installing modules
• Automatically regenerated app.go file that is not meant to be edited by hand (will be generated by Starport)

So far, so good. I would still prefer to have Starport's YAML config and modules YAML file be in the same file, but that's not a dealbreaker.

Programmatically adding features is another story. Instead of making proposals on the architecture, I'll go from end-users' perspective. Here is an example of a scaffolded vanilla chain with starport type post title body. This command adds CRUD logic for a "post" type with two fields. Proto files, handlers, keepers, everything to implement simple CRUD.

fadeev/chain@ced62c7

Problem areas (placeholders):

• proto/chain/genesis.proto
• proto/chain/query.proto
• proto/chain/tx.proto
• x/chain/client/cli/query.go
• x/chain/client/cli/tx.go
• x/chain/genesis.go and here
• x/chain/handler.go
• x/chain/types/codec.go
• x/chain/types/genesis.go

A framework user should be able to add CRUD functionality programmatically with a tool like Starport without having to rely on placeholders.

[aaronc]

So far, so good. I would still prefer to have Starport's YAML config and modules YAML file be in the same file, but that's not a dealbreaker.

I think this should be possible.

I imagine we will want to move to an ORM/Object Store type approach as default in the future with code generation to support. Would that help with some of the CRUD?

[fadeev]

Possibly! We would have to implement a simple CRUD example like the one above using the new framework to understand if the pain points have been resolved.

[fadeev]

And, of course, CRUD is just an example of what Starport scaffolds. Most developers also use message, query, and module scaffolding, which involves less code, but still relies on placeholders. CRUD is like a macro-example of all the things combined.

[fdymylja]

I'm listing here my thoughts after yesterday's call.

Pros:

• smaller app.go
• developers do not need to concern with other module internals.
• modules are more self contained.
• forward compatible approach.
• overall reduction of complexity.

Cons:

• devs need to be more aware on what they need to provide
• higher learning curve (complexity has reduced but there has also been a shift of it towards learning about dig + provide)
• endorses a pattern that should go away ASAP: sharing keepers.

Question:

• What about circular dependencies? IIRC we have some (auth.Accounts -> sdk.Core) hidden via interface aliasing.

Suggestions:

Provide a minimum viable module definition similar to poc, example:

type Module struct {
 Name string // name of module
 MsgHandler interface{} // grpc msg server
 QueryHandler interface{} // grpc query server
 GenesisHandler GenesisHandler // interface that handles genesis with methods Default/Validate/Import/Export
 TxCmd cobra.Command
 QueryCmd cobra.Command
 
 Provides interface{} // provides is what  module exposes with dig outputs
 Needs interface{} // Needs is what this module needs with dig inputs
}

A module is represented by this structure, and this structure should be what a module is.
Everything should be optional except from the name.
I assume this is more dev friendly on what needs to be provided.

Suggestion n2:

Suffer some more with messy app.go and put more efforts into ADR-033 and ADR-033 migration. This is what I feel is future proof.

[aaronc]

Thanks for sharing these thoughts @fdymylja.

What about circular dependencies? IIRC we have some (auth.Accounts -> sdk.Core) hidden via interface aliasing.

I haven't run into this yet but it could be a concern. The DI container will complain at startup if there is a circular dependency. I imagine we can work around this somehow. Do you see valid cases where we actually want to allow circular dependencies between keepers @fdymylja ?

So regarding your Module struct with Provides and Needs as interface{}, I see what you're getting at but with a DI container the type actually needs to be concretely defined for the dependencies to be resolved. If interface{} is used the container will not know which constructors to call in what order to actually resolve dependencies. So for better or worse, we do need to define the dependency types concretely as either constructor in/out params or dig.In/Out structs.

One thing that maybe we could do to simplify the learning curve, is get rid of the need for group tags. This could be done by telling the injector that certain types are automatically grouped together. i.e. if a constructor returns Handler then it knows that this will get grouped into []Handler. What do you think of that approach? Would that simplify the learning curve a bit? The downside would be that things like *cobra.Command would need to be aliased to allow for different groups (i.e. type QueryCommand *cobra.Command), but I think that's actually maybe not a bad idea. Writing group:"query" as a tag is somewhat fragile.

As for skipping DI and trying to move straight to ADR 033, I wish we could do that but at the same time the SDK is receiving rather strong demands to build new modules using the Keeper pattern to not delay new functionality (see ADR 043 for an example). Maybe I should insist more strongly that all new modules use ADR 033, but I'm sort of trying to pick my battles as I've said. If ADR 033 had more let's say "community support", it would probably be a lot easier to push it forward as "the standard" rather than just an option.

[fdymylja]

Do you see valid cases where we actually want to allow circular dependencies between keepers @fdymylja ?

There are cases, but I wouldn't say they're justified!
staking->distribution share keepers between eachother, this circular dependency is hidden via interface aliasing, so I'm not sure if it might be a problem from DI layer perspective. Ref: staking <-> distribution

So for better or worse, we do need to define the dependency types concretely as either constructor in/out params or dig.In/Out structs.

I'm not sure if I understood correctly but my idea was that, for example, Needs which is of type interface{} is a struct which embeds dig.Input. Maybe it would be simpler to understand if it could be done in this way. The general objective is to provide something that describes a module which is just partly reliant on dig.

What do you think of that approach? Would that simplify the learning curve a bit? The downside would be that things like *cobra.Command would need to be aliased to allow for different groups (i.e. type QueryCommand *cobra.Command), but I think that's actually maybe not a bad idea. Writing group:"query" as a tag is somewhat fragile.

We would need to try and see how modules look like, but so far I think it would reduce the learning curve.

If ADR 033 had more let's say "community support", it would probably be a lot easier to push it forward as "the standard" rather than just an option.

I think we should put more efforts into "ADR-033" propaganda 😸 , not only from app wiring perspective but because the more we post-pone this the harder it will be to roll it out.
The lack of an intermodule-comunication layer is causing the rise of confusing, error prone and overall bad patterns such as this. The more we let this happen and the harder it will be to remove/refactor it. Then there are a million more reasons (observability of the state machine, easier extensibility
etc), but this deserves another discussion most likely.

[hxrts]

If ADR33 becomes mandatory we'd need very clear module upgrade instructions + it would need to be coupled to a major release with other major user benefits. Similar to amino depreciation, coupling to IBC made upgrading a much easier pill to swallow. Will still be very unpopular.