Merge PR cosmos#3970: Fix Tx Sign Offline Mode

- Add shorthand flags `-a` and `-s` for the account and sequence numbers respectively
- Mark the account and sequence numbers required during "offline" mode
- Always do an RPC query for account and sequence number during "online" mode
  - If clients wish to provide such values, they must use `--offline`. This makes the whole flow/UX easier to reason about.

closes: cosmos#3893

.pending/improvements/gaia/3893-Improve-gaiacli-tx-sign-command

@@ -0,0 +1,4 @@
+#3893 Improve `gaiacli tx sign` command
+  * Add shorthand flags -a and -s for the account and sequence numbers respectively
+  * Mark the account and sequence numbers required during "offline" mode
+  * Always do an RPC query for account and sequence number during "online" mode

client/flags.go

@@ -81,8 +81,8 @@ func PostCommands(cmds ...*cobra.Command) []*cobra.Command {
 	for _, c := range cmds {
 		c.Flags().Bool(FlagIndentResponse, false, "Add indent to JSON response")
 		c.Flags().String(FlagFrom, "", "Name or address of private key with which to sign")
-		c.Flags().Uint64(FlagAccountNumber, 0, "AccountNumber number to sign the tx")
-		c.Flags().Uint64(FlagSequence, 0, "Sequence number to sign the tx")
+		c.Flags().Uint64P(FlagAccountNumber, "a", 0, "The account number number of the signing account (offline mode only)")
+		c.Flags().Uint64P(FlagSequence, "s", 0, "The sequence number of the signing account (offline mode only)")
 		c.Flags().String(FlagMemo, "", "Memo to send along with transaction")
 		c.Flags().String(FlagFees, "", "Fees to pay along with transaction; eg: 10uatom")
 		c.Flags().String(FlagGasPrices, "", "Gas prices to determine the transaction fee (e.g. 10uatom)")

client/utils/utils.go

@@ -164,15 +164,17 @@ func PrintUnsignedStdTx(
 	return
 }
 
-// SignStdTx appends a signature to a StdTx and returns a copy of a it. If appendSig
+// SignStdTx appends a signature to a StdTx and returns a copy of it. If appendSig
 // is false, it replaces the signatures already attached with the new signature.
 // Don't perform online validation or lookups if offline is true.
-func SignStdTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string, stdTx auth.StdTx, appendSig bool, offline bool) (auth.StdTx, error) {
-	var signedStdTx auth.StdTx
+func SignStdTx(
+	txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string,
+	stdTx auth.StdTx, appendSig bool, offline bool,
+) (auth.StdTx, error) {
 
-	keybase := txBldr.Keybase()
+	var signedStdTx auth.StdTx
 
-	info, err := keybase.Get(name)
+	info, err := txBldr.Keybase().Get(name)
 	if err != nil {
 		return signedStdTx, err
 	}
@@ -185,8 +187,7 @@ func SignStdTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string,
 	}
 
 	if !offline {
-		txBldr, err = populateAccountFromState(
-			txBldr, cliCtx, sdk.AccAddress(addr))
+		txBldr, err = populateAccountFromState(txBldr, cliCtx, sdk.AccAddress(addr))
 		if err != nil {
 			return signedStdTx, err
 		}
@@ -244,25 +245,21 @@ func ReadStdTxFromFile(cdc *amino.Codec, filename string) (stdTx auth.StdTx, err
 	return
 }
 
-func populateAccountFromState(txBldr authtxb.TxBuilder, cliCtx context.CLIContext,
-	addr sdk.AccAddress) (authtxb.TxBuilder, error) {
-	if txBldr.AccountNumber() == 0 {
-		accNum, err := cliCtx.GetAccountNumber(addr)
-		if err != nil {
-			return txBldr, err
-		}
-		txBldr = txBldr.WithAccountNumber(accNum)
+func populateAccountFromState(
+	txBldr authtxb.TxBuilder, cliCtx context.CLIContext, addr sdk.AccAddress,
+) (authtxb.TxBuilder, error) {
+
+	accNum, err := cliCtx.GetAccountNumber(addr)
+	if err != nil {
+		return txBldr, err
 	}
 
-	if txBldr.Sequence() == 0 {
-		accSeq, err := cliCtx.GetAccountSequence(addr)
-		if err != nil {
-			return txBldr, err
-		}
-		txBldr = txBldr.WithSequence(accSeq)
+	accSeq, err := cliCtx.GetAccountSequence(addr)
+	if err != nil {
+		return txBldr, err
 	}
 
-	return txBldr, nil
+	return txBldr.WithAccountNumber(accNum).WithSequence(accSeq), nil
 }
 
 // GetTxEncoder return tx encoder from global sdk configuration if ones is defined.

x/auth/client/cli/sign.go

@@ -27,33 +27,35 @@ const (
 	flagOutfile      = "output-document"
 )
 
-// GetSignCommand returns the sign command
+// GetSignCommand returns the transaction sign command.
 func GetSignCommand(codec *amino.Codec) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "sign [file]",
 		Short: "Sign transactions generated offline",
 		Long: `Sign transactions created with the --generate-only flag.
-Read a transaction from [file], sign it, and print its JSON encoding.
+It will read a transaction from [file], sign it, and print its JSON encoding.
 
-If the flag --signature-only flag is on, it outputs a JSON representation
+If the flag --signature-only flag is set, it will output a JSON representation
 of the generated signature only.
 
-If the flag --validate-signatures is on, then the command would check whether all required
+If the flag --validate-signatures is set, then the command would check whether all required
 signers have signed the transactions, whether the signatures were collected in the right
 order, and if the signature is valid over the given transaction. If the --offline
-flag is also provided, signature validation over the transaction will be not be
-performed as that will require communication with a full node.
+flag is also set, signature validation over the transaction will be not be
+performed as that will require RPC communication with a full node.
 
-The --offline flag makes sure that the client will not reach out to an external node.
-Thus account number or sequence number lookups will not be performed and it is
-recommended to set such parameters manually.
+The --offline flag makes sure that the client will not reach out to full node.
+As a result, the account and sequence number queries will not be performed and
+it is required to set such parameters manually. Note, invalid values will cause
+the transaction to fail.
 
 The --multisig=<multisig_key> flag generates a signature on behalf of a multisig account
 key. It implies --signature-only. Full multisig signed transactions may eventually
 be generated via the 'multisign' command.
 `,
-		RunE: makeSignCmd(codec),
-		Args: cobra.ExactArgs(1),
+		PreRun: preSignCmd,
+		RunE:   makeSignCmd(codec),
+		Args:   cobra.ExactArgs(1),
 	}
 
 	cmd.Flags().String(
@@ -69,11 +71,22 @@ be generated via the 'multisign' command.
 		"Print the addresses that must sign the transaction, those who have already signed it, and make sure that signatures are in the correct order",
 	)
 	cmd.Flags().Bool(flagSigOnly, false, "Print only the generated signature, then exit")
-	cmd.Flags().Bool(flagOffline, false, "Offline mode. Do not query a full node")
+	cmd.Flags().Bool(flagOffline, false, "Offline mode; Do not query a full node")
 	cmd.Flags().String(flagOutfile, "", "The document will be written to the given file instead of STDOUT")
 
-	// add the flags here and return the command
-	return client.PostCommands(cmd)[0]
+	cmd = client.PostCommands(cmd)[0]
+	cmd.MarkFlagRequired(client.FlagFrom)
+
+	return cmd
+}
+
+func preSignCmd(cmd *cobra.Command, _ []string) {
+	// Conditionally mark the account and sequence numbers required as no RPC
+	// query will be done.
+	if viper.GetBool(flagOffline) {
+		cmd.MarkFlagRequired(client.FlagAccountNumber)
+		cmd.MarkFlagRequired(client.FlagSequence)
+	}
 }
 
 func makeSignCmd(cdc *amino.Codec) func(cmd *cobra.Command, args []string) error {
@@ -95,11 +108,6 @@ func makeSignCmd(cdc *amino.Codec) func(cmd *cobra.Command, args []string) error
 			return nil
 		}
 
-		from := viper.GetString(client.FlagFrom)
-		if from == "" {
-			return fmt.Errorf("required flag '%s' has not been set", client.FlagFrom)
-		}
-
 		// if --signature-only is on, then override --append
 		var newTx auth.StdTx
 		generateSignatureOnly := viper.GetBool(flagSigOnly)

x/auth/client/txbuilder/txbuilder.go

@@ -245,7 +245,7 @@ func (bldr TxBuilder) BuildTxForSim(msgs []sdk.Msg) ([]byte, error) {
 	return bldr.txEncoder(auth.NewStdTx(signMsg.Msgs, signMsg.Fee, sigs, signMsg.Memo))
 }
 
-// SignStdTx appends a signature to a StdTx and returns a copy of a it. If append
+// SignStdTx appends a signature to a StdTx and returns a copy of it. If append
 // is false, it replaces the signatures already attached with the new signature.
 func (bldr TxBuilder) SignStdTx(name, passphrase string, stdTx auth.StdTx, appendSig bool) (signedStdTx auth.StdTx, err error) {
 	stdSignature, err := MakeSignature(bldr.keybase, name, passphrase, StdSignMsg{

x/bank/client/cli/sendtx.go

@@ -62,5 +62,9 @@ func SendTxCmd(cdc *codec.Codec) *cobra.Command {
 			return utils.GenerateOrBroadcastMsgs(cliCtx, txBldr, []sdk.Msg{msg}, false)
 		},
 	}
-	return client.PostCommands(cmd)[0]
+
+	cmd = client.PostCommands(cmd)[0]
+	cmd.MarkFlagRequired(client.FlagFrom)
+
+	return cmd
 }