bdk icon indicating copy to clipboard operation
bdk copied to clipboard

add extended descriptor multisig metadata to policy extraction

Open bucko13 opened this issue 2 years ago • 3 comments

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 and cargo 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

bucko13 avatar Jun 07 '22 03:06 bucko13

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

bucko13 avatar Aug 21 '22 15:08 bucko13

No issues.. This is a valuable PR IMO.. It will be helpful to get more info out from our policy language..

rajarshimaitra avatar Aug 22 '22 17:08 rajarshimaitra

Thanks again for the review @rajarshimaitra. Just to be clear, the request for updates is basically two parts:

  1. We want a new GlobalPolicy struct that has two parts: ScriptType and Policy, where Policy is the normal Policy as it is now.
  2. 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)

bucko13 avatar Sep 12 '22 22:09 bucko13

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)

danielabrozzoni avatar Mar 16 '23 17:03 danielabrozzoni

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.

bucko13 avatar Mar 16 '23 17:03 bucko13