Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add clear guidelines for allowed Protobuf changes #9477

Closed
4 tasks
amaury1093 opened this issue Jun 8, 2021 · 0 comments · Fixed by #9613
Closed
4 tasks

Add clear guidelines for allowed Protobuf changes #9477

amaury1093 opened this issue Jun 8, 2021 · 0 comments · Fixed by #9613
Assignees
Labels
S:needs architecture review To discuss on the next architecture review call to come to alignment T:Docs Changes and features related to documentation. tooling dev tooling within the sdk

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Jun 8, 2021

Summary

Add clear guidelines for allowed Protobuf changes

Problem Definition

App developers are not clear which changes are allowed in proto files (adding fields? removing fields? renaming fields?...).

This led for example to the SDK to add reserved fields to some proto messages (e.g. Vote), which turned out to be a PITA for proto clients.

Proposal

We should define clear guidelines on allowed Protobuf changes within the SDK context.

We currently have the buf linter as a CI check. However, SDK proto checks should be stricter than only buf. Some items that should be checked:

  • Never rename fields.
  • Never remove fields (even if we add a reserved keyword).
  • Never add fields to Msg*s, or else the unknown field rejection will error
    • We also need to be extra careful about adding fields on structs that can be deeply nested in Msg (maybe even in Anys?), because the unknown field rejector might also error on those
  • Think about gogoproto annotations: they are not proto-breaking, but might create API-breaking changes for app developers.

Meeting Notes where this was originally proposed.

Additional notes

The idea to automate this check has also been discussed, but is a nice-to-have in a future step.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093 amaury1093 added the T:Docs Changes and features related to documentation. label Jun 8, 2021
@amaury1093 amaury1093 added this to the Feature Backlog milestone Jun 8, 2021
@amaury1093 amaury1093 added Status: Backlog tooling dev tooling within the sdk labels Jun 8, 2021
@ryanchristo ryanchristo added the S:needs architecture review To discuss on the next architecture review call to come to alignment label Jul 8, 2021
@mergify mergify bot closed this as completed in #9613 Aug 2, 2021
mergify bot pushed a commit that referenced this issue Aug 2, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## 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](https://github.com/cosmos/cosmos-sdk/blob/am/adr-044-protobuf/docs/architecture/adr-044-protobuf-updates-guidelines.md)

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

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

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

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

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

- [x] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] 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
- [x] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:needs architecture review To discuss on the next architecture review call to come to alignment T:Docs Changes and features related to documentation. tooling dev tooling within the sdk
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants