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 x/auth cmd's to TxGenerator marshaling #6391

Merged
merged 38 commits into from
Jun 18, 2020
Merged

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Jun 10, 2020

This is one of several bite-size PR's that are being pulled out of #6216 to implement #6213.

Summary

This PR makes incremental progress on migrating the remaining x/auth CLI commands to the new infrastructure for protobuf as well as setting the foundation for #6190.

Details

  • add TxEncoder|Decoder and TxJSONEncoder|Decoder methods to TxGenerator and remove Marshal method.
  • migrate x/auth encode, decode, broadcast, sign, multisign, and validate-signatures Cmd's to use the TxGenerator Encoder/Decoder methods
  • add EncodingConfig struct and MakeEncodingConfig methods to simapp and simapp/params to incrementally move towards Full amino encoding support #6190.
  • remove std.Codec (which is no longer needed because of Any) and replace with codec.Marshaler

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

@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #6391 into master will increase coverage by 0.12%.
The diff coverage is 38.04%.

@@            Coverage Diff             @@
##           master    #6391      +/-   ##
==========================================
+ Coverage   56.13%   56.26%   +0.12%     
==========================================
  Files         467      469       +2     
  Lines       28061    28091      +30     
==========================================
+ Hits        15753    15806      +53     
+ Misses      11201    11170      -31     
- Partials     1107     1115       +8     

@aaronc aaronc changed the title Migrate encode, decode, and broadcast cmd's to use TxGenerator marsha… Migrate encode, decode, and broadcast cmd's to TxGenerator methods Jun 10, 2020
@aaronc aaronc changed the title Migrate encode, decode, and broadcast cmd's to TxGenerator methods Migrate x/auth cmd's to TxGenerator marshaling Jun 10, 2020
@clevinson clevinson added this to the v0.39 milestone Jun 11, 2020
atheeshp and others added 3 commits June 15, 2020 15:28
* fixed lint issue of "txEncodeRespStr"

* added tests for encode.go

* WIP: added tests for decode.go

* added txEncoder at bytes

* updated decode test

* updated txBytes to use TxEncoder in decoder test

* added a require after TxEncoder

* removed file save

* debug decode command

* fixed decode tests

* added decode cli

* updated encode and decode in a single file

* separated decode test from encode test

* review changes

* removed register interface

* review change
@aaronc aaronc marked this pull request as ready for review June 15, 2020 19:37
x/auth/client/cli/broadcast_test.go Show resolved Hide resolved
x/auth/client/cli/cli_test.go Outdated Show resolved Hide resolved
x/auth/client/cli/cli_test.go Outdated Show resolved Hide resolved
simapp/cmd/simcli/main.go Show resolved Hide resolved
x/auth/client/cli/validate_sigs.go Show resolved Hide resolved
x/auth/client/cli/tx_multisign.go Show resolved Hide resolved
x/auth/client/cli/encode_test.go Outdated Show resolved Hide resolved
@aaronc
Copy link
Member Author

aaronc commented Jun 18, 2020

@fedekunze @anilcse can we please postpone tests for sign, multisign and validate-signatures to other PRs? I am planning to migrate all of those cmd's to protobuf in my next PRs, but I can't do that because I'm blocked by this PR. We have added tests for stuff that makes sense for this PR.

@fedekunze can you take another look and see if it's alright to move forward with this PR with the understanding that this is part of a larger migration of those other commands.

@aaronc
Copy link
Member Author

aaronc commented Jun 18, 2020

Still fixing one test btw...

@aaronc
Copy link
Member Author

aaronc commented Jun 18, 2020

@fedekunze fixed tests and added client.TestAccountRetriever in order to get TestPrepareTxBuilder working.

@aaronc aaronc requested a review from anilcse June 18, 2020 16:59
@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 18, 2020
Copy link
Collaborator

@anilcse anilcse 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 aaronc removed the A:automerge Automatically merge PR once all prerequisites pass. label Jun 18, 2020
@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 18, 2020
@fedekunze
Copy link
Collaborator

@aaronc looks like a test is failing

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Nice one! ACK'd - pending tests pass

@mergify mergify bot merged commit 712d237 into master Jun 18, 2020
@mergify mergify bot deleted the aaronc/6213-cli-encode branch June 18, 2020 20:29
@aaronc aaronc mentioned this pull request Jul 7, 2020
27 tasks
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* Migrate encode, decode, and broadcast cmd's to use TxGenerator marshal methods

* Fix tests, add EncodingConfig

* Add simapp/encoding.go and wire up with simcli

* add godocs

* fix tests

* Debugging CLI Tests

* Fix integration test

* 6391 - lint issues and code coverage (cosmos#6414)

* fixed lint issue of "txEncodeRespStr"

* added tests for encode.go

* WIP: added tests for decode.go

* added txEncoder at bytes

* updated decode test

* updated txBytes to use TxEncoder in decoder test

* added a require after TxEncoder

* removed file save

* debug decode command

* fixed decode tests

* added decode cli

* updated encode and decode in a single file

* separated decode test from encode test

* review changes

* removed register interface

* review change

* Fix tests

* WIP add test for tx sign

* removed commented code

* Fix flags

* WIP add test for sign

* Address review suggestions

* fixed command issue

* Add tests for TxEncoder

* Revert sign changes

* Fix TxEncoder tests

* Fix GetSign Cmd

* Add tx test

* Remove duplicate validation

* Add tests for TxDecoder

* Fix tests

* Fix tests

* Output to clientCtx.Output

* Fix cli_tests

Co-authored-by: atheeshp <59333759+atheeshp@users.noreply.github.com>
Co-authored-by: atheesh <atheesh@vitwit.com>
Co-authored-by: anilCSE <anil@vitwit.com>
Co-authored-by: sahith-narahari <sahithnarahari@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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. C:x/auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants