R4R: Address gov cli ux issues and add additional input validation for better errors #2938

jackzampolin · 2018-11-29T01:41:28Z

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.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

jackzampolin · 2018-11-29T01:44:15Z

codecov · 2018-11-29T01:49:11Z

Codecov Report

Merging #2938 into develop will decrease coverage by 0.46%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #2938      +/-   ##
===========================================
- Coverage    56.19%   55.72%   -0.47%     
===========================================
  Files          120      120              
  Lines         8383     8468      +85     
===========================================
+ Hits          4711     4719       +8     
- Misses        3350     3427      +77     
  Partials       322      322

alessio · 2018-11-29T16:40:11Z

@jack rebase on top of develop please - the test failures will eventually go

x/gov/client/cli/query.go

cwgoes · 2018-11-29T21:03:42Z

x/gov/client/cli/tx.go

 			amount, err := sdk.ParseCoins(proposal.Deposit)
 			if err != nil {
 				return err
 			}

+			// ensure account has enough coins


This isn't wrong, but it's a weird way to write this logic - we shouldn't be doing this in CLI commands generally, we should be simulating transactions and displaying the error to the user before broadcasting them (if the simulation failed).

If we simulate a submit-proposal transaction with not enough sender balance, it should fail.

Ok. I copied this pattern from the x/auth/client/cli/sign.go. Is there a place where this pattern is shown? The code in this PR fixes some really bad errors and UX in the governance and I works well in the current form.

Discussed, fine for now, let's fix the root cause though (separately) - #2953

x/gov/client/cli/tx.go

cwgoes · 2018-11-29T21:04:28Z

x/gov/client/cli/tx.go

 			if err != nil {
 				return err
 			}

-			msg := gov.NewMsgDeposit(depositorAddr, proposalID, amount)
+			// ensure account has enough coins


Same comment as above (we should simulate instead).

Same comment as above

Co-Authored-By: jackzampolin <jack.zampolin@gmail.com>

fedekunze

these new commands look all very confusing (see comments). IMHO the even worsen the current implementation's UX. Also there's no issue with a proposal that's been discussed and accepted.

x/gov/client/cli/query.go

cmd/gaia/cli_test/cli_test.go

x/gov/client/cli/query.go

jackzampolin · 2018-11-30T00:00:10Z

Here are a couple of issues around the UX on the gov CLI addressed by this PR:

And here is one endorsing the use of positional arguments (which links to one about required flags):

Required flags should be changed to positional arguments where appropriate #1956

If there are one or two required arguments I don't think its confusing (especially if the documentation is accurate and helpful, which the help text is). It may be confusing the first time the user uses the tool, but it's a HUGE improvement for anyone who uses the tool more than once (the users we care about). It also makes commands much more realistic to type in.

Also we have WAY too many flags in the current CLI and this eliminates some of them.

alessio · 2018-11-30T13:45:15Z

I think docs require update

fedekunze · 2018-11-30T15:32:38Z

Here are a couple of issues around the UX on the gov CLI addressed by this PR:

Thanks for the clarification, you forgot to mention those on the description and updating PENDING.md, so there was no clear issue which was being resolved/addressed by this PR.

If there are one or two required arguments I don't think its confusing (especially if the documentation is accurate and helpful, which the help text is) ...

Also we have WAY too many flags in the current CLI and this eliminates some of them.

Indeed the docs are accurate, but switching to args just because we have too many flags is not a great solution imo. If we want it to be descriptive and objective about the args we should stick with flags. Users can use shorthands for flags if they are tired of writing the full name

jackzampolin · 2018-12-03T19:18:53Z

Because 1/2 of the commands changed here required 1 flag: --proposal-id which doesn't have a short example and works just as well as a positional argument. The other 1/2 require --proposal-id and an address. I think this works just as well as positional arguments. I haven't changed commands like submit-proposal that require a number of flags.

Also may of these "required" flags we not Required by our code causing bad user experience issues.

fedekunze

utACK. Minor comments to keep consistency with the cmds descriptions

docs/gaia/gaiacli.md

fedekunze · 2018-12-03T20:22:03Z

docs/gaia/gaiacli.md

