bdk
bdk copied to clipboard
add extended descriptor multisig metadata to policy extraction
Description
This is a first attempt at resolving #597. When using the policy module to parse descriptors, it can be useful to return additional information about the descriptor. This PR adds some of that in, namely key origin details about each key involved and the script type (p2sh, p2sh-p2wsh, or p2wsh).
Notes to the reviewers
An earlier version of this took advantage of PkOrF
being a struct
, but unfortunately just as I was ready to push up for a PR a bunch of changes came in that impacted this, specifically using an enum
for PkOrF
instead which changed a lot of the assumptions and tests. The enum makes most sense when these objects are relatively one dimensional (also makes for more straightforward tests), but when the key item can have more properties such as including more of the key origin, it got a little trickier.
I only updated the tests for the test I added for this feature plus one other I saw breaking. I thought I'd put it up as a draft first to get any feedback before tweaking any further. It also wasn't clear if some of the tests were breaking for other reasons.
The keys also come out a little weirder when serialized now then when they were done as a struct. Since they are basically tagged with the struct name, each key is an object with a single key xpub_origin
whose value is an object with the origin details: fingerprint
(which might be better called xfp
or root_fingerprint
?), derivation_path
, and xkey
).
I also considered whether or not to add the full_derivation_path or trailing path which you can sometimes have in a descriptor including wildcard (e.g. /0/*
), but figured I could wait for that too.
Note, I'm very new at Rust (this is my first production Rust code written), so I'm sure there's a lot that could be more idiomatic or that I've just done wrong completely. All suggestions on how to improve welcome!
Checklists
- [ ] fix remaining tests
- [ ] add full_derivation_path (or similar) (?)
All Submissions:
- [x] I've signed all my commits
- [x] I followed the contribution guidelines
- [x] I ran
cargo fmt
andcargo clippy
before committing
New Features:
- [x] I've added tests for the new feature
- [ ] I've added docs for the new feature
- [ ] I've updated
CHANGELOG.md
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
Thanks for the review @rajarshimaitra! I’m afk for a little bit but at a high level this makes sense to me. Hopefully I can get some time to refactor when I’m back in September
No issues.. This is a valuable PR IMO.. It will be helpful to get more info out from our policy language..
Thanks again for the review @rajarshimaitra. Just to be clear, the request for updates is basically two parts:
- We want a new
GlobalPolicy
struct that has two parts:ScriptType
andPolicy
, wherePolicy
is the normalPolicy
as it is now. - Xpub origin data concept is good as is, but sounds like we want to clear up the language/naming a bit (see my comments in the PR on that)
Hey, we are in the process of releasing BDK 1.0, which will under the hood work quite differently from the current BDK. For this reason, I'm closing all the PRs that don't really apply anymore. If you think this is a mistake, feel free to rebase on master and re-open!
(More specifically, we're going to use the miniscript "planning module" instead of Policies)
Got it. Thanks for the update! We do want to revisit this soon but sounds like it’ll make sense to do it on the new API. Largely if we can export the policy info (or whatever it’s called 😅) and have it accessible via wasm bindings than that is really all we were trying to get from this.