client: fix signing algorithm (#6405)

* crypto/keyring: fix signing algorithm

* client: tests

* minor fixes

* changelog

* address @alexanderbez comments

* Update crypto/keyring/keyring.go

* Update crypto/keyring/signing_algorithms.go

* Update crypto/keyring/keyring.go

* Update crypto/keyring/signing_algorithms.go

Co-authored-by: Alessio Treglia <alessio@tendermint.com>

* fix test

Co-authored-by: Alessio Treglia <alessio@tendermint.com>

CHANGELOG.md

@@ -160,6 +160,7 @@ be used to retrieve the actual proposal `Content`. Also the `NewMsgSubmitProposa
 
 ### Bug Fixes
 
+* (client) [\#6402](https://github.com/cosmos/cosmos-sdk/issues/6402) Fix `keys add` `--algo` flag which only worked for Tendermint's `secp256k1` default key signing algorithm.
 * (x/bank) [\#6283](https://github.com/cosmos/cosmos-sdk/pull/6283) Create account if recipient does not exist on handing `MsgMultiSend`.
 * (x/distribution) [\#6210](https://github.com/cosmos/cosmos-sdk/pull/6210) Register `MsgFundCommunityPool` in distribution amino codec.
 * (x/staking) [\#6061](https://github.com/cosmos/cosmos-sdk/pull/6061) Allow a validator to immediately unjail when no signing info is present due to

client/keys/add.go

@@ -120,9 +120,10 @@ func RunAddCmd(cmd *cobra.Command, args []string, kb keyring.Keyring, inBuf *buf
 	interactive := viper.GetBool(flagInteractive)
 	showMnemonic := !viper.GetBool(flagNoBackup)
 
-	algo, err := keyring.NewSigningAlgoFromString(viper.GetString(flagKeyAlgo))
+	keyringAlgos, _ := kb.SupportedAlgorithms()
+	algo, err := keyring.NewSigningAlgoFromString(viper.GetString(flagKeyAlgo), keyringAlgos)
 	if err != nil {
-		algo = hd.Secp256k1
+		return err
 	}
 
 	if !viper.GetBool(flags.FlagDryRun) {

client/keys/add_ledger_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/tendermint/tendermint/libs/cli"
 
 	"github.com/cosmos/cosmos-sdk/client/flags"
+	"github.com/cosmos/cosmos-sdk/crypto/hd"
 	"github.com/cosmos/cosmos-sdk/crypto/keyring"
 	"github.com/cosmos/cosmos-sdk/tests"
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -45,8 +46,10 @@ func Test_runAddCmdLedgerWithCustomCoinType(t *testing.T) {
 	viper.Set(flagIndex, "0")
 	viper.Set(flagCoinType, "330")
 
-	/// Test Text
+	// Test Text
 	viper.Set(cli.OutputFlag, OutputFormatText)
+	// set algo flag value to the default
+	viper.Set(flagKeyAlgo, string(hd.Secp256k1Type))
 	// Now enter password
 	mockIn, _, _ := tests.ApplyMockIO(cmd)
 	mockIn.Reset("test1234\ntest1234\n")
@@ -57,7 +60,7 @@ func Test_runAddCmdLedgerWithCustomCoinType(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, kb)
 	t.Cleanup(func() {
-		kb.Delete("keyname1")
+		_ = kb.Delete("keyname1")
 	})
 	mockIn.Reset("test1234\n")
 	key1, err := kb.Key("keyname1")
@@ -89,8 +92,10 @@ func Test_runAddCmdLedger(t *testing.T) {
 	viper.Set(flags.FlagHome, kbHome)
 	viper.Set(flags.FlagUseLedger, true)
 
-	/// Test Text
+	// Test Text
 	viper.Set(cli.OutputFlag, OutputFormatText)
+	// set algo flag value to the default
+	viper.Set(flagKeyAlgo, string(hd.Secp256k1Type))
 	// Now enter password
 	mockIn.Reset("test1234\ntest1234\n")
 	viper.Set(flagCoinType, sdk.CoinType)
@@ -101,7 +106,7 @@ func Test_runAddCmdLedger(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, kb)
 	t.Cleanup(func() {
-		kb.Delete("keyname1")
+		_ = kb.Delete("keyname1")
 	})
 	mockIn.Reset("test1234\n")
 	key1, err := kb.Key("keyname1")

client/keys/add_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/tendermint/tendermint/libs/cli"
 
 	"github.com/cosmos/cosmos-sdk/client/flags"
+	"github.com/cosmos/cosmos-sdk/crypto/hd"
 	"github.com/cosmos/cosmos-sdk/crypto/keyring"
 	"github.com/cosmos/cosmos-sdk/tests"
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -17,6 +18,7 @@ import (
 func Test_runAddCmdBasic(t *testing.T) {
 	cmd := AddKeyCommand()
 	require.NotNil(t, cmd)
+
 	mockIn, _, _ := tests.ApplyMockIO(cmd)
 
 	kbHome, kbCleanUp := tests.NewTestCaseDir(t)
@@ -27,11 +29,14 @@ func Test_runAddCmdBasic(t *testing.T) {
 	viper.Set(flags.FlagUseLedger, false)
 
 	mockIn.Reset("y\n")
+	// set algo flag value to the default
+	viper.Set(flagKeyAlgo, string(hd.Secp256k1Type))
+
 	kb, err := keyring.New(sdk.KeyringServiceName(), viper.GetString(flags.FlagKeyringBackend), kbHome, mockIn)
 	require.NoError(t, err)
 	t.Cleanup(func() {
-		kb.Delete("keyname1") // nolint:errcheck
-		kb.Delete("keyname2") // nolint:errcheck
+		_ = kb.Delete("keyname1")
+		_ = kb.Delete("keyname2")
 	})
 	require.NoError(t, runAddCmd(cmd, []string{"keyname1"}))
 

crypto/keyring/keyring.go

@@ -14,18 +14,18 @@ import (
 	"github.com/99designs/keyring"
 	"github.com/cosmos/go-bip39"
 	"github.com/pkg/errors"
-
 	"github.com/tendermint/crypto/bcrypt"
 	tmcrypto "github.com/tendermint/tendermint/crypto"
+	cryptoamino "github.com/tendermint/tendermint/crypto/encoding/amino"
 
 	"github.com/cosmos/cosmos-sdk/client/input"
 	"github.com/cosmos/cosmos-sdk/crypto"
-	cryptoAmino "github.com/cosmos/cosmos-sdk/crypto/codec"
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 )
 
+// Backend options for Keyring
 const (
 	BackendFile    = "file"
 	BackendOS      = "os"
@@ -51,6 +51,9 @@ type Keyring interface {
 	// List all keys.
 	List() ([]Info, error)
 
+	// Supported signing algorithms for Keyring and Ledger respectively.
+	SupportedAlgorithms() (SigningAlgoList, SigningAlgoList)
+
 	// Key and KeyByAddress return keys by uid and address respectively.
 	Key(uid string) (Info, error)
 	KeyByAddress(address sdk.Address) (Info, error)
@@ -114,9 +117,11 @@ type Exporter interface {
 // Option overrides keyring configuration options.
 type Option func(options *Options)
 
-//Options define the options of the Keyring
+// Options define the options of the Keyring.
 type Options struct {
-	SupportedAlgos       SigningAlgoList
+	// supported signing algorithms for keyring
+	SupportedAlgos SigningAlgoList
+	// supported signing algorithms for Ledger
 	SupportedAlgosLedger SigningAlgoList
 }
 
@@ -127,7 +132,7 @@ func NewInMemory(opts ...Option) Keyring {
 	return newKeystore(keyring.NewArrayKeyring(nil), opts...)
 }
 
-// NewKeyring creates a new instance of a keyring.
+// New creates a new instance of a keyring.
 // Keyring ptions can be applied when generating the new instance.
 // Available backends are "os", "file", "kwallet", "memory", "pass", "test".
 func New(
@@ -233,7 +238,7 @@ func (ks keystore) ExportPrivateKeyObject(uid string) (tmcrypto.PrivKey, error)
 			return nil, err
 		}
 
-		priv, err = cryptoAmino.PrivKeyFromBytes([]byte(linfo.PrivKeyArmor))
+		priv, err = cryptoamino.PrivKeyFromBytes([]byte(linfo.PrivKeyArmor))
 		if err != nil {
 			return nil, err
 		}
@@ -282,7 +287,7 @@ func (ks keystore) ImportPubKey(uid string, armor string) error {
 		return err
 	}
 
-	pubKey, err := cryptoAmino.PubKeyFromBytes(pubBytes)
+	pubKey, err := cryptoamino.PubKeyFromBytes(pubBytes)
 	if err != nil {
 		return err
 	}
@@ -309,7 +314,7 @@ func (ks keystore) Sign(uid string, msg []byte) ([]byte, tmcrypto.PubKey, error)
 			return nil, nil, fmt.Errorf("private key not available")
 		}
 
-		priv, err = cryptoAmino.PrivKeyFromBytes([]byte(i.PrivKeyArmor))
+		priv, err = cryptoamino.PrivKeyFromBytes([]byte(i.PrivKeyArmor))
 		if err != nil {
 			return nil, nil, err
 		}
@@ -518,6 +523,12 @@ func (ks keystore) Key(uid string) (Info, error) {
 	return unmarshalInfo(bs.Data)
 }
 
+// SupportedAlgorithms returns the keystore Options' supported signing algorithm.
+// for the keyring and Ledger.
+func (ks keystore) SupportedAlgorithms() (SigningAlgoList, SigningAlgoList) {
+	return ks.options.SupportedAlgos, ks.options.SupportedAlgosLedger
+}
+
 // SignWithLedger signs a binary message with the ledger device referenced by an Info object
 // and returns the signed bytes and the public key. It returns an error if the device could
 // not be queried or it returned an error.
@@ -690,7 +701,7 @@ func (ks keystore) writeInfo(info Info) error {
 
 	exists, err := ks.existsInDb(info)
 	if exists {
-		return fmt.Errorf("public key already exist in keybase")
+		return errors.New("public key already exist in keybase")
 	}
 
 	if err != nil {

crypto/keyring/signing_algorithms.go

@@ -2,32 +2,48 @@ package keyring
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
 )
 
+// SignatureAlgo defines the interface for a keyring supported algorithm.
 type SignatureAlgo interface {
 	Name() hd.PubKeyType
 	Derive() hd.DeriveFn
 	Generate() hd.GenerateFn
 }
 
-func NewSigningAlgoFromString(str string) (SignatureAlgo, error) {
-	if str != string(hd.Secp256k1.Name()) {
-		return nil, fmt.Errorf("provided algorithm `%s` is not supported", str)
+// NewSigningAlgoFromString creates a supported SignatureAlgo.
+func NewSigningAlgoFromString(str string, algoList SigningAlgoList) (SignatureAlgo, error) {
+	for _, algo := range algoList {
+		if str == string(algo.Name()) {
+			return algo, nil
+		}
 	}
-
-	return hd.Secp256k1, nil
+	return nil, fmt.Errorf("provided algorithm %q is not supported", str)
 }
 
+// SigningAlgoList is a slice of signature algorithms
 type SigningAlgoList []SignatureAlgo
 
-func (l SigningAlgoList) Contains(algo SignatureAlgo) bool {
-	for _, cAlgo := range l {
+// Contains returns true if the SigningAlgoList the given SignatureAlgo.
+func (sal SigningAlgoList) Contains(algo SignatureAlgo) bool {
+	for _, cAlgo := range sal {
 		if cAlgo.Name() == algo.Name() {
 			return true
 		}
 	}
 
 	return false
 }
+
+// String returns a comma separated string of the signature algorithm names in the list.
+func (sal SigningAlgoList) String() string {
+	names := make([]string, len(sal))
+	for i := range sal {
+		names[i] = string(sal[i].Name())
+	}
+
+	return strings.Join(names, ",")
+}

crypto/keyring/signing_algorithms_test.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
@@ -30,13 +29,15 @@ func TestNewSigningAlgoByString(t *testing.T) {
 			"notsupportedalgo",
 			false,
 			nil,
-			fmt.Errorf("provided algorithm `notsupportedalgo` is not supported"),
+			fmt.Errorf("provided algorithm \"notsupportedalgo\" is not supported"),
 		},
 	}
+
+	list := SigningAlgoList{hd.Secp256k1}
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			algorithm, err := NewSigningAlgoFromString(tt.algoStr)
+			algorithm, err := NewSigningAlgoFromString(tt.algoStr, list)
 			if tt.isSupported {
 				require.Equal(t, hd.Secp256k1, algorithm)
 			} else {
@@ -47,12 +48,15 @@ func TestNewSigningAlgoByString(t *testing.T) {
 }
 
 func TestAltSigningAlgoList_Contains(t *testing.T) {
-	list := SigningAlgoList{
-		hd.Secp256k1,
-	}
+	list := SigningAlgoList{hd.Secp256k1}
+
+	require.True(t, list.Contains(hd.Secp256k1))
+	require.False(t, list.Contains(notSupportedAlgo{}))
+}
 
-	assert.True(t, list.Contains(hd.Secp256k1))
-	assert.False(t, list.Contains(notSupportedAlgo{}))
+func TestAltSigningAlgoList_String(t *testing.T) {
+	list := SigningAlgoList{hd.Secp256k1, notSupportedAlgo{}}
+	require.Equal(t, fmt.Sprintf("%s,notSupported", string(hd.Secp256k1Type)), list.String())
 }
 
 type notSupportedAlgo struct {