feat: expose two path descriptor wallet loading#1042
Conversation
thunderbiscuit
left a comment
thunderbiscuit
left a comment
There was a problem hiding this comment.
Just a few things that need fixing IMO, otherwise the feature is good.
| }) | ||
| } | ||
|
|
||
| /// Build a two-path descriptor `Wallet` by loading from persistence. |
There was a problem hiding this comment.
Merged yesterday was an update to the API docs for this. See bitcoindevkit/bdk_wallet@77a727d
They now read:
/// The provided descriptor may only contain extended public keys (`xpub`) with exactly 2 paths,
/// or an error will occur at load time.
In our case you can't use that sentence directly because it's just pure impossible to create the Descriptor with private keys so you won't "error at load time", but I would use the first part:
/// The provided descriptor may only contain extended public keys (`xpub`) with exactly 2 paths
Note that this goes away with miniscript 13 so bdk_wallet 4.0. Issue here: bitcoindevkit/bdk_wallet#511.
| /// Checks that the provided two-path descriptor matches exactly what is loaded | ||
| /// for both the external and internal keychains. | ||
| /// | ||
| /// Note that descriptor secret keys are not persisted to the db. This method |
There was a problem hiding this comment.
This cannot be done so should be removed from API docs.
| let wallet: PersistedWallet<PersistenceType> = BdkWallet::load() | ||
| .two_path_descriptor(descriptor) | ||
| .lookahead(lookahead) | ||
| .extract_keys() |
| persister: Arc<Persister>, | ||
| lookahead: u32, | ||
| ) -> Result<Wallet, LoadWithPersistError> { | ||
| let descriptor = two_path_descriptor.to_string_with_secret(); |
|
Oh shoot sorry @reez I reviewed this commit by commit and was only looking at your commit 3 when I made the comments! My bad. |
thunderbiscuit
left a comment
thunderbiscuit
left a comment
There was a problem hiding this comment.
We can keep the API docs you have here if you like, or you can take the ones that were merged on the Rust side if you prefer.
The only valid comment from the review above is that line 199 can be removed, as there are no keys to extract.
|
undrafted, ready for review updated pr to address previous comments. I will squash everything into one commit when I merge after ack but I made separate commits for the things I addressed so it would be clearer what was addressed and how |
thunderbiscuit
left a comment
thunderbiscuit
left a comment
There was a problem hiding this comment.
ACK 3141d04. Glad to see this one go in!
2 participants
#1030 needs to go in first
Description
Adds
Wallet::load_from_two_path_descriptor, allowing bindings consumers to load a persisted wallet from a BIP 389 two-path descriptor while checking both external and internal keychains.This mirrors the upstream
bdk_walletload flow usingWallet::load().two_path_descriptor(...).load_wallet(...).Notes to the reviewers
Documentation
bdk_wallethttps://docs.rs/bdk_wallet/latest/bdk_wallet/struct.LoadParams.html#method.two_path_descriptor
bitcoinuniffiChangelog
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingchangelog:*labelNew Features:
Bugfixes: