Rethink configurator registration

[amaury1093]

Is your feature request related to a problem? Please describe.

Modules right now implement this method:

func RegisterServices(configurator servermodule.Configurator, accountKeeper AccountKeeper) {
  configurator.RegisterGenesisHandlers(...)
  configurator.RegisterInvariants(...)
  configurator.RegisterBeginBlock(...) // in the future, maybe?
}

It's easy to forget one Register*, and it's not clear what all Register* possibilities are.

ref: #270 (comment)

Describe the solution you'd like

The "old" SDK had all these inside AppModule. It has the advantage of exposing all configurations on a Module, and the skipped ones were just left empty.

One idea could be:

type ModuleConfig interface {
  RegisterMsgServer()
  RegisterQueryServer()
  InitGenesis()
  ExportGenesis()
  BeginBlocker()
}

and then var _ ModuleConfig = impl{}. For example

func (impl serverImpl) RegisterMsgServer(msgServer gogogrpc.Server) error {
  return group.RegisterMsgServer(msgServer, impl)
}

func (impl serverImpl) InitGenesis(ctx types.Context, cdc codec.JSONMarshaler, data json.RawMessage) []abci.ValidatorUpdate {
  // do logic
}

Describe alternatives you've considered

• separate interfaces for genesis, invariants... feat: Add genesis support for new modules and group genesis #270 (comment)
• "we could also have one single entry method for the configurator to register genesis handlers, invariants, etc." feat: Add genesis support for new modules and group genesis #270 (comment)

Additional context
Add any other context or screenshots about the feature request here.

[aaronc]

I think for better compatibility between "old" and "new" (ADR 033) modules, we might want to do something like this:

type Module interface {
  Instantiate(Config) AppModule
}

type Config interface {
  ModuleKey() ModuleKey
  Codec() codec.Marshaler
}

type AppModule interface {
  RegisterMsgServer()
  RegisterQueryServer()
  InitGenesis()
  ExportGenesis()
  BeginBlocker()
}

Basically the idea is that we drop the idea of moving everything into RegisterServices and try to reuse the existing AppModule where possible. Then there would be two ways of registering modules:

• new modules would implement Module and return an AppModule from the Instantiate method
• older modules would setup their AppModule with a ModuleKey separately, possibly wiring other keeper dependencies, and register the AppModule directly OR have the AppModule implement Module and the Instantiate method just returns itself

[robert-zaremba]

Related: #270 (comment) and #270 (comment)

The issue is not only about forgetting to register on something - but in general - complexifing the data flow and duplicating functionality (module.Manager vs ModuleManager vs Configurator).

[robert-zaremba]

Also, related for refactore: #273 and #251

[robert-zaremba]

I think for better compatibility between "old" and "new" (ADR 033) modules, we might want to do something like this:

This works.
@aaronc I assume that in your proposal the RegisterMsgServer takes as a parameters configurator or grpc.Server). In this case we still mix a building responsibility between the module itself (module has to register itself in multiple steps) and

I was also thinking about a declarative approach:

• a module doesn't register itself, instead it returns required object. With that, the module composition and building process is fully handled by the Manager / Composer.

  type AppModule interface {
    MsgSrv() interface{}
    QuerySrv() interface{}
  }
  

• unify types.module.Manager with regen/types.module.server.Manager. Maybe we can create a concept of Composer? This way, maybe, we won't need per module configurator object (as it is currently done in regen/types/module/server/manager.go).

• remove regen/types/module.TypeModule

[aaronc]

type AppModule interface {
MsgSrv() interface{}
QuerySrv() interface{}
}

That doesn't really work based on the way gRPC server registration works. And a single module may want to register multiple services. The simplest reasonable is I think:

type AppModule interface {
  RegisterMsgServices(grpc.ServiceRegistrar)
  RegisterQueryServices(grpc.ServiceRegistrar)

Although I do want to note that a benefit of the current RegisterServices approach is that it prevents forcing modules to implement new methods when AppModule supports new things. Either way that is something I want to avoid. I see two ways to do that: a) RegisterServices or b) interface casting - i.e we add a new interface (ex. MigrationsModule) every time a new method is added and the manager casts to check if modules support the new functionality. I'm not sure b) is better than a) to be honest which is why I went with RegisterServices.

[robert-zaremba]

We discussed this subject during Regen Architecture Meeting -> notes.

Summary

• We agree that instead of having 4 module managers (BaseManager, module.Manager, regen/Manager, Configurator) we see a need to experiment and create one Manager / Composer
• The idea would be to:
  • copy the module.Manager from SDK and rework it without "depending" on other managers. Goal - have only one manager / composer. We can still use Configurator - but not expose it (and not define a public interface).
• We discussed an abstract factory for a module initializer object.

[robert-zaremba]

@aaronc - after thinking a bit more, I see more need for the module initializer / abstract factory. Main reason for that is test-ability. This would allow to have more options for integration tests.

[robert-zaremba]

Maybe it would be easier for tests to mock clients rather than server? If so, client would need to be a constructor parameter, rather than created in constructor as it's done in the example we were discussing today.

[robert-zaremba]

Interesting thing pointed by AiB team is a control of module execution order in BeginBlocker and EndBlocker (control which modules execute first).
cc: @fdymylja , @jgimeno

[aaronc]

Yeah I've been thinking about that as well. Rather than an explicit ordering list could a BeginBlocker declare its dependencies and then a dependency graph determines the order deterministically?

[robert-zaremba]

Exactly - there is no reason for disallowing that.

[aaronc]

Actually in some cases its not explicit dependencies I realize. For instance, the x/upgrade BeginBlocker always needs to run before any other BeginBlocker.

[robert-zaremba]

The DI and App Wiring WG is creating a new way for registering module dependencies and it will be more automatic and different than we have now. Closing because the new way is not going to be compatible.