From 1c611842e313c49b0445074760d6f9767ada7a59 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Wed, 30 Jun 2021 12:53:02 +0200 Subject: [PATCH 01/26] adr: ADR-044 Guidelines for updating proto defs --- .../adr-044-protobuf-updates-guidelines.md | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 docs/architecture/adr-044-protobuf-updates-guidelines.md diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md new file mode 100644 index 000000000000..f4b085ba2b00 --- /dev/null +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -0,0 +1,95 @@ +# ADR 044: Guidelines for Updating Protobuf Definitions + +## Changelog + +- 28.06.2021: Initial Draft + +## Status + +Draft + +## Abstract + +This ADR provides guidelines and recommended practices when updating Protobuf definitions. These guidelines are targeted at module developers. + +## Context + +The SDK maintains a set of [Protobuf definitions](https://github.com/cosmos/cosmos-sdk/tree/master/proto/cosmos). When making changes to these Protobuf definitions, the SDK currently only follows [Buf's](https://docs.buf.build/) recommendations. We noticed however that Buf's recommendations might still result in breaking changes in the SDK in some cases. For example: + +- Adding fields to `Msg`s. Adding fields is a not a Protubuf spec-breaking operation. However, when adding new fields to `Msg`s, the unknown field rejection will throw an error when sending the new `Msg` to an older node. +- Marking fields as `reserved`. Protobuf proposes the `reserved` keyword for removing fields without the need to bump the package version. However, by doing so, client backwards compatability is broken as Protobuf doesn't generate anything for `reserved` fields. See [#9446](https://github.com/cosmos/cosmos-sdk/issues/9446) for more details on this issue. + +Morever, module developers often face other questions around Protobuf definitions such as "Can I rename a field?" or "Can I deprecate a field?" This ADR aims to answer all these questions by providing clear guidelines on allowed updates for Protobuf definitions. + +## Decision + +We decide to keep [Buf's](https://docs.buf.build/) recommendations with the following exceptions (TODO document what they mean, and why we use them): + +- UNARY_RPC +- COMMENT_FIELD +- SERVICE_SUFFIX +- PACKAGE_VERSION_SUFFIX +- RPC_REQUEST_STANDARD_NAME + +On top of Buf's recommendations we add the following guidelines that are specific to the SDK. + +### Updating Protobuf Definition Withouth Bumping Version + +#### 1. `Msg`s SHALL NOT have new fields. + +TODO because of unknown fields rejection. +TODO mention nested structs too (TODO how about anys?) +TODO queries can have new fields + +#### 2. Fields MAY be marked as `deprecated`, and nodes MAY implement a protocol-breaking change for handling these fields. + +Protobuf supports the [`deprecated` field option](https://developers.google.com/protocol-buffers/docs/proto#options), and this option can be used on any fields inside `Msg`s or in other models. If a node handles a Protobuf message with a non-empty deprecated field, the node MAY change its behavior upon processing it. When possible, the node SHOULD allow backwards compatibility. + +Two examples: + +- the SDK recently removed support for [time-based software upgrades](https://github.com/cosmos/cosmos-sdk/pull/8849). As such, the `time` field has been marked as deprecated in `cosmos.upgrade.v1beta1.Plan`. Moreover, the node will reject any proposal containing an upgrade Plan whose `time` field is non-empty. +- the SDK now supports [governance split votes](./adr-037-gov-split-vote.md). When querying for votes, the returned `cosmos.gov.v1beta1.Vote` message has its `option` field (for 1 vote option) deprecated in favor of its `options` field (allowing multiple vote options). Whenever possible, the SDK still populates the deprecated `option` field: in practice, the `option` field is populated if and only if the `len(options) == 1` and `options[0].Weight == 1.0`. + +#### 3. Fields SHALL NOT be renamed. + +TODO as this will break client compatibility. + +### Bumping Protobuf Package Version + +TODO, needs architecture review. Some topics: + +- When bumping versions, should the SDK support both versions? + - i.e. v1beta1 -> v1, should we have two folders in the SDK, and handlers for both versions? + +## Consequences + +> This section describes the resulting context, after applying the decision. All consequences should be listed here, not just the "positive" ones. A particular decision may have positive, negative, and neutral consequences, but all of them affect the team and project in the future. + +### Backwards Compatibility + +> All ADRs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. The ADR must explain how the author proposes to deal with these incompatibilities. ADR submissions without a sufficient backwards compatibility treatise may be rejected outright. + +### Positive + +{positive consequences} + +### Negative + +{negative consequences} + +### Neutral + +{neutral consequences} + +## Further Discussions + +While an ADR is in the DRAFT or PROPOSED stage, this section should contain a summary of issues to be solved in future iterations (usually referencing comments from a pull-request discussion). +Later, this section can optionally list ideas or improvements the author or reviewers found during the analysis of this ADR. + +## Test Cases [optional] + +Test cases for an implementation are mandatory for ADRs that are affecting consensus changes. Other ADRs can choose to include links to test cases if applicable. + +## References + +- {reference link} From dd22934953f6079c4329bca468bfd39f4159a1fb Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Wed, 30 Jun 2021 12:57:38 +0200 Subject: [PATCH 02/26] More details --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index f4b085ba2b00..b6529e30b2ed 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -45,10 +45,10 @@ TODO queries can have new fields Protobuf supports the [`deprecated` field option](https://developers.google.com/protocol-buffers/docs/proto#options), and this option can be used on any fields inside `Msg`s or in other models. If a node handles a Protobuf message with a non-empty deprecated field, the node MAY change its behavior upon processing it. When possible, the node SHOULD allow backwards compatibility. -Two examples: +The v0.42 to v0.43 update contained two Protobuf-breaking changes, listed below. Instead of bumping the package versions from `v1beta1` to `v1`, the SDK team decided to follow this guideline, by reverting the breaking changes, marking those changes as deprecated, and modifying the node implementation when processing messages with deprecated fields. More specifically: - the SDK recently removed support for [time-based software upgrades](https://github.com/cosmos/cosmos-sdk/pull/8849). As such, the `time` field has been marked as deprecated in `cosmos.upgrade.v1beta1.Plan`. Moreover, the node will reject any proposal containing an upgrade Plan whose `time` field is non-empty. -- the SDK now supports [governance split votes](./adr-037-gov-split-vote.md). When querying for votes, the returned `cosmos.gov.v1beta1.Vote` message has its `option` field (for 1 vote option) deprecated in favor of its `options` field (allowing multiple vote options). Whenever possible, the SDK still populates the deprecated `option` field: in practice, the `option` field is populated if and only if the `len(options) == 1` and `options[0].Weight == 1.0`. +- the SDK now supports [governance split votes](./adr-037-gov-split-vote.md). When querying for votes, the returned `cosmos.gov.v1beta1.Vote` message has its `option` field (used for 1 vote option) deprecated in favor of its `options` field (allowing multiple vote options). Whenever possible, the SDK still populates the deprecated `option` field, that is, if and only if the `len(options) == 1` and `options[0].Weight == 1.0`. #### 3. Fields SHALL NOT be renamed. From 184095c6ef75236c3375f174f426c8886129a34c Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Wed, 30 Jun 2021 15:20:33 +0200 Subject: [PATCH 03/26] Tweaks --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index b6529e30b2ed..7e3508f15c87 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -43,9 +43,9 @@ TODO queries can have new fields #### 2. Fields MAY be marked as `deprecated`, and nodes MAY implement a protocol-breaking change for handling these fields. -Protobuf supports the [`deprecated` field option](https://developers.google.com/protocol-buffers/docs/proto#options), and this option can be used on any fields inside `Msg`s or in other models. If a node handles a Protobuf message with a non-empty deprecated field, the node MAY change its behavior upon processing it. When possible, the node SHOULD allow backwards compatibility. +Protobuf supports the [`deprecated` field option](https://developers.google.com/protocol-buffers/docs/proto#options), and this option MAY be used on any fields, including `Msg`s. If a node handles a Protobuf message with a non-empty deprecated field, the node MAY change its behavior upon processing it, even in a protocol-breaking way. When possible, the node SHOULD handle backwards compatibility. -The v0.42 to v0.43 update contained two Protobuf-breaking changes, listed below. Instead of bumping the package versions from `v1beta1` to `v1`, the SDK team decided to follow this guideline, by reverting the breaking changes, marking those changes as deprecated, and modifying the node implementation when processing messages with deprecated fields. More specifically: +As an example, the SDK v0.42 to v0.43 update contained two Protobuf-breaking changes, listed below. Instead of bumping the package versions from `v1beta1` to `v1`, the SDK team decided to follow this guideline, by reverting the breaking changes, marking those changes as deprecated, and modifying the node implementation when processing messages with deprecated fields. More specifically: - the SDK recently removed support for [time-based software upgrades](https://github.com/cosmos/cosmos-sdk/pull/8849). As such, the `time` field has been marked as deprecated in `cosmos.upgrade.v1beta1.Plan`. Moreover, the node will reject any proposal containing an upgrade Plan whose `time` field is non-empty. - the SDK now supports [governance split votes](./adr-037-gov-split-vote.md). When querying for votes, the returned `cosmos.gov.v1beta1.Vote` message has its `option` field (used for 1 vote option) deprecated in favor of its `options` field (allowing multiple vote options). Whenever possible, the SDK still populates the deprecated `option` field, that is, if and only if the `len(options) == 1` and `options[0].Weight == 1.0`. From 2853012d39d4268c11871a4427f93116b7b95315 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Mon, 5 Jul 2021 16:14:19 +0200 Subject: [PATCH 04/26] Add more content --- .../adr-044-protobuf-updates-guidelines.md | 34 ++++++++++++------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 7e3508f15c87..2dfc5e901baa 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -23,13 +23,13 @@ Morever, module developers often face other questions around Protobuf definition ## Decision -We decide to keep [Buf's](https://docs.buf.build/) recommendations with the following exceptions (TODO document what they mean, and why we use them): +We decide to keep [Buf's](https://docs.buf.build/) recommendations with the following exceptions: -- UNARY_RPC -- COMMENT_FIELD -- SERVICE_SUFFIX -- PACKAGE_VERSION_SUFFIX -- RPC_REQUEST_STANDARD_NAME +- `UNARY_RPC`: the SDK currently does not support streaming RPCs. +- `COMMENT_FIELD`: the SDK allows fields with no comments. +- `SERVICE_SUFFIX`: we use the `Query` and `Msg` services convention, which don't have the `-Service` suffix. +- `PACKAGE_VERSION_SUFFIX`: some packages, such as `cosmos.crypto.ed25519`, don't use version suffix. +- `RPC_REQUEST_STANDARD_NAME`: Requests for the `Msg` service don't have the `-Request` suffix to keep backwards compatibility. On top of Buf's recommendations we add the following guidelines that are specific to the SDK. @@ -37,9 +37,15 @@ On top of Buf's recommendations we add the following guidelines that are specifi #### 1. `Msg`s SHALL NOT have new fields. -TODO because of unknown fields rejection. -TODO mention nested structs too (TODO how about anys?) -TODO queries can have new fields +When processing `Msg`s, the SDK's antehandlers are strict and don't allow unknown fields in `Msg`s. This is checked by the unknown field rejection in the [`codec/unknownproto` package](https://github.com/cosmos/cosmos-sdk/blob/master/codec/unknownproto). + +Now imagine a v0.43 node accepting a `MsgExample` transaction, and in v0.44 the chain developer decides to add a field to `MsgExample`. A client developer, which only manipulates Protobuf definitions, would see that `MsgExample` has a new field, and will populate it. However, sending the new `MsgExample` to an old v0.43 node would cause the v0.43 node to reject the `Msg` because of the unknown field. The expectation that the same Protobuf version can be used across multiple node versions should be guaranteed. + +For this reason, module developers SHALL NOT add new fields to existing `Msg`s. + +It is worth mentioning that this does not limit to adding fields to a `Msg`, but also to all nested structs and `Any`s inside the `Msg`. + +On the other hand, module developers MAY add new fields to Protobuf definitions related to the `Query` service, as the unknown field rejection does not apply to queries. #### 2. Fields MAY be marked as `deprecated`, and nodes MAY implement a protocol-breaking change for handling these fields. @@ -52,14 +58,16 @@ As an example, the SDK v0.42 to v0.43 update contained two Protobuf-breaking cha #### 3. Fields SHALL NOT be renamed. -TODO as this will break client compatibility. +Whereas the official Protobuf recommendations do not prohibit renaming fields, as it does not break the Protobuf binary representation, the SDK explicitly forbids renaming fields in Protobuf structs. The main reason for this choice is to avoid introducing breaking changes for clients, which often rely on hard-coded fields from generated types. Morever, renaming fields will lead to client-breaking JSON representations of Protobuf definitions, used in REST endpoints and in the CLI. ### Bumping Protobuf Package Version TODO, needs architecture review. Some topics: +- Bumping versions frequency - When bumping versions, should the SDK support both versions? - i.e. v1beta1 -> v1, should we have two folders in the SDK, and handlers for both versions? +- mention ADR-023 Protobuf naming ## Consequences @@ -83,8 +91,7 @@ TODO, needs architecture review. Some topics: ## Further Discussions -While an ADR is in the DRAFT or PROPOSED stage, this section should contain a summary of issues to be solved in future iterations (usually referencing comments from a pull-request discussion). -Later, this section can optionally list ideas or improvements the author or reviewers found during the analysis of this ADR. +This ADR is still in the DRAFT stage, and the "Bumping Protobuf Package Version" will be filled in once we make a decision on how to correctly bump Protobuf package versions. ## Test Cases [optional] @@ -92,4 +99,5 @@ Test cases for an implementation are mandatory for ADRs that are affecting conse ## References -- {reference link} +- [#9445](https://github.com/cosmos/cosmos-sdk/issues/9445) Release proto definitions v1 +- [#9446](https://github.com/cosmos/cosmos-sdk/issues/9446) Address v1beta1 proto breaking changes From a0e9b4618dfab3d81bcd7a473478256a45cfe1c1 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Tue, 13 Jul 2021 14:52:53 +0200 Subject: [PATCH 05/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 2dfc5e901baa..823fc60aafe4 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -28,7 +28,7 @@ We decide to keep [Buf's](https://docs.buf.build/) recommendations with the foll - `UNARY_RPC`: the SDK currently does not support streaming RPCs. - `COMMENT_FIELD`: the SDK allows fields with no comments. - `SERVICE_SUFFIX`: we use the `Query` and `Msg` services convention, which don't have the `-Service` suffix. -- `PACKAGE_VERSION_SUFFIX`: some packages, such as `cosmos.crypto.ed25519`, don't use version suffix. +- `PACKAGE_VERSION_SUFFIX`: some packages, such as `cosmos.crypto.ed25519`, don't use a version suffix. - `RPC_REQUEST_STANDARD_NAME`: Requests for the `Msg` service don't have the `-Request` suffix to keep backwards compatibility. On top of Buf's recommendations we add the following guidelines that are specific to the SDK. From e1f7bd4897be62f5f5c54cd2242b983ee8069f9d Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Tue, 13 Jul 2021 14:56:26 +0200 Subject: [PATCH 06/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 823fc60aafe4..5a4de6f84dbb 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -19,7 +19,7 @@ The SDK maintains a set of [Protobuf definitions](https://github.com/cosmos/cosm - Adding fields to `Msg`s. Adding fields is a not a Protubuf spec-breaking operation. However, when adding new fields to `Msg`s, the unknown field rejection will throw an error when sending the new `Msg` to an older node. - Marking fields as `reserved`. Protobuf proposes the `reserved` keyword for removing fields without the need to bump the package version. However, by doing so, client backwards compatability is broken as Protobuf doesn't generate anything for `reserved` fields. See [#9446](https://github.com/cosmos/cosmos-sdk/issues/9446) for more details on this issue. -Morever, module developers often face other questions around Protobuf definitions such as "Can I rename a field?" or "Can I deprecate a field?" This ADR aims to answer all these questions by providing clear guidelines on allowed updates for Protobuf definitions. +Moreover, module developers often face other questions around Protobuf definitions such as "Can I rename a field?" or "Can I deprecate a field?" This ADR aims to answer all these questions by providing clear guidelines on allowed updates for Protobuf definitions. ## Decision From 6f6060e8610b32ad2f74c3c43ada9cfba4e7174b Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Tue, 13 Jul 2021 14:56:33 +0200 Subject: [PATCH 07/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 5a4de6f84dbb..ee2638aad689 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -17,7 +17,7 @@ This ADR provides guidelines and recommended practices when updating Protobuf de The SDK maintains a set of [Protobuf definitions](https://github.com/cosmos/cosmos-sdk/tree/master/proto/cosmos). When making changes to these Protobuf definitions, the SDK currently only follows [Buf's](https://docs.buf.build/) recommendations. We noticed however that Buf's recommendations might still result in breaking changes in the SDK in some cases. For example: - Adding fields to `Msg`s. Adding fields is a not a Protubuf spec-breaking operation. However, when adding new fields to `Msg`s, the unknown field rejection will throw an error when sending the new `Msg` to an older node. -- Marking fields as `reserved`. Protobuf proposes the `reserved` keyword for removing fields without the need to bump the package version. However, by doing so, client backwards compatability is broken as Protobuf doesn't generate anything for `reserved` fields. See [#9446](https://github.com/cosmos/cosmos-sdk/issues/9446) for more details on this issue. +- Marking fields as `reserved`. Protobuf proposes the `reserved` keyword for removing fields without the need to bump the package version. However, by doing so, client backwards compatibility is broken as Protobuf doesn't generate anything for `reserved` fields. See [#9446](https://github.com/cosmos/cosmos-sdk/issues/9446) for more details on this issue. Moreover, module developers often face other questions around Protobuf definitions such as "Can I rename a field?" or "Can I deprecate a field?" This ADR aims to answer all these questions by providing clear guidelines on allowed updates for Protobuf definitions. From b400f3437ee19ac54b62e3f7ea4307d0c55050b5 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Tue, 13 Jul 2021 14:56:40 +0200 Subject: [PATCH 08/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index ee2638aad689..343f0a66f519 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -49,7 +49,7 @@ On the other hand, module developers MAY add new fields to Protobuf definitions #### 2. Fields MAY be marked as `deprecated`, and nodes MAY implement a protocol-breaking change for handling these fields. -Protobuf supports the [`deprecated` field option](https://developers.google.com/protocol-buffers/docs/proto#options), and this option MAY be used on any fields, including `Msg`s. If a node handles a Protobuf message with a non-empty deprecated field, the node MAY change its behavior upon processing it, even in a protocol-breaking way. When possible, the node SHOULD handle backwards compatibility. +Protobuf supports the [`deprecated` field option](https://developers.google.com/protocol-buffers/docs/proto#options), and this option MAY be used on any field, including `Msg` fields. If a node handles a Protobuf message with a non-empty deprecated field, the node MAY change its behavior upon processing it, even in a protocol-breaking way. When possible, the node SHOULD handle backwards compatibility. As an example, the SDK v0.42 to v0.43 update contained two Protobuf-breaking changes, listed below. Instead of bumping the package versions from `v1beta1` to `v1`, the SDK team decided to follow this guideline, by reverting the breaking changes, marking those changes as deprecated, and modifying the node implementation when processing messages with deprecated fields. More specifically: From c065ffa60929d0dad9b8d845255cb6b775dfea54 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Tue, 13 Jul 2021 14:56:48 +0200 Subject: [PATCH 09/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 343f0a66f519..3c74cfe01305 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -58,7 +58,7 @@ As an example, the SDK v0.42 to v0.43 update contained two Protobuf-breaking cha #### 3. Fields SHALL NOT be renamed. -Whereas the official Protobuf recommendations do not prohibit renaming fields, as it does not break the Protobuf binary representation, the SDK explicitly forbids renaming fields in Protobuf structs. The main reason for this choice is to avoid introducing breaking changes for clients, which often rely on hard-coded fields from generated types. Morever, renaming fields will lead to client-breaking JSON representations of Protobuf definitions, used in REST endpoints and in the CLI. +Whereas the official Protobuf recommendations do not prohibit renaming fields, as it does not break the Protobuf binary representation, the SDK explicitly forbids renaming fields in Protobuf structs. The main reason for this choice is to avoid introducing breaking changes for clients, which often rely on hard-coded fields from generated types. Moreover, renaming fields will lead to client-breaking JSON representations of Protobuf definitions, used in REST endpoints and in the CLI. ### Bumping Protobuf Package Version From c9308604656022860c7373dfb879b90583fc9deb Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Tue, 13 Jul 2021 14:57:05 +0200 Subject: [PATCH 10/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 3c74cfe01305..5ce3b4ec8e5a 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -53,8 +53,8 @@ Protobuf supports the [`deprecated` field option](https://developers.google.com/ As an example, the SDK v0.42 to v0.43 update contained two Protobuf-breaking changes, listed below. Instead of bumping the package versions from `v1beta1` to `v1`, the SDK team decided to follow this guideline, by reverting the breaking changes, marking those changes as deprecated, and modifying the node implementation when processing messages with deprecated fields. More specifically: -- the SDK recently removed support for [time-based software upgrades](https://github.com/cosmos/cosmos-sdk/pull/8849). As such, the `time` field has been marked as deprecated in `cosmos.upgrade.v1beta1.Plan`. Moreover, the node will reject any proposal containing an upgrade Plan whose `time` field is non-empty. -- the SDK now supports [governance split votes](./adr-037-gov-split-vote.md). When querying for votes, the returned `cosmos.gov.v1beta1.Vote` message has its `option` field (used for 1 vote option) deprecated in favor of its `options` field (allowing multiple vote options). Whenever possible, the SDK still populates the deprecated `option` field, that is, if and only if the `len(options) == 1` and `options[0].Weight == 1.0`. +- The SDK recently removed support for [time-based software upgrades](https://github.com/cosmos/cosmos-sdk/pull/8849). As such, the `time` field has been marked as deprecated in `cosmos.upgrade.v1beta1.Plan`. Moreover, the node will reject any proposal containing an upgrade Plan whose `time` field is non-empty. +- The SDK now supports [governance split votes](./adr-037-gov-split-vote.md). When querying for votes, the returned `cosmos.gov.v1beta1.Vote` message has its `option` field (used for 1 vote option) deprecated in favor of its `options` field (allowing multiple vote options). Whenever possible, the SDK still populates the deprecated `option` field, that is, if and only if the `len(options) == 1` and `options[0].Weight == 1.0`. #### 3. Fields SHALL NOT be renamed. From eb5793fb063b8ce6b612b1620af63ce089ccd0f8 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Tue, 13 Jul 2021 14:57:16 +0200 Subject: [PATCH 11/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 5ce3b4ec8e5a..34d43008b839 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -43,7 +43,7 @@ Now imagine a v0.43 node accepting a `MsgExample` transaction, and in v0.44 the For this reason, module developers SHALL NOT add new fields to existing `Msg`s. -It is worth mentioning that this does not limit to adding fields to a `Msg`, but also to all nested structs and `Any`s inside the `Msg`. +It is worth mentioning that this does not limit adding fields to a `Msg`, but also to all nested structs and `Any`s inside a `Msg`. On the other hand, module developers MAY add new fields to Protobuf definitions related to the `Query` service, as the unknown field rejection does not apply to queries. From 8788891b2611d5993b750b3dad4b024657ae9ae6 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Tue, 13 Jul 2021 14:57:28 +0200 Subject: [PATCH 12/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 34d43008b839..c853e9149f69 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -16,7 +16,7 @@ This ADR provides guidelines and recommended practices when updating Protobuf de The SDK maintains a set of [Protobuf definitions](https://github.com/cosmos/cosmos-sdk/tree/master/proto/cosmos). When making changes to these Protobuf definitions, the SDK currently only follows [Buf's](https://docs.buf.build/) recommendations. We noticed however that Buf's recommendations might still result in breaking changes in the SDK in some cases. For example: -- Adding fields to `Msg`s. Adding fields is a not a Protubuf spec-breaking operation. However, when adding new fields to `Msg`s, the unknown field rejection will throw an error when sending the new `Msg` to an older node. +- Adding fields to `Msg`s. Adding fields is a not a Protobuf spec-breaking operation. However, when adding new fields to `Msg`s, the unknown field rejection will throw an error when sending the new `Msg` to an older node. - Marking fields as `reserved`. Protobuf proposes the `reserved` keyword for removing fields without the need to bump the package version. However, by doing so, client backwards compatibility is broken as Protobuf doesn't generate anything for `reserved` fields. See [#9446](https://github.com/cosmos/cosmos-sdk/issues/9446) for more details on this issue. Moreover, module developers often face other questions around Protobuf definitions such as "Can I rename a field?" or "Can I deprecate a field?" This ADR aims to answer all these questions by providing clear guidelines on allowed updates for Protobuf definitions. From 55741cc92915379aff80c18eb8e87c7114a555b4 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Tue, 13 Jul 2021 14:57:38 +0200 Subject: [PATCH 13/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index c853e9149f69..daf5d5e6fa01 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -27,7 +27,7 @@ We decide to keep [Buf's](https://docs.buf.build/) recommendations with the foll - `UNARY_RPC`: the SDK currently does not support streaming RPCs. - `COMMENT_FIELD`: the SDK allows fields with no comments. -- `SERVICE_SUFFIX`: we use the `Query` and `Msg` services convention, which don't have the `-Service` suffix. +- `SERVICE_SUFFIX`: we use the `Query` and `Msg` service naming convention, which doesn't use the `-Service` suffix. - `PACKAGE_VERSION_SUFFIX`: some packages, such as `cosmos.crypto.ed25519`, don't use a version suffix. - `RPC_REQUEST_STANDARD_NAME`: Requests for the `Msg` service don't have the `-Request` suffix to keep backwards compatibility. From 28e11e1c10f132dc01ac91c2302931b7616ebfd7 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Tue, 13 Jul 2021 14:57:47 +0200 Subject: [PATCH 14/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index daf5d5e6fa01..db9f6123d605 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -33,7 +33,7 @@ We decide to keep [Buf's](https://docs.buf.build/) recommendations with the foll On top of Buf's recommendations we add the following guidelines that are specific to the SDK. -### Updating Protobuf Definition Withouth Bumping Version +### Updating Protobuf Definition Without Bumping Version #### 1. `Msg`s SHALL NOT have new fields. From 5574a57ad1a4450aa727817dd94c0ad22d397fff Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:10:18 +0200 Subject: [PATCH 15/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index db9f6123d605..6bd7b8518926 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -10,7 +10,7 @@ Draft ## Abstract -This ADR provides guidelines and recommended practices when updating Protobuf definitions. These guidelines are targeted at module developers. +This ADR provides guidelines and recommended practices when updating Protobuf definitions. These guidelines are targeting module developers. ## Context From 25b2dea5b7c72deb63b3b4ecda1929c0fc19a2ec Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:10:30 +0200 Subject: [PATCH 16/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 6bd7b8518926..3578dbcd5788 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -19,7 +19,7 @@ The SDK maintains a set of [Protobuf definitions](https://github.com/cosmos/cosm - Adding fields to `Msg`s. Adding fields is a not a Protobuf spec-breaking operation. However, when adding new fields to `Msg`s, the unknown field rejection will throw an error when sending the new `Msg` to an older node. - Marking fields as `reserved`. Protobuf proposes the `reserved` keyword for removing fields without the need to bump the package version. However, by doing so, client backwards compatibility is broken as Protobuf doesn't generate anything for `reserved` fields. See [#9446](https://github.com/cosmos/cosmos-sdk/issues/9446) for more details on this issue. -Moreover, module developers often face other questions around Protobuf definitions such as "Can I rename a field?" or "Can I deprecate a field?" This ADR aims to answer all these questions by providing clear guidelines on allowed updates for Protobuf definitions. +Moreover, module developers often face other questions around Protobuf definitions such as "Can I rename a field?" or "Can I deprecate a field?" This ADR aims to answer all these questions by providing clear guidelines about allowed updates for Protobuf definitions. ## Decision From c1c51ed25ad50c6edae8380cf3717c1a4d637d68 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:10:53 +0200 Subject: [PATCH 17/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 3578dbcd5788..e2d5486f6328 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -39,7 +39,7 @@ On top of Buf's recommendations we add the following guidelines that are specifi When processing `Msg`s, the SDK's antehandlers are strict and don't allow unknown fields in `Msg`s. This is checked by the unknown field rejection in the [`codec/unknownproto` package](https://github.com/cosmos/cosmos-sdk/blob/master/codec/unknownproto). -Now imagine a v0.43 node accepting a `MsgExample` transaction, and in v0.44 the chain developer decides to add a field to `MsgExample`. A client developer, which only manipulates Protobuf definitions, would see that `MsgExample` has a new field, and will populate it. However, sending the new `MsgExample` to an old v0.43 node would cause the v0.43 node to reject the `Msg` because of the unknown field. The expectation that the same Protobuf version can be used across multiple node versions should be guaranteed. +Now imagine a v0.43 node accepting a `MsgExample` transaction, and in v0.44 the chain developer decides to add a field to `MsgExample`. A client developer, which only manipulates Protobuf definitions, would see that `MsgExample` has a new field, and will populate it. However, sending the new `MsgExample` to an old v0.43 node would cause the v0.43 node to reject the `MsgExample` because of the unknown field. The expectation that the same Protobuf version can be used across multiple node versions MUST be guaranteed. For this reason, module developers SHALL NOT add new fields to existing `Msg`s. From a0fdf0fabc2c98922d3e6e24e2eeb74331600c9c Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:11:55 +0200 Subject: [PATCH 18/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index e2d5486f6328..07f6156453c8 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -56,7 +56,7 @@ As an example, the SDK v0.42 to v0.43 update contained two Protobuf-breaking cha - The SDK recently removed support for [time-based software upgrades](https://github.com/cosmos/cosmos-sdk/pull/8849). As such, the `time` field has been marked as deprecated in `cosmos.upgrade.v1beta1.Plan`. Moreover, the node will reject any proposal containing an upgrade Plan whose `time` field is non-empty. - The SDK now supports [governance split votes](./adr-037-gov-split-vote.md). When querying for votes, the returned `cosmos.gov.v1beta1.Vote` message has its `option` field (used for 1 vote option) deprecated in favor of its `options` field (allowing multiple vote options). Whenever possible, the SDK still populates the deprecated `option` field, that is, if and only if the `len(options) == 1` and `options[0].Weight == 1.0`. -#### 3. Fields SHALL NOT be renamed. +#### 3. Fields MUST NOT be renamed. Whereas the official Protobuf recommendations do not prohibit renaming fields, as it does not break the Protobuf binary representation, the SDK explicitly forbids renaming fields in Protobuf structs. The main reason for this choice is to avoid introducing breaking changes for clients, which often rely on hard-coded fields from generated types. Moreover, renaming fields will lead to client-breaking JSON representations of Protobuf definitions, used in REST endpoints and in the CLI. From 3a1634342df27673a7f96abf8e463a92243c61ce Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:13:05 +0200 Subject: [PATCH 19/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 07f6156453c8..8e6e6661729b 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -35,7 +35,7 @@ On top of Buf's recommendations we add the following guidelines that are specifi ### Updating Protobuf Definition Without Bumping Version -#### 1. `Msg`s SHALL NOT have new fields. +#### 1. `Msg`s MUST NOT have new fields. When processing `Msg`s, the SDK's antehandlers are strict and don't allow unknown fields in `Msg`s. This is checked by the unknown field rejection in the [`codec/unknownproto` package](https://github.com/cosmos/cosmos-sdk/blob/master/codec/unknownproto). From eaf00cd0fd49c86a8e988dd3c7a96dd9178d4044 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:13:16 +0200 Subject: [PATCH 20/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 8e6e6661729b..a061e44dfbaf 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -41,7 +41,7 @@ When processing `Msg`s, the SDK's antehandlers are strict and don't allow unknow Now imagine a v0.43 node accepting a `MsgExample` transaction, and in v0.44 the chain developer decides to add a field to `MsgExample`. A client developer, which only manipulates Protobuf definitions, would see that `MsgExample` has a new field, and will populate it. However, sending the new `MsgExample` to an old v0.43 node would cause the v0.43 node to reject the `MsgExample` because of the unknown field. The expectation that the same Protobuf version can be used across multiple node versions MUST be guaranteed. -For this reason, module developers SHALL NOT add new fields to existing `Msg`s. +For this reason, module developers MUST NOT add new fields to existing `Msg`s. It is worth mentioning that this does not limit adding fields to a `Msg`, but also to all nested structs and `Any`s inside a `Msg`. From 5799e5527831fc05ff61f2712858dc58671635d1 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:13:33 +0200 Subject: [PATCH 21/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index a061e44dfbaf..0d58652eb9ca 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -49,7 +49,7 @@ On the other hand, module developers MAY add new fields to Protobuf definitions #### 2. Fields MAY be marked as `deprecated`, and nodes MAY implement a protocol-breaking change for handling these fields. -Protobuf supports the [`deprecated` field option](https://developers.google.com/protocol-buffers/docs/proto#options), and this option MAY be used on any field, including `Msg` fields. If a node handles a Protobuf message with a non-empty deprecated field, the node MAY change its behavior upon processing it, even in a protocol-breaking way. When possible, the node SHOULD handle backwards compatibility. +Protobuf supports the [`deprecated` field option](https://developers.google.com/protocol-buffers/docs/proto#options), and this option MAY be used on any field, including `Msg` fields. If a node handles a Protobuf message with a non-empty deprecated field, the node MAY change its behavior upon processing it, even in a protocol-breaking way. When possible, the node MUST handle backwards compatibility without breaking the consensus (unless we increment the proto version). As an example, the SDK v0.42 to v0.43 update contained two Protobuf-breaking changes, listed below. Instead of bumping the package versions from `v1beta1` to `v1`, the SDK team decided to follow this guideline, by reverting the breaking changes, marking those changes as deprecated, and modifying the node implementation when processing messages with deprecated fields. More specifically: From e143f191425e70cac8bde96644678670157ade83 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:13:47 +0200 Subject: [PATCH 22/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 0d58652eb9ca..02b473bf64dd 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -79,7 +79,9 @@ TODO, needs architecture review. Some topics: ### Positive -{positive consequences} ++ less pain to tool developers ++ more compatibility in the ecosystem ++ ... ### Negative From 9934ff21223fec6b3a745a5ab41caede6cbe716f Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:13:54 +0200 Subject: [PATCH 23/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 02b473bf64dd..a0f7559fb15e 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -89,7 +89,7 @@ TODO, needs architecture review. Some topics: ### Neutral -{neutral consequences} ++ more rigor in Protobuf review ## Further Discussions From ba6f85974f07c87298d2d0d3c25a1f509b43cae5 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:14:24 +0200 Subject: [PATCH 24/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index a0f7559fb15e..05194436ee74 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -93,7 +93,7 @@ TODO, needs architecture review. Some topics: ## Further Discussions -This ADR is still in the DRAFT stage, and the "Bumping Protobuf Package Version" will be filled in once we make a decision on how to correctly bump Protobuf package versions. +This ADR is still in the DRAFT stage, and the "Incrementing Protobuf Package Version" will be filled in once we make a decision on how to correctly do it. ## Test Cases [optional] From ca0fe2af4d5eedebff3d096c7c6b90b9e686bd20 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:14:34 +0200 Subject: [PATCH 25/26] Update docs/architecture/adr-044-protobuf-updates-guidelines.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-044-protobuf-updates-guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 05194436ee74..20e6de662741 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -60,7 +60,7 @@ As an example, the SDK v0.42 to v0.43 update contained two Protobuf-breaking cha Whereas the official Protobuf recommendations do not prohibit renaming fields, as it does not break the Protobuf binary representation, the SDK explicitly forbids renaming fields in Protobuf structs. The main reason for this choice is to avoid introducing breaking changes for clients, which often rely on hard-coded fields from generated types. Moreover, renaming fields will lead to client-breaking JSON representations of Protobuf definitions, used in REST endpoints and in the CLI. -### Bumping Protobuf Package Version +### Increment Protobuf Package Version TODO, needs architecture review. Some topics: From 2db5c078a9646c2ec87d08fc29ecaa4602bd4e9a Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Fri, 30 Jul 2021 13:40:45 +0200 Subject: [PATCH 26/26] Apply suggestions --- docs/architecture/README.md | 4 ++++ .../adr-044-protobuf-updates-guidelines.md | 22 +++++++++++-------- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 5455df9f8556..dcc0d542ae8e 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -77,3 +77,7 @@ When writing ADRs, follow the same best practices for writing RFCs. When writing - [ADR 038: State Listening](./adr-038-state-listening.md) - [ADR 039: Epoched Staking](./adr-039-epoched-staking.md) - [ADR 040: Storage and SMT State Commitments](./adr-040-storage-and-smt-state-commitments.md) + +### Draft + +- [ADR 044: Guidelines for Updating Protobuf Definitions](./adr-044-protobuf-updates-guidelines.md) diff --git a/docs/architecture/adr-044-protobuf-updates-guidelines.md b/docs/architecture/adr-044-protobuf-updates-guidelines.md index 20e6de662741..bc92feb88b9f 100644 --- a/docs/architecture/adr-044-protobuf-updates-guidelines.md +++ b/docs/architecture/adr-044-protobuf-updates-guidelines.md @@ -14,7 +14,9 @@ This ADR provides guidelines and recommended practices when updating Protobuf de ## Context -The SDK maintains a set of [Protobuf definitions](https://github.com/cosmos/cosmos-sdk/tree/master/proto/cosmos). When making changes to these Protobuf definitions, the SDK currently only follows [Buf's](https://docs.buf.build/) recommendations. We noticed however that Buf's recommendations might still result in breaking changes in the SDK in some cases. For example: +The SDK maintains a set of [Protobuf definitions](https://github.com/cosmos/cosmos-sdk/tree/master/proto/cosmos). It is important to correctly design Protobuf definitions to avoid any breaking changes within the same version. The reasons are to not break tooling (including indexers and explorers), wallets and other third-party integrations. + +When making changes to these Protobuf definitions, the SDK currently only follows [Buf's](https://docs.buf.build/) recommendations. We noticed however that Buf's recommendations might still result in breaking changes in the SDK in some cases. For example: - Adding fields to `Msg`s. Adding fields is a not a Protobuf spec-breaking operation. However, when adding new fields to `Msg`s, the unknown field rejection will throw an error when sending the new `Msg` to an older node. - Marking fields as `reserved`. Protobuf proposes the `reserved` keyword for removing fields without the need to bump the package version. However, by doing so, client backwards compatibility is broken as Protobuf doesn't generate anything for `reserved` fields. See [#9446](https://github.com/cosmos/cosmos-sdk/issues/9446) for more details on this issue. @@ -45,9 +47,11 @@ For this reason, module developers MUST NOT add new fields to existing `Msg`s. It is worth mentioning that this does not limit adding fields to a `Msg`, but also to all nested structs and `Any`s inside a `Msg`. -On the other hand, module developers MAY add new fields to Protobuf definitions related to the `Query` service, as the unknown field rejection does not apply to queries. +#### 2. Non-`Msg`-related Protobuf definitions MAY have new fields. + +On the other hand, module developers MAY add new fields to Protobuf definitions related to the `Query` service or to objects which are saved in the store. This recommendation follows the Protobuf specification, but is added in this document for clarity. -#### 2. Fields MAY be marked as `deprecated`, and nodes MAY implement a protocol-breaking change for handling these fields. +#### 3. Fields MAY be marked as `deprecated`, and nodes MAY implement a protocol-breaking change for handling these fields. Protobuf supports the [`deprecated` field option](https://developers.google.com/protocol-buffers/docs/proto#options), and this option MAY be used on any field, including `Msg` fields. If a node handles a Protobuf message with a non-empty deprecated field, the node MAY change its behavior upon processing it, even in a protocol-breaking way. When possible, the node MUST handle backwards compatibility without breaking the consensus (unless we increment the proto version). @@ -56,11 +60,11 @@ As an example, the SDK v0.42 to v0.43 update contained two Protobuf-breaking cha - The SDK recently removed support for [time-based software upgrades](https://github.com/cosmos/cosmos-sdk/pull/8849). As such, the `time` field has been marked as deprecated in `cosmos.upgrade.v1beta1.Plan`. Moreover, the node will reject any proposal containing an upgrade Plan whose `time` field is non-empty. - The SDK now supports [governance split votes](./adr-037-gov-split-vote.md). When querying for votes, the returned `cosmos.gov.v1beta1.Vote` message has its `option` field (used for 1 vote option) deprecated in favor of its `options` field (allowing multiple vote options). Whenever possible, the SDK still populates the deprecated `option` field, that is, if and only if the `len(options) == 1` and `options[0].Weight == 1.0`. -#### 3. Fields MUST NOT be renamed. +#### 4. Fields MUST NOT be renamed. Whereas the official Protobuf recommendations do not prohibit renaming fields, as it does not break the Protobuf binary representation, the SDK explicitly forbids renaming fields in Protobuf structs. The main reason for this choice is to avoid introducing breaking changes for clients, which often rely on hard-coded fields from generated types. Moreover, renaming fields will lead to client-breaking JSON representations of Protobuf definitions, used in REST endpoints and in the CLI. -### Increment Protobuf Package Version +### Incrementing Protobuf Package Version TODO, needs architecture review. Some topics: @@ -79,9 +83,9 @@ TODO, needs architecture review. Some topics: ### Positive -+ less pain to tool developers -+ more compatibility in the ecosystem -+ ... +- less pain to tool developers +- more compatibility in the ecosystem +- ... ### Negative @@ -89,7 +93,7 @@ TODO, needs architecture review. Some topics: ### Neutral -+ more rigor in Protobuf review +- more rigor in Protobuf review ## Further Discussions