From 3753d969afe0c148da38ece2c649b902158ac77f Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 6 Mar 2020 10:05:01 -0500 Subject: [PATCH 1/6] ADR 020 skeleton --- docs/architecture/README.md | 1 + .../adr-019-protobuf-state-encoding.md | 2 -- .../adr-020-protobuf-client-encoding.md | 23 +++++++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 docs/architecture/adr-020-protobuf-client-encoding.md diff --git a/docs/architecture/README.md b/docs/architecture/README.md index e327a5b64..6476ccab6 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -45,3 +45,4 @@ Please add a entry below in your Pull Request for an ADR. - [ADR 017: Historical Header Module](./adr-017-historical-header-module.md) - [ADR 018: Extendable Voting Periods](./adr-018-extendable-voting-period.md) - [ADR 019: Protocol Buffer State Encoding](./adr-019-protobuf-state-encoding.md) +- [ADR 020: Protocol Buffer Client Encoding](./adr-020-protobuf-client-encoding.md) diff --git a/docs/architecture/adr-019-protobuf-state-encoding.md b/docs/architecture/adr-019-protobuf-state-encoding.md index d09052866..78a338ccc 100644 --- a/docs/architecture/adr-019-protobuf-state-encoding.md +++ b/docs/architecture/adr-019-protobuf-state-encoding.md @@ -309,8 +309,6 @@ at the application-level. ### Neutral -{neutral consequences} - ## References 1. https://github.com/cosmos/cosmos-sdk/issues/4977 diff --git a/docs/architecture/adr-020-protobuf-client-encoding.md b/docs/architecture/adr-020-protobuf-client-encoding.md new file mode 100644 index 000000000..8e20ff6e9 --- /dev/null +++ b/docs/architecture/adr-020-protobuf-client-encoding.md @@ -0,0 +1,23 @@ +# ADR 020: Protocol Buffer Client Encoding + +## Changelog + +- 2020 March 06: Initial Draft + +## Status + +Proposed + +## Context + +## Decision + +## Consequences + +### Positive + +### Negative + +### Neutral + +## References From b064e1841cfc448c8ee031ceb1db51e1e8a97988 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 6 Mar 2020 10:16:29 -0500 Subject: [PATCH 2/6] Update context --- docs/architecture/adr-020-protobuf-client-encoding.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/architecture/adr-020-protobuf-client-encoding.md b/docs/architecture/adr-020-protobuf-client-encoding.md index 8e20ff6e9..ab28ad4d5 100644 --- a/docs/architecture/adr-020-protobuf-client-encoding.md +++ b/docs/architecture/adr-020-protobuf-client-encoding.md @@ -10,6 +10,17 @@ Proposed ## Context +This ADR is a continuation of the motivation, design, and context established in +[ADR 019](./adr-019-protobuf-state-encoding.md), namely, we aim to design the +Protocol Buffer migration path for the client-side of the Cosmos SDK. + +Specifically, the client-side migration path primarily includes tx generation and +signing, message construction and routing, in addition to CLI & REST handlers and +business logic (i.e. queriers). + +With this in mind, we will tackle the migration path via two main areas, txs and +querying. + ## Decision ## Consequences From 812ed54239fb4d21491c22faa1c2853a08189ea5 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 6 Mar 2020 10:54:29 -0500 Subject: [PATCH 3/6] Update transactions section --- .../adr-020-protobuf-client-encoding.md | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/docs/architecture/adr-020-protobuf-client-encoding.md b/docs/architecture/adr-020-protobuf-client-encoding.md index ab28ad4d5..9c928ef0e 100644 --- a/docs/architecture/adr-020-protobuf-client-encoding.md +++ b/docs/architecture/adr-020-protobuf-client-encoding.md @@ -23,6 +23,59 @@ querying. ## Decision +### Transactions + +Since the messages that an application is known and allowed to handle are specific +to the application itself, so must the transactions be specific to the application +itself. Similar to how we described in [ADR 019](./adr-019-protobuf-state-encoding.md), +the concrete types will be defined at the application level via Protobuf `oneof`. + +The application will define a single canonical `Message` Protobuf message +with a single `oneof` that implements the SDK's `Msg` interface. + +Example: + +```protobuf +// app/codec/codec.proto + +message Message { + option (cosmos_proto.interface_type) = "github.com/cosmos/cosmos-sdk/types.Msg"; + + oneof sum { + bank.MsgSend = 1; + staking.MsgCreateValidator = 2; + staking.MsgDelegate = 3; + // ... + } +} +``` + +Because an application needs to define it's unique `Message` Protobuf message, it +will by proxy have to define a `Transaction` Protobuf message that encapsulates this +`Message` type. The `Transaction` message type must implement the SDK's `Tx` interface. + +Example: + +```protobuf +// app/codec/codec.proto + +message Transaction { + option (cosmos_proto.interface_type) = "github.com/cosmos/cosmos-sdk/types.Tx"; + + StdTxBase base = 1; + repeated Message msgs = 2; +} +``` + +Note, the `Transaction` type includes `StdTxBase` which will be defined by the SDK +and includes all the core field members that are common across all transaction types. +Developers do not have to include `StdTxBase` if they wish, so it is meant to be +used as an auxiliary type. + +### Signing + +### CLI, REST, & Querying + ## Consequences ### Positive From aaa2bcc79364258beb8dc3f3f6610727ee52cc79 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 6 Mar 2020 11:28:24 -0500 Subject: [PATCH 4/6] Add section on signing --- .../adr-020-protobuf-client-encoding.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/architecture/adr-020-protobuf-client-encoding.md b/docs/architecture/adr-020-protobuf-client-encoding.md index 9c928ef0e..a70099eba 100644 --- a/docs/architecture/adr-020-protobuf-client-encoding.md +++ b/docs/architecture/adr-020-protobuf-client-encoding.md @@ -74,6 +74,24 @@ used as an auxiliary type. ### Signing +Signing of a `Transaction` must be canonical across clients and binaries. In order +to provide canonical representation of a `Transaction` to sign over, clients must +obey the following rules: + +- Encode `StdSignDoc` (see below) via [Protobuf's canonical JSON encoding](https://developers.google.com/protocol-buffers/docs/proto3#json). + - Default and zero values must be stripped from the output (`0`, `“”`, `null`, `false`, `[]`, and `{}`). +- Generate canonical JSON to sign via the [JSON Canonical Form Spec](https://gibson042.github.io/canonicaljson-spec/). + - This spec should be trivial to interpret and implement in any language. + +```Protobuf +// app/codec/codec.proto + +message StdSignDoc { + StdSignDocBase base = 1; + repeated Message msgs = 2; +} +``` + ### CLI, REST, & Querying ## Consequences From c56915d5a5669521040cc70683031ebe9215e28b Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 6 Mar 2020 12:29:44 -0500 Subject: [PATCH 5/6] Add remaining sections --- .../adr-020-protobuf-client-encoding.md | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/docs/architecture/adr-020-protobuf-client-encoding.md b/docs/architecture/adr-020-protobuf-client-encoding.md index a70099eba..08845faed 100644 --- a/docs/architecture/adr-020-protobuf-client-encoding.md +++ b/docs/architecture/adr-020-protobuf-client-encoding.md @@ -94,12 +94,79 @@ message StdSignDoc { ### CLI, REST, & Querying +Currently, the queriers, REST, CLI and handlers encode and decode types via Amino +JSON encoding using a concrete Amino codec. Being that some of the types dealt with +in the client can be interfaces, similar to how we described in [ADR 019](./adr-019-protobuf-state-encoding.md), +the client logic will now need to take a codec interface that knows not only how +to handle all the types, but also knows how to generate transactions, signatures, +and messages. + +```go +type TxGenerator interface { + sdk.Tx + + NewTx() sdk.Tx + SetMsgs(...sdk.Msg) error + GetSignatures() []StdSignature + SetSignatures(...StdSignature) error +} +``` + +We then extend `codec.Marshaler` to also require fulfillment of `TxGenerator`. + +```go +type ClientMarshaler interface { + TxGenerator + codec.Marshaler +} +``` + +Then, each module will at the minimum accept a `ClientMarshaler` instead of a concrete +Amino codec. If the module needs to work with any interface types, it will use +the `Codec` interface defined by the module which now also includes `ClientMarshaler`. + +## Future Improvements + +Requiring application developers to have to redefine their `Message` Protobuf types +can be extremely tedious and may increase the surface area of bugs by potentially +missing one or more messages in the `oneof`. + +To circumvent this, an optional strategy can be taken that has each module define +it's own `oneof` and then the application-level `Message` simply imports each module's +`oneof`. However, this requires additional tooling and the use of reflection. + +Example: + +```protobuf +// app/codec/codec.proto + +message Message { + option (cosmos_proto.interface_type) = "github.com/cosmos/cosmos-sdk/types.Msg"; + + oneof sum { + bank.Msg = 1; + staking.Msg = 2; + // ... + } +} +``` + ## Consequences ### Positive +- Significant performance gains. +- Supports backward and forward type compatibility. +- Better support for cross-language clients. + ### Negative +- Learning curve required to understand and implement Protobuf messages. +- Less flexibility in cross-module type registration. We now need to define types +at the application-level. +- Client business logic and tx generation become a bit more complex as developers +have to define more types and implement more interfaces. + ### Neutral ## References From 9a78fc3b19b54038357e86ff03bd5a1baba7de9d Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Mon, 9 Mar 2020 09:49:30 -0400 Subject: [PATCH 6/6] Address review --- docs/architecture/README.md | 2 +- ... adr-020-protobuf-transaction-encoding.md} | 28 +++++++++++++------ 2 files changed, 20 insertions(+), 10 deletions(-) rename docs/architecture/{adr-020-protobuf-client-encoding.md => adr-020-protobuf-transaction-encoding.md} (86%) diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 6476ccab6..737217bb2 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -45,4 +45,4 @@ Please add a entry below in your Pull Request for an ADR. - [ADR 017: Historical Header Module](./adr-017-historical-header-module.md) - [ADR 018: Extendable Voting Periods](./adr-018-extendable-voting-period.md) - [ADR 019: Protocol Buffer State Encoding](./adr-019-protobuf-state-encoding.md) -- [ADR 020: Protocol Buffer Client Encoding](./adr-020-protobuf-client-encoding.md) +- [ADR 020: Protocol Buffer Transaction Encoding](./adr-020-protobuf-transaction-encoding.md) diff --git a/docs/architecture/adr-020-protobuf-client-encoding.md b/docs/architecture/adr-020-protobuf-transaction-encoding.md similarity index 86% rename from docs/architecture/adr-020-protobuf-client-encoding.md rename to docs/architecture/adr-020-protobuf-transaction-encoding.md index 08845faed..39d3b1421 100644 --- a/docs/architecture/adr-020-protobuf-client-encoding.md +++ b/docs/architecture/adr-020-protobuf-transaction-encoding.md @@ -1,4 +1,4 @@ -# ADR 020: Protocol Buffer Client Encoding +# ADR 020: Protocol Buffer Transaction Encoding ## Changelog @@ -19,7 +19,8 @@ signing, message construction and routing, in addition to CLI & REST handlers an business logic (i.e. queriers). With this in mind, we will tackle the migration path via two main areas, txs and -querying. +querying. However, this ADR solely focuses on transactions. Querying should be +addressed in a future ADR, but it should build off of these proposals. ## Decision @@ -78,7 +79,7 @@ Signing of a `Transaction` must be canonical across clients and binaries. In ord to provide canonical representation of a `Transaction` to sign over, clients must obey the following rules: -- Encode `StdSignDoc` (see below) via [Protobuf's canonical JSON encoding](https://developers.google.com/protocol-buffers/docs/proto3#json). +- Encode `SignDoc` (see below) via [Protobuf's canonical JSON encoding](https://developers.google.com/protocol-buffers/docs/proto3#json). - Default and zero values must be stripped from the output (`0`, `“”`, `null`, `false`, `[]`, and `{}`). - Generate canonical JSON to sign via the [JSON Canonical Form Spec](https://gibson042.github.io/canonicaljson-spec/). - This spec should be trivial to interpret and implement in any language. @@ -86,15 +87,15 @@ obey the following rules: ```Protobuf // app/codec/codec.proto -message StdSignDoc { +message SignDoc { StdSignDocBase base = 1; repeated Message msgs = 2; } ``` -### CLI, REST, & Querying +### CLI & REST -Currently, the queriers, REST, CLI and handlers encode and decode types via Amino +Currently, the REST and CLI handlers encode and decode types and txs via Amino JSON encoding using a concrete Amino codec. Being that some of the types dealt with in the client can be interfaces, similar to how we described in [ADR 019](./adr-019-protobuf-state-encoding.md), the client logic will now need to take a codec interface that knows not only how @@ -103,12 +104,21 @@ and messages. ```go type TxGenerator interface { - sdk.Tx + NewTx() ClientTx + SignBytes func(chainID string, num, seq uint64, fee StdFee, msgs []sdk.Msg, memo string) ([]byte, error) +} + +type ClientTx interface { + sdk.Tx + codec.ProtoMarshaler - NewTx() sdk.Tx SetMsgs(...sdk.Msg) error GetSignatures() []StdSignature SetSignatures(...StdSignature) error + GetFee() StdFee + SetFee(StdFee) + GetMemo() string + SetMemo(string) } ``` @@ -123,7 +133,7 @@ type ClientMarshaler interface { Then, each module will at the minimum accept a `ClientMarshaler` instead of a concrete Amino codec. If the module needs to work with any interface types, it will use -the `Codec` interface defined by the module which now also includes `ClientMarshaler`. +the `Codec` interface defined by the module which also extends `ClientMarshaler`. ## Future Improvements