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

Convert Int and Dec to string's on Coin and DecCoin proto types #5505

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Jan 10, 2020


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 Jan 11, 2020

Codecov Report

Merging #5505 into bez/5444-types-proto-enc will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@                     Coverage Diff                     @@
##           bez/5444-types-proto-enc   #5505      +/-   ##
===========================================================
- Coverage                      53.5%   53.5%   -0.01%     
===========================================================
  Files                           314     314              
  Lines                         19317   19319       +2     
===========================================================
  Hits                          10336   10336              
- Misses                         8185    8187       +2     
  Partials                        796     796
Impacted Files Coverage Δ
types/types.pb.go 11.05% <0%> (-0.06%) ⬇️

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.

Proto changes look good. Please revert everything else 👍

.gitignore Outdated
@@ -10,7 +10,6 @@
*.pyc

# Build
vendor
Copy link
Contributor

Choose a reason for hiding this comment

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

Undo 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I did that because otherwise I had no idea vendor was messing up protoc. I can revert, but if anyone else runs go mod vendor they'll have problems.

go.mod Outdated
@@ -15,6 +15,7 @@ require (
github.com/pelletier/go-toml v1.6.0
github.com/pkg/errors v0.8.1
github.com/rakyll/statik v0.1.6
github.com/regen-network/cosmos-proto v0.0.0-20200110230910-60122eba6250 // indirect
Copy link
Contributor

@alexanderbez alexanderbez Jan 13, 2020

Choose a reason for hiding this comment

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

Undo. Instead, update the protoc-gen-gocosmos target to install a specific version:

protoc-gen-gocosmos:
  @(cd /tmp && github.com/regen-network/cosmos-proto/protoc-gen-gocosmos@VERSION)

We cd into tmp to not pollute go.mod -- hacky, but idk another way.

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 7501af3 into cosmos:bez/5444-types-proto-enc Jan 13, 2020
@ryanchristo ryanchristo deleted the aaronc/5444-types-proto-enc branch December 12, 2022 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants