Prompt User Confirmation Prior to Signing & Broadcasting #3698

alexanderbez · 2019-02-20T16:57:56Z

Print an unsigned version of the tx asking for confirmation prior to signing and broadcasting (by default). Clients by bypass with -y/--yes.

closes: #3653

/cc @gamarin2

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

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)

jackzampolin · 2019-02-20T17:46:52Z

client/flags.go

@@ -89,9 +90,14 @@ func PostCommands(cmds ...*cobra.Command) []*cobra.Command {
 		c.Flags().Bool(FlagTrustNode, true, "Trust connected full node (don't verify proofs for responses)")
 		c.Flags().Bool(FlagDryRun, false, "ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it")
 		c.Flags().Bool(FlagGenerateOnly, false, "build an unsigned transaction and write it to STDOUT")


Maybe we can add some other short flags here cc @alessio

Sure! But most definitely in a separate PR :)

jackzampolin · 2019-02-20T17:48:08Z

client/utils/utils.go

+		fmt.Fprintf(os.Stderr, "%s\n\n", cliCtx.Codec.MustMarshalJSON(stdSignMsg))
+
+		buf := client.BufferStdin()
+		ok, err := client.GetConfirmation("confirm transaction before signing and broadcasting", buf)


It looks like this signs the transaction, then asks for confirmation of the signing? Seems a bit off?

I don't follow? Nothing is signed at this point. This asks to confirm the tx before signing and broadcasting which happen on subsequent lines.

Ah. Thank you for getting me straight here. I was confused by the txBldr.BuildSignMsg call (looked at function, understand now)

codecov · 2019-02-20T18:09:03Z

Codecov Report

Merging #3698 into develop will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #3698      +/-   ##
===========================================
- Coverage    61.18%   61.15%   -0.03%     
===========================================
  Files          190      190              
  Lines        14042    14052      +10     
===========================================
+ Hits          8591     8593       +2     
- Misses        4913     4921       +8     
  Partials       538      538

jackzampolin · 2019-02-20T20:12:21Z

Kicked the test. CLI tests failed because it took more than 10m to run. This is generally an indication of a hanging test.

alexanderbez · 2019-02-20T20:17:18Z

@jackzampolin yes, TestGaiaCLIConfirmTx was broken -- I fixed it and CLI tests are passing now.

gamarin2 · 2019-02-21T10:51:11Z

client/utils/utils.go

+		fmt.Fprintf(os.Stderr, "%s\n\n", cliCtx.Codec.MustMarshalJSON(stdSignMsg))
+
+		buf := client.BufferStdin()
+		ok, err := client.GetConfirmation("confirm transaction before signing and broadcasting", buf)


I think the confirmation should focus on getting the user to double check the important information, not the entire transaction. These would be:

For all messages, the fee amount.

For send message, the recipient.

Maybe the transaction can be displayed as well, but it's especially important that users check the two above.

Ehhh this is pretty complicated @gamarin2. I don't understand why we can just display the (unsigned) tx. It already includes sender/recipient and it already includes the fee and gas -- everything the client should know.

Otherwise, we'll need to have a type switch here on every single msg in the tx and the UX will be ugly and not to mention spaghetti code.

Hmm the point of the original issue was to avoid excessive fees. For a user, the two most important things to check are fees for all message types and recipient if msgType = send.

I was hoping a simple display of "This will cost you X atoms in fees", with the conversion uatoms atoms performed for the end user. If you see {"fee":{"amount":"3000000000000000", "denom":"uatom"}} it's actually hard to check if you aren't paying ten times more fees than expected, which was the original concern.

imho, I think simply displaying the unsigned tx is the easiest and best way to go here. If you want, in addition to the unsigned tx, we may also display the fees in atoms.

I'm not arguing with easiest, i'm arguing with best :p

All I'm saying is the point of the original issue was to make sure users do not make a mistake/typo that will end up in them losing funds. I think displaying the fees in atoms is absolutely necessary to achieve that.

Sure, but I also would like to add showing the full unsigned tx is also the best. Shall we compromise and additionally add a section to the prompt stating the fees in atoms?

Sounds good 👍

Thing is, this now ties into #3510

jackzampolin · 2019-02-25T19:33:13Z

cmd/gaia/cli_test/cli_test.go

