Proposal: Simplification of AppModule interface.

[jgimeno]

Summary

Right now in AppModule we find for the routes next methods:

	Route() string
	NewHandler() sdk.Handler
	QuerierRoute() string
	NewQuerierHandler() sdk.Querier

I think this can be simplified to only:

	NewRoute() sdk.Route
	NewQueryRoute() sdk.QueryRoute

Problem Definition

The first problem I see is that we have this Route() and QuerierRoute() that can be embedded into a more generic call.

With the spirit of simplify I saw that is very common to have empty handlers, so we find a lot of modules which provides something like (from x/auth):

// Route returns the message routing key for the auth module.
func (AppModule) Route() string { return "" }

// NewHandler returns an sdk.Handler for the auth module.
func (AppModule) NewHandler() sdk.Handler { return nil }

Another thing that can happens is that we can have an empty handler to a key, or viceversa.

With the change this is reduced to:

func (AppModule) NewHandler() sdk.Handler { return nil }

Proposal

When I looked when is Route() used I saw that is only used once, meaning that we have 2 functions that we can reduce this to one.

So where in module/module.go we have:

// RegisterRoutes registers all module routes and module querier routes
func (m *Manager) RegisterRoutes(router sdk.Router, queryRouter sdk.QueryRouter) {
	for _, module := range m.Modules {
		if module.Route() != "" {
			router.AddRoute(module.Route(), module.NewHandler())
		}
		if module.QuerierRoute() != "" {
			queryRouter.AddRoute(module.QuerierRoute(), module.NewQuerierHandler())
		}
	}
}

We can reduce it to:

// RegisterRoutes registers all module routes and module querier routes
func (m *Manager) RegisterRoutes(router sdk.Router, queryRouter sdk.QueryRouter) {
	for _, module := range m.Modules {
		if route := module.NewRoute(); route != nil {
			router.AddRoute(route)
		}

		if queryRoute := module.NewQueryRoute(); queryRoute != nil {
			queryRouter.AddRoute(queryRoute)
		}
	}
}

I have opened a PR with the basic interface concept, and if we like it we can go for it.

#6231

It will need some changes in all modules and it is a breaking change for other project modules, but I think it is simpler.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[aaronc]

What is sdk.Route defined as?

Querier routing will already change based on ADR 021/gRPC #5894.

[jgimeno]

I changed this other interfaces:

From:

// Router provides handlers for each transaction type.
type Router interface {
	AddRoute(r string, h Handler) Router
	Route(ctx Context, path string) Handler
}

// QueryRouter provides queryables for each query path.
type QueryRouter interface {
	AddRoute(r string, h Querier) QueryRouter
	Route(path string) Querier
}

To:

// Router provides handlers for each transaction type.
type Router interface {
	AddRoute(r Route) Router
	Route(ctx Context, path string) Handler
}

type Route interface {
	Name() string
	Handler() Handler
}

// QueryRouter provides queryables for each query path.
type QueryRouter interface {
	AddRoute(route QueryRoute) QueryRouter
	Route(path string) Querier
}

type QueryRoute interface {
	Name() string
	Querier() Querier
}

I don't know if this will help or not with the gRPC. Is just an idea to simplify to AppModule. Do you think it can help @aaronc ?

[aaronc]

Okay makes sense now. Route could probably just be a struct with this design.

I've always wondered why the SDK does routing the way it does in general.

Maybe that's a larger question, but why don't we just dispatch with map[reflect.Type]sdk.Handler? Then all of these string route keys would be unnecessary and we would have something like:

type Router interface {
  RegisterRoute(msg interface{}, handler sdk.Handler)
}

type AppModule interface {
  RegisterRoutes(Router)
}

// x/bank/module.go
func (am AppModule) RegisterRoutes(router Router) {
  router.RegisterRoute(MsgSend{}, handleMsgSend)
}

[alessio]

Having fewer methods in AppModule would mean making a step torwards a kind of simpler design.
I'd just keep the same name for the Route and QuerierRoute - instead of prefixing both with New (either it's an API breaking change, but at least naming would be IMO more logical).

Seconded - I like the proposal.

[alessio]

Plus, this does not seem to introduce any auto-magical, convoluted mechanisms that usually make debugging painful like hell.