From c7d926da2dd7529582c483022039eac107090a88 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Fri, 9 Oct 2020 15:13:30 -0400 Subject: [PATCH] ADR 031: Protobuf Msg Services (#7458) * Ideas * WIP * Add ADR 031 * WIP * WIP * Revert changes * Revert changes * Revert changes * Updates * Updates * Apply suggestions from code review Co-authored-by: Amaury Martiny Co-authored-by: Anil Kumar Kammari * Add package names * Adding clarifications + Adding an abstract + Updating the first paragraph of decision * Update Context and wording * Update wording * Address proto naming Co-authored-by: Amaury Martiny Co-authored-by: Anil Kumar Kammari Co-authored-by: Robert Zaremba Co-authored-by: Cory --- docs/architecture/README.md | 3 +- .../adr-021-protobuf-query-encoding.md | 4 +- docs/architecture/adr-031-msg-service.md | 238 ++++++++++++++++++ 3 files changed, 242 insertions(+), 3 deletions(-) create mode 100644 docs/architecture/adr-031-msg-service.md diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 2e17fb394..a2bce8f77 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -54,4 +54,5 @@ Please add a entry below in your Pull Request for an ADR. - [ADR 025: IBC Passive Channels](./adr-025-ibc-passive-channels.md) - [ADR 026: IBC Client Recovery Mechanisms](./adr-026-ibc-client-recovery-mechanisms.md) - [ADR 027: Deterministic Protobuf Serialization](./adr-027-deterministic-protobuf-serialization.md) -- [ADR 029: Fee Grant Module](./adr-029-fee-grant-module.md) \ No newline at end of file +- [ADR 029: Fee Grant Module](./adr-029-fee-grant-module.md) +- [ADR 031: Protobuf Msg Services](./adr-031-msg-service.md) diff --git a/docs/architecture/adr-021-protobuf-query-encoding.md b/docs/architecture/adr-021-protobuf-query-encoding.md index 178c582df..ba0b35cd9 100644 --- a/docs/architecture/adr-021-protobuf-query-encoding.md +++ b/docs/architecture/adr-021-protobuf-query-encoding.md @@ -109,12 +109,12 @@ func (q Querier) QueryBalance(ctx context.Context, params *types.QueryBalancePar ### Custom Query Registration and Routing Query server implementations as above would be registered with `AppModule`s using -a new method `RegisterQueryServer(grpc.Server)` which could be implemented simply +a new method `RegisterQueryService(grpc.Server)` which could be implemented simply as below: ```go // x/bank/module.go -func (am AppModule) RegisterQueryServer(server grpc.Server) { +func (am AppModule) RegisterQueryService(server grpc.Server) { types.RegisterQueryServer(server, keeper.Querier{am.keeper}) } ``` diff --git a/docs/architecture/adr-031-msg-service.md b/docs/architecture/adr-031-msg-service.md new file mode 100644 index 000000000..bd67d090d --- /dev/null +++ b/docs/architecture/adr-031-msg-service.md @@ -0,0 +1,238 @@ +# ADR 031: Protobuf Msg Services + +## Changelog + +- 2020-10-05: Initial Draft + +## Status + +Proposed + +## Abstract + +We want to leverage protobuf `service` definitions for defining `Msg`s which will give us significant developer UX +improvements in terms of the code that is generated and the fact that return types will now be well defined. + +## Context + +Currently `Msg` handlers in the Cosmos SDK do have return values that are placed in the `data` field of the response. +These return values, however, are not specified anywhere except in the golang handler code. + +In early conversations [it was proposed](https://docs.google.com/document/d/1eEgYgvgZqLE45vETjhwIw4VOqK-5hwQtZtjVbiXnIGc/edit) +that `Msg` return types be captured using a protobuf extension field, ex: + +```protobuf +package cosmos.gov; + +message MsgSubmitProposal + option (cosmos_proto.msg_return) = “uint64”; + string delegator_address = 1; + string validator_address = 2; + repeated sdk.Coin amount = 3; +} +``` + +This was never adopted, however. + +Having a well-specified return value for `Msg`s would improve client UX. For instance, +in `x/gov`, `MsgSubmitProposal` returns the proposal ID as a big-endian `uint64`. +This isn’t really documented anywhere and clients would need to know the internals +of the SDK to parse that value and return it to users. + +Also, there may be cases where we want to use these return values programatically. +For instance, https://github.com/cosmos/cosmos-sdk/issues/7093 proposes a method for +doing inter-module Ocaps using the `Msg` router. A well-defined return type would +improve the developer UX for this approach. + +In addition, handler registration of `Msg` types tends to add a bit of +boilerplate on top of keepers and is usually done through manual type switches. +This isn't necessarily bad, but it does add overhead to creating modules. + +## Decision + +We decide to use protobuf `service` definitions for defining `Msg`s as well as +the code generated by them as a replacement for `Msg` handlers. + +Below we define how this will look for the `SubmitProposal` message from `x/gov` module. +We start with a `Msg` `service` definition: + +```proto +package cosmos.gov; + +service Msg { + rpc SubmitProposal(MsgSubmitProposal) returns (MsgSubmitProposalResponse); +} + +// Note that for backwards compatibility this uses MsgSubmitProposal as the request +// type instead of the more canonical MsgSubmitProposalRequest +message MsgSubmitProposal { + google.protobuf.Any content = 1; + string proposer = 2; +} + +message MsgSubmitProposalResponse { + uint64 proposal_id; +} +``` + +While this is most commonly used for gRPC, overloading protobuf `service` definitions like this does not violate +the intent of the [protobuf spec](https://developers.google.com/protocol-buffers/docs/proto3#services) which says: +> If you don’t want to use gRPC, it’s also possible to use protocol buffers with your own RPC implementation. +With this approach, we would get an auto-generated `MsgServer` interface: + +In addition to clearly specifying return types, this has the benefit of generating client and server code. On the server +side, this is almost like an automatically generated keeper method and could maybe be used intead of keepers eventually +(see [\#7093](https://github.com/cosmos/cosmos-sdk/issues/7093)): + +```go +package gov + +type MsgServer interface { + SubmitProposal(context.Context, *MsgSubmitProposal) (*MsgSubmitProposalResponse, error) +} +``` + +On the client side, developers could take advantage of this by creating RPC implementations that encapsulate transaction +logic. Protobuf libraries that use asynchronous callbacks, like [protobuf.js](https://github.com/protobufjs/protobuf.js#using-services) +could use this to register callbacks for specific messages even for transactions that include multiple `Msg`s. + +For backwards compatibility, existing `Msg` types should be used as the request parameter +for `service` definitions. Newer `Msg` types which only support `service` definitions +should use the more canonical `Msg...Request` names. + +### Encoding + +Currently, we are encoding `Msg`s as `Any` in `Tx`s which involves packing the +binary-encoded `Msg` with its type URL. + +The type URL for `MsgSubmitProposal` based on the proto3 spec is `/cosmos.gov.MsgSubmitProposal`. + +The fully-qualified name for the `SubmitProposal` service method above (also +based on the proto3 and gRPC specs) is `/cosmos.gov.Msg/SubmitProposal` which varies +by a single `/` character. The generated `.pb.go` files for protobuf `service`s +include names of this form and any compliant protobuf/gRPC code generator will +generate the same name. + +In order to encode service methods in transactions, we encode them as `Any`s in +the same `TxBody.messages` field as other `Msg`s. We simply set `Any.type_url` +to the full-qualified method name (ex. `/cosmos.gov.Msg/SubmitProposal`) and +set `Any.value` to the protobuf encoding of the request message +(`MsgSubmitProposal` in this case). + +### Decoding + +When decoding, `TxBody.UnpackInterfaces` will need a special case +to detect if `Any` type URLs match the service method format (ex. `/cosmos.gov.Msg/SubmitProposal`) +by checking for two `/` characters. Messages that are method names plus request parameters +instead of a normal `Any` messages will get unpacked into the `ServiceMsg` struct: + +```go +type ServiceMsg struct { + // MethodName is the fully-qualified service name + MethodName string + // Request is the request payload + Request MsgRequest +} +``` + +### Routing + +In the future, `service` definitions may become the primary method for defining +`Msg`s. As a starting point, we need to integrate with the SDK's existing routing +and `Msg` interface. + +To do this, `ServiceMsg` implements the `sdk.Msg` interface and its handler does the +actual method routing, allowing this feature to be added incrementally on top of +existing functionality. + +### `MsgRequest` interface + +All request messages will need to implement the `MsgRequest` interface which is a +simplified version of `Msg`, without `Route()`, `Type()` and `GetSignBytes()` which +are no longer needed: + +```go +type MsgRequest interface { + proto.Message + ValidateBasic() error + GetSigners() []AccAddress +} +``` + +`ServiceMsg` will forward its `ValidateBasic` and `GetSigners` methods to the `MsgRequest` +methods. + +### Module Configuration + +In [ADR 021](./adr-021-protobuf-query-encoding.md), we introduced a method `RegisterQueryService` +to `AppModule` which allows for modules to register gRPC queriers. + +To register `Msg` services, we attempt a more extensible approach by converting `RegisterQueryService` +to a more generic `RegisterServices` method: + +```go +type AppModule interface { + RegisterServices(Configurator) + ... +} + +type Configurator interface { + QueryServer() grpc.Server + MsgServer() grpc.Server +} + +// example module: +func (am AppModule) RegisterServices(cfg Configurator) { + types.RegisterQueryServer(cfg.QueryServer(), keeper) + types.RegisterMsgServer(cfg.MsgServer(), keeper) +} +``` + +The `RegisterServices` method and the `Configurator` interface are intended to +evolve to satisfy the use cases discussed in [\#7093](https://github.com/cosmos/cosmos-sdk/issues/7093) +and [\#7122](https://github.com/cosmos/cosmos-sdk/issues/7421). + +When `Msg` services are registered, the framework _should_ verify that all `Msg...Request` types +implement the `MsgRequest` interface described above and throw an error during initialization rather +than later when transactions are processed. + +### `Msg` Service Implementation + +Just like query services, `Msg` service methods can retrieve the `sdk.Context` +from the `context.Context` parameter method using the `sdk.UnwrapSDKContext` +method: + +```go +package gov + +func (k Keeper) SubmitProposal(goCtx context.Context, params *types.MsgSubmitProposal) (*MsgSubmitProposalResponse, error) { + ctx := sdk.UnwrapSDKContext(goCtx) + ... +} +``` + +The `sdk.Context` should have an `EventManager` already attached by the `ServiceMsg` +router. + +Separate handler definition is no longer needed with this approach. + +## Consequences + +### Pros +- communicates return type clearly +- manual handler registration and return type marshaling is no longer needed, just implement the interface and register it +- some keeper code could be automatically generate, this would improve the UX of [\#7093](https://github.com/cosmos/cosmos-sdk/issues/7093) approach (1) if we chose to adopt that +- generated client code could be useful for clients + +### Cons +- supporting both this and the current concrete `Msg` type approach simultaneously could be confusing +(we could choose to deprecate the current approach) +- using `service` definitions outside the context of gRPC could be confusing (but doesn’t violate the proto3 spec) + +## References + +- [Initial Github Issue \#7122](https://github.com/cosmos/cosmos-sdk/issues/7122) +- [proto 3 Language Guide: Defining Services](https://developers.google.com/protocol-buffers/docs/proto3#services) +- [Initial pre-`Any` `Msg` designs](https://docs.google.com/document/d/1eEgYgvgZqLE45vETjhwIw4VOqK-5hwQtZtjVbiXnIGc) +- [ADR 020](./adr-020-protobuf-transaction-encoding.md) +- [ADR 021](./adr-021-protobuf-query-encoding.md)