docs(adr): ADR-044 Guidelines for updating proto defs #9613

amaury1093 · 2021-06-30T12:47:16Z

Description

This ADR is to be merged as "DRAFT" status for now, as the details for the "Bumping Protobuf Package Version" section have not been sorted out yet.

This ADR comes from discussion with @webmaster128 and @robert-zaremba about proto updates strategy. We decided to go for an ADR to document our decision for v0.43, and for visibility for other chains doing proto upgrades.

rendered

Closes: #9477
ref: #9446
ref: #9445

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

amaury1093 · 2021-07-05T14:17:16Z

docs/architecture/adr-044-protobuf-updates-guidelines.md

+
+## Abstract
+
+This ADR provides guidelines and recommended practices when updating Protobuf definitions. These guidelines are targeted at module developers.


this is a good example where it's halfway between ADR and docs.

Talking with some people, we decided to go with an ADR, as it may provide more visibility for other chain & client developers

we decided to go with an ADR, as it may provide more visibility for other chain & client developers

I think this makes sense. I'm wondering if we should also be pointing module developers to this ADR somewhere within the building modules documentation. Adding a line or two that mentions best practices when making updates to Protobuf definitions that points to these guidelines might provide a bit more visibility. For example, I think a simple reference to these guidelines could be added in the Msg Service documentation.

Definitely makes sense to me!

amaury1093 · 2021-07-05T14:19:15Z

docs/architecture/adr-044-protobuf-updates-guidelines.md

+
+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.


In v0.43, we deprecated a field, added some proto comments around it, and introduced a protocol-breaking change related to this field. Do we want to continue doing so in the future?

An argument could be made that protocol-breaking change => proto pkg bump. Or more generally, what's a "breaking change" which would warrant a proto pkg bump?

Do we want to continue doing so in the future?

Ideally, I think we want to avoid this in the future and a protocol-breaking change should warrant a proto pkg bump but this guideline might be worth keeping as a recovery solution should there be a need again.

ryanchristo

I think this is a solid initial draft. Nice work! A few suggestions and comments.

ryanchristo · 2021-07-08T19:02:00Z

docs/architecture/adr-044-protobuf-updates-guidelines.md

+
+## Abstract
+
+This ADR provides guidelines and recommended practices when updating Protobuf definitions. These guidelines are targeted at module developers.


we decided to go with an ADR, as it may provide more visibility for other chain & client developers

I think this makes sense. I'm wondering if we should also be pointing module developers to this ADR somewhere within the building modules documentation. Adding a line or two that mentions best practices when making updates to Protobuf definitions that points to these guidelines might provide a bit more visibility. For example, I think a simple reference to these guidelines could be added in the Msg Service documentation.

ryanchristo · 2021-07-08T19:03:07Z

docs/architecture/adr-044-protobuf-updates-guidelines.md

+- `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.


Suggested change

- `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.

Would it make sense to use the version suffix and include the crypto packages in v1? Why was it decided that these packages should not be versioned?

IIRC we thought these defs should never change. But tbh i'm not opposed to have everything version-suffixed and put crypto as v1.

docs/architecture/adr-044-protobuf-updates-guidelines.md

ryanchristo · 2021-07-08T21:22:56Z

docs/architecture/adr-044-protobuf-updates-guidelines.md

+
+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.


Do we want to continue doing so in the future?

Ideally, I think we want to avoid this in the future and a protocol-breaking change should warrant a proto pkg bump but this guideline might be worth keeping as a recovery solution should there be a need again.

Co-authored-by: Ryan Christoffersen <[email protected]>

orijbot · 2021-07-13T12:58:25Z

Visit https://dashboard.github.orijtech.com?pr=9613&repo=cosmos%2Fcosmos-sdk to see benchmark details.

amaury1093 · 2021-07-13T12:59:17Z

Thanks @ryanchristo for the review!

robert-zaremba

Pre approving

let's use more MUST instead of SHALL
Let's add "Draft" section to the README and add this ADR there
Also left few suggestions

docs/architecture/adr-044-protobuf-updates-guidelines.md

robert-zaremba · 2021-07-30T09:53:45Z

docs/architecture/adr-044-protobuf-updates-guidelines.md

+
+#### 1. `Msg`s SHALL 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).


Shall we be more specific here:

we allow new fields for objects which are saved in the store

we don't allow new fields for the Msg service arguments

It's a bit related to #9613 (comment), so I added a new section

docs/architecture/adr-044-protobuf-updates-guidelines.md

Co-authored-by: Robert Zaremba <[email protected]>

amaury1093 · 2021-08-02T09:28:29Z

I'm going to put automerge on. It's been 1+ month, and have 2 approvals from relevant people, so as a draft it should be okay. Can have another round of review with external parties once this goes into "proposed"

amaury1093 added 2 commits June 30, 2021 12:53

adr: ADR-044 Guidelines for updating proto defs

1c61184

More details

dd22934

github-actions bot added the T: ADR An issue or PR relating to an architectural decision record label Jun 30, 2021

