Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Hardware-wallets Clean up things I missed in the latest PR #8890

Merged
merged 2 commits into from
Jun 18, 2018
Merged

Hardware-wallets Clean up things I missed in the latest PR #8890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
251aaad
Clean up things that I missed
niklasad1 Jun 5, 2018
a194156
Address grumbles
niklasad1 Jun 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 23 additions & 24 deletions hw/src/ledger.rs
View file
Open in desktop

Large diffs are not rendered by default.

38 changes: 21 additions & 17 deletions hw/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

//! Hardware wallet management.

#[warn(missing_docs)]
#[warn(warnings)]
#![warn(missing_docs)]
#![warn(warnings)]

extern crate ethereum_types;
extern crate ethkey;
Expand All @@ -34,22 +34,24 @@ extern crate trezor_sys;
mod ledger;
mod trezor;

use ethkey::{Address, Signature};

use parking_lot::Mutex;
use std::{fmt, time::Duration};
use std::sync::{Arc, atomic, atomic::AtomicBool};
use std::{fmt, time::Duration};

use ethereum_types::U256;
use ethkey::{Address, Signature};
use parking_lot::Mutex;

const USB_DEVICE_CLASS_DEVICE: u8 = 0;
const POLLING_DURATION: Duration = Duration::from_millis(500);

/// `HardwareWallet` device
#[derive(Debug)]
pub struct Device {
path: String,
info: WalletInfo,
}

/// `Wallet` trait
pub trait Wallet<'a> {
/// Error
type Error;
Expand Down Expand Up @@ -109,7 +111,7 @@ pub enum Error {
}

