Ledger wallet support #4486

arkpar · 2017-02-08T19:25:47Z

@General-Beck libudev-dev is now required to build on linux
@tomusdrw please review RPC/signer changes

rphmeier · 2017-02-08T19:40:16Z

hw/src/ledger.rs

+	/// Ethereum wallet protocol error.
+	ProtocolError(&'static str),
+	/// Hidapi error.
+	UsbError(hidapi::HidError),


style: no Error suffix

rphmeier · 2017-02-08T19:41:26Z

hw/src/lib.rs

+	/// Ledger device error.
+	LedgerDeviceError(ledger::Error),
+	/// USB error.
+	UsbError(libusb::Error),


style: no Error suffix

rphmeier · 2017-02-08T19:42:00Z

parity/account.rs

@@ -18,7 +18,7 @@ use std::path::PathBuf;
 use ethcore::ethstore::{EthStore, SecretStore, import_accounts, read_geth_accounts};
 use ethcore::ethstore::dir::RootDiskDirectory;
 use ethcore::ethstore::SecretVaultRef;
-use ethcore::account_provider::AccountProvider;
+use ethcore::account_provider::{AccountProvider, AccountProviderSetttings};


Setttings -> Settings

rphmeier · 2017-02-08T19:42:55Z

parity/params.rs

@@ -184,6 +185,7 @@ impl Default for AccountsConfig {
 			testnet: false,
 			password_files: Vec::new(),
 			unlocked_accounts: Vec::new(),
+			enable_hardware_wallets: false,


shouldn't this be true if it's supposed to be opt-out?

It is not needed for most use cases of account management such as creating or listing accounts.

tomusdrw

Some grumbles.

tomusdrw · 2017-02-09T08:06:12Z

ethcore/src/account_provider/mod.rs

@@ -278,14 +328,36 @@ impl AccountProvider {
 		Ok(r)
 	}

+	/// Returns each hardware account along with name and meta.
+	pub fn hardware_accounts_info(&self) -> Result<HashMap<Address, AccountMeta>, Error> {
+		let r: HashMap<Address, AccountMeta> = self.hardware_accounts()?


Is type annotation necessary here?

tomusdrw · 2017-02-09T08:12:58Z

hw/src/ledger.rs

+		}
+
+		let (major, minor, patch) = (ver[1], ver[2], ver[3]);
+		if major <= 1 && minor == 0 && patch < 3 {


Is this condition correct? Should it mean that anything greater than 1.0.3 is supported?
If so then it needs to be:

if major < 1 || (major == 1 && minor == 0 && patch < 3)

Currently version 0.x.0 (x > 0) will be treated as correct, no?

tomusdrw · 2017-02-09T08:17:26Z

rpc/src/v1/impls/parity.rs

@@ -138,6 +138,22 @@ impl<C, M, S: ?Sized, U> Parity for ParityClient<C, M, S, U> where
 		)
 	}

+	fn hardware_accounts_info(&self) -> Result<BTreeMap<String, BTreeMap<String, String>>, Error> {


Could be BTreeMap<H160, BTreeMap<String, String>> or even better BTreeMap<H160, HwAccountInfo>

svyatonik · 2017-02-09T09:41:20Z

hw/src/ledger.rs

+			KeyPath::EthereumClassic => etc_path,
+		};
+		let key_and_address = Self::send_apdu(handle, commands::GET_ETH_PUBLIC_ADDRESS, 0, 0, derivation_path)?;
+		if key_and_address.len() != 107 { // 1 + 65 PK + 1 + 20 Addr


Is it correct? 1+65+1+20 = 87, not 107. A bit confusing :)

Address actually takes 40 because it comes ascii-hex encoded. Will update the comment.

rphmeier · 2017-02-09T12:14:29Z

hw/src/ledger.rs

+impl fmt::Display for Error {
+	fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+		match *self {
+			Error::Protocol(ref s) => write!(f, "Ledget protocol error: {}", s),


typo: Ledget

General-Beck · 2017-02-09T21:14:11Z

libudev-dev added

gavofyork · 2017-02-10T00:06:20Z

@arkpar need README altering for linux build instructions, too?

arkpar · 2017-02-10T10:46:14Z

already altered

karalabe · 2017-02-14T08:25:52Z

hw/src/ledger.rs

+			let mut chunk: [u8; HID_PACKET_SIZE] = [0; HID_PACKET_SIZE];
+			let chunk_size = handle.read(&mut chunk)?;
+			trace!("read {:?}", &chunk[..]);
+			if chunk_size < 5 || chunk[1] != 0x01 || chunk[1] != 0x01 || chunk[2] != APDU_TAG {


If you copy our code, at least copy correctly. First should be chunk[0].

Ehm, I'm pretty sure this is correct. Why do you think it should be [0]?

This is rust code btw

It's checking chunk[1] twice so either one should be chunk[0] or second one is redundant.

This is rust code btw

You don't say.

The communication channel part of the header is 2 bytes, both 0x01.

@arkpar Take another look at the code you copied, it's correct there.

hidapi sends HID report id in the first byte. I've noticed you are using libusb for communication. This is a poor choice because it does not allow communicating with HID devices on macOS. I'm only using libusb for hotplug notifications - a feature that your code does not have at all. Perhaps you should be more careful with such accusations.

I didn't say where you copied it from. Seems you're awfully familiar with it though.

But ok, let's play. I dare you to publicly say you did not write this code segment based on the go-ethereum implementation.

I've sure seen your PR but I did not copy it. I've looked into the reference scala code mostly. In particular https://github.com/LedgerHQ/ledger-wallet-ethereum-chrome/blob/67864c09fc88fd744cd17a28ba9460e20987b898/src/main/scala/co/ledger/wallet/web/ethereum/core/device/LedgerTransportHelper.scala#L39
There's only a few ways you can write a simple function that splits data into 64 byte chunks and appends a simple header.

arkpar added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Feb 8, 2017

arkpar requested a review from tomusdrw February 8, 2017 19:25

rphmeier reviewed Feb 8, 2017

View reviewed changes

arkpar force-pushed the ledger branch from 0d1923e to d7cbd05 Compare February 8, 2017 21:32

tomusdrw reviewed Feb 9, 2017

View reviewed changes

svyatonik reviewed Feb 9, 2017

View reviewed changes

Ledger devices support

4c444d4

arkpar force-pushed the ledger branch from 0db3607 to cfc7b61 Compare February 9, 2017 10:16

rphmeier reviewed Feb 9, 2017

View reviewed changes

structs for RPC types

b980b7f

arkpar force-pushed the ledger branch from cfc7b61 to b980b7f Compare February 9, 2017 12:46

rphmeier mentioned this pull request Feb 9, 2017

Trezor hardware wallet support #4500

Closed

rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 9, 2017

gavofyork merged commit a7e6d87 into master Feb 10, 2017

gavofyork deleted the ledger branch February 10, 2017 00:07

rphmeier mentioned this pull request Feb 10, 2017

Preparation for Light client RPC #4485

Merged

karalabe reviewed Feb 14, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ledger wallet support #4486

Ledger wallet support #4486

arkpar commented Feb 8, 2017 •

edited

Loading

rphmeier Feb 8, 2017

rphmeier Feb 8, 2017

rphmeier Feb 8, 2017

rphmeier Feb 8, 2017

arkpar Feb 8, 2017

tomusdrw left a comment

tomusdrw Feb 9, 2017

tomusdrw Feb 9, 2017 •

edited

Loading

tomusdrw Feb 9, 2017

svyatonik Feb 9, 2017

arkpar Feb 9, 2017

rphmeier Feb 9, 2017

General-Beck commented Feb 9, 2017

gavofyork commented Feb 10, 2017 •

edited

Loading

arkpar commented Feb 10, 2017

karalabe Feb 14, 2017

arkpar Feb 14, 2017

arkpar Feb 14, 2017

tomusdrw Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

arkpar Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017 •

edited

Loading

arkpar Feb 14, 2017

Ledger wallet support #4486

Ledger wallet support #4486

Conversation

arkpar commented Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

General-Beck commented Feb 9, 2017

gavofyork commented Feb 10, 2017 • edited Loading

arkpar commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karalabe Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Feb 8, 2017 •

edited

Loading

tomusdrw Feb 9, 2017 •

edited

Loading

gavofyork commented Feb 10, 2017 •

edited

Loading

karalabe Feb 14, 2017 •

edited

Loading