bdk icon indicating copy to clipboard operation
bdk copied to clipboard

Multi Descriptor Wallet API [MVP Refactoring]

Open rajarshimaitra opened this issue 1 year ago • 11 comments

Description

Fixes #486 .

This describes the minimal viable changes to make the wallet APIs generic on K. This gives us a good starting point on to base the remaining questions of multi-desc wallet designs.

The commits are divided into logical chunks. I couldn't separate the major wallet module refactoring commit easily, so that's still a bit pain to review. Following are the commit summaries; more details are added in the commit message.

  • https://github.com/bitcoindevkit/bdk/pull/966/commits/7aefa75bc9403c4947d3b78c8dc90049b550ca27: Removes keychain info from LocalUTXO as it's redundant and forces us to mark LocalUTXO and all other derived structure generic on K.

  • https://github.com/bitcoindevkit/bdk/pull/966/commits/aba7b96baaa0decd01a4e69ea8a64cc500c03d23: Modifies the TxBuilder to only work on the default wallet. Because we don't have a suitable TxParam that can satisfy multi-desc requirements, we keep the TxBuilder default wallet only. This maintains all the previous API and behaviors of TxBuilder. Better multi-desc support for TxBuilder and TxParams can be done on top of this in a future PR.

  • https://github.com/bitcoindevkit/bdk/pull/966/commits/ce6ee5c3d94d01d908aada8dcb0825244518805d: The Major refactoring change. This changes the wallet API to work with generic K. Creates a default wallet to work with KeychainKind. And adds the following new multi-desc APIs,

    • new_multi_descriptor() : create a multi descriptor wallet.
    • add_descriptor() : Adds a descriptor and marks it with a K.
    • get_address_by_keychain (): Get an address for a given keychain.
    • list_utxos_by_keychain (): does what it says.
    • list_transactions_by_keychain() : ''
    • get_balance_by_keychain() : Get a balance for one keychain. The unconfirmed balance is reported as trusted.
    • get_balance_for_keychains() : Get accumulated balance for a list of keychains. uses should_trust predicate to determine "untrusted" keychains.
    • create_tx_multi() : The multi-descriptor create transaction API.

The rest of the commits are self-explanatory.

Notes to Reviewers

The Balance type is moved out from bdk_chain::keychain module into bdk::types. This is to solve the problem described here https://github.com/bitcoindevkit/bdk/pull/966#issuecomment-1536043323. It feels easier to not burden bdk_chain to hand us out the balances. It already gives us all the data we need to calculate the balance, so bdk_chain can be less opinionated about balance calculations and we can handle the required get_balance_* functions at the wallet layer.

The create_tx_multi API takes in a list of keychains to spend from. And a list of change keychains to generate change addresses from. For change_keychains is an iterator instead of a single keychain because later, we wanna add the recipient address matching capability of the change address. There could be other ways of doing it, but as it's the simplest of all the floating solutions, kept the API accordingly. Currently, multiple keychains do nothing, and a single one is selected at random to generate change. This will be handled in a separate PR.

TODO

Tests for basic multi-desc APIs. The newly added APIs and their behaviors should be asserted.

Checklists

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:

  • [ ] I've added tests for the new feature
  • [x] I've added docs for the new feature

rajarshimaitra avatar May 04 '23 14:05 rajarshimaitra