Move device enumeration to device specific drivers and load device drivers without explicit import by achow101 · Pull Request #66 · bitcoin-core/HWI

achow101 · 2018-11-26T23:32:23Z

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.

instagibbs

minor commit history nit, looks pretty good.

instagibbs · 2018-11-27T15:39:34Z

Ok Ledger stuff at least works now.

limited tACK

Sjors

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.

Sjors · 2018-11-29T13:04:07Z

@@ -53,41 +53,12 @@ def get_client(device_type, device_path, password=None):
 # Get a list of all available hardware wallets
 def enumerate(args):


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

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.

…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

…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 force-pushed the dev-specific-enum branch 2 times, most recently from d252fa4 to 46f92c2 Compare November 27, 2018 00:00

instagibbs reviewed Nov 27, 2018

View reviewed changes

Comment thread hwilib/commands.py

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

instagibbs reviewed Nov 27, 2018

View reviewed changes

Comment thread hwilib/devices/ledger.py Outdated

instagibbs requested changes Nov 27, 2018

View reviewed changes

Comment thread hwilib/commands.py Outdated

instagibbs reviewed Nov 27, 2018

View reviewed changes

Comment thread hwilib/devices/coldcard.py

achow101 force-pushed the dev-specific-enum branch from dd0bac8 to 113da86 Compare November 27, 2018 15:35

achow101 force-pushed the dev-specific-enum branch from 113da86 to 409ef39 Compare November 27, 2018 17:01

achow101 mentioned this pull request Nov 28, 2018

Tests for Trezor and Coldcard implementations and enable Travis CI #69

Merged

Sjors approved these changes Nov 29, 2018

View reviewed changes

Sjors mentioned this pull request Nov 29, 2018

WebUSB support (e.g. Trezor model T) #55

Closed

achow101 added 6 commits November 29, 2018 10:11

Move device specific things to devices sub folder

1c334c5

Unify device client constructor to path and password

c625e03

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.

Move device enumeration to device specific code

ce2fd26

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.

Rename classes to fit pattern of first uppercase letter, then lowercase

15b8cbd

This will help later when doing dynamic object creation

Remove unnecessary device_ids.py

e86669c

achow101 force-pushed the dev-specific-enum branch from 409ef39 to e86669c Compare November 29, 2018 15:11

This was referenced Nov 30, 2018

Trezor: displayaddress #71

Merged

WIP: descriptor request using "getkeys" #73

Closed

WIP: bitcoind signer support achow101/bitcoin#2

Closed

achow101 merged commit e86669c into master Dec 4, 2018

achow101 deleted the dev-specific-enum branch December 10, 2018 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move device enumeration to device specific drivers and load device drivers without explicit import#66

Move device enumeration to device specific drivers and load device drivers without explicit import#66
achow101 merged 6 commits into
masterfrom
dev-specific-enum

achow101 commented Nov 26, 2018

Uh oh!

instagibbs left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Nov 27, 2018 •

edited

Loading

Uh oh!

Sjors left a comment

Uh oh!

Uh oh!

Sjors Nov 29, 2018

Uh oh!

instagibbs Nov 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -53,41 +53,12 @@ def get_client(device_type, device_path, password=None):
		# Get a list of all available hardware wallets
		def enumerate(args):

Conversation

achow101 commented Nov 26, 2018

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

instagibbs Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

instagibbs commented Nov 27, 2018 •

edited

Loading