feat: Add new_wsh_sortedmulti and new_pk

[ItoroD]

Description

This Pr is based on my comment from #687 here.

Notes to the reviewers

In bdk-wallet we can do the following to create a 1 of 2 multisig descriptor. Thanks so miniscript helper functions

let multisig_descriptor = Descriptor::new_wsh_sortedmulti(1, vec![key.clone(), key2.clone()]).unwrap();

In bindings: kotlin, we do:

val multisigDescriptor = Descriptor("wsh(multi(1,$key,$key2))", Network.REGTEST)

Current we have to concatenate a string with the keys and script type to parse the descriptor.

Much easier and safer to be able to do:

val multisigDescriptor = Descriptor.newWshSortedmulti(2u, pkList);

Documentation

• new_wsh_sortedmulti - https://docs.rs/miniscript/12.3.5/miniscript/descriptor/enum.Descriptor.html#method.new_sh_wsh_sortedmulti

• new_pk - https://docs.rs/miniscript/12.3.5/miniscript/descriptor/enum.Descriptor.html#method.new_pk

• bdk_wallet

• bitcoin

• uniffi

• Other:

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing
• I've added a changelog in the next release tracking issue (see example)
• I've linked the relevant upstream docs or specs above

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

[reez]

this is really cool @ItoroD

[ItoroD]

this is really cool @ItoroD

Thanks!

If this get approved, I am hoping to bring in the other similar miniscript methods that take either a list of PKs or single Pk. There are other ones that take more complex params that I can focus my attention on later once these simple ones are in.

I did just these 2 in this pr just as a sort of concept and starter for the rest.

[reez]

Should we avoid reparsing raw strings into BdkDescriptorPublicKey here? Right now malformed keys become DescriptorError::Miniscript while the rest of the API exposes key parsing through DescriptorPublicKey and key oriented error variants. Taking DescriptorPublicKey directly, or adding a dedicated conversion for key parse failures would keep this constructor consistent with the existing bindings surface I think

[ItoroD]

Taking DescriptorPublicKey directly will mean the user will parse his string first before passing it to our new methods? Isn't it better if the method handles both?

With regards to the error, currently you will get a DescriptorKeyException if you pass a malformed key. Also, some of these constructors are not going to be taking pk alone, they will take actual miniscript like here

[ItoroD]

I just ran the android test with a malformed key and indeed its a DescriptorException$Miniscript error that shows up. I should be able to change it to give a DescriptorKeyException like it does when you parse with DescriptorPublicKey.

About whether to take strings as param or DescriptorPublicKey, I was only thinking about the way we have been passing things into the descriptor type so far (with strings). Hoping it will not be too disruptive it use DescriptorPublicKey.fromString first before they they pass it to this constructor.

[reez]

Yeah your right keeping String params here is good w me, that makes sense.

I’d be interested to hear what you find if you look into the error mapping. If it’s an easy fix, that would be great, but if it's not then I am def good with merging the pr as is.

Good work on this overall, its has some tricky bits for sure.

[ItoroD]

I have looked at the Error variant we are using again and I come to the conclusion that DescriptorError::Key might be the best to use that case. It maps to KeyError in bdk-wallet and it seems to have a wide range of error variants.

[reez]

Should we assert the resulting descriptor string or descType() here? I think this only checks that construction succeeds so it I dont know if it would catch a wrapper bug that returns the wrong descriptor variant

[ItoroD]

Yes good call. Will update

[reez]

ACK ec37928