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.

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

feat: add HWISigner, moved from bdk_hwi #104

Conversation

oleonardolima
Copy link
Contributor

partially addresses bitcoindevkit/bdk#1516

Description

  • adds a new signer.rs that contains the previous implementation of HWISigner, which implements bdk_wallet::signer::{SignerCommon, TransactionSigner} traits.
  • expose the new signer::HWISigner as public.
  • updates the crate documentation.
  • TODO: re-add test that relies on bdk_wallet::tests::common::get_funded_wallet helper methods.

Notes for Reviewers

I'm unsure if the documentation covers everything needed, please let me know if I'm missing something.

oleonardolima added a commit to oleonardolima/bdk that referenced this pull request Aug 16, 2024
- removes `bdk_hwi` crate, as `HWISigner`'s being moved to
rust-hwi.
- please refer to: bitcoindevkit/rust-hwi#104
@oleonardolima oleonardolima force-pushed the feat/add-hwi-signer-from-bdk-hwi branch from 248d5b4 to 2785083 Compare August 16, 2024 01:04
oleonardolima added a commit to oleonardolima/bdk that referenced this pull request Aug 16, 2024
- removes `bdk_hwi` crate, as `HWISigner`'s being moved to
rust-hwi.
- please refer to: bitcoindevkit/rust-hwi#104
- adds a new `signer.rs` that contains the previous implementation of
  `HWISigner`, which implements `bdk_wallet::signer::{SignerCommon,
TransactionSigner}` traits.
- expose the new `signer::HWISigner` as public.
- updates the crate documentation.
- TODO: re-add test that relies on `bdk_wallet::tests::common::get_funded_wallet`
  helper methods.
@oleonardolima oleonardolima force-pushed the feat/add-hwi-signer-from-bdk-hwi branch from 2785083 to b25168a Compare August 16, 2024 01:16
Cargo.toml Outdated Show resolved Hide resolved
@oleonardolima oleonardolima force-pushed the feat/add-hwi-signer-from-bdk-hwi branch 2 times, most recently from 7408373 to 7c7adf5 Compare August 21, 2024 18:33
@oleonardolima oleonardolima force-pushed the feat/add-hwi-signer-from-bdk-hwi branch from 7c7adf5 to 7747b3f Compare August 21, 2024 18:38
oleonardolima added a commit to oleonardolima/bdk that referenced this pull request Aug 26, 2024
- removes `bdk_hwi` crate, as `HWISigner`'s being moved to
rust-hwi.
- please refer to: bitcoindevkit/rust-hwi#104
@notmandatory notmandatory added this to the 1.0.0-beta milestone Aug 29, 2024
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 7747b3f

@oleonardolima
Copy link
Contributor Author

oleonardolima commented Sep 5, 2024

As the tests were already commented out and not running on bdk_hwi, I think we can go forward with this one (at current state) and add tests in a further PR (with bdk_testenv).

If you both don't see any problems related to it, otherwise this would need to wait for another release on BDK side.

@notmandatory notmandatory merged commit 4004b0a into bitcoindevkit:master Sep 11, 2024
11 checks passed
notmandatory pushed a commit to oleonardolima/bdk that referenced this pull request Sep 11, 2024
- removes `bdk_hwi` crate, as `HWISigner`'s being moved to
rust-hwi.
- please refer to: bitcoindevkit/rust-hwi#104
notmandatory pushed a commit to oleonardolima/bdk that referenced this pull request Sep 11, 2024
- removes `bdk_hwi` crate, as `HWISigner`'s being moved to
rust-hwi.
- please refer to: bitcoindevkit/rust-hwi#104
@oleonardolima oleonardolima deleted the feat/add-hwi-signer-from-bdk-hwi branch September 11, 2024 16:14
notmandatory added a commit to bitcoindevkit/bdk that referenced this pull request Sep 11, 2024
…ing moved to `rust-hwi`

cd8ddfe chore: remove `ci/automation.json` file, used by Dockerfile.ledger (Leonardo Lima)
75c97a6 fix(ci)!: remove `Dockerfile.ledger` and `hwi` steps from coverage step (Leonardo Lima)
b118b82 refactor(bdk_hwi)!: remove `bdk_hwi` (Leonardo Lima)
43257cf refactor(wallet)!: remove dangling unused `hardwaresigner.rs` file (Leonardo Lima)

Pull request description:

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->
  fixes #1516
  depends on bitcoindevkit/rust-hwi#104

  ### Description

  <!-- Describe the purpose of this PR, what's being adding and/or fixed -->

  This PR removed the `bdk_hwi` crate, and the dangling `hardwaresigner.rs` file from `bdk_wallet` crate.

  It simplifies the BDK project workspace as it removes the dependency on `rust-hwi`, pyo3 and some required steps on CI.

  The removal is fine, as the `HWISigner` is being moved to `rust-hwi` project instead, please check: bitcoindevkit/rust-hwi#104

  ### Notes to the reviewers

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  Please let me know what you think about the removal, and if I missed anything.

  ### Changelog notice

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  - Removed dangling unused `hardwaresigner.rs` file from `bdk_wallet` crate.
  - Removed `bdk_hwi` crate, moved to `rust-hwi` project.

  ### 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

  #### New Features:

  * [ ] I've added tests for the new feature
  * [ ] I've added docs for the new feature

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [ ] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  notmandatory:
    ACK cd8ddfe

Tree-SHA512: 43fe716a3f107ae806b1c9bf83bd0a9f403d3c331443d78c9c4c55ac995577fd8b02ba13b77bbadb72b329e0fc94c22a7a9c8b2478ecad303d2c8db28393da2d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants