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

Pre-Stargate CHANGELOG.md cleanup #7387

Closed
wants to merge 11 commits into from

Conversation

clevinson
Copy link
Contributor

@clevinson clevinson commented Sep 25, 2020

Description

Cleaning up changelog to make a bit more readable in preperation for Stargate release.

  • Client Breaking
  • API Breaking
  • Features
  • Bug Fixes
  • State Machine Breaking
  • Improvements

rendered

Some general comments / questions about protobuf mentions in changelog:

  • All module level migrations of state serialization to protobuf is documented as State Machine Breaking
  • No mention of gRPC or gRPC Gateway (Should these be added in Features section?)
  • No mention of new proto tx signing path (Should this be added in Features section?)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@clevinson clevinson added the T:Docs Changes and features related to documentation. label Sep 25, 2020
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried rendering the markdown, it looks good. I added some small comments here and there

CHANGELOG.md Outdated Show resolved Hide resolved
### Client Breaking
* (modules) [\#7243](https://github.com/cosmos/cosmos-sdk/pull/7243) Rename `RegisterCodec` to `RegisterLegacyAminoCodec` and `codec.New()` is now renamed to `codec.NewLegacyAmino()`
* (cli) [\#6651](https://github.com/cosmos/cosmos-sdk/pull/6651) The `gentx` command has been improved. No longer are `--from` and `--name` flags required. Instead, a single argument, `name`, is required which refers to the key pair in the Keyring. In addition, an optional
### Client Breaking Changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add as a first sentence that the biggest breaking change is actually the introduction of Protobuf?

something like:

"This release introduces Protobuf (with link) as the new default encoding format. Please refer to ADR-019, ADR-020 and ADR-021 (with links) to learn how Protobuf is used in the SDK."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking this high level information would be better communicated in a RELEASE_NOTES.md ?

Afaik there isn't any breaking client breaking changes w/r/t protobuf migration, as legacy endpoints are all preserved. Are there any way in which CLI endpoints have changed systematically due to protobuf migration?

The state machine breaking changes for each module is documented already. I can add a more general top level one that points to ADR019

I do think we should definitely add a changelog entry to "Features" or "Improvements" to illustrate the new gRPC & gRPC Gateway endpoints. I can add separate sections for Tx & Query paths that correspond to ADR020 & ADR021.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking this high level information would be better communicated in a RELEASE_NOTES.md ?

I believe some people will read the CHANGELOG without reading the RELEASE_NOTES, and vice versa. We could possibly add two links to reference them together.

Afaik there isn't any breaking client breaking changes w/r/t protobuf migration, as legacy endpoints are all preserved. Are there any way in which CLI endpoints have changed systematically due to protobuf migration?

Good point regarding legacy endpoints. All CLI commands are breaking though, since we output protobuf JSON everywhere.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@clevinson clevinson marked this pull request as ready for review September 29, 2020 00:58
@clevinson clevinson marked this pull request as draft September 29, 2020 00:59
@clevinson clevinson closed this Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants