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

ref(hwi): Move hwi out of bdk #1161

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

danielabrozzoni
Copy link
Member

Fixes #872

Description

This commit moves the hardwaresigner outside of bdk and inside bdk_hwi

Notes to the reviewers

There are currently two issues with the code:

  • TransactionSigner dictates that sign_transaction must return a SignerError - but being SignerError defined inside of bdk, we can't modify it to include an hwi specific error! I don't know how we could fix this (other than getting rid of the trait altogether :)); for now I just added a SignerError::Generic variant, lmk if you have better ideas!
  • The hwi tests used the bdk utils to get a funded wallet for testing, which aren't available in bdk_hwi, which made me realize - maybe we should expose them so that we can use them across our crates, and also our users can use them to test their code?
    For now, I just left the test commented.

Changelog notice

  • The old hardwaresigner module has been moved out of bdk and inside bdk_hwi.

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

@nondiremanuel
Copy link

As per today's discussion in the Lib Team Call, this PR probably needs to be rebased after the ci fix of #1182

Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK

I also agree it would be nice to move some common bdk test utils to a shared crate - similar to #1171

self.client
.sign_tx(psbt)
.map_err(|e| {
SignerError::Generic(format!("While signing with hardware wallet: {}", e))
Copy link
Contributor

Choose a reason for hiding this comment

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

would this work?

Suggested change
SignerError::Generic(format!("While signing with hardware wallet: {}", e))
SignerError::HWIError(e.to_string())

Copy link
Member Author

Choose a reason for hiding this comment

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

In order for this to work, we would need to add the HWIError variant inside of bdk, which is not really desirable since it wouldn't be used inside bdk but in an outside crate.

@evanlinjin
Copy link
Member

This PR moves harwaresigner into a separate crate, but the new crate still depends on bdk. Ideally, bdk_hwi should not depend on bdk. The best way to do this is to have the Signer traits in a separate crate as well - however, this is also non-ideal since those traits will most likely be scrapped in the future.

I would really love to have hwi as a separate crate. However, I don't think it makes sense to do it with the current state of Wallet.

Introducing SignerError::Generic is also non-ideal. Now the error variant for HWI is ambiguous.

In conclusion, I think we should try this again later down the line.

@danielabrozzoni
Copy link
Member Author

Ideally, bdk_hwi should not depend on bdk

Why? We already have rust_hwi available if someone wants to use hardware wallets without using bdk. bdk_hwi is supposed to be for people that want to use hwi and bdk together, so I don't see why it shouldn't depend on bdk.

Introducing SignerError::Generic is also non-ideal. Now the error variant for HWI is ambiguous.

True, but at the moment everyone trying to implement the Signer trait in a crate outside of bdk encounters the same problem, so I think we should find a way to fix this anyways (without having to wait for a complete refactoring on the signer/wallet, since that will take a while). We could add either good docs to SignerError::Generic, create a SignerError::External (for external crates), or maybe we could refactor the trait to return a generic Box<dyn std::error::Error> (I haven't tested this so it might not be feasible)

I agree that this code is very suboptimal given the state of the Wallet, but we won't refactor the Wallet until bdk 2.0; do we want to leave hwi inside bdk, untested, until then?

@nondiremanuel
Copy link

@evanlinjin do you have any feedback on Daniela's last comment?

@notmandatory
Copy link
Member

Please rebase now that #1183 has been merged.

@danielabrozzoni
Copy link
Member Author

Done, thanks!

SignerError::HWIError(e)
}
/// A generic error
Generic(String),
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a big fan of the SignerError::Generic since I just removed bdk::Error::Generic in #1028. But in this case I don't see an simple way around it. If it becomes annoying we can fix it in 2.0 release.

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 f1124d4

@notmandatory
Copy link
Member

notmandatory commented Dec 20, 2023

Can merge this one after the new CI issues are fixed with #1247 (after it's ACKd and merged).

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

ACK f1124d4

I don't like SignerError::Generic, however I don't see a better option without taking too much time. We will change the API greatly for 2.0 anyway (as @danielabrozzoni mentioned: #1161 (comment)).

@danielabrozzoni
Copy link
Member Author

Do you think it would be better to rename SignerError::Generic to SignerError::ExternalTraitImplementationError or something better? This would make it clear that it's needed only for external libraries implementing the Signer trait.

@notmandatory
Copy link
Member

notmandatory commented Jan 6, 2024

How about SignerError::External with doc that explains this is for external trait implementation errors? Or I'd be fine with the longer version too.

@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.3, 1.0.0 Jan 6, 2024
@danielabrozzoni
Copy link
Member Author

Thanks, I rebased and changed Error::Generic to Error::External. LMK if the docs look ok or if they can be improved!

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.

reACK 105d70e

@notmandatory notmandatory merged commit 6e6bad9 into bitcoindevkit:master Jan 8, 2024
12 checks passed
@notmandatory notmandatory added the api A breaking API change label Jan 16, 2024
@notmandatory notmandatory mentioned this pull request Jan 16, 2024
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Move hardware signer into its own crate
5 participants