Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ledger integration #931

Merged
merged 416 commits into from
Jun 29, 2018
Merged

Ledger integration #931

merged 416 commits into from
Jun 29, 2018

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Apr 30, 2018

Closes #1117
Mostly blocked on finalization of #1119 Merged!
Wants tendermint/tendermint#1803, but doesn't need to block merge

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md
  • Updated Gaia/Examples
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

ethanfrey and others added 30 commits October 24, 2017 11:38
Fixes tendermint/go-crypto#43

Avoid susceptibility to timing/side channel attacks by ensuring
that private key and signature comparisons use
`subtle.ConstantTimeCompare`
instead of
`bytes.Equal`
Bugfix: ledger nano/hid compiles on osx/golang1.9
Fixes tendermint/go-crypto#48.

This previously skewed up my fuzzing tests so ensure
that on error we return the zero value PubKey.
…pty-pubKey

PubKeyFromBytes: return zero value PubKey on error
@cwgoes cwgoes force-pushed the cwgoes/ledger-integration branch from 2db4edf to 03459a7 Compare June 28, 2018 23:13
@cwgoes
Copy link
Contributor Author

cwgoes commented Jun 28, 2018

For reasons unknown the linter is linting a file in vendor/; I think that error can safely be ignored.

@ValarDragon
Copy link
Contributor

idk whats going on there either, --vendor is eliminating the rest of the output for the vendors directory

@cwgoes cwgoes force-pushed the cwgoes/ledger-integration branch from e86a284 to 22414b3 Compare June 28, 2018 23:57
ValarDragon
ValarDragon previously approved these changes Jun 29, 2018
Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

// Only need a passphrase for locally-stored keys
if info.GetType() == "local" {
passphrase, err = ctx.GetPassphraseFromStdin(name)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done through an enum type, or something similar, and have associated method "getPassphrase from stdin". Doesn't block this PR.

@@ -32,10 +36,13 @@ If you select --seed/-s you can recover a key from the seed
phrase, otherwise, a new key will be generated.`,
RunE: runAddCmd,
}
cmd.Flags().StringP(flagType, "t", "ed25519", "Type of private key (ed25519|secp256k1|ledger)")
cmd.Flags().StringP(flagType, "t", "secp256k1", "Type of private key (secp256k1|ed25519)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default changing from ed25519 intentional?

ValarDragon
ValarDragon previously approved these changes Jun 29, 2018
@cwgoes cwgoes force-pushed the cwgoes/ledger-integration branch from c31df61 to 2150b33 Compare June 29, 2018 00:38
@cwgoes cwgoes merged commit 59aadf4 into develop Jun 29, 2018
@cwgoes cwgoes deleted the cwgoes/ledger-integration branch June 29, 2018 00:54
adrianbrink pushed a commit that referenced this pull request Jul 2, 2018
Merges the keybase and Ledger code from go-crypto (which is no more) into the SDK
Adds support for Ledger into gaiacli
Cherry-picks updated error handling from #1158
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.