+gaiacli query gov tally <proposal_id>
+```
+
+#### Query governance parameters


++ thanks !

Co-Authored-By: jackzampolin <jack.zampolin@gmail.com>

…ter errors (#2938) * Fix governance cli ux issues and add additional transaction validation

jackzampolin requested review from cwgoes, ebuchman and rigelrozanski as code owners November 29, 2018 01:41

jackzampolin requested a review from zramsay as a code owner November 29, 2018 16:53

jackzampolin changed the title ~~WIP: Address gov cli ux issues and add additional input validation for better errors~~ R4R: Address gov cli ux issues and add additional input validation for better errors Nov 29, 2018

jackzampolin added the ready-for-review label Nov 29, 2018

jackzampolin added 4 commits November 29, 2018 12:40

Fix governance cli ux issues and add additional transaction validation

9e61f37

Remove last usage of proposal-id flag

429d184

Add additional help text and begin to fix tests

bf950de

Fix broken test

67ea707

jackzampolin force-pushed the jack/gov-cli-fixes branch from 8ccc88c to 67ea707 Compare November 29, 2018 20:43

cwgoes suggested changes Nov 29, 2018

View reviewed changes

cwgoes and others added 4 commits November 29, 2018 13:05

Update x/gov/client/cli/query.go

5306b14

Co-Authored-By: jackzampolin <jack.zampolin@gmail.com>

Update x/gov/client/cli/tx.go

77776c0

Co-Authored-By: jackzampolin <jack.zampolin@gmail.com>

Update x/gov/client/cli/query.go

c9abcc5

Co-Authored-By: jackzampolin <jack.zampolin@gmail.com>

Update x/gov/client/cli/query.go

a298eb8

Co-Authored-By: jackzampolin <jack.zampolin@gmail.com>

cwgoes approved these changes Nov 29, 2018

View reviewed changes

fedekunze suggested changes Nov 29, 2018

View reviewed changes

x/gov/client/cli/query.go Show resolved Hide resolved

cmd/gaia/cli_test/cli_test.go Show resolved Hide resolved

cmd/gaia/cli_test/cli_test.go Show resolved Hide resolved

x/gov/client/cli/query.go Show resolved Hide resolved

jackzampolin added 2 commits December 3, 2018 11:06

Update docs and add PENDING entry

28c7d85

Merge branch 'develop' into jack/gov-cli-fixes

3f5b37e

fedekunze approved these changes Dec 3, 2018

View reviewed changes

fedekunze and others added 2 commits December 3, 2018 12:29

Update docs/gaia/gaiacli.md

266380c

Co-Authored-By: jackzampolin <jack.zampolin@gmail.com>

Update docs/gaia/gaiacli.md

8c3c454

Co-Authored-By: jackzampolin <jack.zampolin@gmail.com>

jackzampolin merged commit 886bd35 into develop Dec 3, 2018

jackzampolin deleted the jack/gov-cli-fixes branch December 3, 2018 20:48

mircea-c pushed a commit that referenced this pull request Dec 5, 2018

Address gov cli ux issues and add additional input validation for bet…

02b00cc

…ter errors (#2938) * Fix governance cli ux issues and add additional transaction validation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Address gov cli ux issues and add additional input validation for better errors #2938

R4R: Address gov cli ux issues and add additional input validation for better errors #2938

jackzampolin commented Nov 29, 2018

jackzampolin commented Nov 29, 2018

codecov bot commented Nov 29, 2018 •

edited

Loading

alessio commented Nov 29, 2018

cwgoes Nov 29, 2018

jackzampolin Nov 29, 2018

cwgoes Nov 29, 2018

cwgoes Nov 29, 2018

jackzampolin Nov 29, 2018

fedekunze left a comment •

edited

Loading

jackzampolin commented Nov 30, 2018

alessio commented Nov 30, 2018

fedekunze commented Nov 30, 2018

jackzampolin commented Dec 3, 2018

fedekunze left a comment

fedekunze Dec 3, 2018

jackzampolin Dec 3, 2018

R4R: Address gov cli ux issues and add additional input validation for better errors #2938

R4R: Address gov cli ux issues and add additional input validation for better errors #2938

Conversation

jackzampolin commented Nov 29, 2018

jackzampolin commented Nov 29, 2018

codecov bot commented Nov 29, 2018 • edited Loading

Codecov Report

alessio commented Nov 29, 2018

cwgoes Nov 29, 2018

Choose a reason for hiding this comment

jackzampolin Nov 29, 2018

Choose a reason for hiding this comment

cwgoes Nov 29, 2018

Choose a reason for hiding this comment

cwgoes Nov 29, 2018

Choose a reason for hiding this comment

jackzampolin Nov 29, 2018

Choose a reason for hiding this comment

fedekunze left a comment • edited Loading

Choose a reason for hiding this comment

jackzampolin commented Nov 30, 2018

alessio commented Nov 30, 2018

fedekunze commented Nov 30, 2018

jackzampolin commented Dec 3, 2018

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze Dec 3, 2018

Choose a reason for hiding this comment

jackzampolin Dec 3, 2018

Choose a reason for hiding this comment

codecov bot commented Nov 29, 2018 •

edited

Loading

fedekunze left a comment •

edited

Loading