Ledger integration by cwgoes · Pull Request #85 · tendermint/go-crypto

cwgoes · 2018-04-30T14:43:08Z

Refactored the existing API, although I'm not quite sure if that interface is necessary - did we use to use this in the keybase more directly?

CC @adrianbrink is #68 a duplicate?

(@liamsi edited to autom. close related issues on merge):
resolves #99
resolves #93

cwgoes · 2018-04-30T17:35:05Z

Works now. Use WITH_LEDGER=1 make test to run the Ledger testcases (you'll need a physical device plugged in, with the Cosmos app open).

cwgoes · 2018-04-30T17:37:41Z

According to Greg, we can ignore the Jenkins failure.

cwgoes · 2018-05-28T23:18:33Z

Needs to be updated for some upstream changes

cwgoes · 2018-05-30T21:13:25Z

Can be reviewed.

ValarDragon

Looks good to me, though I think we may want to test the signing of messages significantly larger than one hash block size.

liamsi

LGTM with some minor nitpicks, a few questions

liamsi · 2018-05-30T22:45:36Z

amino.go

 	cdc.RegisterConcrete(PubKeyEd25519{},
 		"tendermint/PubKeyEd25519", nil)
+	cdc.RegisterConcrete(PubKeyLedgerEd25519{},
+		"tendermint/PubKeyLedgerEd25519", nil)


(Maybe stupid) question: Do we actually need to support Ed25519 here? Isn't this ledger device for end-user wallets? My understanding was we do only use Secp256k1 for user wallets. Might have misunderstood that though.

Per discussion now removed.

liamsi · 2018-05-30T22:45:50Z

amino.go

+	cdc.RegisterConcrete(PrivKeyLedgerSecp256k1{},
+		"tendermint/PrivKeyLedgerSecp256k1", nil)
+	cdc.RegisterConcrete(PrivKeyLedgerEd25519{},
+		"tendermint/PrivKeyLedgerEd25519", nil)


same as above

liamsi · 2018-05-30T22:46:15Z

ledger_ed25519.go

+	"fmt"
+	"github.com/pkg/errors"
+
+	// "github.com/tendermint/ed25519"


File deleted

liamsi · 2018-05-30T22:47:25Z

Gopkg.lock


+[[projects]]
+  branch = "master"
+  name = "github.com/brejski/hid"


Question: Is this package used anywhere? Can't find it. Is it necessary to include it because of ledger-goclient ?

Pulled in because of ledger-goclient, yes

liamsi · 2018-05-30T23:15:05Z

ledger_ed25519.go

+}
+
+// PubKey returns the stored PubKey
+// TODO: query the ledger if not there, once it is not volatile


Is there an issue that keeps track of the open TODOs here?
Group them together and open issues for them. Ideally assign them to someone. Otherwise, they might be forgotten and always stay todos in the code.

Indeed, these were left over from an older version of this code, moved to #114

liamsi · 2018-05-30T23:16:17Z

ledger_ed25519.go

