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

Migrate upgrade module to protobuf #5659

Merged
merged 11 commits into from
Feb 24, 2020

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Feb 17, 2020

Description

Migrates x/upgrade to support protobuf encoding.


For contributor use:

  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Feb 17, 2020

Codecov Report

Merging #5659 into master will decrease coverage by 0.65%.
The diff coverage is 1.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5659      +/-   ##
==========================================
- Coverage   36.12%   35.46%   -0.66%     
==========================================
  Files         330      331       +1     
  Lines       31976    32581     +605     
==========================================
+ Hits        11550    11556       +6     
- Misses      19203    19802     +599     
  Partials     1223     1223
Impacted Files Coverage Δ
x/upgrade/types/keys.go 0% <ø> (ø)
x/upgrade/types/plan.go 88.88% <ø> (ø)
x/upgrade/types/querier.go 0% <ø> (ø)
x/upgrade/types/proposal.go 82.35% <ø> (ø)
x/upgrade/types/codec.go 100% <ø> (ø)
x/upgrade/types/types.pb.go 0.99% <0.99%> (ø)
simapp/app.go 89.1% <100%> (ø) ⬆️

simapp/app.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK -- Thank @aaronc! A few minor things:

  1. Use appCodec in simapp now.
  2. Address changelog merge conflict
  3. Remove use of internal/ pkg (just bump the directory up to the parent; ref: Remove Internal from Module Spec + Update Modules #5646)

simapp/app.go Outdated Show resolved Hide resolved
x/upgrade/client/cli/tx.go Outdated Show resolved Hide resolved
x/upgrade/client/rest/query.go Outdated Show resolved Hide resolved
x/upgrade/client/rest/tx.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK 🌮 there are just a few minor linting/formatting issues.

x/upgrade/alias.go Outdated Show resolved Hide resolved
x/upgrade/alias.go Outdated Show resolved Hide resolved
x/upgrade/client/rest/tx.go Outdated Show resolved Hide resolved
x/upgrade/keeper/keeper.go Outdated Show resolved Hide resolved
x/upgrade/keeper/querier.go Outdated Show resolved Hide resolved
@alexanderbez alexanderbez added the T: State Machine Breaking State machine breaking changes (impacts consensus). label Feb 19, 2020
x/upgrade/client/rest/tx.go Outdated Show resolved Hide resolved
x/upgrade/alias.go Outdated Show resolved Hide resolved
@aaronc
Copy link
Member Author

aaronc commented Feb 19, 2020

@alexanderbez I think I took care of most of the issues. I'm a bit confused by a couple of golangcibot's recommendations for the CLI package as those imports do appear to be needed.

@aaronc
Copy link
Member Author

aaronc commented Feb 19, 2020

Looks like there are a few minor formatting things. Will take a look. And something is weird with CircleCI proto.

@alexanderbez
Copy link
Contributor

proto-breaking doesn't work with PRs that are created from different remotes (i.e. forks). Not sure how to get this working. I need to ask in the Buf repo how to do this.

aaronc and others added 9 commits February 24, 2020 11:19
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK

@alexanderbez alexanderbez merged commit df65016 into cosmos:master Feb 24, 2020
@ryanchristo ryanchristo deleted the aaronc/5444-proto-upgrade branch December 12, 2022 18:16
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/upgrade T: State Machine Breaking State machine breaking changes (impacts consensus).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants