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

Audit / Refactor proto packages layout #6905

Merged
merged 36 commits into from
Aug 11, 2020
Merged

Audit / Refactor proto packages layout #6905

merged 36 commits into from
Aug 11, 2020

Conversation

clevinson
Copy link
Contributor

@clevinson clevinson commented Jul 31, 2020

Description

New proposed layout:

  • Follow buf's best practices and add package version suffix everywhere
  • Current version is v1beta1, and every package has this version
.
├── cosmos
│   ├── bank
│   │   └── v1beta1
│   │       ├── bank.proto
│   │       ├── genesis.proto
│   │       ├── query.proto
│   │       └── tx.proto
│   ├── base
│   │   ├── abci
│   │   │   └── v1beta1
│   │   │        └── abci.proto
│   │   ├── crypto
│   │   │   └── v1beta1
│   │   │       └── crypto.proto
│   │   ├── store
│   │   │   └── v1beta1
│   │   │       └── kv.proto
│   │   ├── query
│   │   │   └── v1beta1
│   │   │       └── pagination.proto
│   │   └── v1beta1
│   │       └── coin.proto
│   └── tx
│       └── v1beta1
│           └── tx.proto

Highlights of the main changes made:

  1. cosmos/cosmos.proto refactored to cosmos.base
    a. ValidatorAddresses moved to cosmos.staking
    b. cosmos.base left with 2 files:
    1. cosmos/base/abci.proto, containing all proto definitions related to simulation / broadcast / responses from tendermint (e.g. Result, GasInfo, TxResponse...)
    2. cosmos/base/coin.proto, containing coin related proto definitions
      c. all remaining non-module packages (kv, query, tx) moved under cosmos.base
  2. base packages marked as stable (v1), and module packages all marked as beta (this was discussed July 31st's architecture call, notes here)
  3. cosmos module Msg's moved into a tx.proto file within each module package (as proposed by @amaurymartiny when commenting on Audit Protobuf package names & update RegisterInterface calls #6822)

closes: #6822


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
Copy link
Contributor Author

clevinson commented Jul 31, 2020

Some notes on the various messages that I'd still like to be pulled out of cosmos.base:

My initial thought was to pile everything inside packages within cosmos.tx, but there's a few reasons why that may not be the best solution...

  • SimulationResponse, Result, GasInfo
    • Could be cosmos.tx.simulation, but there are uses in go files of Result & GasInfo that dont necessarily correspond to a SimulationResponse (e.g. types/result.go)...
  • MsgData and TxData
    • Seems most related to the cosmos.tx package?
  • TxResponse, ABCIMessageLog, etc.
    • Could put them in a new cosmos.tx.broadcast package as these are mostly used in broadcast, but there are other places where `ABCIMessageLog is used directly from the proto definition, so perhaps needs broader contextual home?

@codecov
Copy link

codecov bot commented Aug 1, 2020

Codecov Report

Merging #6905 into master will increase coverage by 3.49%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master    #6905      +/-   ##
==========================================
+ Coverage   61.86%   65.36%   +3.49%     
==========================================
  Files         520      390     -130     
  Lines       32108    24271    -7837     
==========================================
- Hits        19865    15864    -4001     
+ Misses      10628     7165    -3463     
+ Partials     1615     1242     -373     

@clevinson
Copy link
Contributor Author

Please see updated description above, which gives an overview of the changes being made in this PR.

The piece I'm least certain about is the beta naming of all module packages (basically every proto package that corresponds to an "sdk module that lives in x/"). It was discussed in the architecture call today (notes here) that there really isn't anything that we can confidently say won't change in the future, except maybe that in cosmos.base.

Not sure if thats 100% the best way to structure stuff, but this is my attempt at it. Also- i'll be on vacation next week and am happy for anyone else to pick this up while i'm out and push it over the finish line.

@clevinson clevinson marked this pull request as ready for review August 1, 2020 02:24
@clevinson clevinson requested review from amaury1093 and anilcse August 1, 2020 02:24
@clevinson clevinson added the Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. label Aug 1, 2020
@clevinson clevinson added this to the v0.40 [Stargate] milestone Aug 1, 2020
proto/cosmos/auth/beta/query.proto Outdated Show resolved Hide resolved
proto/cosmos/bank/beta/bank.proto Outdated Show resolved Hide resolved
proto/cosmos/auth/beta/auth.proto Outdated Show resolved Hide resolved
@amaury1093 amaury1093 self-assigned this Aug 3, 2020
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Why beta and not the more standard v1beta?

I think cosmos and cosmos.tx should both be beta as well, at least cosmos.tx

@aaronc
Copy link
Member

aaronc commented Aug 7, 2020

I've thought about this a bit more. My biggest hesitancy about putting everything into beta is that then we need to move everything out of beta soon which will break things.

At the same time, I also feel that calling everything stable and fixed right away does tie our hands a bit and might be a bit rushed. We're doing the first SDK protobuf release ever and there are a lot of proto definitions. We might have gotten some major things wrong or just find out a radically better way to do certain things 1-2 months after the release. So maintaining the optionality of beta status does seem merited.

What if we:

  1. do our best to tag things we know are alpha/beta/etc. as proposed above
  2. announce clearly that we will enforce buf break check starting with the next 0.41 release (which should have a cool name of its own - maybe "Horizon")

This reduces the churn of marking and then unmarking everything as beta but relieves the pressure of having to make all the 1.0 time decisions right now, already late for a release.

What do people think?

@aaronc
Copy link
Member

aaronc commented Aug 7, 2020

So it seems like we agreed in the meeting today to move everything into v1beta1 packages and just communicate to early adopters that types will be moved into stable packages in future releases.

This appears to be preferred over my proposal to just delay making stable packages actually "stable".

@amaury1093
Copy link
Contributor

This PR is ready for another round of review. I has been updated to reflect last Friday's call decision, see also 1st comment for an overview of the folder arch.

@amaury1093 amaury1093 mentioned this pull request Aug 10, 2020
9 tasks
@clevinson clevinson self-assigned this Aug 10, 2020
Copy link
Contributor Author

@clevinson clevinson left a comment

Choose a reason for hiding this comment

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

@amaurymartiny Left a few comments, otherwise LGTM. I cannot approve / request changes, as i was the original author.

codec/types/interface_registry.go Outdated Show resolved Hide resolved
types/codec.go Outdated
@@ -14,5 +14,5 @@ func RegisterCodec(cdc *codec.LegacyAmino) {

// Register the sdk message type
func RegisterInterfaces(registry types.InterfaceRegistry) {
registry.RegisterInterface("cosmos_sdk.v1.Msg", (*Msg)(nil))
registry.RegisterInterface("cosmos.Msg", (*Msg)(nil))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably still register this as a specific version (e.g. cosmos.v1beta1.Msg), right? Only thing is, since it's not actually tied to a specific protobuf definition, it's not super clear when we would update the version in the interface registry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, I changed to "cosmos.v1beta1.Msg" for now, since everything is in beta. But yeah, I guess we should also define which interfaces are beta/stable, maybe add a line somewhere in #6954?

proto/cosmos/tx/v1beta1/tx.proto Show resolved Hide resolved
@amaury1093 amaury1093 force-pushed the proto-packages-audit branch from 413d2c3 to 2a175d1 Compare August 11, 2020 10:17
@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 11, 2020
@mergify mergify bot merged commit 61a97ef into master Aug 11, 2020
@mergify mergify bot deleted the proto-packages-audit branch August 11, 2020 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. T: Proto Breaking Protobuf breaking changes: never don't do this! Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Audit Protobuf package names & update RegisterInterface calls
8 participants