Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(hwi,wallet): enhance hardware wallet integration and testing #1492

Closed
wants to merge 10 commits into from
Closed

refactor(hwi,wallet): enhance hardware wallet integration and testing #1492

wants to merge 10 commits into from

Conversation

pluveto
Copy link

@pluveto pluveto commented Jun 27, 2024

Overview

This PR introduces enhancements to the hardware wallet integration within the hwi crate and improves the testing utilities in the wallet crate.

Changes

  1. Enabling test-util Feature
    The bdk_wallet dependency in the hwi crate now includes the test-util feature, allowing the use of additional testing utilities provided by the bdk_wallet crate.
  2. refining TransactionSigner implementation
  3. Re-enabling and updating hardware wallet signer test
    The previously disabled hardware wallet signer test has been re-enabled and updated to use the test-util feature. This includes improvements to the test structure and error handling, ensuring robust and comprehensive testing.
  4. Move common test functions as test-util inside wallet crate
    A new test-util module has been added to the wallet crate to organize and provide utilities for testing. This promotes code reuse and maintainability across different tests.

@pluveto pluveto force-pushed the refactor/recover-testing branch 2 times, most recently from b8ea85a to 8f19aa3 Compare June 27, 2024 04:13
@evanlinjin
Copy link
Member

Thanks for this PR. I haven't had the opportunity to review this properly yet, however I am wondering if there is a particular reason why bdk_wallet is not a dev-dependency?

@evanlinjin
Copy link
Member

Also, can you please remove the merge commit and do a rebase instead? Thank you

@pluveto pluveto force-pushed the refactor/recover-testing branch from 1193348 to 8f19aa3 Compare July 12, 2024 08:00
@pluveto
Copy link
Author

pluveto commented Jul 12, 2024

OK.

insert_tx has been refactor in 324eeb3. I need to reorganize the code.

@pluveto pluveto force-pushed the refactor/recover-testing branch from 8f19aa3 to c335f35 Compare July 12, 2024 08:25
@pluveto
Copy link
Author

pluveto commented Jul 12, 2024

Reorganized and moved bdk_wallet with test-util feature flag into dev-dependency.

Ready for review. Please take a look.

@pluveto
Copy link
Author

pluveto commented Jul 12, 2024 •
edited
Loading

It seems that the HWI emulator is disabled, which caused the HWI-related tests to fail.

Therefore, I have added it (add HWI emulator setup).

@notmandatory notmandatory added this to the 1.0.0-beta milestone Jul 12, 2024
@pluveto
Copy link
Author

pluveto commented Jul 13, 2024

CI issue is fixed by specific OpenSSL config. 15d6d9a

@nondiremanuel nondiremanuel added the discussion There's still a discussion ongoing label Jul 16, 2024
@ValuedMammal
Copy link
Contributor

I appreciate what is accomplished in this PR. I'm somewhat ambivalent toward having this change. Main concern is bringing python dependency back to CI considering the age of this emulator software and that the nano s hasn't seen a firmware update since 2021 which amounts to more of a maintenance burden than I think is warranted. Other folks might have different opinions. In any case if you decide to continue work on this branch, we'd like if you also signed commits, although for the record I realized that policy isn't well documented.

@pluveto pluveto force-pushed the refactor/recover-testing branch from 6e27d25 to d817537 Compare July 17, 2024 05:14
@pluveto
Copy link
Author

pluveto commented Jul 17, 2024

Thank you for your feedback. I've updated the emulator model used for the firmware to a more recent version. Additionally, I've resubmitted the commits with signatures. Thanks!

@oleonardolima
Copy link
Contributor

Thanks for working on this! However, after #1561 the HWISigner and therefore the TransactionSigner implementation has moved to rust-hwi, so I guess we could close this PR (cc @notmandatory).

Feel free to open a PR in rust-hwi with your suggested enhancements, I also think we could reuse the approach of having a test util, exposing it through bdk_testenv, and use it there at rust-hwi for testing.

@ValuedMammal ValuedMammal mentioned this pull request Nov 5, 2024
Merged
5 tasks
notmandatory added a commit that referenced this pull request Nov 12, 2024
@notmandatory
Merge #1658: fix(wallet)!: Improve test utilities
91d7d3c 
b0dc3dd feat(wallet)!: make `seen_at` mandatory for `Wallet::apply_update_at` (志宇)
00c568d revert(wallet)!: rm `Wallet::unbroadcast_transactions` (志宇)
200a16d fix(wallet)!: delete method `insert_tx` (valued mammal)
ab27884 test(wallet): improve usage of test utils (valued mammal)
9bdf4cb chore: fix imports (valued mammal)
28d8061 test(wallet): fix test descriptor getters (valued mammal)
3135e29 test(wallet): add helpers to `test_utils` (valued mammal)
823bb39 feat(wallet): add module `test_utils` (valued mammal)
297bd9a test(wallet): refactor helper `insert_anchor_from_conf` (valued mammal)

Pull request description:

  Follow up to #1643, refactor `insert_anchor_from_conf` to just insert an anchor of type `ConfirmationBlockTime`

  ### Notes to the reviewers

  The PR introduces a public `test_utils` module and "test-utils" cargo feature that exposes common helpers such as `get_funded_wallet`. Credit to #1492 for inspiring that idea. Usage of test utilities is enhanced overall, and tests are less dependent on problematic APIs that may be removed in the future.

  ### Changelog notice

  - `bdk_wallet`: Added "test-utils" feature flag that exposes common helpers for testing and development
  - Removed methods `Wallet::insert_tx`, `Wallet::insert_checkpoint`, `Wallet::unbroadcast_transactions`

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing
  * [x] I've added docs for the new feature
  * [x] This pull request breaks the existing API

Top commit has no ACKs.

Tree-SHA512: 561757595c65b4531dbf8b81f44387af6ac60114ecca493693cd975188741b5ff7b75a0dcf1dafc9d5750566baad81c644e7463c3c412a8331ad73de29601016
@notmandatory notmandatory removed this from the 1.0.0-beta milestone Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pluveto pluveto

Labels
discussion There's still a discussion ongoing tests
Projects
BDK
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pluveto @evanlinjin @ValuedMammal @oleonardolima @notmandatory @nondiremanuel