feat: expose wallet signing with provided signers#1043
Conversation
There was a problem hiding this comment.
I have one small question regarding the usefulness of the is_empty method, but otherwise this is ready to go.
Because it's one of the core new features/workflow for the library and hints at where we're going moving forward (creating wallets using only public keys and whatnot (see the upcoming Design page on this topic), I'd like to have it expanded upon in tests if possible.
I'd be happy to add tests if you'd like me to push on this branch @reez. That will get me playing with the API a bit more too. But they could also come in follow-up PRs, I'm not picky.
| } | ||
|
|
||
| /// Returns true when the container has no signers. | ||
| pub fn is_empty(&self) -> bool { |
There was a problem hiding this comment.
Is it possible for us to hit a false here? The way our constructor works, it appears you'll always have one, unless maybe the public descriptors sort of silently fail and don't populate/produce a valid signer?
The Rust type has a SignersContainer::new constructor which creates one without any signers in it, but here we're not offering that (and I'm not sure why we would either even if we could). https://docs.rs/bdk_wallet/latest/bdk_wallet/signer/struct.SignersContainer.html#method.new
There was a problem hiding this comment.
Is it possible for us to hit a false here?
Im looking at it like secret descriptors make is_empty() return false, public descriptors make it return true
The way our constructor works, it appears you'll always have one,
Only when the descriptor contains secret key material, public only descriptors have an empty key_map so that's how I see the constructor can produce a container with no signers
unless maybe the public descriptors sort of silently fail and don't populate/produce a valid signer?
I don't think they silently fail, a public descriptor is valid but it doesnt have secret key material in its key_map so there's just nothing for SignersContainer::build(...) to turn into a signer
The Rust type has a SignersContainer::new constructor which creates one without any signers in it, but here we're not offering that (and I'm not sure why we would either even if we could).
Agreed
Still have this in draft because I need to update it to make sure it's easy to review after breaking it out from the larger pr, so just keep that in mind but if you do want to check it out and have feedback feel free to post it and I can make the updates myself as I get this pr in cleanest shape |
|
addressed questions (I think all of them?) from #1030 in a few of the follow up commits here I'll squash everything into one commit when I merge it though for sure Just FYI |
#1030 first
Description
Adds wrapper for
bdk_wallet::Wallet::sign_with_signers, allowing bindings consumers to sign a PSBT with explicitSignersContainervalues instead of only the wallet-owned signers.This also adds a
SignersContainerwrapper that can be constructed from descriptor key material and passed into the new wallet signing method.Notes to the reviewers
Documentation
bdk_wallethttps://docs.rs/bdk_wallet/3.1.0/bdk_wallet/struct.Wallet.html#method.sign_with_signers
bitcoinuniffiChangelog
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingchangelog:*labelNew Features:
Bugfixes: