From ea46da7126ea9490786368038744f00a7349caa9 Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Tue, 26 Mar 2019 10:36:10 -0400 Subject: [PATCH] Merge PR #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: #3893 --- .../gaia/3893-Improve-gaiacli-tx-sign-command | 4 ++ client/flags.go | 4 +- client/utils/utils.go | 41 ++++++++--------- x/auth/client/cli/sign.go | 46 +++++++++++-------- x/auth/client/txbuilder/txbuilder.go | 2 +- x/bank/client/cli/sendtx.go | 6 ++- 6 files changed, 58 insertions(+), 45 deletions(-) create mode 100644 .pending/improvements/gaia/3893-Improve-gaiacli-tx-sign-command diff --git a/.pending/improvements/gaia/3893-Improve-gaiacli-tx-sign-command b/.pending/improvements/gaia/3893-Improve-gaiacli-tx-sign-command new file mode 100644 index 0000000000..3d8b67791a --- /dev/null +++ b/.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 diff --git a/client/flags.go b/client/flags.go index 28e5cc7b9c..4fd899131c 100644 --- a/client/flags.go +++ b/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)") diff --git a/client/utils/utils.go b/client/utils/utils.go index 20867df071..6e8aec8b46 100644 --- a/client/utils/utils.go +++ b/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) { +func SignStdTx( + txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string, + stdTx auth.StdTx, appendSig bool, offline bool, +) (auth.StdTx, error) { + var signedStdTx auth.StdTx - keybase := txBldr.Keybase() - - 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. diff --git a/x/auth/client/cli/sign.go b/x/auth/client/cli/sign.go index e30fd2e4f4..6d3a9fe298 100644 --- a/x/auth/client/cli/sign.go +++ b/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= 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) diff --git a/x/auth/client/txbuilder/txbuilder.go b/x/auth/client/txbuilder/txbuilder.go index 3ea6989e65..480d8fd416 100644 --- a/x/auth/client/txbuilder/txbuilder.go +++ b/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{ diff --git a/x/bank/client/cli/sendtx.go b/x/bank/client/cli/sendtx.go index 7e8a29a921..2f1d45b95a 100644 --- a/x/bank/client/cli/sendtx.go +++ b/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 }