Merge PR #3586: Fix keybase storage of ledger accounts

PENDING.md

@@ -43,6 +43,7 @@ BUG FIXES
 * Gaia REST API
 
 * Gaia CLI
+  * [\#3586](https://github.com/cosmos/cosmos-sdk/pull/3586) Incomplete ledger derivation paths in keybase 
 
 * Gaia
   * [\#3585] Fix setting the tx hash in `NewResponseFormatBroadcastTxCommit`.
@@ -50,4 +51,4 @@ BUG FIXES
 * SDK
   * [\#3582](https://github.com/cosmos/cosmos-sdk/pull/3582) Running `make test_unit was failing due to a missing tag
 
-* Tendermint
+* Tendermint

client/keys/add.go

@@ -77,7 +77,7 @@ the flag --nosort is set.
 	cmd.Flags().Bool(flagNoBackup, false, "Don't print out seed phrase (if others are watching the terminal)")
 	cmd.Flags().Bool(flagDryRun, false, "Perform action, but don't add key to local keystore")
 	cmd.Flags().Uint32(flagAccount, 0, "Account number for HD derivation")
-	cmd.Flags().Uint32(flagIndex, 0, "Index number for HD derivation")
+	cmd.Flags().Uint32(flagIndex, 0, "Address index number for HD derivation")
 	return cmd
 }
 

client/keys/add_ledger_test.go

@@ -0,0 +1,55 @@
+//+build ledger,test_ledger_mock
+
+package keys
+
+import (
+	"bufio"
+	"strings"
+	"testing"
+
+	"github.com/cosmos/cosmos-sdk/crypto/keys"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	"github.com/spf13/viper"
+	"github.com/tendermint/tendermint/libs/cli"
+
+	"github.com/cosmos/cosmos-sdk/tests"
+
+	"github.com/cosmos/cosmos-sdk/client"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_runAddCmdLedger(t *testing.T) {
+	cmd := addKeyCommand()
+	assert.NotNil(t, cmd)
+
+	// Prepare a keybase
+	kbHome, kbCleanUp := tests.NewTestCaseDir(t)
+	assert.NotNil(t, kbHome)
+	defer kbCleanUp()
+	viper.Set(cli.HomeFlag, kbHome)
+	viper.Set(client.FlagUseLedger, true)
+
+	/// Test Text
+	viper.Set(cli.OutputFlag, OutputFormatText)
+	// Now enter password
+	cleanUp1 := client.OverrideStdin(bufio.NewReader(strings.NewReader("test1234\ntest1234\n")))
+	defer cleanUp1()
+	err := runAddCmd(cmd, []string{"keyname1"})
+	assert.NoError(t, err)
+
+	// Now check that it has been stored properly
+	kb, err := NewKeyBaseFromHomeFlag()
+	assert.NoError(t, err)
+	assert.NotNil(t, kb)
+	key1, err := kb.Get("keyname1")
+	assert.NoError(t, err)
+	assert.NotNil(t, key1)
+
+	assert.Equal(t, "keyname1", key1.GetName())
+	assert.Equal(t, keys.TypeLedger, key1.GetType())
+	assert.Equal(t,
+		"cosmospub1addwnpepqd87l8xhcnrrtzxnkql7k55ph8fr9jarf4hn6udwukfprlalu8lgw0urza0",
+		sdk.MustBech32ifyAccPub(key1.GetPubKey()))
+}

client/keys/add_test.go

@@ -23,10 +23,6 @@ func Test_runAddCmdBasic(t *testing.T) {
 	cmd := addKeyCommand()
 	assert.NotNil(t, cmd)
 
-	// Missing input (enter password)
-	err := runAddCmd(cmd, []string{"keyname"})
-	assert.EqualError(t, err, "EOF")
-
 	// Prepare a keybase
 	kbHome, kbCleanUp := tests.NewTestCaseDir(t)
 	assert.NotNil(t, kbHome)
@@ -38,7 +34,7 @@ func Test_runAddCmdBasic(t *testing.T) {
 	// Now enter password
 	cleanUp1 := client.OverrideStdin(bufio.NewReader(strings.NewReader("test1234\ntest1234\n")))
 	defer cleanUp1()
-	err = runAddCmd(cmd, []string{"keyname1"})
+	err := runAddCmd(cmd, []string{"keyname1"})
 	assert.NoError(t, err)
 
 	/// Test Text - Replace? >> FAIL

crypto/keys/codec.go

@@ -1,10 +1,9 @@
 package keys
 
 import (
-	amino "github.com/tendermint/go-amino"
+	"github.com/cosmos/cosmos-sdk/crypto/keys/hd"
+	"github.com/tendermint/go-amino"
 	"github.com/tendermint/tendermint/crypto/encoding/amino"
-
-	ccrypto "github.com/cosmos/cosmos-sdk/crypto"
 )
 
 var cdc = amino.NewCodec()
@@ -12,8 +11,7 @@ var cdc = amino.NewCodec()
 func init() {
 	cryptoAmino.RegisterAmino(cdc)
 	cdc.RegisterInterface((*Info)(nil), nil)
-	cdc.RegisterConcrete(ccrypto.PrivKeyLedgerSecp256k1{},
+	cdc.RegisterConcrete(hd.BIP44Params{}, "crypto/keys/hd/BIP44Params", nil)
-		"tendermint/PrivKeyLedgerSecp256k1", nil)
 	cdc.RegisterConcrete(localInfo{}, "crypto/keys/localInfo", nil)
 	cdc.RegisterConcrete(ledgerInfo{}, "crypto/keys/ledgerInfo", nil)
 	cdc.RegisterConcrete(offlineInfo{}, "crypto/keys/offlineInfo", nil)

crypto/keys/hd/hdpath.go

@@ -33,25 +33,25 @@ const (
 
 // BIP44Params wraps BIP 44 params (5 level BIP 32 path).
 // To receive a canonical string representation ala
-// m / purpose' / coin_type' / account' / change / address_index
+// m / purpose' / coinType' / account' / change / addressIndex
 // call String() on a BIP44Params instance.
 type BIP44Params struct {
-	purpose    uint32
+	Purpose      uint32 `json:"purpose"`
-	coinType   uint32
+	CoinType     uint32 `json:"coinType"`
-	account    uint32
+	Account      uint32 `json:"account"`
-	change     bool
+	Change       bool   `json:"change"`
-	addressIdx uint32
+	AddressIndex uint32 `json:"addressIndex"`
 }
 
 // NewParams creates a BIP 44 parameter object from the params:
-// m / purpose' / coin_type' / account' / change / address_index
+// m / purpose' / coinType' / account' / change / addressIndex
 func NewParams(purpose, coinType, account uint32, change bool, addressIdx uint32) *BIP44Params {
 	return &BIP44Params{
-		purpose:    purpose,
+		Purpose:      purpose,
-		coinType:   coinType,
+		CoinType:     coinType,
-		account:    account,
+		Account:      account,
-		change:     change,
+		Change:       change,
-		addressIdx: addressIdx,
+		AddressIndex: addressIdx,
 	}
 }
 
@@ -105,11 +105,11 @@ func NewParamsFromPath(path string) (*BIP44Params, error) {
 	}
 
 	return &BIP44Params{
-		purpose:    purpose,
+		Purpose:      purpose,
-		coinType:   coinType,
+		CoinType:     coinType,
-		account:    account,
+		Account:      account,
-		change:     change > 0,
+		Change:       change > 0,
-		addressIdx: addressIdx,
+		AddressIndex: addressIdx,
 	}, nil
 }
 
@@ -139,32 +139,32 @@ func NewFundraiserParams(account uint32, addressIdx uint32) *BIP44Params {
 // DerivationPath returns the BIP44 fields as an array.
 func (p BIP44Params) DerivationPath() []uint32 {
 	change := uint32(0)
-	if p.change {
+	if p.Change {
 		change = 1
 	}
 	return []uint32{
-		p.purpose,
+		p.Purpose,
-		p.coinType,
+		p.CoinType,
-		p.account,
+		p.Account,
 		change,
-		p.addressIdx,
+		p.AddressIndex,
 	}
 }
 
 func (p BIP44Params) String() string {
 	var changeStr string
-	if p.change {
+	if p.Change {
 		changeStr = "1"
 	} else {
 		changeStr = "0"
 	}
-	// m / purpose' / coin_type' / account' / change / address_index
+	// m / Purpose' / coin_type' / Account' / Change / address_index
 	return fmt.Sprintf("%d'/%d'/%d'/%s/%d",
-		p.purpose,
+		p.Purpose,
-		p.coinType,
+		p.CoinType,
-		p.account,
+		p.Account,
 		changeStr,
-		p.addressIdx)
+		p.AddressIndex)
 }
 
 // ComputeMastersFromSeed returns the master public key, master secret, and chain code in hex.

crypto/keys/keybase.go

@@ -284,9 +284,9 @@ func (kb dbKeybase) ExportPrivateKeyObject(name string, passphrase string) (tmcr
 			return nil, err
 		}
 	case ledgerInfo:
-		return nil, errors.New("Only works on local private keys")
+		return nil, errors.New("only works on local private keys")
 	case offlineInfo:
-		return nil, errors.New("Only works on local private keys")
+		return nil, errors.New("only works on local private keys")
 	}
 	return priv, nil
 }
@@ -428,7 +428,8 @@ func (kb dbKeybase) writeOfflineKey(name string, pub tmcrypto.PubKey) Info {
 func (kb dbKeybase) writeInfo(name string, info Info) {
 	// write the info by key
 	key := infoKey(name)
-	kb.db.SetSync(key, writeInfo(info))
+	serializedInfo := writeInfo(info)
+	kb.db.SetSync(key, serializedInfo)
 	// store a pointer to the infokey by address for fast lookup
 	kb.db.SetSync(addrKey(info.GetAddress()), key)
 }

crypto/keys/keybase_test.go

@@ -68,19 +68,34 @@ func TestCreateLedger(t *testing.T) {
 	// test_cover does not compile some dependencies so ledger is disabled
 	// test_unit may add a ledger mock
 	// both cases are acceptable
-	ledger, err := kb.CreateLedger("some_account", Secp256k1, 0, 1)
+	ledger, err := kb.CreateLedger("some_account", Secp256k1, 3, 1)
 
 	if err != nil {
 		assert.Error(t, err)
 		assert.Equal(t, "ledger nano S: support for ledger devices is not available in this executable", err.Error())
 		assert.Nil(t, ledger)
-	} else {
+		t.Skip("ledger nano S: support for ledger devices is not available in this executable")
-		// The mock is available, check that the address is correct
+		return
-		pubKey := ledger.GetPubKey()
-		addr, err := sdk.Bech32ifyAccPub(pubKey)
-		assert.NoError(t, err)
-		assert.Equal(t, "cosmospub1addwnpepqfsdqjr68h7wjg5wacksmqaypasnra232fkgu5sxdlnlu8j22ztxvlqvd65", addr)
 	}
+
+	// The mock is available, check that the address is correct
+	pubKey := ledger.GetPubKey()
+	pk, err := sdk.Bech32ifyAccPub(pubKey)
+	assert.NoError(t, err)
+	assert.Equal(t, "cosmospub1addwnpepqdszcr95mrqqs8lw099aa9h8h906zmet22pmwe9vquzcgvnm93eqygufdlv", pk)
+
+	// Check that restoring the key gets the same results
+	restoredKey, err := kb.Get("some_account")
+	assert.NotNil(t, restoredKey)
+	assert.Equal(t, "some_account", restoredKey.GetName())
+	assert.Equal(t, TypeLedger, restoredKey.GetType())
+	pubKey = restoredKey.GetPubKey()
+	pk, err = sdk.Bech32ifyAccPub(pubKey)
+	assert.Equal(t, "cosmospub1addwnpepqdszcr95mrqqs8lw099aa9h8h906zmet22pmwe9vquzcgvnm93eqygufdlv", pk)
+
+	linfo := restoredKey.(ledgerInfo)
+	assert.Equal(t, "44'/118'/3'/0/1", linfo.GetPath().String())
+
 }
 
 // TestKeyManagement makes sure we can manipulate these keys well

crypto/keys/types.go

@@ -150,6 +150,10 @@ func (i ledgerInfo) GetAddress() types.AccAddress {
 	return i.PubKey.Address().Bytes()
 }
 
+func (i ledgerInfo) GetPath() hd.BIP44Params {
+	return i.Path
+}
+
 // offlineInfo is the public information about an offline key
 type offlineInfo struct {
 	Name   string        `json:"name"`

crypto/keys/types_test.go

@@ -0,0 +1,40 @@
+package keys
+
+import (
+	"encoding/hex"
+	"testing"
+
+	"github.com/cosmos/cosmos-sdk/crypto/keys/hd"
+	"github.com/cosmos/cosmos-sdk/types"
+	"github.com/stretchr/testify/assert"
+	"github.com/tendermint/tendermint/crypto/secp256k1"
+)
+
+func Test_writeReadLedgerInfo(t *testing.T) {
+	var tmpKey secp256k1.PubKeySecp256k1
+	bz, _ := hex.DecodeString("035AD6810A47F073553FF30D2FCC7E0D3B1C0B74B61A1AAA2582344037151E143A")
+	copy(tmpKey[:], bz)
+
+	lInfo := ledgerInfo{
+		"some_name",
+		tmpKey,
+		*hd.NewFundraiserParams(5, 1)}
+	assert.Equal(t, TypeLedger, lInfo.GetType())
+	assert.Equal(t, "44'/118'/5'/0/1", lInfo.GetPath().String())
+	assert.Equal(t,
+		"cosmospub1addwnpepqddddqg2glc8x4fl7vxjlnr7p5a3czm5kcdp4239sg6yqdc4rc2r5wmxv8p",
+		types.MustBech32ifyAccPub(lInfo.GetPubKey()))
+
+	// Serialize and restore
+	serialized := writeInfo(lInfo)
+	restoredInfo, err := readInfo(serialized)
+	assert.NoError(t, err)
+	assert.NotNil(t, restoredInfo)
+
+	// Check both keys match
+	assert.Equal(t, lInfo.GetName(), restoredInfo.GetName())
+	assert.Equal(t, lInfo.GetType(), restoredInfo.GetType())
+	assert.Equal(t, lInfo.GetPubKey(), restoredInfo.GetPubKey())
+
+	assert.Equal(t, lInfo.GetPath(), restoredInfo.(ledgerInfo).GetPath())
+}

crypto/ledger_test.go

@@ -79,7 +79,7 @@ func TestPublicKeyHDPath(t *testing.T) {
 		// Store and restore
 		serializedPk := priv.Bytes()
 		require.NotNil(t, serializedPk)
-		require.Equal(t, 44, len(serializedPk))
+		require.True(t, len(serializedPk) >= 50)
 
 		privKeys[i] = priv
 	}