bdk icon indicating copy to clipboard operation
bdk copied to clipboard

Slight rethinking of `Wallet` constructors (breaking change)

Open thunderbiscuit opened this issue 1 year ago • 0 comments

Why This PR

Our current "constructors" on the Wallet type are as follow:

1. Wallet::new()
2. Wallet::new_with_genesis_hash()
3. Wallet::load_from_changeset()
4. Wallet::new_or_load()
5. Wallet::new_or_load_with_genesis_hash()

This PR proposes a revised set of constructors offering clearer boundaries between them and their intended use:

1. Wallet::new()
2. Wallet::new_with_genesis_hash()
3. Wallet::load()
4. Wallet::load_from_changeset() -> renamed load_no_priv()

A few observations on our current set of constructors and what led me to propose this PR:

  • Wallet.new_or_load(). So... do you want to create a new wallet or load an existing one? It feels like you should know. If you don't know whether you're about to create a new wallet or load one from persistence, to me your issue lies upstream of this line in your code (but maybe there is a use case I'm not thinking of? let me know).
  • The new_or_load_with_genesis_hash() is sort of named backwards (you don't load with a specific genesis hash, you can only do that on a new wallet). The clearer name for it would be new_with_genesis_hash_or_load().
  • Loading a wallet is always done with a changeset; there is no other way to do it. In this sense, Wallet.load_from_changeset() is not optimally named. In fact, the name is also missing the core feature of the constructor: your wallet will not have private keys. An extra commit on this PR renames this method to load_no_priv(); feel free to bikeshed on the name I don't know that it's a particular good one yet and we can also just take that commit out if people like the load_from_changeset name.

Note that this PR doesn't touch the inner logic of the constructors. I simply renamed the methods, removed the Option on the changeset when calling the load methods, and fixed up the tests to break apart the behaviour tested. It did require removing the NewOrLoadError and adding the required variants to the LoadError.

Changelog notice

Breaking
    - The Wallet.new_or_load() method was removed [#500]
    - The Wallet.load_from_changeset() method was renamed Wallet.load_no_priv() [#500]
 
Added
    - A new method Wallet.load() was added to construct wallets [#500]

[#500]: https://github.com/bitcoindevkit/bdk/pull/1500

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:

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

thunderbiscuit avatar Jul 03 '24 16:07 thunderbiscuit