Skip to content
New issue

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

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

Already on GitHub? # to your account

Protobuf: x/auth & x/supply #5533

Merged
merged 70 commits into from
Feb 18, 2020
Merged

Protobuf: x/auth & x/supply #5533

merged 70 commits into from
Feb 18, 2020

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Jan 16, 2020

Protocol Buffer migration in x/auth based on #5491. This PR addresses the following:

  • Migrate x/auth to protobuf
  • Migrate x/supply to protobuf
    • Remove internal/ sub-pkg
  • Refactor app-level codec (simapp/codec)
    • We now have a single codec that we pass to all keepers/modules
  • Refactor AppModuleBasic and AppModuleGenesis to now accept a codec for modular serialization of genesis state.
  • Accounts now have their pubkey represented as a bech32 string.
  • Remove Codec from already migrated modules in favor of ModuleCdc = codec.NewHybridCodec(amino).

TODO:

  • Figure out cleaner way to handle x/auth client logic that relies on a codec.

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)

@alexanderbez alexanderbez added R4R and removed WIP labels Feb 14, 2020
@alexanderbez alexanderbez marked this pull request as ready for review February 14, 2020 20:14
@alexanderbez alexanderbez changed the title Protobuf: x/auth Protobuf: x/auth & x/supply Feb 14, 2020
// AuthCodec interface and must be the same codec that passed to the x/auth
// module.
//
// TODO:/XXX: Using a package-level global isn't ideal and we should consider
Copy link
Member

Choose a reason for hiding this comment

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

can we open an issue as a proposal about this?

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 actually want to tackle it in this PR -- still thinking.

//
// This type should only be used internally for testing purposes. The application's
// concrete Account type should be defined in the application-level codec.
message VestingAccount {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe name to something like TestVestingAccount just so it isn't used mistakenly.

@aaronc aaronc mentioned this pull request Feb 17, 2020
11 tasks
@codecov
Copy link

codecov bot commented Feb 18, 2020

Codecov Report

Merging #5533 into master will decrease coverage by 0.12%.
The diff coverage is 53.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5533      +/-   ##
==========================================
- Coverage   39.54%   39.42%   -0.13%     
==========================================
  Files         326      326              
  Lines       29302    29014     -288     
==========================================
- Hits        11588    11438     -150     
+ Misses      16486    16386     -100     
+ Partials     1228     1190      -38
Impacted Files Coverage Δ
x/auth/vesting/types/types.pb.go 38.9% <0%> (-4%) ⬇️
x/genutil/client/cli/init.go 70.9% <100%> (ø) ⬆️
types/rest/rest.go 53.8% <85.71%> (+1.72%) ⬆️
x/auth/vesting/types/codec.go 100% <0%> (+100%) ⬆️

@alexanderbez alexanderbez merged commit 794a496 into master Feb 18, 2020
@alexanderbez alexanderbez deleted the bez/5444-auth-proto-enc branch February 18, 2020 12:50
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C:x/auth T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants