RegisterInterfaces registers service Msg type_urls (cosmos#7671)

* Add RegisterMsgServiceDesc * Refactor newSendTxMsgServiceCmd * Add test * Register in all modules * Remove RegisterMsgServiceDesc from baseapp * Add explicit error message * Add comment * Update comment * Add test * Update comment * Remove duplicate import * Fix lint * Forgot vesting * Fix lint * Fix test * Put in types/module * Put in types/msgservice * Add comment about panic * Update baseapp/msg_service_router.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Update baseapp/msg_service_router.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Update baseapp/msg_service_router.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Add comment * Add better test * Update baseapp/msg_service_router.go Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com> Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
yihuang · Oct 28, 2020 · 82f15f3 · 82f15f3
1 parent 1b0d654
commit 82f15f3
Show file tree

Hide file tree

Showing 19 changed files with 259 additions and 117 deletions.
diff --git a/baseapp/msg_service_router.go b/baseapp/msg_service_router.go
@@ -39,27 +39,32 @@ func (msr *MsgServiceRouter) Handler(methodName string) MsgServiceHandler {
 
 // RegisterService implements the gRPC Server.RegisterService method. sd is a gRPC
 // service description, handler is an object which implements that gRPC service.
+//
+// This function PANICs if it is called before the service `Msg`s have been
+// registered using RegisterInterfaces.
 func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler interface{}) {
 	// Adds a top-level query handler based on the gRPC service name.
 	for _, method := range sd.Methods {
 		fqMethod := fmt.Sprintf("/%s/%s", sd.ServiceName, method.MethodName)
 		methodHandler := method.Handler
 
-		// NOTE: This is how we pull the concrete request type for each handler for registering in the InterfaceRegistry.
-		// This approach is maybe a bit hacky, but less hacky than reflecting on the handler object itself.
-		// We use a no-op interceptor to avoid actually calling into the handler itself.
-		_, _ = methodHandler(nil, context.Background(), func(i interface{}) error {
-			msg, ok := i.(proto.Message)
-			if !ok {
-				// We panic here because there is no other alternative and the app cannot be initialized correctly
-				// this should only happen if there is a problem with code generation in which case the app won't
-				// work correctly anyway.
-				panic(fmt.Errorf("can't register request type %T for service method %s", i, fqMethod))
-			}
-
-			msr.interfaceRegistry.RegisterCustomTypeURL((*sdk.MsgRequest)(nil), fqMethod, msg)
-			return nil
-		}, noopInterceptor)
+		// Check that the service Msg fully-qualified method name has already
+		// been registered (via RegisterInterfaces). If the user registers a
+		// service without registering according service Msg type, there might be
+		// some unexpected behavior down the road. Since we can't return an error
+		// (`Server.RegisterService` interface restriction) we panic (at startup).
+		serviceMsg, err := msr.interfaceRegistry.Resolve(fqMethod)
+		if err != nil || serviceMsg == nil {
+			panic(
+				fmt.Errorf(
+					"type_url %s has not been registered yet. "+
+						"Before calling RegisterService, you must register all interfaces by calling the `RegisterInterfaces` "+
+						"method on module.BasicManager. Each module should call `msgservice.RegisterMsgServiceDesc` inside its "+
+						"`RegisterInterfaces` method with the `_Msg_serviceDesc` generated by proto-gen",
+					fqMethod,
+				),
+			)
+		}
 
 		msr.routes[fqMethod] = func(ctx sdk.Context, req sdk.MsgRequest) (*sdk.Result, error) {
 			ctx = ctx.WithEventManager(sdk.NewEventManager())
@@ -89,9 +94,4 @@ func (msr *MsgServiceRouter) SetInterfaceRegistry(interfaceRegistry codectypes.I
 	msr.interfaceRegistry = interfaceRegistry
 }
 
-// gRPC NOOP interceptor
-func noopInterceptor(_ context.Context, _ interface{}, _ *grpc.UnaryServerInfo, _ grpc.UnaryHandler) (interface{}, error) {
-	return nil, nil
-}
-
 func noopDecoder(_ interface{}) error { return nil }
diff --git a/baseapp/msg_service_router_test.go b/baseapp/msg_service_router_test.go
@@ -4,25 +4,48 @@ import (
 	"os"
 	"testing"
 
-	"github.com/cosmos/cosmos-sdk/baseapp"
-
-	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
-
 	"github.com/stretchr/testify/require"
 	abci "github.com/tendermint/tendermint/abci/types"
 	"github.com/tendermint/tendermint/libs/log"
+	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
 	dbm "github.com/tendermint/tm-db"
 
+	"github.com/cosmos/cosmos-sdk/baseapp"
 	"github.com/cosmos/cosmos-sdk/client/tx"
 	"github.com/cosmos/cosmos-sdk/simapp"
 	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
 	authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
 )
 
+func TestRegisterService(t *testing.T) {
+	db := dbm.NewMemDB()
+
+	// Create an encoding config that doesn't register testdata Msg services.
+	encCfg := simapp.MakeTestEncodingConfig()
+	app := baseapp.NewBaseApp("test", log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, encCfg.TxConfig.TxDecoder())
+	app.SetInterfaceRegistry(encCfg.InterfaceRegistry)
+	require.Panics(t, func() {
+		testdata.RegisterMsgServer(
+			app.MsgServiceRouter(),
+			testdata.MsgServerImpl{},
+		)
+	})
+
+	// Register testdata Msg services, and rerun `RegisterService`.
+	testdata.RegisterInterfaces(encCfg.InterfaceRegistry)
+	require.NotPanics(t, func() {
+		testdata.RegisterMsgServer(
+			app.MsgServiceRouter(),
+			testdata.MsgServerImpl{},
+		)
+	})
+}
+
 func TestMsgService(t *testing.T) {
 	priv, _, _ := testdata.KeyTestPubAddr()
 	encCfg := simapp.MakeTestEncodingConfig()
+	testdata.RegisterInterfaces(encCfg.InterfaceRegistry)
 	db := dbm.NewMemDB()
 	app := baseapp.NewBaseApp("test", log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, encCfg.TxConfig.TxDecoder())
 	app.SetInterfaceRegistry(encCfg.InterfaceRegistry)

diff --git a/testutil/testdata/codec.go b/testutil/testdata/codec.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/cosmos/cosmos-sdk/codec/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/types/msgservice"
 )
 
 func NewTestInterfaceRegistry() types.InterfaceRegistry {
@@ -30,6 +31,8 @@ func RegisterInterfaces(registry types.InterfaceRegistry) {
 		(*HasHasAnimalI)(nil),
 		&HasHasAnimal{},
 	)
+
+	msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)
 }
 
 func NewTestAmino() *amino.Codec {

diff --git a/types/msgservice/msg_service.go b/types/msgservice/msg_service.go
@@ -0,0 +1,44 @@
+package msgservice
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/gogo/protobuf/proto"
+	"google.golang.org/grpc"
+
+	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+// RegisterMsgServiceDesc registers all type_urls from Msg services described
+// in `sd` into the registry.
+func RegisterMsgServiceDesc(registry codectypes.InterfaceRegistry, sd *grpc.ServiceDesc) {
+	// Adds a top-level type_url based on the Msg service name.
+	for _, method := range sd.Methods {
+		fqMethod := fmt.Sprintf("/%s/%s", sd.ServiceName, method.MethodName)
+		methodHandler := method.Handler
+
+		// NOTE: This is how we pull the concrete request type for each handler for registering in the InterfaceRegistry.
+		// This approach is maybe a bit hacky, but less hacky than reflecting on the handler object itself.
+		// We use a no-op interceptor to avoid actually calling into the handler itself.
+		_, _ = methodHandler(nil, context.Background(), func(i interface{}) error {
+			msg, ok := i.(proto.Message)
+			if !ok {
+				// We panic here because there is no other alternative and the app cannot be initialized correctly
+				// this should only happen if there is a problem with code generation in which case the app won't
+				// work correctly anyway.
+				panic(fmt.Errorf("can't register request type %T for service method %s", i, fqMethod))
+			}
+
+			registry.RegisterCustomTypeURL((*sdk.MsgRequest)(nil), fqMethod, msg)
+			return nil
+		}, noopInterceptor)
+
+	}
+}
+
+// gRPC NOOP interceptor
+func noopInterceptor(_ context.Context, _ interface{}, _ *grpc.UnaryServerInfo, _ grpc.UnaryHandler) (interface{}, error) {
+	return nil, nil
+}
diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go
@@ -174,9 +174,11 @@ func (s *IntegrationTestSuite) TestCLIQueryTxCmd() {
 	account2, err := val.ClientCtx.Keyring.Key("newAccount2")
 	s.Require().NoError(err)
 
-	// Send coins.
 	sendTokens := sdk.NewInt64Coin(s.cfg.BondDenom, 10)
-	out, err := bankcli.MsgSendExec(
+
+	// Send coins, try both with legacy Msg and with Msg service.
+	// Legacy Msg.
+	legacyMsgOut, err := bankcli.MsgSendExec(
 		val.ClientCtx,
 		val.Address,
 		account2.GetAddress(),
@@ -187,31 +189,55 @@ func (s *IntegrationTestSuite) TestCLIQueryTxCmd() {
 		fmt.Sprintf("--gas=%d", flags.DefaultGasLimit),
 	)
 	s.Require().NoError(err)
+	var legacyMsgTxRes sdk.TxResponse
+	s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(legacyMsgOut.Bytes(), &legacyMsgTxRes))
 
+	// Service Msg.
+	out, err := bankcli.ServiceMsgSendExec(
+		val.ClientCtx,
+		val.Address,
+		account2.GetAddress(),
+		sdk.NewCoins(sendTokens),
+		fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
+		fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
+		fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
+		fmt.Sprintf("--gas=%d", flags.DefaultGasLimit),
+	)
+	s.Require().NoError(err)
 	var txRes sdk.TxResponse
 	s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(out.Bytes(), &txRes))
 
 	s.Require().NoError(s.network.WaitForNextBlock())
 
 	testCases := []struct {
-		name      string
-		args      []string
-		expectErr bool
+		name           string
+		args           []string
+		expectErr      bool
+		rawLogContains string
 	}{
 		{
 			"with invalid hash",
 			[]string{"somethinginvalid", fmt.Sprintf("--%s=json", tmcli.OutputFlag)},
 			true,
+			"",
 		},
 		{
 			"with valid and not existing hash",
 			[]string{"C7E7D3A86A17AB3A321172239F3B61357937AF0F25D9FA4D2F4DCCAD9B0D7747", fmt.Sprintf("--%s=json", tmcli.OutputFlag)},
 			true,
+			"",
 		},
 		{
-			"happy case",
+			"happy case (legacy Msg)",
+			[]string{legacyMsgTxRes.TxHash, fmt.Sprintf("--%s=json", tmcli.OutputFlag)},
+			false,
+			"",
+		},
+		{
+			"happy case (service Msg)",
 			[]string{txRes.TxHash, fmt.Sprintf("--%s=json", tmcli.OutputFlag)},
 			false,
+			"/cosmos.bank.v1beta1.Msg/Send",
 		},
 	}
 
@@ -230,6 +256,7 @@ func (s *IntegrationTestSuite) TestCLIQueryTxCmd() {
 				var result sdk.TxResponse
 				s.Require().NoError(val.ClientCtx.JSONMarshaler.UnmarshalJSON(out.Bytes(), &result))
 				s.Require().NotNil(result.Height)
+				s.Require().Contains(result.RawLog, tc.rawLogContains)
 			}
 		})
 	}

diff --git a/x/auth/vesting/types/codec.go b/x/auth/vesting/types/codec.go
@@ -4,6 +4,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/codec/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/types/msgservice"
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 	"github.com/cosmos/cosmos-sdk/x/auth/vesting/exported"
 )
@@ -49,6 +50,8 @@ func RegisterInterfaces(registry types.InterfaceRegistry) {
 		(*sdk.Msg)(nil),
 		&MsgCreateVestingAccount{},
 	)
+
+	msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)
 }
 
 var amino = codec.NewLegacyAmino()

diff --git a/x/bank/client/cli/cli_test.go b/x/bank/client/cli/cli_test.go
@@ -5,13 +5,6 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/spf13/cobra"
-
-	"github.com/cosmos/cosmos-sdk/client/tx"
-
-	"github.com/gogo/protobuf/grpc"
-	grpc2 "google.golang.org/grpc"
-
 	"github.com/gogo/protobuf/proto"
 	"github.com/stretchr/testify/suite"
 	tmcli "github.com/tendermint/tendermint/libs/cli"
@@ -302,82 +295,6 @@ func (s *IntegrationTestSuite) TestNewSendTxCmd() {
 	}
 }
 
-// serviceMsgClientConn is an instance of grpc.ClientConn that is used to test building
-// transactions with MsgClient's. It is intended to be replaced by the work in
-// https://github.com/cosmos/cosmos-sdk/issues/7541 when that is ready.
-type serviceMsgClientConn struct {
-	msgs []sdk.Msg
-}
-
-func (t *serviceMsgClientConn) Invoke(_ context.Context, method string, args, _ interface{}, _ ...grpc2.CallOption) error {
-	req, ok := args.(sdk.MsgRequest)
-	if !ok {
-		return fmt.Errorf("%T should implement %T", args, (*sdk.MsgRequest)(nil))
-	}
-
-	err := req.ValidateBasic()
-	if err != nil {
-		return err
-	}
-
-	t.msgs = append(t.msgs, sdk.ServiceMsg{
-		MethodName: method,
-		Request:    req,
-	})
-
-	return nil
-}
-
-func (t *serviceMsgClientConn) NewStream(context.Context, *grpc2.StreamDesc, string, ...grpc2.CallOption) (grpc2.ClientStream, error) {
-	return nil, fmt.Errorf("not supported")
-}
-
-var _ grpc.ClientConn = &serviceMsgClientConn{}
-
-// newSendTxMsgServiceCmd is just for the purpose of testing ServiceMsg's in an end-to-end case. It is effectively
-// NewSendTxCmd but using MsgClient.
-func newSendTxMsgServiceCmd() *cobra.Command {
-	cmd := &cobra.Command{
-		Use: "send [from_key_or_address] [to_address] [amount]",
-		Short: `Send funds from one account to another. Note, the'--from' flag is
-ignored as it is implied from [from_key_or_address].`,
-		Args: cobra.ExactArgs(3),
-		RunE: func(cmd *cobra.Command, args []string) error {
-			cmd.Flags().Set(flags.FlagFrom, args[0])
-
-			clientCtx := client.GetClientContextFromCmd(cmd)
-			clientCtx, err := client.ReadTxCommandFlags(clientCtx, cmd.Flags())
-			if err != nil {
-				return err
-			}
-
-			toAddr, err := sdk.AccAddressFromBech32(args[1])
-			if err != nil {
-				return err
-			}
-
-			coins, err := sdk.ParseCoins(args[2])
-			if err != nil {
-				return err
-			}
-
-			msg := types.NewMsgSend(clientCtx.GetFromAddress(), toAddr, coins)
-			svcMsgClientConn := &serviceMsgClientConn{}
-			bankMsgClient := types.NewMsgClient(svcMsgClientConn)
-			_, err = bankMsgClient.Send(context.Background(), msg)
-			if err != nil {
-				return err
-			}
-
-			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), svcMsgClientConn.msgs...)
-		},
-	}
-
-	flags.AddTxFlagsToCmd(cmd)
-
-	return cmd
-}
-
 // TestBankMsgService does a basic test of whether or not service Msg's as defined
 // in ADR 031 work in the most basic end-to-end case.
 func (s *IntegrationTestSuite) TestBankMsgService() {
@@ -419,10 +336,7 @@ func (s *IntegrationTestSuite) TestBankMsgService() {
 		s.Run(tc.name, func() {
 			clientCtx := val.ClientCtx
 
-			args := []string{tc.from.String(), tc.to.String(), tc.amount.String()}
-			args = append(args, tc.args...)
-
-			bz, err := clitestutil.ExecTestCLICmd(clientCtx, newSendTxMsgServiceCmd(), args)
+			bz, err := banktestutil.ServiceMsgSendExec(clientCtx, tc.from, tc.to, tc.amount, tc.args...)
 			if tc.expectErr {
 				s.Require().Error(err)
 			} else {