/// This is the transaction info we need to supply to Trezor message. It's more
/// or less a duplicate of ethcore::transaction::Transaction, but we can't
/// or less a duplicate of `ethcore::transaction::Transaction`, but we can't
/// import ethcore here as that would be a circular dependency.
pub struct TransactionInfo {
/// Nonce
Expand Down Expand Up @@ -163,7 +165,7 @@ impl fmt::Display for Error {
}

impl From<ledger::Error> for Error {
fn from(err: ledger::Error) -> Error {
fn from(err: ledger::Error) -> Self {
match err {
ledger::Error::KeyNotFound => Error::KeyNotFound,
_ => Error::LedgerDevice(err),
Expand All @@ -172,7 +174,7 @@ impl From<ledger::Error> for Error {
}

impl From<trezor::Error> for Error {
fn from(err: trezor::Error) -> Error {
fn from(err: trezor::Error) -> Self {
match err {
trezor::Error::KeyNotFound => Error::KeyNotFound,
_ => Error::TrezorDevice(err),
Expand All @@ -181,16 +183,18 @@ impl From<trezor::Error> for Error {
}

impl From<libusb::Error> for Error {
fn from(err: libusb::Error) -> Error {
fn from(err: libusb::Error) -> Self {
Error::Usb(err)
}
}

/// Specifies the direction of the `HardwareWallet` i.e, whether it arrived or left
#[derive(Debug, Copy, Clone)]
pub enum DeviceDirection {
Arrived,
Left,
/// Device arrived
Copy link
Collaborator

Choose a reason for hiding this comment

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

As in "user pulled out the wallet from the usb port"? Maybe be explicit here and say Device became available/Device was removed from the system?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dvdplm do you want s/Arrived/Available/ s/Left/Unavailable renamed completely including the enum patterns?

Happy to change, I have no strong opinions here!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that would make sense, yes, but my grumble was about the doc-comment. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I having second thoughts on this because DeviceDirection doesn't actually indicate whether a device became available or unavailable. Essentially, it only tells we got a notification that a USB device was connected or disconnected with manufacturer ID (Ledger or Trezor). To actually say that a device become available/unavailable we need to do additional checks, for instance, request an Ethereum address and so on!

So, I will keep this! Ok?!

Arrived,
/// Device left
Left,
}

impl fmt::Display for DeviceDirection {
Expand All @@ -211,16 +215,16 @@ pub struct HardwareWalletManager {

impl HardwareWalletManager {
/// Hardware wallet constructor
pub fn new() -> Result<HardwareWalletManager, Error> {
pub fn new() -> Result<Self, Error> {
let exiting = Arc::new(AtomicBool::new(false));
let hidapi = Arc::new(Mutex::new(hidapi::HidApi::new().map_err(|e| Error::Hid(e.to_string().clone()))?));
let ledger = ledger::Manager::new(hidapi.clone(), exiting.clone())?;
let trezor = trezor::Manager::new(hidapi.clone(), exiting.clone())?;

Ok(HardwareWalletManager {
exiting: exiting,
ledger: ledger,
trezor: trezor,
Ok(Self {
exiting,
ledger,
trezor,
})
}

Expand Down
62 changes: 28 additions & 34 deletions hw/src/trezor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,10 @@
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

//! Trezor hardware wallet module. Supports Trezor v1.
//! See http://doc.satoshilabs.com/trezor-tech/api-protobuf.html
//! and https://github.com/trezor/trezor-common/blob/master/protob/protocol.md
//! See <http://doc.satoshilabs.com/trezor-tech/api-protobuf.html>
//! and <https://github.com/trezor/trezor-common/blob/master/protob/protocol.md>
//! for protocol details.

use super::{DeviceDirection, WalletInfo, TransactionInfo, KeyPath, Wallet, Device, USB_DEVICE_CLASS_DEVICE, POLLING_DURATION};

use std::cmp::{min, max};
use std::sync::{atomic, atomic::AtomicBool, Arc, Weak};
use std::time::{Duration, Instant};
Expand All @@ -31,18 +29,17 @@ use ethkey::Signature;
use hidapi;
use libusb;
use parking_lot::{Mutex, RwLock};
use protobuf::{Message, ProtobufEnum};
use protobuf;

use protobuf::{self, Message, ProtobufEnum};
use super::{DeviceDirection, WalletInfo, TransactionInfo, KeyPath, Wallet, Device, USB_DEVICE_CLASS_DEVICE, POLLING_DURATION};
use trezor_sys::messages::{EthereumAddress, PinMatrixAck, MessageType, EthereumTxRequest, EthereumSignTx, EthereumGetAddress, EthereumTxAck, ButtonAck};

/// Trezor v1 vendor ID
const TREZOR_VID: u16 = 0x534c;
/// Trezor product IDs
const TREZOR_PIDS: [u16; 1] = [0x0001];

const ETH_DERIVATION_PATH: [u32; 5] = [0x8000002C, 0x8000003C, 0x80000000, 0, 0]; // m/44'/60'/0'/0/0
const ETC_DERIVATION_PATH: [u32; 5] = [0x8000002C, 0x8000003D, 0x80000000, 0, 0]; // m/44'/61'/0'/0/0
const ETH_DERIVATION_PATH: [u32; 5] = [0x8000_002C, 0x8000_003C, 0x8000_0000, 0, 0]; // m/44'/60'/0'/0/0
const ETC_DERIVATION_PATH: [u32; 5] = [0x8000_002C, 0x8000_003D, 0x8000_0000, 0, 0]; // m/44'/61'/0'/0/0

/// Hardware wallet error.
#[derive(Debug)]
Expand Down Expand Up @@ -84,13 +81,13 @@ impl fmt::Display for Error {
}

impl From<hidapi::HidError> for Error {
fn from(err: hidapi::HidError) -> Error {
fn from(err: hidapi::HidError) -> Self {
Error::Usb(err)
}
}

impl From<protobuf::ProtobufError> for Error {
fn from(_: protobuf::ProtobufError) -> Error {
fn from(_: protobuf::ProtobufError) -> Self {
Error::Protocol(&"Could not read response from Trezor Device")
}
}
Expand All @@ -111,8 +108,8 @@ enum HidVersion {

impl Manager {
/// Create a new instance.
pub fn new(hidapi: Arc<Mutex<hidapi::HidApi>>, exiting: Arc<AtomicBool>) -> Result<Arc<Manager>, libusb::Error> {
let manager = Arc::new(Manager {
pub fn new(hidapi: Arc<Mutex<hidapi::HidApi>>, exiting: Arc<AtomicBool>) -> Result<Arc<Self>, libusb::Error> {
let manager = Arc::new(Self {
usb: hidapi,
devices: RwLock::new(Vec::new()),
locked_devices: RwLock::new(Vec::new()),
Expand Down Expand Up @@ -171,7 +168,7 @@ impl Manager {
}

fn u256_to_be_vec(&self, val: &U256) -> Vec<u8> {
let mut buf = [0u8; 32];
let mut buf = [0_u8; 32];
val.to_big_endian(&mut buf);
buf.iter().skip_while(|x| **x == 0).cloned().collect()
}
Expand Down Expand Up @@ -226,8 +223,8 @@ impl Manager {
let mut data = Vec::new();
let hid_version = self.probe_hid_version(device)?;
// Magic constants
data.push('#' as u8);
data.push('#' as u8);
data.push(b'#');
data.push(b'#');
// Convert msg_id to BE and split into bytes
data.push(((msg_id >> 8) & 0xFF) as u8);
data.push((msg_id & 0xFF) as u8);
Expand All @@ -243,8 +240,8 @@ impl Manager {
let mut total_written = 0;
for chunk in data.chunks(63) {
let mut padded_chunk = match hid_version {
HidVersion::V1 => vec!['?' as u8],
HidVersion::V2 => vec![0, '?' as u8],
HidVersion::V1 => vec![b'?'],
HidVersion::V2 => vec![0, b'?'],
};
padded_chunk.extend_from_slice(&chunk);
total_written += device.write(&padded_chunk)?;
Expand All @@ -253,10 +250,10 @@ impl Manager {
}

fn probe_hid_version(&self, device: &hidapi::HidDevice) -> Result<HidVersion, Error> {
let mut buf2 = [0xFFu8; 65];
let mut buf2 = [0xFF_u8; 65];
buf2[0] = 0;
buf2[1] = 63;
let mut buf1 = [0xFFu8; 64];
let mut buf1 = [0xFF_u8; 64];
buf1[0] = 63;
if device.write(&buf2)? == 65 {
Ok(HidVersion::V2)
Expand All @@ -272,7 +269,7 @@ impl Manager {
let mut buf = vec![0; 64];

let first_chunk = device.read_timeout(&mut buf, 300_000)?;
if first_chunk < 9 || buf[0] != '?' as u8 || buf[1] != '#' as u8 || buf[2] != '#' as u8 {
if first_chunk < 9 || buf[0] != b'?' || buf[1] != b'#' || buf[2] != b'#' {
return Err(protocol_err);
}
let msg_type = MessageType::from_i32(((buf[3] as i32 & 0xFF) << 8) + (buf[4] as i32 & 0xFF)).ok_or(protocol_err)?;
Expand Down Expand Up @@ -308,11 +305,8 @@ impl <'a>Wallet<'a> for Manager {
message.set_gas_price(self.u256_to_be_vec(&t_info.gas_price));
message.set_value(self.u256_to_be_vec(&t_info.value));

match t_info.to {
Some(addr) => {
message.set_to(addr.to_vec())
}
None => (),
if let Some(addr) = t_info.to {
message.set_to(addr.to_vec())
}
let first_chunk_length = min(t_info.data.len(), 1024);
let chunk = &t_info.data[0..first_chunk_length];
Expand Down Expand Up @@ -387,9 +381,9 @@ impl <'a>Wallet<'a> for Manager {
Ok(Device {
path: dev_info.path.clone(),
info: WalletInfo {
name: name,
manufacturer: manufacturer,
serial: serial,
name,
manufacturer,
serial,
address: addr,
},
})
Expand Down Expand Up @@ -439,7 +433,7 @@ impl <'a>Wallet<'a> for Manager {
}

// Try to connect to the device using polling in at most the time specified by the `timeout`
fn try_connect_polling(trezor: Arc<Manager>, duration: &Duration, dir: DeviceDirection) -> bool {
fn try_connect_polling(trezor: &Manager, duration: &Duration, dir: DeviceDirection) -> bool {
let start_time = Instant::now();
while start_time.elapsed() <= *duration {
if let Ok(num_devices) = trezor.update_devices(dir) {
Expand All @@ -462,15 +456,15 @@ struct EventHandler {
impl EventHandler {
/// Trezor event handler constructor
pub fn new(trezor: Weak<Manager>) -> Self {
Self { trezor: trezor }
Self { trezor }
}
}

impl libusb::Hotplug for EventHandler {
fn device_arrived(&mut self, _device: libusb::Device) {
debug!(target: "hw", "Trezor V1 arrived");
if let Some(trezor) = self.trezor.upgrade() {
if try_connect_polling(trezor, &POLLING_DURATION, DeviceDirection::Arrived) != true {
if try_connect_polling(&trezor, &POLLING_DURATION, DeviceDirection::Arrived) != true {
trace!(target: "hw", "No Trezor connected");
}
}
Expand All @@ -479,7 +473,7 @@ impl libusb::Hotplug for EventHandler {
fn device_left(&mut self, _device: libusb::Device) {
debug!(target: "hw", "Trezor V1 left");
if let Some(trezor) = self.trezor.upgrade() {
if try_connect_polling(trezor, &POLLING_DURATION, DeviceDirection::Left) != true {
if try_connect_polling(&trezor, &POLLING_DURATION, DeviceDirection::Left) != true {
trace!(target: "hw", "No Trezor disconnected");
}
}
Expand All @@ -500,7 +494,7 @@ fn test_signature() {

let addr: Address = H160::from("some_addr");

assert_eq!(try_connect_polling(manager.clone(), &POLLING_DURATION, DeviceDirection::Arrived), true);
assert_eq!(try_connect_polling(&manager.clone(), &POLLING_DURATION, DeviceDirection::Arrived), true);

let t_info = TransactionInfo {
nonce: U256::from(1),
Expand Down