Remove ServiceMsgs from ADR-031

CHANGELOG.md

@@ -36,7 +36,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
-## Features
+### Features
+
 * [\#8965](https://github.com/cosmos/cosmos-sdk/pull/8965) cosmos reflection now provides more information on the application such as: deliverable msgs, sdk.Config info etc (still in alpha stage).
 * [\#8559](https://github.com/cosmos/cosmos-sdk/pull/8559) Added Protobuf compatible secp256r1 ECDSA signatures.
 * [\#8786](https://github.com/cosmos/cosmos-sdk/pull/8786) Enabled secp256r1 in x/auth.
@@ -45,6 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [#9088](https://github.com/cosmos/cosmos-sdk/pull/9088) Added implementation to ADR-28 Derived Addresses.
 
 ### Client Breaking Changes
+
 * [\#8363](https://github.com/cosmos/cosmos-sdk/pull/8363) Addresses no longer have a fixed 20-byte length. From the SDK modules' point of view, any 1-255 bytes-long byte array is a valid address.
 * [\#8346](https://github.com/cosmos/cosmos-sdk/pull/8346) All CLI `tx` commands generate ServiceMsgs by default. Graceful Amino support has been added to ServiceMsgs to support signing legacy Msgs.
 * (crypto/ed25519) [\#8690] Adopt zip1215 ed2559 verification rules.
@@ -83,7 +85,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (auth/tx) [\#8926](https://github.com/cosmos/cosmos-sdk/pull/8926) The `ProtoTxProvider` interface used as a workaround for transaction simulation has been removed.
 * (x/bank) [\#8798](https://github.com/cosmos/cosmos-sdk/pull/8798) `GetTotalSupply` is removed in favour of `GetPaginatedTotalSupply`
 * (x/bank/types) [\#9061](https://github.com/cosmos/cosmos-sdk/pull/9061) `AddressFromBalancesStore` now returns an error for invalid key instead of panic.
-
+* [\#9139](https://github.com/cosmos/cosmos-sdk/pull/9139) ServiceMsgs as defined in ADR-031 have been removed, so that the SDK adheres to the Protobuf spec of `Any` packing. This has multiple consequences:
+  * The `sdk.ServiceMsg` interface has been removed.
+  * `sdk.Msg` now only contains `ValidateBasic` and `GetSigners` methods. The remaining methods `GetSignBytes`, `Route` and `Type` are moved to `legacytx.LegacyMsg`.
+  * The `RegisterCustomTypeURL` function and the `cosmos.base.v1beta1.ServiceMsg` interface have been removed from the interface registry.
 
 
 ### State Machine Breaking

baseapp/baseapp.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/store/rootmulti"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+	"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"
 )
 
 const (
@@ -703,37 +704,35 @@ func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, mode runTxMode) (*s
 		}
 
 		var (
-			msgEvents sdk.Events
-			msgResult *sdk.Result
-			msgFqName string
-			err       error
+			msgResult      *sdk.Result
+			msgEventAction string // name to use as value in event `message.action`
+			err            error
 		)
 
-		if svcMsg, ok := msg.(sdk.ServiceMsg); ok {
-			msgFqName = svcMsg.MethodName
-			handler := app.msgServiceRouter.Handler(msgFqName)
-			if handler == nil {
-				return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized message service method: %s; message index: %d", msgFqName, i)
-			}
-			msgResult, err = handler(ctx, svcMsg.Request)
-		} else {
+		if handler := app.msgServiceRouter.Handler(msg); handler != nil {
+			// ADR 031 request type routing
+			msgResult, err = handler(ctx, msg)
+			msgEventAction = sdk.MsgName(msg)
+		} else if legacyMsg, ok := msg.(legacytx.LegacyMsg); ok {
 			// legacy sdk.Msg routing
-			msgRoute := msg.Route()
-			msgFqName = msg.Type()
+			msgRoute := legacyMsg.Route()
+			msgEventAction = legacyMsg.Type()
 			handler := app.router.Route(ctx, msgRoute)
 			if handler == nil {
 				return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized message route: %s; message index: %d", msgRoute, i)
 			}
 
 			msgResult, err = handler(ctx, msg)
+		} else {
+			return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "can't route message %+v", msg)
 		}
 
 		if err != nil {
 			return nil, sdkerrors.Wrapf(err, "failed to execute message; message index: %d", i)
 		}
 
-		msgEvents = sdk.Events{
-			sdk.NewEvent(sdk.EventTypeMessage, sdk.NewAttribute(sdk.AttributeKeyAction, msgFqName)),
+		msgEvents := sdk.Events{
+			sdk.NewEvent(sdk.EventTypeMessage, sdk.NewAttribute(sdk.AttributeKeyAction, msgEventAction)),
 		}
 		msgEvents = msgEvents.AppendEvents(msgResult.GetEvents())
 
@@ -743,7 +742,7 @@ func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, mode runTxMode) (*s
 		// separate each result.
 		events = events.AppendEvents(msgEvents)
 
-		txMsgData.Data = append(txMsgData.Data, &sdk.MsgData{MsgType: msg.Type(), Data: msgResult.Data})
+		txMsgData.Data = append(txMsgData.Data, &sdk.MsgData{MsgType: sdk.MsgName(msg), Data: msgResult.Data})
 		msgLogs = append(msgLogs, sdk.NewABCIMessageLog(uint32(i), msgResult.Log, msgEvents))
 	}
 

baseapp/msg_service_router.go

@@ -29,12 +29,17 @@ func NewMsgServiceRouter() *MsgServiceRouter {
 }
 
 // MsgServiceHandler defines a function type which handles Msg service message.
-type MsgServiceHandler = func(ctx sdk.Context, req sdk.MsgRequest) (*sdk.Result, error)
+type MsgServiceHandler = func(ctx sdk.Context, req sdk.Msg) (*sdk.Result, error)
 
-// Handler returns the MsgServiceHandler for a given query route path or nil
+// Handler returns the MsgServiceHandler for a given msg or nil if not found.
+func (msr *MsgServiceRouter) Handler(msg sdk.Msg) MsgServiceHandler {
+	return msr.routes[sdk.MsgTypeURL(msg)]
+}
+
+// HandlerbyTypeURL returns the MsgServiceHandler for a given query route path or nil
 // if not found.
-func (msr *MsgServiceRouter) Handler(methodName string) MsgServiceHandler {
-	return msr.routes[methodName]
+func (msr *MsgServiceRouter) HandlerbyTypeURL(msgName string) MsgServiceHandler {
+	return msr.routes[msgName]
 }
 
 // RegisterService implements the gRPC Server.RegisterService method. sd is a gRPC
@@ -50,20 +55,41 @@ func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler inter
 		fqMethod := fmt.Sprintf("/%s/%s", sd.ServiceName, method.MethodName)
 		methodHandler := method.Handler
 
+		var requestTypeName string
+
+		// 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.(sdk.Msg)
+			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))
+			}
+
+			requestTypeName = sdk.MsgTypeURL(msg)
+			return nil
+		}, func(_ context.Context, _ interface{}, _ *grpc.UnaryServerInfo, _ grpc.UnaryHandler) (interface{}, error) {
+			return nil, nil
+		},
+		)
+
 		// 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 {
+		reqType, err := msr.interfaceRegistry.Resolve(requestTypeName)
+		if err != nil || reqType == 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,
+					requestTypeName,
 				),
 			)
 		}
@@ -72,7 +98,7 @@ func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler inter
 		// registered more than once, then we should error. Since we can't
 		// return an error (`Server.RegisterService` interface restriction) we
 		// panic (at startup).
-		_, found := msr.routes[fqMethod]
+		_, found := msr.routes[requestTypeName]
 		if found {
 			panic(
 				fmt.Errorf(
@@ -83,7 +109,7 @@ func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler inter
 			)
 		}
 
-		msr.routes[fqMethod] = func(ctx sdk.Context, req sdk.MsgRequest) (*sdk.Result, error) {
+		handler := func(ctx sdk.Context, req sdk.Msg) (*sdk.Result, error) {
 			ctx = ctx.WithEventManager(sdk.NewEventManager())
 			interceptor := func(goCtx context.Context, _ interface{}, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
 				goCtx = context.WithValue(goCtx, sdk.SdkContextKey, ctx)
@@ -103,6 +129,8 @@ func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler inter
 
 			return sdk.WrapServiceResult(ctx, resMsg, err)
 		}
+
+		msr.routes[requestTypeName] = handler
 	}
 }
 

baseapp/msg_service_router_test.go

@@ -80,11 +80,11 @@ func TestMsgService(t *testing.T) {
 	)
 	_ = app.BeginBlock(abci.RequestBeginBlock{Header: tmproto.Header{Height: 1}})
 
-	msg := testdata.NewServiceMsgCreateDog(&testdata.MsgCreateDog{Dog: &testdata.Dog{Name: "Spot"}})
+	msg := testdata.MsgCreateDog{Dog: &testdata.Dog{Name: "Spot"}}
 	txBuilder := encCfg.TxConfig.NewTxBuilder()
 	txBuilder.SetFeeAmount(testdata.NewTestFeeAmount())
 	txBuilder.SetGasLimit(testdata.NewTestGasLimit())
-	err := txBuilder.SetMsgs(msg)
+	err := txBuilder.SetMsgs(&msg)
 	require.NoError(t, err)
 
 	// First round: we gather all the signer infos. We use the "set empty

client/tx/legacy_test.go

@@ -13,7 +13,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/simapp/params"
 	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	"github.com/cosmos/cosmos-sdk/types"
-	sdk "github.com/cosmos/cosmos-sdk/types"
 	signing2 "github.com/cosmos/cosmos-sdk/types/tx/signing"
 	"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"
 	"github.com/cosmos/cosmos-sdk/x/auth/signing"
@@ -39,10 +38,7 @@ var (
 		},
 	}
 	msg0 = banktypes.NewMsgSend(addr1, addr2, types.NewCoins(types.NewInt64Coin("wack", 1)))
-	msg1 = sdk.ServiceMsg{
-		MethodName: "/cosmos.bank.v1beta1.Msg/Send",
-		Request:    banktypes.NewMsgSend(addr1, addr2, types.NewCoins(types.NewInt64Coin("wack", 2))),
-	}
+	msg1 = banktypes.NewMsgSend(addr1, addr2, types.NewCoins(types.NewInt64Coin("wack", 2)))
 )
 
 func buildTestTx(t *testing.T, builder client.TxBuilder) {
@@ -88,7 +84,7 @@ func (s *TestSuite) TestCopyTx() {
 	s.Require().Equal(sigsV2_1, sigsV2_2)
 	s.Require().Equal(protoBuilder.GetTx().GetSigners(), protoBuilder2.GetTx().GetSigners())
 	s.Require().Equal(protoBuilder.GetTx().GetMsgs()[0], protoBuilder2.GetTx().GetMsgs()[0])
-	s.Require().Equal(protoBuilder.GetTx().GetMsgs()[1].(sdk.ServiceMsg).Request, protoBuilder2.GetTx().GetMsgs()[1]) // We lose the "ServiceMsg" information
+	s.Require().Equal(protoBuilder.GetTx().GetMsgs()[1], protoBuilder2.GetTx().GetMsgs()[1])
 
 	// amino -> proto -> amino
 	aminoBuilder = s.aminoCfg.NewTxBuilder()
@@ -120,7 +116,7 @@ func (s *TestSuite) TestConvertTxToStdTx() {
 	s.Require().Equal(gas, stdTx.Fee.Gas)
 	s.Require().Equal(fee, stdTx.Fee.Amount)
 	s.Require().Equal(msg0, stdTx.Msgs[0])
-	s.Require().Equal(msg1.Request, stdTx.Msgs[1])
+	s.Require().Equal(msg1, stdTx.Msgs[1])
 	s.Require().Equal(timeoutHeight, stdTx.TimeoutHeight)
 	s.Require().Equal(sig.PubKey, stdTx.Signatures[0].PubKey)
 	s.Require().Equal(sig.Data.(*signing2.SingleSignatureData).Signature, stdTx.Signatures[0].Signature)
@@ -140,7 +136,7 @@ func (s *TestSuite) TestConvertTxToStdTx() {
 	s.Require().Equal(gas, stdTx.Fee.Gas)
 	s.Require().Equal(fee, stdTx.Fee.Amount)
 	s.Require().Equal(msg0, stdTx.Msgs[0])
-	s.Require().Equal(msg1.Request, stdTx.Msgs[1])
+	s.Require().Equal(msg1, stdTx.Msgs[1])
 	s.Require().Equal(timeoutHeight, stdTx.TimeoutHeight)
 	s.Require().Empty(stdTx.Signatures)
 

codec/types/interface_registry.go

@@ -46,17 +46,6 @@ type InterfaceRegistry interface {
 	//  registry.RegisterImplementations((*sdk.Msg)(nil), &MsgSend{}, &MsgMultiSend{})
 	RegisterImplementations(iface interface{}, impls ...proto.Message)
 
-	// RegisterCustomTypeURL allows a protobuf message to be registered as a
-	// google.protobuf.Any with a custom typeURL (besides its own canonical
-	// typeURL). iface should be an interface as type, as in RegisterInterface
-	// and RegisterImplementations.
-	//
-	// Ex:
-	// This will allow us to pack service methods in Any's using the full method name
-	// as the type URL and the request body as the value, and allow us to unpack
-	// such packed methods using the normal UnpackAny method for the interface iface.
-	RegisterCustomTypeURL(iface interface{}, typeURL string, impl proto.Message)
-
 	// ListAllInterfaces list the type URLs of all registered interfaces.
 	ListAllInterfaces() []string
 
@@ -127,15 +116,6 @@ func (registry *interfaceRegistry) RegisterImplementations(iface interface{}, im
 	}
 }
 
-// RegisterCustomTypeURL registers a concrete type which implements the given
-// interface under `typeURL`.
-//
-// This function PANICs if different concrete types are registered under the
-// same typeURL.
-func (registry *interfaceRegistry) RegisterCustomTypeURL(iface interface{}, typeURL string, impl proto.Message) {
-	registry.registerImpl(iface, typeURL, impl)
-}
-
 // registerImpl registers a concrete type which implements the given
 // interface under `typeURL`.
 //

codec/types/types_test.go

@@ -1,18 +1,13 @@
 package types_test
 
 import (
-	"context"
-	"fmt"
 	"strings"
 	"testing"
 
-	"github.com/gogo/protobuf/grpc"
 	"github.com/gogo/protobuf/jsonpb"
 	"github.com/gogo/protobuf/proto"
 	"github.com/stretchr/testify/require"
-	grpc2 "google.golang.org/grpc"
 
-	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/codec/types"
 	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 )
@@ -185,60 +180,3 @@ func TestAny_ProtoJSON(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, spot, ha2.Animal.GetCachedValue())
 }
-
-// this instance of grpc.ClientConn is used to test packing service method
-// requests into Any's
-type testAnyPackClient struct {
-	any               types.Any
-	interfaceRegistry types.InterfaceRegistry
-}
-
-var _ grpc.ClientConn = &testAnyPackClient{}
-
-func (t *testAnyPackClient) Invoke(_ context.Context, method string, args, _ interface{}, _ ...grpc2.CallOption) error {
-	reqMsg, ok := args.(proto.Message)
-	if !ok {
-		return fmt.Errorf("can't proto marshal %T", args)
-	}
-
-	// registry the method request type with the interface registry
-	t.interfaceRegistry.RegisterCustomTypeURL((*interface{})(nil), method, reqMsg)
-
-	bz, err := proto.Marshal(reqMsg)
-	if err != nil {
-		return err
-	}
-
-	t.any.TypeUrl = method
-	t.any.Value = bz
-
-	return nil
-}
-
-func (t *testAnyPackClient) NewStream(context.Context, *grpc2.StreamDesc, string, ...grpc2.CallOption) (grpc2.ClientStream, error) {
-	return nil, fmt.Errorf("not supported")
-}
-
-func TestAny_ServiceRequestProtoJSON(t *testing.T) {
-	interfaceRegistry := types.NewInterfaceRegistry()
-	anyPacker := &testAnyPackClient{interfaceRegistry: interfaceRegistry}
-	dogMsgClient := testdata.NewMsgClient(anyPacker)
-	_, err := dogMsgClient.CreateDog(context.Background(), &testdata.MsgCreateDog{Dog: &testdata.Dog{
-		Name: "spot",
-	}})
-	require.NoError(t, err)
-
-	// marshal JSON
-	cdc := codec.NewProtoCodec(interfaceRegistry)
-	bz, err := cdc.MarshalJSON(&anyPacker.any)
-	require.NoError(t, err)
-	require.Equal(t,
-		`{"@type":"/testdata.Msg/CreateDog","dog":{"size":"","name":"spot"}}`,
-		string(bz))
-
-	// unmarshal JSON
-	var any2 types.Any
-	err = cdc.UnmarshalJSON(bz, &any2)
-	require.NoError(t, err)
-	require.Equal(t, anyPacker.any, any2)
-}