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

R4R: gaiad gentx subcommands refactoring #2874

Merged
merged 2 commits into from
Nov 21, 2018

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Nov 21, 2018

  • Replace STDIN/STDOUT redirection in gaiad gentx with subcommands command line options to redirect streams to file since viper does not handle redirection well.

  • Use BuildCreateValidatorMsg to build a MsgCreateValidator rather than redirecting to gaiacli tx stake create-validator.

  • PrintUnsignedStdTx now takes an io.Writer object.

  • Mark --pubkey, --amount and --moniker as required flags instead of validating them manually.

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added entries in PENDING.md with issue #

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alessio alessio force-pushed the alessio/tx-create-validator-refactor branch from 90baf04 to 94c6c74 Compare November 21, 2018 12:11
@codecov
Copy link

codecov bot commented Nov 21, 2018

Codecov Report

Merging #2874 into develop will increase coverage by 0.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #2874      +/-   ##
===========================================
+ Coverage    56.85%   56.86%   +0.01%     
===========================================
  Files          120      120              
  Lines         8293     8298       +5     
===========================================
+ Hits          4715     4719       +4     
- Misses        3260     3261       +1     
  Partials       318      318

@alessio alessio changed the title WIP: Alessio/tx create validator refactor R4R: gaiad gentx subcommands refactoring Nov 21, 2018
@cwgoes
Copy link
Contributor

cwgoes commented Nov 21, 2018

@alessio Looks like test_cover is failing?

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

A few changes req'd, otherwise looks like a nice cleanup!

@alessio alessio mentioned this pull request Nov 21, 2018
4 tasks
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Tested ACK 👍

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.

Left some surface level feedback. Will test shortly 👌

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.

Tested and works fine. I think we should just remove that shorthand CLI flag and that's all ☕️

@alessio
Copy link
Contributor Author

alessio commented Nov 21, 2018

All comments were addressed @alexanderbez @cwgoes, thanks!

- Replace STDIN/STDOUT redirection in `gaiad gentx` with subcommands
  command line options to redirect streams to file since viper does
  not handle redirection well.
- Use `BuildCreateValidatorMsg` to build a `MsgCreateValidator` rather
  than redirecting to `gaiacli tx stake create-validator`.
- `PrintUnsignedStdTx` now takes an `io.Writer` object.
- Mark `--pubkey`, `--amount` and `--moniker` as required flags
  instead of validating them manually.
- Use stake.NewDescription() to make a new Description - ref #2835
@alessio alessio force-pushed the alessio/tx-create-validator-refactor branch from 7488178 to 486f65c Compare November 21, 2018 17:09
@cwgoes cwgoes merged commit 3e68e44 into develop Nov 21, 2018
@cwgoes cwgoes deleted the alessio/tx-create-validator-refactor branch November 21, 2018 23:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants