LtbLightning / bdk-flutter

Bitcoin Development Kit - Flutter Package
MIT License
60 stars 27 forks source link

BdkFlutter.createWallet will create incorrect default wallet if only mnemonic is provided #34

Closed thunderbiscuit closed 1 year ago

thunderbiscuit commented 1 year ago

If I understand correctly the API, if the developer only passes a mnemonic to the createWallet() method the descriptor created will use the pk() function (type: Descriptor.P2PK) with a default descriptor path for a testnet BIP84 wallet ("m/84'/1'/0'/0").

2 things to note:

  1. This will create an non-standard descriptor, mixing the path for a BIP84 testnet with a pk() function (should be wpkh())
  2. If the user provides a network that is not testnet (say mainnet), the descriptor will also be wrong because it will use mainnet but with a derivation path for testnet (node 3 in the derivation path should be set to 0).

var key = await createDescriptors(
    network: network??DEFAULT_NETWORK,
    mnemonic: mnemonic.toString(),
    password: password,
    type: Descriptor.P2PK,
    descriptorPath: DEFAUTLT_DESCRIPTOR_PATH,
    changeDescriptorPath: DEFAUTLT_CHANGE_DESCRIPTOR_PATH
);
thunderbiscuit commented 1 year ago

There is a lot going on here, and I don't pretend to know exactly the perfect API, but here are some thoughts:

  1. The default network coupled with the default path creates problems because it means you can create provide one and it won't match what the other is set to. A short-term first stab at the issue would be to require the network and not allow a default one. That forces the developer to be explicit and know what they build the wallet for (also help reviewers).
  2. The bitcoindevkit has always been about using descriptors and enforcing that. Why not enforce it here as well? You could have helper functions that help developers build descriptors from their mnemonic, but the BdkFlutter.createWallet() could take descriptors only. That way you ensure by the time they create the wallet, they're all good to go, decoupling the issue of building correct descriptors from the action of initializing the wallet.
  3. This issue of conflicting defaults with provided inputs also arises in the createDeriveKey() method, where one could pass a network like mainnet but no path, and the API would use the default path (which is a testnet path). Better to force devs to provide a path, or in the case of templates to create the path based on the network and BIP provided.
BitcoinZavior commented 1 year ago

Thanks @thunderbiscuit I think the best will be to align the wallet creation interface to what bdk-ffi currently provides.