Remove ServiceMsgs from ADR-031 (#9139)

* wip

* wip

* wip

* wip on refactoring adr 031 type URLs

* Fix msg_service_router

* Fix gov client queries

* Fix some modules tests

* Remove all instances of "*.Msg/*"

* Uncomment code

* Remove commented code

* // simulation.NewWeightedOperation(

* Fix CopyTx

* Fix more tests

* Fix x/gov test

* proto.MessageName->sdk.MsgName

* Fix authz tests

* Use MsgRoute in feegrant and staking/authz

* Fix more tests

* Fix sims?

* Add norace tag

* Add CL

* rebuild rosetta api test data

* Update ADR

* Rename MsgRoute -> MsgTypeURL

* Fix codec registration

* Remove sdk.GetLegacySignBytes

* Update types/tx_msg.go

* Update x/authz/simulation/operations.go

* Move LegacyMsg to legacytx

* Update CHANGELOG.md

Co-authored-by: Aaron Craelius <aaron@regen.network>

* Remove NewAnyWithCustomTypeURL

* Keep support for ServiceMsgs

* Fix TxBody UnpackInterfaces

* Fix test

* Address review

* Remove support for ServiceMsg typeURLs

* Fix lint

* Update changelog

* Fix tests

* Use sdk.MsgTypeURL everywhere

* Fix tests

* Fix rosetta, run make rosetta-data

* Fix rosetta thanks to froydi

* Address reviews

* Fix test

* Remove stray log

* Update CL

Co-authored-by: Aaron Craelius <aaronc@users.noreply.github.com>
Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Co-authored-by: Aaron Craelius <aaron@regen.network>

CHANGELOG.md

@@ -37,7 +37,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ## [Unreleased]
 * [\#9205](https://github.com/cosmos/cosmos-sdk/pull/9205) Improve readability in `abci` handleQueryP2P
 
-## 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.
@@ -48,6 +49,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/staking) [\#9214](https://github.com/cosmos/cosmos-sdk/pull/9214) Added `new_shares` attribute inside `EventTypeDelegate` event.
 
 ### 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.
@@ -59,6 +61,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
   * CLI: removed `--text` flag from `show-node-id` command; the text format for public keys is not used any more - instead we use ProtoJSON.
 * (types) [\#9079](https://github.com/cosmos/cosmos-sdk/issues/9079) Add `AddAmount`/`SubAmount` methods to `sdk.Coin`.
 * [\#8628](https://github.com/cosmos/cosmos-sdk/issues/8628) Commands no longer print outputs using `stderr` by default
+* [\#9139](https://github.com/cosmos/cosmos-sdk/pull/9139) Querying events:
+  * via `ServiceMsg` TypeURLs (e.g. `message.action='/cosmos.bank.v1beta1.Msg/Send'`) does not work anymore,
+  * via legacy `msg.Type()` (e.g. `message.action='send'`) is being deprecated, new `Msg`s won't emit these events.
+  * Please use concrete `Msg` TypeURLs instead (e.g. `message.action='/cosmos.bank.v1beta1.MsgSend'`).
 
 ### API Breaking Changes
 
@@ -92,6 +98,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
   * `codec.JSONMarshaler` → `codec.JSONCodec`
   * Removed `BinaryBare` suffix from `BinaryCodec` methods (`MarshalBinaryBare`, `UnmarshalBinaryBare`, ...)
   * Removed `Binary` infix from `BinaryCodec` methods (`MarshalBinaryLengthPrefixed`, `UnmarshalBinaryLengthPrefixed`, ...)
+* [\#9139](https://github.com/cosmos/cosmos-sdk/pull/9139) `ServiceMsg` TypeURLs (e.g. `/cosmos.bank.v1beta1.Msg/Send`) have been removed, as they don't comply to the Probobuf `Any` spec. Please use `Msg` type TypeURLs (e.g. `/cosmos.bank.v1beta1.MsgSend`). This has multiple consequences:
+  * The `sdk.ServiceMsg` struct 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.
 
 
 

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,39 @@ 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
+			eventMsgName 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)
+			eventMsgName = sdk.MsgTypeURL(msg)
+		} else if legacyMsg, ok := msg.(legacytx.LegacyMsg); ok {
 			// legacy sdk.Msg routing
-			msgRoute := msg.Route()
-			msgFqName = msg.Type()
+			// Assuming that the app developer has migrated all their Msgs to
+			// proto messages and has registered all `Msg services`, then this
+			// path should never be called, because all those Msgs should be
+			// registered within the `msgServiceRouter` already.
+			msgRoute := legacyMsg.Route()
+			eventMsgName = 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, eventMsgName)),
 		}
 		msgEvents = msgEvents.AppendEvents(msgResult.GetEvents())
 
@@ -743,7 +746,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.MsgTypeURL(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(typeURL string) MsgServiceHandler {
+	return msr.routes[typeURL]
 }
 
 // RegisterService implements the gRPC Server.RegisterService method. sd is a gRPC
@@ -50,20 +55,38 @@ 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
+		}, 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 {
+		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 +95,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 +106,7 @@ func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler inter
 			)
 		}
 
-		msr.routes[fqMethod] = func(ctx sdk.Context, req sdk.MsgRequest) (*sdk.Result, error) {
+		msr.routes[requestTypeName] = 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)
@@ -112,3 +135,6 @@ func (msr *MsgServiceRouter) SetInterfaceRegistry(interfaceRegistry codectypes.I
 }
 
 func noopDecoder(_ interface{}) error { return nil }
+func noopInterceptor(_ context.Context, _ interface{}, _ *grpc.UnaryServerInfo, _ grpc.UnaryHandler) (interface{}, error) {
+	return nil, nil
+}

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/any.go

@@ -64,20 +64,14 @@ func NewAnyWithValue(v proto.Message) (*Any, error) {
 	if v == nil {
 		return nil, sdkerrors.Wrap(sdkerrors.ErrPackAny, "Expecting non nil value to create a new Any")
 	}
-	return NewAnyWithCustomTypeURL(v, "/"+proto.MessageName(v))
-}
 
-// NewAnyWithCustomTypeURL same as NewAnyWithValue, but sets a custom type url, instead
-// using the one from proto.Message.
-// NOTE: This functions should be only used for types with additional logic bundled
-// into the protobuf Any serialization. For simple marshaling you should use NewAnyWithValue.
-func NewAnyWithCustomTypeURL(v proto.Message, typeURL string) (*Any, error) {
 	bz, err := proto.Marshal(v)
 	if err != nil {
 		return nil, err
 	}
+
 	return &Any{
-		TypeUrl:     typeURL,
+		TypeUrl:     "/" + proto.MessageName(v),
 		Value:       bz,
 		cachedValue: v,
 	}, nil

codec/types/any_test.go

@@ -23,16 +23,14 @@ func (eom *errOnMarshal) XXX_Marshal(b []byte, deterministic bool) ([]byte, erro
 	return nil, errAlways
 }
 
-const fauxURL = "/anyhere"
-
 var eom = &errOnMarshal{}
 
 // Ensure that returning an error doesn't suddenly allocate and waste bytes.
 // See https://github.com/cosmos/cosmos-sdk/issues/8537
 func TestNewAnyWithCustomTypeURLWithErrorNoAllocation(t *testing.T) {
 	var ms1, ms2 runtime.MemStats
 	runtime.ReadMemStats(&ms1)
-	any, err := types.NewAnyWithCustomTypeURL(eom, fauxURL)
+	any, err := types.NewAnyWithValue(eom)
 	runtime.ReadMemStats(&ms2)
 	// Ensure that no fresh allocation was made.
 	if diff := ms2.HeapAlloc - ms1.HeapAlloc; diff > 0 {
@@ -52,7 +50,7 @@ func BenchmarkNewAnyWithCustomTypeURLWithErrorReturned(b *testing.B) {
 	b.ResetTimer()
 	b.ReportAllocs()
 	for i := 0; i < b.N; i++ {
-		any, err := types.NewAnyWithCustomTypeURL(eom, fauxURL)
+		any, err := types.NewAnyWithValue(eom)
 		if err == nil {
 			b.Fatal("err wasn't returned")
 		}

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