Skip to content

Add more tests and related fixes#118

Merged
achow101 merged 9 commits into
bitcoin-core:masterfrom
achow101:moar-tests
Feb 12, 2019
Merged

Add more tests and related fixes#118
achow101 merged 9 commits into
bitcoin-core:masterfrom
achow101:moar-tests

Conversation

@achow101
Copy link
Copy Markdown
Member

@achow101 achow101 commented Feb 4, 2019

Adds tests for nearly all commands for all devices and related fixes. Only restore for trezor and keepkey are untested as I could not get them to work. All other commands for all devices have tests and related bug fixes.

Copy link
Copy Markdown
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.

I'm all for more tests. Light review of d097646 only found one problem, but I'm not very familiar with the test suite.

Comment thread hwilib/base58.py
pubkey = data[-37:-4]
return pubkey_to_address(pubkey, testnet)

def xpub_to_pub_hex(xpub):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I still need xpub_to_pub_hex in #117, because the inferred descriptors from Core use the pubkey in hex.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've added it back in

Tests and fixes for promptpin, sendpin, and restore and backup commands on Coldcard
Adds tests for setup, wipe, backup, restore, displayaddress,
promptpin, and sendpin.
Tests and fixes for Trezor signmessage, setup, wipe, backup, promptpin, and sendpin
Tests for setup, wipe, restore, backup, promptpin, sendpin
Tests and fixes for promptpin, sendpin, setup, wipe, restore, backup,
and signmessage commands
@achow101
Copy link
Copy Markdown
Member Author

Merging as tests pass

@achow101 achow101 merged commit a38e993 into bitcoin-core:master Feb 12, 2019
achow101 added a commit that referenced this pull request Feb 12, 2019
a38e993 Tests and fixes for remaining Ledger commands (Andrew Chow)
d627634 Tests and fixes for remainig Keepkey commands. (Andrew Chow)
cf152b7 Tests and fixes for remaining Trezor commands (Andrew Chow)
40abbc2 Tests and fixes for remaining Digital Bitbox commands (Andrew Chow)
0085b8f Tests and fixes remaining commands for Coldcard (Andrew Chow)
5c54c0f Add tests and fixes for invalid paths (Andrew Chow)
a7d0121 Tests for nonexistent fingerprints and device types (Andrew Chow)
cd0c994 Test signtx with all three output types as tx outputs (Andrew Chow)
a1b36e2 Remove several unused functions (Andrew Chow)

Pull request description:

  Adds tests for nearly all commands for all devices and related fixes. Only `restore` for `trezor` and `keepkey` are untested as I could not get them to work. All other commands for all devices have tests and related bug fixes.

Tree-SHA512: 2c49e0fce037955f1a10ed24664f7eabae8a7db6803ac3afc296c9a2763ba8f91c5e2bc598847719a1cad82f8afaad352eaf98aa4a3e1aa0475d5f6efab5b9c3
achow101 added a commit that referenced this pull request Feb 14, 2019
7c0fd1e Update dependencies in the readme (Andrew Chow)
681a52d Replace btchip-python with our own stripped down version of it (Andrew Chow)
f7d04ff Replace ckcc dependency with our own version of the library itself (Andrew Chow)
f0c5aa7 Add Keepkey to Trezorlib and have KeepkeyClient use TrezorClient (Andrew Chow)
2693ae3 Change TrezorClient to do what TrezorNoInit did (Andrew Chow)
aa3665c Replace trezorlib with our own stripped down version (Andrew Chow)

Pull request description:

  This PR replaces all of the device libraries (`trezor`, `keepkey`, `ckcc-protocol,` and `btchip-python`) with stripped down versions of them directly in the source tree. These stripped down versions have several unused functions removed, including support for outdated hardware (e.g. Ledger HW.1) and altcoins. Some additional modifications have been made to them in order to make our implementations cleaner and easier to understand.

  One major part of this change is replacing the Keepkey implementation with the Trezor implementation entirely. I found during testing that the Keepkey and Trezor use largely the same protocol. Some parts were different but those did not affect our usage of the Keepkey and the Trezor, so those differences were removed and reconciled. This allows us to have only one library which serves both the Trezor and Keepkey and have just one implementation that works for both Trezor and Keepkey.

  Overall, by replacing the vendor provided libraries with our own versions of them, we are able to reduce the total number of dependencies required, and reduce some possible implementation errors (e.g. it would be a simple mistake to let Trezor or Keepkey do their default behavior of phoning home to fetch previous transactions). We can also make changes to the lower level implementations as needed instead of waiting on changes and fixes to be merged into upstream libraries.

  This PR is based on #118 so that everything that was changed could be tested.

Tree-SHA512: 2e8e581ebf533a9306d76edfff607d6c5fe85f3764e80015b03638abb15edc76f174b013a64604d3b3e70dacf8b70d00c5a1484702d4fbb7c26625dd18da54dc
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.

2 participants