amaury1093 changed the title ~~adr: ADR-044 Guidelines for updating proto defs~~ docs(adr): ADR-044 Guidelines for updating proto defs Jun 30, 2021

amaury1093 requested review from webmaster128, aaronc and robert-zaremba June 30, 2021 13:01

amaury1093 added 2 commits June 30, 2021 15:20

Tweaks

184095c

Add more content

2853012

amaury1093 marked this pull request as ready for review July 5, 2021 14:15

amaury1093 requested a review from alexanderbez as a code owner July 5, 2021 14:15

Merge branch 'master' into am/adr-044-protobuf

c552637

amaury1093 commented Jul 5, 2021

View reviewed changes

blushi requested a review from ryanchristo July 8, 2021 15:09

clevinson assigned ryanchristo Jul 8, 2021

ryanchristo approved these changes Jul 8, 2021

View reviewed changes

amaury1093 mentioned this pull request Jul 9, 2021

Param name change during on-chain upgrade breaks the start command #9637

Closed

4 tasks

amaury1093 and others added 11 commits July 13, 2021 14:52

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

a0e9b46

Co-authored-by: Ryan Christoffersen <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

e1f7bd4

Co-authored-by: Ryan Christoffersen <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

6f6060e

Co-authored-by: Ryan Christoffersen <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

b400f34

Co-authored-by: Ryan Christoffersen <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

c065ffa

Co-authored-by: Ryan Christoffersen <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

c930860

Co-authored-by: Ryan Christoffersen <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

eb5793f

Co-authored-by: Ryan Christoffersen <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

8788891

Co-authored-by: Ryan Christoffersen <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

55741cc

Co-authored-by: Ryan Christoffersen <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

28e11e1

Co-authored-by: Ryan Christoffersen <[email protected]>

Merge branch 'master' into am/adr-044-protobuf

69a90eb

robert-zaremba approved these changes Jul 30, 2021

View reviewed changes

amaury1093 and others added 11 commits July 30, 2021 13:10

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

5574a57

Co-authored-by: Robert Zaremba <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

25b2dea

Co-authored-by: Robert Zaremba <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

c1c51ed

Co-authored-by: Robert Zaremba <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

a0fdf0f

Co-authored-by: Robert Zaremba <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

3a16343

Co-authored-by: Robert Zaremba <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

eaf00cd

Co-authored-by: Robert Zaremba <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

5799e55

Co-authored-by: Robert Zaremba <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

e143f19

Co-authored-by: Robert Zaremba <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

9934ff2

Co-authored-by: Robert Zaremba <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

ba6f859

Co-authored-by: Robert Zaremba <[email protected]>

Update docs/architecture/adr-044-protobuf-updates-guidelines.md

ca0fe2a

Co-authored-by: Robert Zaremba <[email protected]>

amaury1093 mentioned this pull request Jul 30, 2021

feat: add array of messages to a proposal #9810

Closed

19 tasks

amaury1093 added 2 commits July 30, 2021 13:40

Apply suggestions

2db5c07

Merge branch 'master' into am/adr-044-protobuf

cb07512

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 2, 2021

mergify bot merged commit 56589f1 into master Aug 2, 2021

mergify bot deleted the am/adr-044-protobuf branch August 2, 2021 09:32

amaury1093 mentioned this pull request Aug 4, 2021

revert: Mark upgrade query field as deprecated instead of reserved #9847

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(adr): ADR-044 Guidelines for updating proto defs #9613

docs(adr): ADR-044 Guidelines for updating proto defs #9613

amaury1093 commented Jun 30, 2021 •

edited by ryanchristo

Loading

amaury1093 Jul 5, 2021

ryanchristo Jul 8, 2021

amaury1093 Jul 13, 2021

amaury1093 Jul 5, 2021

ryanchristo Jul 8, 2021

ryanchristo left a comment

ryanchristo Jul 8, 2021

ryanchristo Jul 8, 2021

ryanchristo Jul 8, 2021

amaury1093 Jul 13, 2021

ryanchristo Jul 8, 2021

orijbot commented Jul 13, 2021

amaury1093 commented Jul 13, 2021

robert-zaremba left a comment •

edited by amaury1093

Loading

robert-zaremba Jul 30, 2021

amaury1093 Jul 30, 2021

amaury1093 commented Aug 2, 2021


		## Abstract

		This ADR provides guidelines and recommended practices when updating Protobuf definitions. These guidelines are targeted at module developers.


		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.

	- `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.


		#### 1. `Msg`s SHALL 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).

docs(adr): ADR-044 Guidelines for updating proto defs #9613

docs(adr): ADR-044 Guidelines for updating proto defs #9613

Conversation

amaury1093 commented Jun 30, 2021 • edited by ryanchristo Loading

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orijbot commented Jul 13, 2021

amaury1093 commented Jul 13, 2021

robert-zaremba left a comment • edited by amaury1093 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 commented Aug 2, 2021

amaury1093 commented Jun 30, 2021 •

edited by ryanchristo

Loading

robert-zaremba left a comment •

edited by amaury1093

Loading