createWallet failing with only descriptor and no changeDescriptor

[MrUnfunny]

Hi @BitcoinZavior, Kudos for the new changes to the package. Everything I tested seems to work fine so far. Just having an issue with creating wallet.

If we try to create wallet as follows:

  await wallet1.createWallet(
    network: Network.TESTNET,
    descriptor: descriptor,
    blockchainConfig: electrumConfig,
  );

It throws this error: Unhandled Exception: WalletException.unexpected(e: called `Result::unwrap()` on an `Err` value: Descriptor(Miniscript(Unexpected("null(0 args) while parsing Miniscript")))). I don't think this is intended behaviour.

This is happening due to this line. Here we should check if changeDescriptor or descriptor is not null or empty before calling toString().

[BitcoinZavior]

Yes, we can add a condition there to have the change descriptor as optional. btw have you seen this method, it creates both descriptor and change descriptor: https://github.com/LtbLightning/bdk-flutter#createdescriptor

[MrUnfunny]

Yes, I saw createDescriptor and it's working perfectly fine but I feel changeDescriptor should not be mandatory for creating wallet(Please correct me if I'm wrong).

[ConorOkus]

I'd double check you are able to sync and get the correct balance when "restoring" a wallet if no change descriptor is provided. I don't think that will work.

[MrUnfunny]

@ConorOkus yes. To recover a wallet that was previously using a changeDescriptor, we will need to provide it here as well to get all funds but I'm not sure of the case where the wallet never had a changeDescriptor previously. I think these nuisances must be managed by the developer and should probably not be enforced. Please correct me if I'm wrong.

[BitcoinZavior]

Currently, if createWallet is called with a mnemonic and no path is provided then it uses the default path. When restoring if the user passes a mnemonic and no path then the same default will be used.

When creating a descriptor the same logic applies, when no path is specified when creating a descriptor a default path is used.

One additional logic that should be added is to derive a changeDescriptor from the descriptor if it's not provided. So in this scenario, if no changeDescriptor was provided to create the wallet then the wallet will still restore correctly when no path is provided when restoring.

One useful change that would help is to have an option to specify a descriptor and change the descriptor individually when creating a wallet. Currently, createDescriptor creates and returns both descriptor and changeDescriptor. Create wallet also expects descriptor and changeDescriptor inside the descriptor object. Which I think is not intuitive.

[MrUnfunny]

Yes, currently the createWallet is somewhat confusing as it does not enforce passing correct arguments. For instance, we can pass both mnemonic and descriptor or neither of them. These will be caught at compile time or run time but still it is a little confusing.

I would suggest to create different factory constructors like createWalletFromMnemonic, createWalletFromDescriptor for creating from descriptor only, createWalletFromPathDescriptor for creating from both descriptor and changeDescriptor etc. This way the API will be lot less confusing and we can avoid so many runtime errors.

[BitcoinZavior]

Yes thats one way to do it, I think the readme can do with a bit of improvement too, the readme for bdk-rn is easier to understand: https://github.com/LtbLightning/bdk-rn#createwallet