Update compiler example to a Policy example

[rajarshimaitra]

Description

Fixes #729.

There is an "unmaintained" warning in a old version of clap, which triggered the issue.

Ideally, we should not have clap in bdk's dependency. It was only used for the compiler.rs example, which was a very tiny clap app compiling miniscript policies, and it wasn't really an example for bdk.

This PR rewrites the example as a policy.rs which demos the BDK's Policy module and policy structures.

• Use a wsh(multi(2, Privkey, Pubkey)) descriptor, which has only one part private and other part public.
• use into_wallet_descriptor() to turn that into a Descriptor and KeyMap.
• Use the KeyMap to create a custom signer.
• Extract the descriptor Policy structure from the given keymap.

I am not very sure on how much this example is helpful. I still find it hard to read the Policy structure visually. But if Policy is something we want the user to know about descriptors and bdk wallets, this shows how to extract it for a simple multisig condition.

Note: There is no use of bdk::wallet in the example. BDK uses the Policy extraction internally while transaction creation. But all these are exposed publicly, so can be used independently too.

Questions:

• Should we still have a minscript::policy::compile() example? Which IIUC is very different from bdk::policy:Policy. I didn't include it in this PR, because I am not sure if it fits inside bdk example categories.

• Should we expose extract_policy as an wallet API? All though its possible to get policy without creating a wallet, why not let the wallet also spit one out for itself, if its useful?

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[rajarshimaitra]

This PR is now ready for review. I have updated the PR description..

[rajarshimaitra]

I am not really sure whats going on with the CI.. The checks seems to be passing in my local..

Maybe the existing workflow won't work for this PR because I changed the example name?

[danielabrozzoni]

Sorry, but concept NACK

Ideally, we should not have clap in bdk's dependency.

Well, it's a dev-dependency, so no one including BDK would actually pull clap (see https://doc.rust-lang.org/rust-by-example/testing/dev_dependencies.html)

It was only used for the compiler.rs example, which was a very tiny clap app compiling miniscript policies, and it wasn't really an example for bdk.

I think it's an important example to have, as it shows how to use miniscript and bdk together. True, the main focus of the example is not some exotic bdk feature, but I don't think this is a reason for nuking it.
I'd understand removing a completely broken example... but that's not the case, resolving the issue literally takes three lines of code:

diff --git a/Cargo.toml b/Cargo.toml
index d61e4f3..5770498 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -97,7 +97,7 @@ test-md-docs = ["electrum"]
 [dev-dependencies]
 lazy_static = "1.4"
 env_logger = "0.7"
-clap = "2.33"
+clap = "3.2"
 electrsd = "0.19.1"
 
 [[example]]
