R4R: Various sign command improvements

PENDING.md

@@ -19,6 +19,9 @@ FEATURES
 
 * Gaia CLI  (`gaiacli`)
     * [cli] [\#2569](https://github.com/cosmos/cosmos-sdk/pull/2569) Add commands to query validator unbondings and redelegations
+    * [cli] [\#2569](https://github.com/cosmos/cosmos-sdk/pull/2569) Add commands to query validator unbondings and redelegations
+    * [cli] [\#2524](https://github.com/cosmos/cosmos-sdk/issues/2524) Add support offline mode to `gaiacli tx sign`. Lookups are not performed if the flag `--offline` is on.
+    * [cli] [\#2558](https://github.com/cosmos/cosmos-sdk/issues/2558) Rename --print-sigs to --validate-signatures. It now performs a complete set of sanity checks and reports to the user. Also added --print-signature-only to print the signature only, not the whole transaction.
 
 * Gaia
 

client/utils/utils.go

@@ -123,7 +123,8 @@ func SignStdTx(txBldr authtxb.TxBuilder, cliCtx context.CLIContext, name string,
 
 	// Check whether the address is a signer
 	if !isTxSigner(sdk.AccAddress(addr), stdTx.GetSigners()) {
-		fmt.Fprintf(os.Stderr, "WARNING: The generated transaction's intended signer does not match the given signer: '%v'\n", name)
+		return signedStdTx, fmt.Errorf(
+			"The generated transaction's intended signer does not match the given signer: %q", name)
 	}
 
 	if !offline && txBldr.AccountNumber == 0 {

cmd/gaia/cli_test/cli_test.go

@@ -445,7 +445,8 @@ func TestGaiaCLISendGenerateSignAndBroadcast(t *testing.T) {
 	flags := fmt.Sprintf("--home=%s --node=%v --chain-id=%v", gaiacliHome, servAddr, chainID)
 
 	// start gaiad server
-	proc := tests.GoExecuteTWithStdout(t, fmt.Sprintf("gaiad start --home=%s --rpc.laddr=%v", gaiadHome, servAddr))
+	proc := tests.GoExecuteTWithStdout(t, fmt.Sprintf(
+		"gaiad start --home=%s --rpc.laddr=%v", gaiadHome, servAddr))
 
 	defer proc.Stop(false)
 	tests.WaitForTMStart(port)
@@ -490,11 +491,11 @@ func TestGaiaCLISendGenerateSignAndBroadcast(t *testing.T) {
 	unsignedTxFile := writeToNewTempFile(t, stdout)
 	defer os.Remove(unsignedTxFile.Name())
 
-	// Test sign --print-sigs
+	// Test sign --validate-signatures
 	success, stdout, _ = executeWriteRetStdStreams(t, fmt.Sprintf(
-		"gaiacli tx sign %v --print-sigs %v", flags, unsignedTxFile.Name()))
-	require.True(t, success)
-	require.Equal(t, fmt.Sprintf("Signers:\n 0: %v\n\nSignatures:\n", fooAddr.String()), stdout)
+		"gaiacli tx sign %v --validate-signatures %v", flags, unsignedTxFile.Name()))
+	require.False(t, success)
+	require.Equal(t, fmt.Sprintf("Signers:\n 0: %v\n\nSignatures:\n\n", fooAddr.String()), stdout)
 
 	// Test sign
 	success, stdout, _ = executeWriteRetStdStreams(t, fmt.Sprintf(
@@ -511,15 +512,17 @@ func TestGaiaCLISendGenerateSignAndBroadcast(t *testing.T) {
 
 	// Test sign --print-signatures
 	success, stdout, _ = executeWriteRetStdStreams(t, fmt.Sprintf(
-		"gaiacli tx sign %v --print-sigs %v", flags, signedTxFile.Name()))
+		"gaiacli tx sign %v --validate-signatures %v", flags, signedTxFile.Name()))
 	require.True(t, success)
-	require.Equal(t, fmt.Sprintf("Signers:\n 0: %v\n\nSignatures:\n 0: %v\n", fooAddr.String(), fooAddr.String()), stdout)
+	require.Equal(t, fmt.Sprintf("Signers:\n 0: %v\n\nSignatures:\n 0: %v\t[OK]\n\n", fooAddr.String(),
+		fooAddr.String()), stdout)
 
 	// Test broadcast
 	fooAcc := executeGetAccount(t, fmt.Sprintf("gaiacli query account %s %v", fooAddr, flags))
 	require.Equal(t, int64(50), fooAcc.GetCoins().AmountOf("steak").Int64())
 
-	success, stdout, _ = executeWriteRetStdStreams(t, fmt.Sprintf("gaiacli tx broadcast %v --json %v", flags, signedTxFile.Name()))
+	success, stdout, _ = executeWriteRetStdStreams(t, fmt.Sprintf(
+		"gaiacli tx broadcast %v --json %v", flags, signedTxFile.Name()))
 	require.True(t, success)
 	var result struct {
 		Response abci.ResponseDeliverTx

docs/sdk/clients.md

@@ -181,6 +181,12 @@ gaiacli tx sign \
   unsignedSendTx.json > signedSendTx.json
 ```
 
+You can validate the transaction's signagures by typing the following:
+
+```bash
+gaiacli tx sign --validate-signatures signedSendTx.json
+```
+
 You can broadcast the signed transaction to a node by providing the JSON file to the following command:
 
 ```

x/auth/client/cli/sign.go

@@ -2,9 +2,9 @@ package cli
 
 import (
 	"fmt"
-	"io/ioutil"
-
+	"github.com/pkg/errors"
 	"github.com/spf13/viper"
+	"io/ioutil"
 
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/context"
@@ -13,13 +13,14 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/auth"
 	authtxb "github.com/cosmos/cosmos-sdk/x/auth/client/txbuilder"
 	"github.com/spf13/cobra"
-	amino "github.com/tendermint/go-amino"
+	"github.com/tendermint/go-amino"
 )
 
 const (
-	flagAppend    = "append"
-	flagPrintSigs = "print-sigs"
-	flagOffline   = "offline"
+	flagAppend       = "append"
+	flagValidateSigs = "validate-signatures"
+	flagOffline      = "offline"
+	flagSigOnly      = "signature-only"
 )
 
 // GetSignCommand returns the sign command
@@ -30,15 +31,25 @@ func GetSignCommand(codec *amino.Codec, decoder auth.AccountDecoder) *cobra.Comm
 		Long: `Sign transactions created with the --generate-only flag.
 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
+of the generated signature only.
+
+If the flag --validate-signatures is on, then the command would check whether all required
+signers have signed the transactions and whether the signatures were collected in the right
+order.
+
 The --offline flag makes sure that the client will not reach out to the local cache.
 Thus account number or sequence number lookups will not be performed and it is
 recommended to set such parameters manually.`,
 		RunE: makeSignCmd(codec, decoder),
 		Args: cobra.ExactArgs(1),
 	}
 	cmd.Flags().String(client.FlagName, "", "Name of private key with which to sign")
-	cmd.Flags().Bool(flagAppend, true, "Append the signature to the existing ones. If disabled, old signatures would be overwritten")
-	cmd.Flags().Bool(flagPrintSigs, false, "Print the addresses that must sign the transaction and those who have already signed it, then exit")
+	cmd.Flags().Bool(flagAppend, true,
+		"Append the signature to the existing ones. If disabled, old signatures would be overwritten")
+	cmd.Flags().Bool(flagSigOnly, false, "Print only the generated signature, then exit.")
+	cmd.Flags().Bool(flagValidateSigs, false, "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(flagOffline, false, "Offline mode. Do not query local cache.")
 	return cmd
 }
@@ -50,24 +61,45 @@ func makeSignCmd(cdc *amino.Codec, decoder auth.AccountDecoder) func(cmd *cobra.
 			return
 		}
 
-		if viper.GetBool(flagPrintSigs) {
-			printSignatures(stdTx)
+		if viper.GetBool(flagValidateSigs) {
+			if !printSignatures(stdTx) {
+				return fmt.Errorf("signatures validation failed")
+			}
 			return nil
 		}
 
 		name := viper.GetString(client.FlagName)
+		if name == "" {
+			return errors.New("required flag \"name\" has not been set")
+		}
 		cliCtx := context.NewCLIContext().WithCodec(cdc).WithAccountDecoder(decoder)
 		txBldr := authtxb.NewTxBuilderFromCLI()
 
-		newTx, err := utils.SignStdTx(txBldr, cliCtx, name, stdTx, viper.GetBool(flagAppend), viper.GetBool(flagOffline))
+		// if --signature-only is on, then override --append
+		generateSignatureOnly := viper.GetBool(flagSigOnly)
+		appendSig := viper.GetBool(flagAppend) && !generateSignatureOnly
+		newTx, err := utils.SignStdTx(txBldr, cliCtx, name, stdTx, appendSig, viper.GetBool(flagOffline))
 		if err != nil {
 			return err
 		}
+
 		var json []byte
-		if cliCtx.Indent {
-			json, err = cdc.MarshalJSONIndent(newTx, "", "  ")
-		} else {
-			json, err = cdc.MarshalJSON(newTx)
+
+		switch generateSignatureOnly {
+		case true:
+			switch cliCtx.Indent {
+			case true:
+				json, err = cdc.MarshalJSONIndent(newTx.Signatures[0], "", "  ")
+			default:
+				json, err = cdc.MarshalJSON(newTx.Signatures[0])
+			}
+		default:
+			switch cliCtx.Indent {
+			case true:
+				json, err = cdc.MarshalJSONIndent(newTx, "", "  ")
+			default:
+				json, err = cdc.MarshalJSON(newTx)
+			}
 		}
 		if err != nil {
 			return err
@@ -77,17 +109,31 @@ func makeSignCmd(cdc *amino.Codec, decoder auth.AccountDecoder) func(cmd *cobra.
 	}
 }
 
-func printSignatures(stdTx auth.StdTx) {
+func printSignatures(stdTx auth.StdTx) bool {
 	fmt.Println("Signers:")
-	for i, signer := range stdTx.GetSigners() {
+	signers := stdTx.GetSigners()
+	for i, signer := range signers {
 		fmt.Printf(" %v: %v\n", i, signer.String())
 	}
+
+	sigs := stdTx.GetSignatures()
 	fmt.Println("")
 	fmt.Println("Signatures:")
+	success := true
+	if len(sigs) != len(signers) {
+		success = false
+	}
 	for i, sig := range stdTx.GetSignatures() {
-		fmt.Printf(" %v: %v\n", i, sdk.AccAddress(sig.Address()).String())
+		sigAddr := sdk.AccAddress(sig.Address())
+		sigSanity := "OK"
+		if i >= len(signers) || !sigAddr.Equals(signers[i]) {
+			sigSanity = fmt.Sprintf("ERROR: signature %d does not match its respective signer", i)
+			success = false
+		}
+		fmt.Printf(" %v: %v\t[%s]\n", i, sigAddr.String(), sigSanity)
 	}
-	return
+	fmt.Println("")
+	return success
 }
 
 func readAndUnmarshalStdTx(cdc *amino.Codec, filename string) (stdTx auth.StdTx, err error) {