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

x/gov: Proto Tx CLI command #5942

Merged
merged 42 commits into from
Apr 14, 2020
Merged

x/gov: Proto Tx CLI command #5942

merged 42 commits into from
Apr 14, 2020

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Apr 6, 2020

ref: #5864

This PR implements protobuf tx CLI and REST endpoints for the x/gov module and in addition provides an example case of how to setup these commands with an interface handled by a oneof at the app codec level. ADR 021 was updated to reflect the approach demonstrated in this PR.


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)

x/gov/client/cli/tx.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 6, 2020

Codecov Report

Merging #5942 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5942   +/-   ##
=======================================
  Coverage   58.23%   58.23%           
=======================================
  Files         396      396           
  Lines       23621    23621           
=======================================
  Hits        13756    13756           
  Misses       8905     8905           
  Partials      960      960           

x/gov/client/rest/tx.go Outdated Show resolved Hide resolved
x/gov/client/rest/tx.go Show resolved Hide resolved
x/gov/client/rest/tx.go Outdated Show resolved Hide resolved
x/gov/client/rest/tx.go Show resolved Hide resolved
x/gov/client/rest/tx.go Outdated Show resolved Hide resolved
@aaronc aaronc force-pushed the aaronc/5864-x-gov branch from e29cb9c to 23b7c90 Compare April 8, 2020 00:21
x/gov/client/cli/tx.go Outdated Show resolved Hide resolved
x/gov/client/rest/tx.go Outdated Show resolved Hide resolved
x/gov/client/rest/rest.go Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Apr 8, 2020

This pull request introduces 3 alerts when merging a4dab80 into 8eb4808 - view on LGTM.com

new alerts:

  • 3 for Useless assignment to field

@lgtm-com
Copy link

lgtm-com bot commented Apr 9, 2020

This pull request introduces 3 alerts when merging 9437697 into d10de8a - view on LGTM.com

new alerts:

  • 3 for Useless assignment to field

@aaronc
Copy link
Member Author

aaronc commented Apr 9, 2020

@alexanderbez I ran into some cyclic import issues here trying to import codec/std in x/gov/simulation. My sense is that simulation should be able to use codec/std in general. But then x/gov imports x/gov/simulation. I was able to solve the issue by changing several x/gov references to x/gov/types.

Going forward, I have the thought that x/... modules shouldn't import simulation at all. The only reason they do is because of AppModuleSimulation afaik. Wonder if you have any thoughts on that.

@alexanderbez
Copy link
Contributor

Yeah I agree they should not need to import simulation if necessary.

codec/std/codec.go Outdated Show resolved Hide resolved
codec/std/msgs.go Show resolved Hide resolved
docs/architecture/adr-020-protobuf-transaction-encoding.md Outdated Show resolved Hide resolved
docs/architecture/adr-020-protobuf-transaction-encoding.md Outdated Show resolved Hide resolved
types/types.pb.go Outdated Show resolved Hide resolved
codec/std/msgs_test.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.

LGTM @aaronc. Just a few minor nitpicks on variables.

codec/std/msgs.go Show resolved Hide resolved
docs/architecture/adr-020-protobuf-transaction-encoding.md Outdated Show resolved Hide resolved
x/gov/client/cli/tx.go Outdated Show resolved Hide resolved
x/gov/client/cli/tx.go Outdated Show resolved Hide resolved
x/gov/client/cli/tx.go Outdated Show resolved Hide resolved
x/gov/client/cli/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 🎉

x/gov/client/cli/tx.go Outdated Show resolved Hide resolved
x/gov/client/cli/tx.go Outdated Show resolved Hide resolved
@aaronc
Copy link
Member Author

aaronc commented Apr 14, 2020

Okay, all that stuff should be resolved now @alexanderbez

@aaronc aaronc merged commit 58a6c4c into master Apr 14, 2020
@aaronc aaronc deleted the aaronc/5864-x-gov branch April 14, 2020 19:05
@clevinson clevinson added this to the v0.39 milestone May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants