module: pass route by value (#6404)

* use instance

* add some comments

* Update types/router.go

* rename Nil to Empty

* run make mocks

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Alessio Treglia <alessio@tendermint.com>

baseapp/baseapp_test.go

@@ -1650,7 +1650,7 @@ type testCustomRouter struct {
 	routes sync.Map
 }
 
-func (rtr *testCustomRouter) AddRoute(route *sdk.Route) sdk.Router {
+func (rtr *testCustomRouter) AddRoute(route sdk.Route) sdk.Router {
 	rtr.routes.Store(route.Path(), route.Handler())
 	return rtr
 }

baseapp/router.go

@@ -21,7 +21,7 @@ func NewRouter() *Router {
 
 // AddRoute adds a route path to the router with a given handler. The route must
 // be alphanumeric.
-func (rtr *Router) AddRoute(route *sdk.Route) sdk.Router {
+func (rtr *Router) AddRoute(route sdk.Route) sdk.Router {
 	if !sdk.IsAlphaNumeric(route.Path()) {
 		panic("route expressions can only contain alphanumeric characters")
 	}

tests/mocks/tendermint_tm_db_DB.go

@@ -6,7 +6,7 @@ package mocks
 
 import (
 	gomock "github.com/golang/mock/gomock"
-	db "github.com/tendermint/tm-db"
+	tm_db "github.com/tendermint/tm-db"
 	reflect "reflect"
 )
 
@@ -106,10 +106,10 @@ func (mr *MockDBMockRecorder) Has(arg0 interface{}) *gomock.Call {
 }
 
 // Iterator mocks base method
-func (m *MockDB) Iterator(arg0, arg1 []byte) (db.Iterator, error) {
+func (m *MockDB) Iterator(arg0, arg1 []byte) (tm_db.Iterator, error) {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "Iterator", arg0, arg1)
-	ret0, _ := ret[0].(db.Iterator)
+	ret0, _ := ret[0].(tm_db.Iterator)
 	ret1, _ := ret[1].(error)
 	return ret0, ret1
 }
@@ -121,10 +121,10 @@ func (mr *MockDBMockRecorder) Iterator(arg0, arg1 interface{}) *gomock.Call {
 }
 
 // NewBatch mocks base method
-func (m *MockDB) NewBatch() db.Batch {
+func (m *MockDB) NewBatch() tm_db.Batch {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "NewBatch")
-	ret0, _ := ret[0].(db.Batch)
+	ret0, _ := ret[0].(tm_db.Batch)
 	return ret0
 }
 
@@ -149,10 +149,10 @@ func (mr *MockDBMockRecorder) Print() *gomock.Call {
 }
 
 // ReverseIterator mocks base method
-func (m *MockDB) ReverseIterator(arg0, arg1 []byte) (db.Iterator, error) {
+func (m *MockDB) ReverseIterator(arg0, arg1 []byte) (tm_db.Iterator, error) {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "ReverseIterator", arg0, arg1)
-	ret0, _ := ret[0].(db.Iterator)
+	ret0, _ := ret[0].(tm_db.Iterator)
 	ret1, _ := ret[1].(error)
 	return ret0, ret1
 }

tests/mocks/types_module_module.go

@@ -437,10 +437,10 @@ func (mr *MockAppModuleMockRecorder) RegisterInvariants(arg0 interface{}) *gomoc
 }
 
 // Route mocks base method
-func (m *MockAppModule) Route() *types.Route {
+func (m *MockAppModule) Route() types.Route {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "Route")
-	ret0, _ := ret[0].(*types.Route)
+	ret0, _ := ret[0].(types.Route)
 	return ret0
 }
 

tests/mocks/types_router.go

@@ -34,7 +34,7 @@ func (m *MockRouter) EXPECT() *MockRouterMockRecorder {
 }
 
 // AddRoute mocks base method
-func (m *MockRouter) AddRoute(r *types.Route) types.Router {
+func (m *MockRouter) AddRoute(r types.Route) types.Router {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "AddRoute", r)
 	ret0, _ := ret[0].(types.Router)

types/module/module.go

@@ -141,7 +141,7 @@ type AppModule interface {
 	RegisterInvariants(sdk.InvariantRegistry)
 
 	// routes
-	Route() *sdk.Route
+	Route() sdk.Route
 	// Deprecated: use RegisterQueryService
 	QuerierRoute() string
 	// Deprecated: use RegisterQueryService
@@ -172,7 +172,7 @@ func NewGenesisOnlyAppModule(amg AppModuleGenesis) AppModule {
 func (GenesisOnlyAppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
 
 // Route empty module message route
-func (GenesisOnlyAppModule) Route() *sdk.Route { return nil }
+func (GenesisOnlyAppModule) Route() sdk.Route { return sdk.Route{} }
 
 // QuerierRoute returns an empty module querier route
 func (GenesisOnlyAppModule) QuerierRoute() string { return "" }
@@ -251,7 +251,7 @@ func (m *Manager) RegisterInvariants(ir sdk.InvariantRegistry) {
 // 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() != nil {
+		if !module.Route().Empty() {
 			router.AddRoute(module.Route())
 		}
 		if module.QuerierRoute() != "" {

types/module/module_test.go

@@ -69,7 +69,7 @@ func TestGenesisOnlyAppModule(t *testing.T) {
 	mockInvariantRegistry := mocks.NewMockInvariantRegistry(mockCtrl)
 	goam := module.NewGenesisOnlyAppModule(mockModule)
 
-	require.Nil(t, goam.Route())
+	require.True(t, goam.Route().Empty())
 	require.Empty(t, goam.QuerierRoute())
 	require.Nil(t, goam.NewQuerierHandler())
 
@@ -140,11 +140,16 @@ func TestManager_RegisterRoutes(t *testing.T) {
 	require.Equal(t, 2, len(mm.Modules))
 
 	router := mocks.NewMockRouter(mockCtrl)
-	handler1, handler2 := sdk.Handler(nil), sdk.Handler(nil)
-	mockAppModule1.EXPECT().Route().Times(2).Return(sdk.NewRoute("route1", handler1))
-	mockAppModule2.EXPECT().Route().Times(2).Return(sdk.NewRoute("route2", handler2))
-	router.EXPECT().AddRoute(gomock.Eq(sdk.NewRoute("route1", handler1))).Times(1)
-	router.EXPECT().AddRoute(gomock.Eq(sdk.NewRoute("route2", handler2))).Times(1)
+	handler1, handler2 := sdk.Handler(func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) {
+		return nil,nil
+	}), sdk.Handler(func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) {
+		return nil,nil
+	})
+	route1 := sdk.NewRoute("route1", handler1)
+	route2 := sdk.NewRoute("route2", handler2)
+	mockAppModule1.EXPECT().Route().Times(2).Return(route1)
+	mockAppModule2.EXPECT().Route().Times(2).Return(route2)
+	router.EXPECT().AddRoute(gomock.Any()).Times(2) // Use of Any due to limitations to compare Functions as the sdk.Handler
 
 	queryRouter := mocks.NewMockQueryRouter(mockCtrl)
 	mockAppModule1.EXPECT().QuerierRoute().Times(2).Return("querierRoute1")

types/router.go

@@ -1,6 +1,9 @@
 package types
 
-import "regexp"
+import (
+	"regexp"
+	"strings"
+)
 
 var (
 	// IsAlphaNumeric defines a regular expression for matching against alpha-numeric
@@ -26,7 +29,7 @@ var (
 
 // Router provides handlers for each transaction type.
 type Router interface {
-	AddRoute(r *Route) Router
+	AddRoute(r Route) Router
 	Route(ctx Context, path string) Handler
 }
 
@@ -35,18 +38,26 @@ type Route struct {
 	handler Handler
 }
 
-func NewRoute(p string, h Handler) *Route {
-	return &Route{path: p, handler: h}
+// NewRoute returns an instance of Route.
+func NewRoute(p string, h Handler) Route {
+	return Route{path: p, handler: h}
 }
 
+// Path returns the path the route has assigned.
 func (r Route) Path() string {
 	return r.path
 }
 
+// Handler returns the handler that handles the route.
 func (r Route) Handler() Handler {
 	return r.handler
 }
 
+// Empty returns true only if both handler and path are not empty.
+func (r Route) Empty() bool {
+	return r.handler == nil || r.path == strings.TrimSpace("")
+}
+
 // QueryRouter provides queryables for each query path.
 type QueryRouter interface {
 	AddRoute(r string, h Querier) QueryRouter

types/router_test.go

@@ -0,0 +1,46 @@
+package types
+
+import (
+	"github.com/stretchr/testify/assert"
+	"testing"
+)
+
+func TestNilRoute(t *testing.T) {
+	tests := []struct{
+		name string
+		route Route
+		expected bool
+	} {
+		{
+			name: "all empty",
+			route: NewRoute("", nil),
+			expected: true,
+		},
+		{
+			name: "only path",
+			route: NewRoute("some", nil),
+			expected: true,
+		},
+		{
+			name: "only handler",
+			route: NewRoute("", func(ctx Context, msg Msg) (*Result, error) {
+				return nil, nil
+			}),
+			expected: true,
+		},
+		{
+			name: "correct route",
+			route: NewRoute("some", func(ctx Context, msg Msg) (*Result, error) {
+				return nil, nil
+			}),
+			expected: false,
+		},
+	}
+
+ 	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, tt.route.Empty())
+		})
+	}
+}

x/auth/module.go

@@ -105,7 +105,7 @@ func (AppModule) Name() string {
 func (AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
 
 // Route returns the message routing key for the auth module.
-func (AppModule) Route() *sdk.Route { return nil }
+func (AppModule) Route() sdk.Route { return sdk.Route{} }
 
 // QuerierRoute returns the auth module's querier route name.
 func (AppModule) QuerierRoute() string {

x/bank/module.go

@@ -108,7 +108,7 @@ func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) {
 }
 
 // Route returns the message routing key for the bank module.
-func (am AppModule) Route() *sdk.Route {
+func (am AppModule) Route() sdk.Route {
 	return sdk.NewRoute(RouterKey, NewHandler(am.keeper))
 }
 

x/capability/module.go

@@ -95,7 +95,7 @@ func (am AppModule) Name() string {
 }
 
 // Route returns the capability module's message routing key.
-func (AppModule) Route() *sdk.Route { return nil }
+func (AppModule) Route() sdk.Route { return sdk.Route{} }
 
 // QuerierRoute returns the capability module's query routing key.
 func (AppModule) QuerierRoute() string { return "" }

x/crisis/module.go

@@ -94,7 +94,7 @@ func (AppModule) Name() string {
 func (AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
 
 // Route returns the message routing key for the crisis module.
-func (am AppModule) Route() *sdk.Route {
+func (am AppModule) Route() sdk.Route {
 	return sdk.NewRoute(RouterKey, NewHandler(*am.keeper))
 }
 

x/distribution/module.go

@@ -119,7 +119,7 @@ func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) {
 }
 
 // Route returns the message routing key for the distribution module.
-func (am AppModule) Route() *sdk.Route {
+func (am AppModule) Route() sdk.Route {
 	return sdk.NewRoute(RouterKey, NewHandler(am.keeper))
 }
 

x/evidence/module.go

@@ -127,7 +127,7 @@ func (am AppModule) Name() string {
 }
 
 // Route returns the evidence module's message routing key.
-func (am AppModule) Route() *sdk.Route {
+func (am AppModule) Route() sdk.Route {
 	return sdk.NewRoute(RouterKey, NewHandler(am.keeper))
 }
 

x/gov/module.go

@@ -135,7 +135,7 @@ func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) {
 }
 
 // Route returns the message routing key for the gov module.
-func (am AppModule) Route() *sdk.Route {
+func (am AppModule) Route() sdk.Route {
 	return sdk.NewRoute(RouterKey, NewHandler(am.keeper))
 }
 

x/ibc-transfer/module.go

@@ -105,7 +105,7 @@ func (AppModule) RegisterInvariants(ir sdk.InvariantRegistry) {
 }
 
 // Route implements the AppModule interface
-func (am AppModule) Route() *sdk.Route {
+func (am AppModule) Route() sdk.Route {
 	return sdk.NewRoute(RouterKey, NewHandler(am.keeper))
 }
 

x/ibc/module.go

@@ -108,7 +108,7 @@ func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) {
 }
 
 // Route returns the message routing key for the ibc module.
-func (am AppModule) Route() *sdk.Route {
+func (am AppModule) Route() sdk.Route {
 	return sdk.NewRoute(RouterKey, NewHandler(*am.keeper))
 }
 

x/mint/module.go

@@ -100,7 +100,7 @@ func (AppModule) Name() string {
 func (am AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
 
 // Route returns the message routing key for the mint module.
-func (AppModule) Route() *sdk.Route { return nil }
+func (AppModule) Route() sdk.Route { return sdk.Route{} }
 
 // QuerierRoute returns the mint module's querier route name.
 func (AppModule) QuerierRoute() string {

x/params/module.go

@@ -84,7 +84,7 @@ func (am AppModule) InitGenesis(_ sdk.Context, _ codec.JSONMarshaler, _ json.Raw
 	return []abci.ValidatorUpdate{}
 }
 
-func (AppModule) Route() *sdk.Route { return nil }
+func (AppModule) Route() sdk.Route { return sdk.Route{} }
 
 // GenerateGenesisState performs a no-op.
 func (AppModule) GenerateGenesisState(simState *module.SimulationState) {}

x/slashing/module.go

@@ -110,7 +110,7 @@ func (AppModule) Name() string {
 func (am AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
 
 // Route returns the message routing key for the slashing module.
-func (am AppModule) Route() *sdk.Route {
+func (am AppModule) Route() sdk.Route {
 	return sdk.NewRoute(RouterKey, NewHandler(am.keeper))
 }
 

x/staking/module.go

@@ -134,7 +134,7 @@ func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) {
 }
 
 // Route returns the message routing key for the staking module.
-func (am AppModule) Route() *sdk.Route {
+func (am AppModule) Route() sdk.Route {
 	return sdk.NewRoute(RouterKey, NewHandler(am.keeper))
 }
 

x/upgrade/module.go

@@ -99,7 +99,7 @@ func NewAppModule(keeper keeper.Keeper) AppModule {
 func (AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
 
 // Route is empty, as we do not handle Messages (just proposals)
-func (AppModule) Route() *sdk.Route { return nil }
+func (AppModule) Route() sdk.Route { return sdk.Route{} }
 
 // QuerierRoute returns the route we respond to for abci queries
 func (AppModule) QuerierRoute() string { return types.QuerierKey }