@@ -86,19 +86,19 @@ func TestGaiaCLIMinimumFees(t *testing.T) {
 	barAddr := f.KeyAddress(keyBar)

 	// Send a transaction that will get rejected
-	success, _, _ := f.TxSend(keyFoo, barAddr, sdk.NewInt64Coin(fee2Denom, 10))
+	success, _, _ := f.TxSend(keyFoo, barAddr, sdk.NewInt64Coin(fee2Denom, 10), "-y")


Is this something we could set in gaiacli config?

jackzampolin

I think we should add the ability to set this via gaiacli config, but other than that LGTM 👍

alexanderbez · 2019-02-25T21:08:48Z

@jackzampolin I don't think we should set this value in gaiacli config. A user should knowingly and willingly specify this value if they so wish.

cwgoes

ACK

Prompt user confirmation prior to sign & broadcasting

5ca934f

alexanderbez added C:CLI T: UX WIP labels Feb 20, 2019

Update confirmation message

cdc4199

jackzampolin reviewed Feb 20, 2019

View reviewed changes

Update and fix existing CLI integration tests

eca7e97

Implement CLI integration test for tx confirmation

d1be71e

alexanderbez marked this pull request as ready for review February 20, 2019 19:33

alexanderbez requested review from cwgoes, ebuchman and rigelrozanski as code owners February 20, 2019 19:33

alexanderbez added ready-for-review and removed WIP labels Feb 20, 2019

Fix order of input into tx send

1fe78f7

gamarin2 reviewed Feb 21, 2019

View reviewed changes

cwgoes added this to the v0.33.0 (Launch) milestone Feb 25, 2019

alexanderbez mentioned this pull request Feb 25, 2019

Gaiad gentx command incorrect for offline/multisig keys #3730

Closed

Merge branch 'develop' into bez/3653-confirm-tx-cli

dfd275b

jackzampolin reviewed Feb 25, 2019

View reviewed changes

jackzampolin approved these changes Feb 25, 2019

View reviewed changes

cwgoes approved these changes Feb 26, 2019

View reviewed changes

cwgoes merged commit feb98bc into develop Feb 26, 2019

cwgoes deleted the bez/3653-confirm-tx-cli branch February 26, 2019 11:34

alexanderbez mentioned this pull request Feb 26, 2019

Add a confirmation displaying the cost of transaction to the CLI #3653

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prompt User Confirmation Prior to Signing & Broadcasting #3698

Prompt User Confirmation Prior to Signing & Broadcasting #3698

alexanderbez commented Feb 20, 2019 •

edited

Loading

jackzampolin Feb 20, 2019

alexanderbez Feb 20, 2019

jackzampolin Feb 20, 2019

alexanderbez Feb 20, 2019 •

edited

Loading

jackzampolin Feb 25, 2019

codecov bot commented Feb 20, 2019 •

edited

Loading

jackzampolin commented Feb 20, 2019

alexanderbez commented Feb 20, 2019

gamarin2 Feb 21, 2019

alexanderbez Feb 21, 2019 •

edited

Loading

gamarin2 Feb 25, 2019 •

edited

Loading

alexanderbez Feb 25, 2019

gamarin2 Feb 25, 2019

alexanderbez Feb 25, 2019

gamarin2 Feb 25, 2019

alexanderbez Feb 25, 2019

gamarin2 Feb 26, 2019

jackzampolin Feb 25, 2019

jackzampolin left a comment

alexanderbez commented Feb 25, 2019

cwgoes left a comment

Prompt User Confirmation Prior to Signing & Broadcasting #3698

Prompt User Confirmation Prior to Signing & Broadcasting #3698

Conversation

alexanderbez commented Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 20, 2019 • edited Loading

Codecov Report

jackzampolin commented Feb 20, 2019

alexanderbez commented Feb 20, 2019

Choose a reason for hiding this comment

alexanderbez Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

gamarin2 Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackzampolin left a comment

Choose a reason for hiding this comment

alexanderbez commented Feb 25, 2019

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez commented Feb 20, 2019 •

edited

Loading

alexanderbez Feb 20, 2019 •

edited

Loading

codecov bot commented Feb 20, 2019 •

edited

Loading

alexanderbez Feb 21, 2019 •

edited

Loading

gamarin2 Feb 25, 2019 •

edited

Loading