diff --git a/examples/compiler.rs b/examples/compiler.rs
index 8be87e8..e432692 100644
--- a/examples/compiler.rs
+++ b/examples/compiler.rs
@@ -53,12 +53,12 @@ fn main() -> Result<(), Box<dyn Error>> {
         .arg(
             Arg::with_name("parsed_policy")
                 .long("parsed_policy")
-                .short("p")
+                .short('p')
                 .help("Also return the parsed spending policy in JSON format"),
         )
         .arg(
             Arg::with_name("network")
-                .short("n")
+                .short('n')
                 .long("network")
                 .help("Sets the network")
                 .takes_value(true)

I like your policy example, and I think it's super useful, as the policy module is a bit trickier to understand and IIRC not greatly documented. But IMHO we should have it alongside the compiler example, and not instead.

I am not really sure whats going on with the CI.. The checks seems to be passing in my local..

I think you haven't committed the actual deletion of examples/compiler.rs, and the CI is still running it (but it doesn't compile anymore, as you removed clap)

[rajarshimaitra]

I think it's an important example to have, as it shows how to use miniscript and bdk together.

I agree with this. There is no good documentation for minscript Policy, even in the miniscript lib. And miniscript policy and bdk's policy are different things, IIUC..

But I still don't think having a clap app is useful.. If I know nothing about miniscript policy, and don't know how to create one, how am I suppose to use the example clap app? I could have just updated the clap version to fix the issue but the whole example wasn't clear to me at all.. An "app" is technically not an "example" of anything. So decided to remove it and produce a bdk specific policy example code.

But agree that this interaction needs to be there somewhere and this is not part of bdk API, so its not documented anywhere in bdk either.. The only other place we have this code is the bdk-cli compile command.. So maybe instead we can show the same code with a concrete example, so a reader would have something to work with..

I think it's an important example to have, as it shows how to use miniscript and bdk together. True, the main focus of the example is not some exotic bdk feature, but I don't think this is a reason for nuking it.

Also agree with this. There is no other place this interaction between miniscript and bdk has is documented.. Ideally the policy to descriptor demo should be in miniscript instead.. So better not to nuke this.. I am open to restore the compiler example but would prefer instead of a clap app, use example concrete policy samples. If they want an app to do it for some reason, bdk-cli already plays the exact same codes in the compile command..

We also need to properly document the difference between bdk's policy and miniscript policy.. This part of the bdk-miniscript universe is not quite well understood among wallet devs out there.. I am not sure whats the best approach to do that.. Looking for suggestion on that too..

[danielabrozzoni]

Honestly, this feels a bit like bikeshedding, but if you prefer to slightly modify the compiler example to hardcode some values instead of asking them to the user, no problem

[rajarshimaitra]

Honestly, this feels a bit like bikeshedding, but if you prefer to slightly modify the compiler example to hardcode some values instead of asking them to the user, no problem

Even if the change might look slight, I think it has meaningful impact on a users looking at examples.. An app is not an intuitive example..

I will revert the compiler code back in the next push.

And I think the new policy example can stay too? Or better to do in a different PR?

[danielabrozzoni]

And I think the new policy example can stay too? Or better to do in a different PR?

We can add it in this PR, no problem

[afilini]

I personally used the compiler a few times, so I would prefer if it wasn't removed entirely. Maybe the current compiler example could become a compiler binary? And then yes, we can add a different example that compiles the policy and it can be hardcoded to make it easier to understand.

[rajarshimaitra]

@afilini by a compiler binary, do you mean compiling a minscript policy into a bdk wallet via a cli app? If that's the case it already seems possible in bdk-cli here?

https://github.com/bitcoindevkit/bdk-cli/blob/be36eebf6e2b0907c3fadcbefb1e539e287d32ca/src/handlers.rs#L459-L482

Maybe a few more steps to get to the wallet after getting the descriptor from the compile command.

Another possible nice thing would be to have this whole flow documented in a tutorial post in the website. Could be an useful doc..

[rajarshimaitra]

Summary of latest push

• Reverted back the compiler example as a demo code with a concrete miniscript policy. (1st Commit)
• Add a policy example demo-ing bdk's policy generation from a descriptor. (2nd commit)
• Rebase on master.

[afilini]

Yes it's true that it's also in bdk-cli. I don't know, if I'm the only one using this then I guess we can also remove it from here, I'll just get used to using the cli instead.

This requires a rebase and cargo fmt to fix the ci.

[afilini]

I would change this to "first address of the descriptor", because the policy is not guaranteed to have a 1-to-1 map with a descriptor, so the same policy could in theory lead to different descriptors and different addresses if compiled with different software or different versions of the same software

[rajarshimaitra]

Okay.. How about "First derived address from the descriptor"? Would that sound more clear?

But I am a little surprised to learn that same policy can result into different descriptor, hence different addresses, hence a different wallet all together?

[afilini]

Yes, that's why we have descriptors. Descriptors map directly to a specific script, so there's no ambiguity there, but policies need to be compiled and different compilers or different versions of the same compiler are free to make different choices and ultimately produce different descriptors.

[rajarshimaitra]

Rebased and addressed comments..

[afilini]

ACK 97b6fb0