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

Prompt User Confirmation Prior to Signing & Broadcasting #3698

Merged
merged 6 commits into from
Feb 26, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions PENDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@

### Gaia CLI

* [\#3653] Prompt user confirmation prior to signing and broadcasting a transaction.

### Gaia

### SDK
Expand Down
2 changes: 2 additions & 0 deletions client/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ type CLIContext struct {
FromAddress sdk.AccAddress
FromName string
Indent bool
SkipConfirm bool
}

// NewCLIContext returns a new initialized CLIContext with parameters from the
Expand Down Expand Up @@ -96,6 +97,7 @@ func NewCLIContext() CLIContext {
FromAddress: fromAddress,
FromName: fromName,
Indent: viper.GetBool(client.FlagIndentResponse),
SkipConfirm: viper.GetBool(client.FlagSkipConfirmation),
}
}

Expand Down
8 changes: 7 additions & 1 deletion client/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const (
FlagSSLCertFile = "ssl-certfile"
FlagSSLKeyFile = "ssl-keyfile"
FlagOutputDocument = "output-document" // inspired by wget -O
FlagSkipConfirmation = "yes"
)

// LineBreak can be included in a command list to provide a blank line
Expand Down Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! But most definitely in a separate PR :)

c.Flags().BoolP(FlagSkipConfirmation, "y", false, "Skip tx broadcasting prompt confirmation")

// --gas can accept integers and "simulate"
c.Flags().Var(&GasFlagVar, "gas", fmt.Sprintf(
"gas limit to set per-transaction; set to %q to calculate required gas automatically (default %d)", GasFlagAuto, DefaultGasLimit))
"gas limit to set per-transaction; set to %q to calculate required gas automatically (default %d)",
GasFlagAuto, DefaultGasLimit,
))

viper.BindPFlag(FlagTrustNode, c.Flags().Lookup(FlagTrustNode))
viper.BindPFlag(FlagUseLedger, c.Flags().Lookup(FlagUseLedger))
viper.BindPFlag(FlagNode, c.Flags().Lookup(FlagNode))
Expand Down
5 changes: 3 additions & 2 deletions client/input.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"errors"

"github.com/bgentry/speakeasy"
"github.com/mattn/go-isatty"
isatty "github.com/mattn/go-isatty"
)

// MinPassLength is the minimum acceptable password length
Expand Down Expand Up @@ -90,8 +90,9 @@ func GetCheckPassword(prompt, prompt2 string, buf *bufio.Reader) (string, error)
func GetConfirmation(prompt string, buf *bufio.Reader) (bool, error) {
for {
if inputIsTty() {
fmt.Print(fmt.Sprintf("%s [y/n]:", prompt))
fmt.Print(fmt.Sprintf("%s [Y/n]: ", prompt))
}

response, err := readLineFromBuf(buf)
if err != nil {
return false, err
Expand Down
16 changes: 16 additions & 0 deletions client/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,22 @@ func CompleteAndBroadcastTxCLI(txBldr authtxb.TxBuilder, cliCtx context.CLIConte
return nil
}

if !cliCtx.SkipConfirm {
stdSignMsg, err := txBldr.BuildSignMsg(msgs)
if err != nil {
return err
}

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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@alexanderbez alexanderbez Feb 20, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@alexanderbez alexanderbez Feb 21, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@gamarin2 gamarin2 Feb 25, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thing is, this now ties into #3510

Copy link
Contributor

Choose a reason for hiding this comment

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

how so?

if err != nil || !ok {
fmt.Fprintf(os.Stderr, "%s\n", "cancelled transaction")
return err
}
}

passphrase, err := keys.GetPassphrase(fromName)
if err != nil {
return err
Expand Down
8 changes: 4 additions & 4 deletions x/auth/client/txbuilder/txbuilder.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,9 @@ func (bldr TxBuilder) WithAccountNumber(accnum uint64) TxBuilder {
return bldr
}

// BuildSignMsg builds a single message to be signed from a TxBuilder given a set of
// messages. It returns an error if a fee is supplied but cannot be parsed.
// BuildSignMsg builds a single message to be signed from a TxBuilder given a
// set of messages. It returns an error if a fee is supplied but cannot be
// parsed.
func (bldr TxBuilder) BuildSignMsg(msgs []sdk.Msg) (StdSignMsg, error) {
chainID := bldr.chainID
if chainID == "" {
Expand Down Expand Up @@ -221,8 +222,7 @@ func (bldr TxBuilder) Sign(name, passphrase string, msg StdSignMsg) ([]byte, err
}

// BuildAndSign builds a single message to be signed, and signs a transaction
// with the built message given a name, passphrase, and a set of
// messages.
// with the built message given a name, passphrase, and a set of messages.
func (bldr TxBuilder) BuildAndSign(name, passphrase string, msgs []sdk.Msg) ([]byte, error) {
msg, err := bldr.BuildSignMsg(msgs)
if err != nil {
Expand Down