Add a custom signer for hardware wallets #682

wszdexdrf · 2022-07-25T06:28:51Z

Also adds a new test in CI for building and testing on a virtual
hardware wallet.

Description

This PR would enable BDK users to sign transactions using a hardware wallet. It is just the beginning hence there are no complex features, but I hope not for long.
I have added a test in CI for building a ledger emulator and running the new test on it. The test is similar to the one on bitcoindevkit/rust-hwi.

Notes to the reviewers

The PR is incomplete (and wouldn't work, as the rust-hwi in cargo.toml is pointing to a local crate, temporarily) as a small change is required in rust-hwi (bitcoindevkit/rust-hwi#42).

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

evanlinjin · 2022-07-25T08:30:09Z

This is exciting that rust-hwi is finally getting integrated into BDK :)

I'm wondering whether "Hardware Wallet" is the best name since we have a struct named Wallet, and there is also a portion of the Bitcoin community wary of the term "Hardware Wallet" anyway. The proposed alternative term is "Hardware Signer" and since our signing traits are already called {}Signers, I think this may be a better term.

wszdexdrf · 2022-07-25T13:45:59Z

I'm wondering whether "Hardware Wallet" is the best name since we have a struct named Wallet

Now that you say it, it makes sense. This isn't anywhere close to the Wallet struct in terms of functionality... so maybe Hardware Signer is better...

danielabrozzoni

Sorry for the early review, I was just too excited to leave this PR alone 😁

This is looking great! I'd split this in multiple commits, at least once for the actual code and one for the CI.

Cargo.toml

src/hardwarewallet/signer.rs

src/hardwaresigner/mod.rs

danielabrozzoni · 2022-08-01T09:31:13Z

src/hardwaresigner/mod.rs

+    }
+
+    #[test]
+    fn test_create_signer() {


In BDK we have two different types of tests:

unit tests

integration tests, which use blockchain backends

This should probably be a unit test, and should have the same structure as the other ones. See, for example, test_sign_single_xprv in src/wallet/mod.rs: there's no blockchain backend being created, only the transaction creation/signing part is tested. This test should also probably be moved in src/wallet/mod.rs.

Then, integration tests could be useful as well. To write one, you should modify the bdk_blockchain_tests in src/testutils/blockchain_tests.rs. Those tests will be run against every backend we support.

danielabrozzoni · 2022-08-03T18:10:25Z

I've just published hwi 0.2.1 btw :)

danielabrozzoni · 2022-08-05T07:15:05Z

Btw, I don't like that much having the hw module under src/hardwaresigner, it feels out of place... I'd say it should be either in src/wallet/signer.rs, or maybe you could make signer.rs a module and put the hardwaresigner in src/wallet/signer/hardwaresigner.rs, as you wish

src/database/memory.rs

src/wallet/mod.rs

src/wallet/signer.rs

src/wallet/hardwaresigner/signer.rs

src/wallet/signer.rs

src/wallet/mod.rs

Cargo.toml

CHANGELOG.md

vladimirfomene

utACK

rajarshimaitra

Review ACK.. This is really exciting.. Very much pumped for BDK HWI integration. Thanks for working on this.. Below I have few comments..

src/database/memory.rs

src/wallet/hardwaresigner/mod.rs

src/wallet/mod.rs

ci/Dockerfile.ledger

rajarshimaitra

tACK 2860c41

rajarshimaitra

Thanks for the quick update.. Few more comments..

src/wallet/mod.rs

CHANGELOG.md

src/wallet/mod.rs

src/wallet/hardwaresigner.rs

danielabrozzoni

ACK 86c1a75 - thanks for taking care of this!

rajarshimaitra

tACK 86c1a75

Also add function to get funded wallet with coinbase

afilini

ACK 138acc3

wszdexdrf mentioned this pull request Jul 25, 2022

Add a new Signer for BDK bitcoindevkit/rust-hwi#41

Closed

danielabrozzoni reviewed Jul 25, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/hardwarewallet/signer.rs Outdated Show resolved Hide resolved

wszdexdrf force-pushed the hardwarewallet branch 2 times, most recently from 1d99858 to 3e5ea35 Compare July 25, 2022 18:42

danielabrozzoni mentioned this pull request Jul 27, 2022

Add build script bitcoindevkit/rust-hwi#34

Merged

wszdexdrf force-pushed the hardwarewallet branch 4 times, most recently from 8cd2442 to 41c8d95 Compare July 31, 2022 11:29

wszdexdrf marked this pull request as ready for review July 31, 2022 11:29

wszdexdrf force-pushed the hardwarewallet branch from 41c8d95 to 432c3ed Compare July 31, 2022 12:03

danielabrozzoni reviewed Aug 1, 2022

View reviewed changes

wszdexdrf force-pushed the hardwarewallet branch 3 times, most recently from 9a859bd to a114eeb Compare August 2, 2022 15:08

danielabrozzoni added this to the Release 0.22.0 Feature Freeze milestone Aug 3, 2022

danielabrozzoni assigned wszdexdrf Aug 4, 2022

wszdexdrf force-pushed the hardwarewallet branch 2 times, most recently from e271984 to 60103e8 Compare August 7, 2022 12:28

danielabrozzoni mentioned this pull request Aug 11, 2022

Refactor testing logic from macros to pure functions #624

Closed

3 tasks

danielabrozzoni reviewed Aug 11, 2022

View reviewed changes

wszdexdrf force-pushed the hardwarewallet branch 2 times, most recently from d1763b3 to 3174208 Compare August 11, 2022 17:09

notmandatory mentioned this pull request Aug 16, 2022

W33 BDK Library Team Call bitcoindevkit/.github#18

Closed

22 tasks

afilini reviewed Aug 16, 2022

View reviewed changes

wszdexdrf force-pushed the hardwarewallet branch from 3174208 to 1dcfa50 Compare August 16, 2022 15:43

danielabrozzoni reviewed Aug 16, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

wszdexdrf force-pushed the hardwarewallet branch 2 times, most recently from 1dae8bd to 6f1d8d1 Compare August 18, 2022 05:13

afilini reviewed Aug 18, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

wszdexdrf force-pushed the hardwarewallet branch from 6f1d8d1 to 2860c41 Compare August 18, 2022 09:31

vladimirfomene reviewed Aug 18, 2022

View reviewed changes

rajarshimaitra reviewed Aug 19, 2022

View reviewed changes

src/database/memory.rs Outdated Show resolved Hide resolved

src/wallet/hardwaresigner/mod.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

ci/Dockerfile.ledger Show resolved Hide resolved

rajarshimaitra reviewed Aug 20, 2022

View reviewed changes

wszdexdrf force-pushed the hardwarewallet branch from 2860c41 to a5d4eda Compare August 20, 2022 13:25

rajarshimaitra suggested changes Aug 21, 2022

View reviewed changes

src/wallet/mod.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

wszdexdrf force-pushed the hardwarewallet branch 4 times, most recently from 6561592 to ab9711b Compare August 22, 2022 13:37

danielabrozzoni reviewed Aug 23, 2022

View reviewed changes

src/wallet/mod.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

src/wallet/hardwaresigner.rs Outdated Show resolved Hide resolved

src/wallet/hardwaresigner.rs Outdated Show resolved Hide resolved

wszdexdrf force-pushed the hardwarewallet branch 3 times, most recently from 872d374 to 86c1a75 Compare August 25, 2022 18:01

danielabrozzoni mentioned this pull request Aug 28, 2022

W35 BDK Library Team Call bitcoindevkit/.github#20

Closed

27 tasks

danielabrozzoni approved these changes Aug 28, 2022

View reviewed changes

rajarshimaitra approved these changes Aug 28, 2022

View reviewed changes

wszdexdrf added 3 commits August 29, 2022 13:53

Add a custom signer for hardware wallets

7603477

Also add function to get funded wallet with coinbase

Change CI to add test using ledger emulator

d6e1dd1

Change populate_test_db to not return empty input

138acc3

wszdexdrf force-pushed the hardwarewallet branch from 86c1a75 to 138acc3 Compare August 29, 2022 08:24

afilini approved these changes Aug 29, 2022

View reviewed changes

afilini merged commit 061f15a into bitcoindevkit:master Aug 29, 2022

danielabrozzoni added the Optech Make Me Famous! 🤩 For anything of interest to Optech newsletter label Aug 30, 2022

danielabrozzoni mentioned this pull request Sep 7, 2022

rust-hwi improvements #479

Closed

afilini mentioned this pull request Sep 8, 2022

Release 0.22.0 #742

Closed

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a custom signer for hardware wallets #682

Add a custom signer for hardware wallets #682

wszdexdrf commented Jul 25, 2022 •

edited

Loading

evanlinjin commented Jul 25, 2022

wszdexdrf commented Jul 25, 2022

danielabrozzoni left a comment

danielabrozzoni Aug 1, 2022

danielabrozzoni commented Aug 3, 2022

danielabrozzoni commented Aug 5, 2022

vladimirfomene left a comment

rajarshimaitra left a comment •

edited

Loading

rajarshimaitra left a comment

rajarshimaitra left a comment

danielabrozzoni left a comment

rajarshimaitra left a comment

afilini left a comment

Add a custom signer for hardware wallets #682

Add a custom signer for hardware wallets #682

Conversation

wszdexdrf commented Jul 25, 2022 • edited Loading

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

evanlinjin commented Jul 25, 2022

wszdexdrf commented Jul 25, 2022

danielabrozzoni left a comment

Choose a reason for hiding this comment

danielabrozzoni Aug 1, 2022

Choose a reason for hiding this comment

danielabrozzoni commented Aug 3, 2022

danielabrozzoni commented Aug 5, 2022

vladimirfomene left a comment

Choose a reason for hiding this comment

rajarshimaitra left a comment • edited Loading

Choose a reason for hiding this comment

rajarshimaitra left a comment

Choose a reason for hiding this comment

rajarshimaitra left a comment

Choose a reason for hiding this comment

danielabrozzoni left a comment

Choose a reason for hiding this comment

rajarshimaitra left a comment

Choose a reason for hiding this comment

afilini left a comment

Choose a reason for hiding this comment

wszdexdrf commented Jul 25, 2022 •

edited

Loading

rajarshimaitra left a comment •

edited

Loading