+func (pk PrivKeyLedgerEd25519) forceGetPubKey() (key PubKey, err error) {
+	dev, err := getLedger()
+	if err != nil {
+		return key, errors.New(fmt.Sprintf("Cannot connect to Ledger device - error: %v", err))


fmt.Errorf instead of errors.New(fmt.Sprintf(..? (here and everywhere else)?

Updated, thanks

liamsi · 2018-05-30T23:27:25Z

ledger_ed25519.go

+func pubkeyLedgerEd25519(device *ledger.Ledger, path DerivationPath) (pub PubKey, err error) {
+	key, err := device.GetPublicKeyED25519(path)
+	if err != nil {
+		return pub, fmt.Errorf("Error fetching public key: %v", err)


pub will be nil in this case. Make this explicit by writing return nil, fmt.Errof(...). Also, error messages are usually not capitalized (as the error might bubble up and wrapped into another error msg).

Updated, lowercased error messages

liamsi · 2018-05-30T23:29:49Z

ledger_ed25519.go

+	}
+	var p PubKeyLedgerEd25519
+	copy(p[:], key[0:32])
+	return p, err


err should be nil here. Either write this, or:

var p PubKeyLedgerEd25519 copy(p[:], key[0:32]) pub = p return

liamsi · 2018-05-30T23:31:21Z

ledger_ed25519.go

+func signLedgerEd25519(device *ledger.Ledger, path DerivationPath, msg []byte) (sig Signature, err error) {
+	bsig, err := device.SignED25519(path, msg)
+	if err != nil {
+		return sig, err


just return, similarly below

liamsi

all comments addressed :-) LGTM

adrianbrink · 2018-06-01T17:55:54Z

Awesome work @cwgoes @liamsi

cwgoes requested review from ebuchman and jaekwon as code owners April 30, 2018 14:43

cwgoes changed the title ~~WIP: Ledger integration~~ Ledger integration Apr 30, 2018

cwgoes mentioned this pull request May 1, 2018

Ledger integration cosmos/cosmos-sdk#931

Merged

6 tasks

ebuchman mentioned this pull request May 7, 2018

Remove Ledger #68

Closed

cwgoes added 7 commits May 14, 2018 15:26

Ledger integration, WIP

1c9ff46

Fix no-Ledger testcase

065c394

Fix testcases, all looks OK

e25a64f

Prevent unnecessary signatures, improve error messages

391936b

Bugfix

86b09b0

Update dependency versions

49e03fb

Update to new Ledger API in progress

337ad8e

cwgoes force-pushed the cwgoes/ledger-integration branch from 6dd92f7 to 12c0e2c Compare May 14, 2018 13:33

Update to latest upstream, debugging information

e6d0ade

cwgoes force-pushed the cwgoes/ledger-integration branch from 12c0e2c to e6d0ade Compare May 14, 2018 14:24

cwgoes mentioned this pull request May 15, 2018

Package _nano is never build, never used, and doesn't compile #99

Closed

liamsi mentioned this pull request May 15, 2018

make sure the passed argument in nano.parseDigest is at least 4 bytes #96

Closed

cwgoes changed the title ~~Ledger integration~~ WIP: Ledger integration May 28, 2018

cwgoes added 3 commits May 30, 2018 01:39

Update upstream Ledger code

e534559

Clarify function names

3186dc4

Add ed25519, tests will fail until ed25519 verification fix

ee411da

cwgoes force-pushed the cwgoes/ledger-integration branch from 6da505e to ee411da Compare May 30, 2018 01:43

cwgoes added 2 commits May 30, 2018 22:07

Implement PubKeyLedgerEd25519

c689f38

Pin to an upstream revision

bb81e4a

cwgoes changed the title ~~WIP: Ledger integration~~ Ledger integration May 30, 2018

cwgoes requested a review from ValarDragon May 30, 2018 20:18

cwgoes mentioned this pull request May 30, 2018

prehashing before ed25519 cosmos/ledger-cosmos-obsolete#60

Closed

ValarDragon approved these changes May 30, 2018

View reviewed changes

cwgoes requested a review from liamsi May 30, 2018 21:40

liamsi suggested changes May 30, 2018

View reviewed changes

Remove Ledger ed25519 support, for now

63aac65

liamsi approved these changes May 31, 2018

View reviewed changes

Move TODOs to #114

80e9752

cwgoes merged commit 2bbad9d into develop May 31, 2018

cwgoes deleted the cwgoes/ledger-integration branch May 31, 2018 19:45

liamsi mentioned this pull request Jun 4, 2018

Seed phrase has same last word when create a new key #116

Closed

Conversation

cwgoes commented Apr 30, 2018 • edited by liamsi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwgoes commented Apr 30, 2018

Uh oh!

cwgoes commented Apr 30, 2018

Uh oh!

cwgoes commented May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwgoes commented May 30, 2018

Uh oh!

ValarDragon left a comment

Choose a reason for hiding this comment

Uh oh!

liamsi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamsi May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamsi May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

adrianbrink commented Jun 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cwgoes commented Apr 30, 2018 •

edited by liamsi

Loading

cwgoes commented May 28, 2018 •

edited

Loading

liamsi left a comment •

edited

Loading

liamsi May 30, 2018 •

edited

Loading

liamsi May 30, 2018 •

edited

Loading