Skip to content

Conversation

@achow101
Copy link
Member

This PR moves device enumeration from the enumerate() function to enumerate() functions within each device driver. These device specific enumerate() functions allow us to enumerate those devices with whatever device specific enumeration required, instead of only enumerating with hid. This allows us to interact with devices connected through other methods such as udp (trezor emulator), webusb (trezor t), unix pipes (coldcard emulator), etc. but without requiring us to add that enumeration to the actual enumerate() function. Because there can be different types of device interfaces, the creation of the device is now handled by the device client class. The device path will be passed in and the client class uses that to know what type of device interface to create.

Furthermore, device drivers have been moved to hwilib/devices/ and both getclient() and enumerate() will call their respective functions and constructors without explicitly importing the device specific drivers. This allows us to easily add a new device driver by simply adding its implementation to the hwilib/devices/ folder. In order to facilitate this automatic discovery and import of device drivers, the client class names and arguments have been normalized. The class name must be the device name (as will be specified in the --device-type parameter), begin with a capital letter, have the rest of the device name in lower case, and followed by Client. Classes must also take two positional arguments path and password, in that order.

This change should enable the use of the Trezor T (so fixes #55) as well as device simulators and emulators so that automated tests can be done.

@achow101 achow101 force-pushed the dev-specific-enum branch 2 times, most recently from d252fa4 to 46f92c2 Compare November 27, 2018 00:00
Copy link
Collaborator

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

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

minor commit history nit, looks pretty good.

@achow101 achow101 force-pushed the dev-specific-enum branch 2 times, most recently from 7fcbb8d to dd0bac8 Compare November 27, 2018 04:16
@instagibbs
Copy link
Collaborator

instagibbs commented Nov 27, 2018

Ok Ledger stuff at least works now.

limited tACK

Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

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

tACK 409ef39 for Trezor Model T on macOS. enumerate finds it, getmasterxpub, getxpub and signtx work. displayaddress and sign_message aren't implemented yet, so I didn't test those. I didn't test the password feature.

return client

# Get a list of all available hardware wallets
def enumerate(args):
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if enumerate didn't ignore --device-type= as it's a bit slow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the suggestion but obviously should wait until this is merged

Instead of creating an HID device and passing that in, pass in the
path. Also pass in the password for each of the clients so that the
interface is consistent.
Instead of enumerating only HID devices globally, have each device
specific section do its own device enumeration so that we can get
devices which use other protocols without effecting global device
enumeration code.
This will help later when doing dynamic object creation
enumerate() and getclient() now will get devices and client objects
by calling the respective functions and constructors in the files
in the hwilib/devices/ folder. It will do this automatically with
any device specific drivers implemented there. So explicitly naming
the devices and hardcoding their constructors in hwilib/commands.py
is no longer necessary.
@achow101 achow101 merged commit e86669c into master Dec 4, 2018
achow101 added a commit that referenced this pull request Dec 4, 2018
…d device drivers without explicit import

e86669c Remove unnecessary device_ids.py (Andrew Chow)
64c1953 Enumerate devices and get client without hardcoding device modules (Andrew Chow)
15b8cbd Rename classes to fit pattern of first uppercase letter, then lowercase (Andrew Chow)
ce2fd26 Move device enumeration to device specific code (Andrew Chow)
c625e03 Unify device client constructor to path and password (Andrew Chow)
1c334c5 Move device specific things to devices sub folder (Andrew Chow)

Pull request description:

  This PR moves device enumeration from the `enumerate()` function to `enumerate()` functions within each device driver. These device specific `enumerate()` functions allow us to enumerate those devices with whatever device specific enumeration required, instead of only enumerating with hid. This allows us to interact with devices connected through other methods such as udp (trezor emulator), webusb (trezor t), unix pipes (coldcard emulator), etc. but without requiring us to add that enumeration to the actual `enumerate()` function. Because there can be different types of device interfaces, the creation of the device is now handled by the device client class. The device path will be passed in and the client class uses that to know what type of device interface to create.

  Furthermore, device drivers have been moved to `hwilib/devices/` and both `getclient()` and `enumerate()` will call their respective functions and constructors without explicitly importing the device specific drivers. This allows us to easily add a new device driver by simply adding its implementation to the `hwilib/devices/` folder. In order to facilitate this automatic discovery and import of device drivers, the client class names and arguments have been normalized. The class name must be the device name (as will be specified in the `--device-type` parameter), begin with a capital letter, have the rest of the device name in lower case, and followed by `Client`. Classes must also take two positional arguments `path` and `password`, in that order.

  This change should enable the use of the Trezor T (so fixes #55) as well as device simulators and emulators so that automated tests can be done.

Tree-SHA512: 057709291194ab2aa9bab138350970f8f6d410216ba5500300ef32c85c06cadc74a2996ed12140849a11f669d9ff01bfe7de4e32a2c7864b29e49606899dcf54
achow101 added a commit that referenced this pull request Dec 10, 2018
…ravis CI

fd19704 Travis config (Andrew Chow)
ced3087 Document tests and how to run them (Andrew Chow)
ddcf842 extra for tests dependencies (Andrew Chow)
8aa0471 Add scripts to run tests and prepare test environments (Andrew Chow)
993b6e8 Update test_bech32.py shebang (Andrew Chow)
a277ac2 Fix psbt serializations to always be consistent (Andrew Chow)
d01cbae Add tests for Coldcard using Coldcard simulator (Andrew Chow)
2272428 Add tests for trezor using the emulator (Andrew Chow)
ccd5f2a Use the DebugLink for the Trezor emulator in order to have tests (Andrew Chow)
64816cb Remove unneeded BIP 32 test (Andrew Chow)

Pull request description:

  This PR is based on top of #66. It adds two test cases for the Trezor and Coldcard implementations by using their respective emulators/simulators. `test_bip32.py` was removed as the BIP 32 implementation was removed and no longer needed. Additionally a test running (`run_tests.py`), a script for building the Trezor emulator, Coldcard simulator, and bitcoind, and instructions for running the tests were all added. Lastly a Travis config has been added and travis builds for this branch are available at https://travis-ci.org/achow101/HWI

Tree-SHA512: 71da24ee39bd845a822d48e83d4c6980cda741544e6cdf04d257db116bc1f7133b3370e235100a66b4fe7c69ecbd0298a4df7879733b75cebeb6a88467f4bf5f
@achow101 achow101 deleted the dev-specific-enum branch December 10, 2018 18:21
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.

WebUSB support (e.g. Trezor model T)

4 participants