Merge bitcoindevkit#1537: Use explicit names for wallet builder metho…

…ds that validate rather than set

2391b76 refactor(wallet)!: rename LoadParams methods (thunderbiscuit)

Pull request description:

  This PR is a follow up to the dev call discussion where we decided it was better to PR these changes separate from bitcoindevkit#1533. This is a breaking change, but I think it's worth it because those will potentially cause runtime errors for users that expect one thing to happen and realize it's something else.

  Left out of this PR but as surfaced in the call probably worth discussing is whether these methods make sense at all or whether they should be removed entirely. What does it mean to return an error when someone loads a wallet from persistence for which the genesis hash doesn't match the one persisted? Maybe worth a new issue; this PR simply attempts to name them correctly.

  ### Description
  See [Q1 in comment here](bitcoindevkit#1533 (comment)) for more context into the initial question.

  Two of the methods on the builder that loads a wallet were checkers/validators rather than setters:
  - `network()`
  - `genesis_hash()`

  This is confusing for users because when loading a wallet from persistence those things are persisted and cannot be changed, and yet seemingly the option to do that is there with those methods (so now you're re-thinking what you think you know about the changeset and persistence). Moreover, the fields on the [`LoadParams` type](https://docs.rs/bdk_wallet/1.0.0-beta.1/src/bdk_wallet/wallet/params.rs.html#116-124) are correctly named `check_network` and `check_genesis_hash`. This PR simply brings those two methods in line with what they actually do and set on the struct they modify.

  This modification was not done on the `descriptors()` method, because that one is both a validator and a setter if the descriptors passed contain private keys.

  Note that I chose the names `validate_network` and `validate_genesis_hash` but I'm not married to them. If others prefer `check_network` and `check_genesis_hash`, I'm happy to fix them that way instead!

  ### Changelog notice

  ```md
  Breaking:
    - The `LoadParams` type used in the wallet load builder renamed its  `network()` and `genesis_hash` methods to `check_network()` and `check_genesis_hash`. [bitcoindevkit#1537]

  [bitcoindevkit#1537]: bitcoindevkit#1537
  ```

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

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

  #### Bugfixes:

  * [x] This pull request breaks the existing API
  * [ ] I've added tests to reproduce the issue which are now passing
  * [ ] I'm linking the issue being fixed by this PR

ACKs for top commit:
  notmandatory:
    ACK 2391b76

Tree-SHA512: 6852ad165bab230a003b92ae0408176055f8c9101a0936d2d5a41c2a3577e258b045a7f4b796d9bab29ed261caf80b738c4338d4ff9322fbddc8c273ab0ff914

crates/wallet/README.md

@@ -79,10 +79,10 @@ let network = Network::Testnet;
 let descriptor = "wpkh(tprv8ZgxMBicQKsPdcAqYBpzAFwU5yxBUo88ggoBqu1qPcHUfSbKK1sKMLmC7EAk438btHQrSdu3jGGQa6PA71nvH5nkDexhLteJqkM4dQmWF9g/84'/1'/0'/0/*)";
 let change_descriptor = "wpkh(tprv8ZgxMBicQKsPdcAqYBpzAFwU5yxBUo88ggoBqu1qPcHUfSbKK1sKMLmC7EAk438btHQrSdu3jGGQa6PA71nvH5nkDexhLteJqkM4dQmWF9g/84'/1'/0'/1/*)";
 let wallet_opt = Wallet::load()
-    .network(network)
     .descriptor(KeychainKind::External, Some(descriptor))
     .descriptor(KeychainKind::Internal, Some(change_descriptor))
     .extract_keys()
+    .check_network(network)
     .load_wallet(&mut db)
     .expect("wallet");
 let mut wallet = match wallet_opt {

crates/wallet/src/wallet/mod.rs

@@ -482,7 +482,7 @@ impl Wallet {
     ///     .keymap(KeychainKind::External, external_keymap)
     ///     .keymap(KeychainKind::Internal, internal_keymap)
     ///     // ensure loaded wallet's genesis hash matches this value
-    ///     .genesis_hash(genesis_hash)
+    ///     .check_genesis_hash(genesis_hash)
     ///     // set a lookahead for our indexer
     ///     .lookahead(101)
     ///     .load_wallet(&mut conn)?

crates/wallet/src/wallet/params.rs

@@ -194,14 +194,14 @@ impl LoadParams {
         self
     }
 
-    /// Check for `network`.
-    pub fn network(mut self, network: Network) -> Self {
+    /// Checks that the given network matches the one loaded from persistence.
+    pub fn check_network(mut self, network: Network) -> Self {
         self.check_network = Some(network);
         self
     }
 
-    /// Check for a `genesis_hash`.
-    pub fn genesis_hash(mut self, genesis_hash: BlockHash) -> Self {
+    /// Checks that the given `genesis_hash` matches the one loaded from persistence.
+    pub fn check_genesis_hash(mut self, genesis_hash: BlockHash) -> Self {
         self.check_genesis_hash = Some(genesis_hash);
         self
     }

crates/wallet/tests/wallet.rs

@@ -137,7 +137,7 @@ fn wallet_is_persisted() -> anyhow::Result<()> {
         {
             let mut db = open_db(&file_path).context("failed to recover db")?;
             let _ = Wallet::load()
-                .network(Network::Testnet)
+                .check_network(Network::Testnet)
                 .load_wallet(&mut db)?
                 .expect("wallet must exist");
         }
@@ -146,7 +146,7 @@ fn wallet_is_persisted() -> anyhow::Result<()> {
             let wallet = Wallet::load()
                 .descriptor(KeychainKind::External, Some(external_desc))
                 .descriptor(KeychainKind::Internal, Some(internal_desc))
-                .network(Network::Testnet)
+                .check_network(Network::Testnet)
                 .load_wallet(&mut db)?
                 .expect("wallet must exist");
 
@@ -218,7 +218,7 @@ fn wallet_load_checks() -> anyhow::Result<()> {
 
         assert_matches!(
             Wallet::load()
-                .network(Network::Regtest)
+                .check_network(Network::Regtest)
                 .load_wallet(&mut open_db(&file_path)?),
             Err(LoadWithPersistError::InvalidChangeSet(LoadError::Mismatch(
                 LoadMismatch::Network {
@@ -228,21 +228,9 @@ fn wallet_load_checks() -> anyhow::Result<()> {
             ))),
             "unexpected network check result: Regtest (check) is not Testnet (loaded)",
         );
-        assert_matches!(
-            Wallet::load()
-                .network(Network::Bitcoin)
-                .load_wallet(&mut open_db(&file_path)?),
-            Err(LoadWithPersistError::InvalidChangeSet(LoadError::Mismatch(
-                LoadMismatch::Network {
-                    loaded: Network::Testnet,
-                    expected: Network::Bitcoin,
-                }
-            ))),
-            "unexpected network check result: Bitcoin (check) is not Testnet (loaded)",
-        );
         let mainnet_hash = BlockHash::from_byte_array(ChainHash::BITCOIN.to_bytes());
         assert_matches!(
-            Wallet::load().genesis_hash(mainnet_hash).load_wallet(&mut open_db(&file_path)?),
+            Wallet::load().check_genesis_hash(mainnet_hash).load_wallet(&mut open_db(&file_path)?),
             Err(LoadWithPersistError::InvalidChangeSet(LoadError::Mismatch(LoadMismatch::Genesis { .. }))),
             "unexpected genesis hash check result: mainnet hash (check) is not testnet hash (loaded)",
         );

example-crates/wallet_electrum/src/main.rs

@@ -26,10 +26,10 @@ fn main() -> Result<(), anyhow::Error> {
     let mut db = Store::<bdk_wallet::ChangeSet>::open_or_create_new(DB_MAGIC.as_bytes(), db_path)?;
 
     let wallet_opt = Wallet::load()
-        .network(NETWORK)
         .descriptor(KeychainKind::External, Some(EXTERNAL_DESC))
         .descriptor(KeychainKind::Internal, Some(INTERNAL_DESC))
         .extract_keys()
+        .check_network(NETWORK)
         .load_wallet(&mut db)?;
     let mut wallet = match wallet_opt {
         Some(wallet) => wallet,

example-crates/wallet_esplora_async/src/main.rs

@@ -23,10 +23,10 @@ async fn main() -> Result<(), anyhow::Error> {
     let mut conn = Connection::open(DB_PATH)?;
 
     let wallet_opt = Wallet::load()
-        .network(NETWORK)
         .descriptor(KeychainKind::External, Some(EXTERNAL_DESC))
         .descriptor(KeychainKind::Internal, Some(INTERNAL_DESC))
         .extract_keys()
+        .check_network(NETWORK)
         .load_wallet(&mut conn)?;
     let mut wallet = match wallet_opt {
         Some(wallet) => wallet,

example-crates/wallet_esplora_blocking/src/main.rs

@@ -22,10 +22,10 @@ fn main() -> Result<(), anyhow::Error> {
     let mut db = Store::<bdk_wallet::ChangeSet>::open_or_create_new(DB_MAGIC.as_bytes(), DB_PATH)?;
 
     let wallet_opt = Wallet::load()
-        .network(NETWORK)
         .descriptor(KeychainKind::External, Some(EXTERNAL_DESC))
         .descriptor(KeychainKind::Internal, Some(INTERNAL_DESC))
         .extract_keys()
+        .check_network(NETWORK)
         .load_wallet(&mut db)?;
     let mut wallet = match wallet_opt {
         Some(wallet) => wallet,

example-crates/wallet_rpc/src/main.rs

@@ -89,10 +89,10 @@ fn main() -> anyhow::Result<()> {
     let mut db =
         Store::<bdk_wallet::ChangeSet>::open_or_create_new(DB_MAGIC.as_bytes(), args.db_path)?;
     let wallet_opt = Wallet::load()
-        .network(args.network)
         .descriptor(KeychainKind::External, Some(args.descriptor.clone()))
         .descriptor(KeychainKind::Internal, Some(args.change_descriptor.clone()))
         .extract_keys()
+        .check_network(args.network)
         .load_wallet(&mut db)?;
     let mut wallet = match wallet_opt {
         Some(wallet) => wallet,