From b9079ee6f2e56518bcf1634b70e76cb2bcef4f42 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sat, 18 Apr 2026 04:52:18 +0700 Subject: [PATCH 1/2] refactor(key-wallet): make WatchOnly / ExternalSignable unit variants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The RootExtendedPubKey carried by `WalletType::WatchOnly` and `WalletType::ExternalSignable` served no functional purpose: - Every Dash derivation path (BIP44, DIP-9 identity, DIP-15 DashPay, DIP-17 platform payment) hits a hardened level before the account index, so a host-side root xpub cannot be used to expand account coverage. Per-account xpubs in `AccountCollection` are the only state needed for tracking. - `Wallet.wallet_id` already holds the 32-byte identity. - External-signable wallets route signing to the external device via derivation paths; the host never needs the root pub key. Drop the payload from both variants and add matching constructors `Wallet::new_watch_only` / `Wallet::new_external_signable` that take (network, wallet_id, accounts). `Wallet::from_xpub` / `from_external_signable` stay as thin adapters that hash the xpub into a wallet id and delegate to the new constructors. `root_extended_pub_key` / `root_extended_pub_key_cow` are now fallible — they return `Err` for the two unit variants instead of fabricating a value. `compute_wallet_id` returns `self.wallet_id` directly for those variants. `MnemonicWithPassphrase` is unchanged (its `root_extended_public_key` is the post-passphrase identity and genuinely distinguishes chains). Breaking: the bincode / serde wire format for `WatchOnly` and `ExternalSignable` changes, and any caller that relied on the previously-infallible pub-key getters now needs `?`/`unwrap`. Covered by a new `tests/unit_variant_wallet_tests.rs` module exercising constructors, helper predicates, error-path shape, address parity after round-trip, and bincode round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) --- key-wallet-manager/src/lib.rs | 29 ++- key-wallet/src/tests/mod.rs | 2 + .../src/tests/unit_variant_wallet_tests.rs | 166 ++++++++++++++++++ key-wallet/src/tests/wallet_tests.rs | 31 ++-- key-wallet/src/wallet/helper.rs | 48 ++--- key-wallet/src/wallet/initialization.rs | 162 ++++++++++------- .../managed_wallet_info/managed_accounts.rs | 9 +- key-wallet/src/wallet/mod.rs | 67 ++++--- key-wallet/src/wallet/root_extended_keys.rs | 59 ++++--- key-wallet/src/wallet_comprehensive_tests.rs | 17 +- 10 files changed, 399 insertions(+), 191 deletions(-) create mode 100644 key-wallet/src/tests/unit_variant_wallet_tests.rs diff --git a/key-wallet-manager/src/lib.rs b/key-wallet-manager/src/lib.rs index 62fcca997..ccb293edd 100644 --- a/key-wallet-manager/src/lib.rs +++ b/key-wallet-manager/src/lib.rs @@ -212,7 +212,6 @@ impl WalletManager { downgrade_to_pubkey_wallet: bool, allow_external_signing: bool, ) -> Result<(Vec, WalletId), WalletError> { - use key_wallet::wallet::WalletType; use zeroize::Zeroize; let mnemonic_obj = Mnemonic::from_phrase(mnemonic, key_wallet::mnemonic::Language::English) @@ -234,30 +233,22 @@ impl WalletManager { // Downgrade to pubkey-only wallet if requested let final_wallet = if downgrade_to_pubkey_wallet { - // Extract the public key and accounts from the full wallet - let root_xpub = wallet.root_extended_pub_key(); - - // Copy the accounts structure (but without private keys) + // Carry over the wallet id and accounts (watch-only variants do not + // need the root xpub — per-account xpubs in `accounts` plus derivation + // paths are enough for address generation and signing routing). + let wallet_id = wallet.wallet_id; let accounts = wallet.accounts.clone(); - - let wallet_type = if allow_external_signing { - WalletType::ExternalSignable(root_xpub) - } else { - WalletType::WatchOnly(root_xpub) - }; - // Create a new wallet with only public keys - let pubkey_wallet = Wallet { - network: wallet.network, - wallet_id: wallet.wallet_id, - wallet_type, - accounts, - }; + let network = wallet.network; // Zeroize the wallet containing private keys before dropping wallet.zeroize(); drop(wallet); - pubkey_wallet + if allow_external_signing { + Wallet::new_external_signable(network, wallet_id, accounts) + } else { + Wallet::new_watch_only(network, wallet_id, accounts) + } } else { wallet }; diff --git a/key-wallet/src/tests/mod.rs b/key-wallet/src/tests/mod.rs index 811240960..d92850c79 100644 --- a/key-wallet/src/tests/mod.rs +++ b/key-wallet/src/tests/mod.rs @@ -26,4 +26,6 @@ mod transaction_tests; mod spent_outpoints_tests; +mod unit_variant_wallet_tests; + mod wallet_tests; diff --git a/key-wallet/src/tests/unit_variant_wallet_tests.rs b/key-wallet/src/tests/unit_variant_wallet_tests.rs new file mode 100644 index 000000000..23a540e71 --- /dev/null +++ b/key-wallet/src/tests/unit_variant_wallet_tests.rs @@ -0,0 +1,166 @@ +//! Tests for the `WalletType::WatchOnly` and `WalletType::ExternalSignable` +//! unit variants + their `new_*` constructors. +//! +//! These variants carry no root key material. Identity lives on `Wallet.wallet_id` +//! and per-account xpubs live in `AccountCollection`; nothing else is needed for +//! the host to track addresses or route signing requests to an external device. + +use crate::account::account_collection::AccountCollection; +use crate::mnemonic::{Language, Mnemonic}; +use crate::wallet::initialization::WalletAccountCreationOptions; +use crate::wallet::{Wallet, WalletType}; +use crate::Network; + +const TEST_MNEMONIC: &str = + "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; + +fn built_full_wallet() -> Wallet { + let mnemonic = Mnemonic::from_phrase(TEST_MNEMONIC, Language::English).unwrap(); + Wallet::from_mnemonic(mnemonic, Network::Testnet, WalletAccountCreationOptions::Default) + .unwrap() +} + +// -------- constructors -------------------------------------------------- + +#[test] +fn new_watch_only_sets_unit_variant_and_preserves_inputs() { + let full = built_full_wallet(); + let accounts = full.accounts.clone(); + + let watch = Wallet::new_watch_only(Network::Testnet, full.wallet_id, accounts); + + assert!(matches!(watch.wallet_type, WalletType::WatchOnly)); + assert!(watch.is_watch_only()); + assert!(!watch.can_sign()); + assert!(!watch.is_external_signable()); + assert!(!watch.has_mnemonic()); + assert!(!watch.has_seed()); + assert_eq!(watch.wallet_id, full.wallet_id); + assert_eq!(watch.compute_wallet_id(), full.wallet_id); + assert_eq!(watch.network, Network::Testnet); + // compute_wallet_id must not re-derive from a fabricated pub key. + // Changing wallet_id without touching accounts must change compute_wallet_id. + let mut tampered = watch.clone(); + tampered.wallet_id = [9u8; 32]; + assert_eq!(tampered.compute_wallet_id(), [9u8; 32]); +} + +#[test] +fn new_external_signable_sets_unit_variant_and_can_sign_externally() { + let full = built_full_wallet(); + let accounts = full.accounts.clone(); + + let ext = Wallet::new_external_signable(Network::Testnet, full.wallet_id, accounts); + + assert!(matches!(ext.wallet_type, WalletType::ExternalSignable)); + assert!(ext.is_external_signable()); + // can_sign reports true for external-signable wallets: signing is routed to + // the external device, not blocked locally. + assert!(ext.can_sign()); + assert!(!ext.is_watch_only()); + assert!(!ext.has_mnemonic()); + assert_eq!(ext.wallet_id, full.wallet_id); + assert_eq!(ext.compute_wallet_id(), full.wallet_id); +} + +// -------- root_extended_pub_key* unavailable --------------------------- + +#[test] +fn root_extended_pub_key_is_err_for_unit_variants() { + let full = built_full_wallet(); + + let watch = Wallet::new_watch_only(Network::Testnet, full.wallet_id, AccountCollection::new()); + let ext = + Wallet::new_external_signable(Network::Testnet, full.wallet_id, AccountCollection::new()); + + assert!(watch.root_extended_pub_key().is_err()); + assert!(watch.root_extended_pub_key_cow().is_err()); + assert!(ext.root_extended_pub_key().is_err()); + assert!(ext.root_extended_pub_key_cow().is_err()); +} + +// -------- signing-path errors shaped the same -------------------------- + +#[test] +fn signing_paths_report_cannot_sign_for_unit_variants() { + use crate::DerivationPath; + + let full = built_full_wallet(); + let watch = Wallet::new_watch_only(Network::Testnet, full.wallet_id, AccountCollection::new()); + let ext = + Wallet::new_external_signable(Network::Testnet, full.wallet_id, AccountCollection::new()); + + let path: DerivationPath = "m/44'/1'/0'/0/0".parse().unwrap(); + + // derive_private_key / derive_extended_private_key go through the same + // "no private key" error path for both unit variants. + assert!(watch.derive_private_key(&path).is_err()); + assert!(watch.derive_extended_private_key(&path).is_err()); + assert!(watch.derive_private_key_as_wif(&path).is_err()); + assert!(ext.derive_private_key(&path).is_err()); + assert!(ext.derive_extended_private_key(&path).is_err()); +} + +// -------- round-trip: full wallet -> watch-only preserves addresses ---- + +#[test] +fn round_trip_full_to_watch_only_has_address_parity_per_account() { + let full = built_full_wallet(); + let wallet_id = full.wallet_id; + let accounts_snapshot = full.accounts.clone(); + + let watch = Wallet::new_watch_only(Network::Testnet, wallet_id, accounts_snapshot); + + // Every account's extended public key — and therefore every address it + // will ever generate — must match its full-wallet counterpart. + assert_eq!(watch.accounts.count(), full.accounts.count()); + + for full_acct in full.all_accounts() { + let matched = watch + .all_accounts() + .into_iter() + .find(|w| w.account_type == full_acct.account_type) + .expect("every full-wallet account must appear in the watch-only snapshot"); + assert_eq!( + matched.extended_public_key(), + full_acct.extended_public_key(), + "account xpub mismatch for {:?}", + full_acct.account_type + ); + } +} + +// -------- bincode round-trip ------------------------------------------- + +#[test] +fn bincode_round_trip_watch_only() { + let full = built_full_wallet(); + let original = Wallet::new_watch_only(Network::Testnet, full.wallet_id, full.accounts.clone()); + + let bytes = bincode::encode_to_vec(&original, bincode::config::standard()).expect("encode"); + let (decoded, _): (Wallet, usize) = + bincode::decode_from_slice(&bytes, bincode::config::standard()).expect("decode"); + + assert!(matches!(decoded.wallet_type, WalletType::WatchOnly)); + assert!(decoded.is_watch_only()); + assert_eq!(decoded.wallet_id, original.wallet_id); + assert_eq!(decoded.network, original.network); + assert_eq!(decoded.accounts.count(), original.accounts.count()); +} + +#[test] +fn bincode_round_trip_external_signable() { + let full = built_full_wallet(); + let original = + Wallet::new_external_signable(Network::Testnet, full.wallet_id, full.accounts.clone()); + + let bytes = bincode::encode_to_vec(&original, bincode::config::standard()).expect("encode"); + let (decoded, _): (Wallet, usize) = + bincode::decode_from_slice(&bytes, bincode::config::standard()).expect("decode"); + + assert!(matches!(decoded.wallet_type, WalletType::ExternalSignable)); + assert!(decoded.is_external_signable()); + assert_eq!(decoded.wallet_id, original.wallet_id); + assert_eq!(decoded.network, original.network); + assert_eq!(decoded.accounts.count(), original.accounts.count()); +} diff --git a/key-wallet/src/tests/wallet_tests.rs b/key-wallet/src/tests/wallet_tests.rs index 862154800..3c4546eda 100644 --- a/key-wallet/src/tests/wallet_tests.rs +++ b/key-wallet/src/tests/wallet_tests.rs @@ -136,14 +136,16 @@ fn test_wallet_creation_watch_only() { assert!(!wallet.has_mnemonic()); assert!(!wallet.is_external_signable()); - // Verify public key is stored - match &wallet.wallet_type { - WalletType::WatchOnly(_) => { - // Check that it's a watch-only wallet type - assert!(wallet.is_watch_only()); - } - _ => panic!("Expected watch-only wallet type"), - } + // The unit variant carries no key material on the WalletType side + assert!(matches!(wallet.wallet_type, WalletType::WatchOnly)); + assert!(wallet.is_watch_only()); + assert!(wallet.root_extended_pub_key().is_err()); + + // But the wallet id must still equal the hash of the source root xpub + // (`from_xpub` derives the id from the xpub at construction time). + let expected_id = Wallet::compute_wallet_id_from_root_extended_pub_key(&root_pub_key); + assert_eq!(wallet.wallet_id, expected_id); + assert_eq!(wallet.compute_wallet_id(), expected_id); } #[test] @@ -400,10 +402,11 @@ fn test_wallet_external_signable() { assert!(wallet.can_sign()); // Can sign with external signer assert!(!wallet.is_watch_only()); // Not purely watch-only - match &wallet.wallet_type { - WalletType::ExternalSignable(key) => { - assert_eq!(key.root_public_key, root_pub_key.root_public_key); - } - _ => panic!("Expected external signable wallet type"), - } + // Unit variant carries no root key material; the wallet id is the hash + // of the root xpub fed into `from_external_signable`. + assert!(matches!(wallet.wallet_type, WalletType::ExternalSignable)); + assert!(wallet.root_extended_pub_key().is_err()); + let expected_id = Wallet::compute_wallet_id_from_root_extended_pub_key(&root_pub_key); + assert_eq!(wallet.wallet_id, expected_id); + assert_eq!(wallet.compute_wallet_id(), expected_id); } diff --git a/key-wallet/src/wallet/helper.rs b/key-wallet/src/wallet/helper.rs index a97b8dfb8..2eeb41a9b 100644 --- a/key-wallet/src/wallet/helper.rs +++ b/key-wallet/src/wallet/helper.rs @@ -3,7 +3,6 @@ //! This module contains helper methods and utility functions for wallets. use super::initialization::WalletAccountCreationOptions; -use super::root_extended_keys::RootExtendedPrivKey; use super::{Wallet, WalletType}; use crate::account::{Account, AccountType, StandardAccountType}; use crate::error::Result; @@ -58,32 +57,16 @@ impl Wallet { indices } - /// Export wallet as watch-only + /// Export wallet as watch-only. + /// + /// Preserves the wallet id so the downgraded copy stays addressable by the + /// same identifier, and replaces every account with its watch-only + /// counterpart. The resulting wallet carries no key material on the + /// [`WalletType`] side — per-account xpubs inside `accounts` are the only + /// state needed for tracking. pub fn to_watch_only(&self) -> Self { let mut watch_only = self.clone(); - - // Get the root public key - let root_pub_key = if let Ok(root_key) = self.root_extended_priv_key() { - root_key.to_root_extended_pub_key() - } else { - // For already watch-only wallets, keep the existing public key - match &self.wallet_type { - WalletType::WatchOnly(pub_key) | WalletType::ExternalSignable(pub_key) => { - pub_key.clone() - } - WalletType::MnemonicWithPassphrase { - root_extended_public_key, - .. - } => root_extended_public_key.clone(), - _ => { - // Fallback - create a dummy key - let dummy_priv = RootExtendedPrivKey::new_master(&[0u8; 64]).unwrap(); - dummy_priv.to_root_extended_pub_key() - } - } - }; - - watch_only.wallet_type = WalletType::WatchOnly(root_pub_key); + watch_only.wallet_type = WalletType::WatchOnly; // Convert all accounts to watch-only for account in watch_only.accounts.all_accounts_mut() { @@ -103,17 +86,17 @@ impl Wallet { /// Check if wallet is watch-only pub fn is_watch_only(&self) -> bool { - matches!(self.wallet_type, WalletType::WatchOnly(_)) + matches!(self.wallet_type, WalletType::WatchOnly) } /// Check if wallet supports external signing pub fn is_external_signable(&self) -> bool { - matches!(self.wallet_type, WalletType::ExternalSignable(_)) + matches!(self.wallet_type, WalletType::ExternalSignable) } /// Check if wallet can sign transactions (has private keys or can get them) pub fn can_sign(&self) -> bool { - !matches!(self.wallet_type, WalletType::WatchOnly(_)) + !matches!(self.wallet_type, WalletType::WatchOnly) } /// Check if wallet needs a passphrase for signing @@ -634,7 +617,7 @@ impl Wallet { .. } => root_extended_private_key.to_extended_priv_key(self.network), WalletType::ExtendedPrivKey(root_priv) => root_priv.to_extended_priv_key(self.network), - WalletType::ExternalSignable(_) | WalletType::WatchOnly(_) => { + WalletType::ExternalSignable | WalletType::WatchOnly => { return Err(Error::InvalidParameter( "Cannot derive private keys from watch-only wallet".to_string(), )); @@ -736,9 +719,12 @@ impl Wallet { let secp = Secp256k1::new(); Ok(ExtendedPubKey::from_priv(&secp, &extended_private)) } else { - // For non-hardened paths, derive directly from public key + // For non-hardened paths, derive directly from the root public key. + // For watch-only / external-signable unit variants there is no root + // key on hand, so this propagates the "root pub key unavailable" error + // from `root_extended_pub_key`. let secp = Secp256k1::new(); - let xpub = self.root_extended_pub_key().to_extended_pub_key(self.network); + let xpub = self.root_extended_pub_key()?.to_extended_pub_key(self.network); xpub.derive_pub(&secp, path).map_err(|e| e.into()) } } diff --git a/key-wallet/src/wallet/initialization.rs b/key-wallet/src/wallet/initialization.rs index efb94b1e9..a46cdc95c 100644 --- a/key-wallet/src/wallet/initialization.rs +++ b/key-wallet/src/wallet/initialization.rs @@ -121,7 +121,16 @@ impl Wallet { Ok(wallet) } - /// Create a wallet from a specific wallet type with no accounts + /// Create a wallet from a signing wallet type with no accounts. + /// + /// This derives the wallet id from the root public key carried by the + /// variant. The [`WalletType::WatchOnly`] and [`WalletType::ExternalSignable`] + /// unit variants have no root key to derive from — use + /// [`Wallet::new_watch_only`] or [`Wallet::new_external_signable`] for those. + /// + /// # Panics + /// Panics if `wallet_type` is `WalletType::WatchOnly` or + /// `WalletType::ExternalSignable`. pub fn from_wallet_type(network: Network, wallet_type: WalletType) -> Self { // Compute wallet ID from root public key let root_pub_key = match &wallet_type { @@ -139,9 +148,14 @@ impl Wallet { WalletType::MnemonicWithPassphrase { root_extended_public_key, .. + } => root_extended_public_key.clone(), + WalletType::ExternalSignable | WalletType::WatchOnly => { + panic!( + "Wallet::from_wallet_type cannot be used with WalletType::WatchOnly or \ + WalletType::ExternalSignable — use Wallet::new_watch_only or \ + Wallet::new_external_signable instead" + ); } - | WalletType::ExternalSignable(root_extended_public_key) - | WalletType::WatchOnly(root_extended_public_key) => root_extended_public_key.clone(), }; let wallet_id = Self::compute_wallet_id_from_root_extended_pub_key(&root_pub_key); @@ -153,6 +167,53 @@ impl Wallet { } } + /// Build a watch-only wallet from its known id + pre-built accounts. + /// + /// Watch-only wallets carry no root key material. Every Dash derivation path + /// (BIP44, DIP-9 identity, DIP-15 DashPay, DIP-17 platform payment) hits a + /// hardened level before the account index, so a host-side root xpub cannot + /// be used to expand account coverage. Supply the accounts you want to track + /// directly via `accounts`. + /// + /// `wallet_id` is the stable identifier for this wallet (typically a hash + /// derived when the wallet was first created, persisted by the caller, and + /// fed back in at restore time). + pub fn new_watch_only( + network: Network, + wallet_id: [u8; 32], + accounts: AccountCollection, + ) -> Self { + Self { + network, + wallet_id, + wallet_type: WalletType::WatchOnly, + accounts, + } + } + + /// Build an external-signable wallet from its known id + pre-built accounts. + /// + /// The external device (hardware wallet, remote signer, …) holds all key + /// material. The host only needs per-account xpubs (for address generation) + /// and derivation paths (to request signatures). Both are carried by + /// `accounts`. + /// + /// `wallet_id` is the stable identifier for this wallet (typically a hash + /// derived when the wallet was first created, persisted by the caller, and + /// fed back in at restore time). + pub fn new_external_signable( + network: Network, + wallet_id: [u8; 32], + accounts: AccountCollection, + ) -> Self { + Self { + network, + wallet_id, + wallet_type: WalletType::ExternalSignable, + accounts, + } + } + /// Create a wallet from a mnemonic phrase /// /// # Arguments @@ -215,91 +276,60 @@ impl Wallet { Ok(wallet) } - /// Create a watch-only or externally signable wallet from extended public key - /// - /// Watch-only wallets can generate addresses and monitor transactions but cannot sign. - /// Externally signable wallets can also create unsigned transactions that can be signed by - /// external devices (hardware wallets, remote signing services, etc.). - /// - /// # Arguments - /// * `master_xpub` - The master extended public key for the wallet - /// * `accounts` - Pre-created account collections. Since watch-only wallets cannot derive - /// private keys, all accounts must be provided with their extended public keys already - /// initialized. - /// * `can_sign_externally` - If true, creates an externally signable wallet that supports - /// transaction creation for external signing. If false, creates a pure watch-only wallet. + /// Create a watch-only or externally signable wallet from an extended public key. /// - /// # Returns - /// A new watch-only or externally signable wallet instance + /// This is a thin adapter that hashes `master_xpub` into a stable wallet id + /// and delegates to [`Wallet::new_watch_only`] or + /// [`Wallet::new_external_signable`]. The xpub itself is **not** retained on + /// the wallet — watch-only and external-signable wallets do not need a root + /// key at rest (see the rationale on [`WalletType::WatchOnly`]). /// - /// # Examples - /// ```ignore - /// // Create a pure watch-only wallet - /// let watch_wallet = Wallet::from_xpub(master_xpub, account_collection, false)?; + /// Prefer the new `new_*` constructors when you already know the wallet id + /// (e.g. restoring from persistence). /// - /// // Create an externally signable wallet (e.g., for hardware wallet) - /// let hw_wallet = Wallet::from_xpub(master_xpub, accounts, true)?; - /// ``` + /// # Arguments + /// * `master_xpub` - The master extended public key. Used only to derive the + /// wallet id; not retained. + /// * `accounts` - Pre-created account collections. Since these wallet types + /// cannot derive private keys, all accounts must be provided with their + /// extended public keys already initialized. + /// * `can_sign_externally` - If true, builds an externally signable wallet + /// (signing delegated to a hardware device or remote signer). If false, + /// builds a pure watch-only wallet. pub fn from_xpub( master_xpub: ExtendedPubKey, accounts: AccountCollection, can_sign_externally: bool, ) -> Result { let root_extended_public_key = RootExtendedPubKey::from_extended_pub_key(&master_xpub); - let wallet_type = if can_sign_externally { - WalletType::ExternalSignable(root_extended_public_key) + let wallet_id = + Self::compute_wallet_id_from_root_extended_pub_key(&root_extended_public_key); + let wallet = if can_sign_externally { + Self::new_external_signable(master_xpub.network, wallet_id, accounts) } else { - WalletType::WatchOnly(root_extended_public_key) + Self::new_watch_only(master_xpub.network, wallet_id, accounts) }; - let mut wallet = Self::from_wallet_type(master_xpub.network, wallet_type); - - wallet.accounts = accounts; - Ok(wallet) } - /// Create an external signable wallet from extended public key + /// Create an external signable wallet from an extended public key. /// - /// External signable wallets support transaction signing through external devices or services. - /// Unlike watch-only wallets which cannot sign at all, these wallets delegate signing to - /// hardware wallets, remote signing services, or other external signing mechanisms. + /// Thin adapter around [`Wallet::new_external_signable`] that derives the + /// wallet id from `master_xpub`. The xpub itself is not retained. /// /// # Arguments - /// * `master_xpub` - The master extended public key from the external signing device. - /// * `accounts` - Pre-created account collections. Since external signable wallets cannot - /// derive private keys, all accounts must be provided with their extended public keys - /// already initialized from the external device. - /// - /// # Returns - /// A new external signable wallet instance that can create transactions but requires - /// the external device/service for signing - /// - /// # Examples - /// ```ignore - /// // Get master xpub from hardware wallet - /// let master_xpub = hardware_wallet.get_master_xpub()?; - /// - /// // Create accounts with xpubs from hardware wallet - /// let accounts = create_accounts_from_hardware_wallet(&hardware_wallet)?; - /// - /// let wallet = Wallet::from_external_signable(master_xpub, accounts)?; - /// - /// // Later, when signing is needed: - /// // let signature = hardware_wallet.sign_transaction(&tx)?; - /// ``` + /// * `master_xpub` - The master extended public key. Used only to derive the + /// wallet id; not retained. + /// * `accounts` - Pre-created account collections with xpubs from the + /// external signing device. pub fn from_external_signable( master_xpub: ExtendedPubKey, accounts: AccountCollection, ) -> Result { let root_extended_public_key = RootExtendedPubKey::from_extended_pub_key(&master_xpub); - let mut wallet = Self::from_wallet_type( - master_xpub.network, - WalletType::ExternalSignable(root_extended_public_key), - ); - - wallet.accounts = accounts; - - Ok(wallet) + let wallet_id = + Self::compute_wallet_id_from_root_extended_pub_key(&root_extended_public_key); + Ok(Self::new_external_signable(master_xpub.network, wallet_id, accounts)) } /// Create a wallet from seed bytes diff --git a/key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs b/key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs index 390d7d400..f21713c1b 100644 --- a/key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs +++ b/key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs @@ -77,14 +77,13 @@ impl ManagedAccountOperations for ManagedWalletInfo { ) -> Result<()> { // Verify this is a passphrase wallet match &wallet.wallet_type { - WalletType::MnemonicWithPassphrase { mnemonic, .. } => { + WalletType::MnemonicWithPassphrase { mnemonic, root_extended_public_key: wallet_pub } => { // Verify the passphrase by deriving and comparing let seed = mnemonic.to_seed(passphrase); let root_key = crate::wallet::root_extended_keys::RootExtendedPrivKey::new_master(&seed)?; // Compare with wallet's stored public key let derived_pub = root_key.to_root_extended_pub_key(); - let wallet_pub = wallet.root_extended_pub_key(); if derived_pub.root_public_key != wallet_pub.root_public_key { return Err(Error::InvalidParameter( @@ -194,14 +193,13 @@ impl ManagedAccountOperations for ManagedWalletInfo { // Verify this is a passphrase wallet match &wallet.wallet_type { - WalletType::MnemonicWithPassphrase { mnemonic, .. } => { + WalletType::MnemonicWithPassphrase { mnemonic, root_extended_public_key: wallet_pub } => { // Verify the passphrase by deriving and comparing let seed = mnemonic.to_seed(passphrase); let root_key = crate::wallet::root_extended_keys::RootExtendedPrivKey::new_master(&seed)?; // Compare with wallet's stored public key let derived_pub = root_key.to_root_extended_pub_key(); - let wallet_pub = wallet.root_extended_pub_key(); if derived_pub.root_public_key != wallet_pub.root_public_key { return Err(Error::InvalidParameter( @@ -313,14 +311,13 @@ impl ManagedAccountOperations for ManagedWalletInfo { // Verify this is a passphrase wallet match &wallet.wallet_type { - WalletType::MnemonicWithPassphrase { mnemonic, .. } => { + WalletType::MnemonicWithPassphrase { mnemonic, root_extended_public_key: wallet_pub } => { // Verify the passphrase by deriving and comparing let seed = mnemonic.to_seed(passphrase); let root_key = crate::wallet::root_extended_keys::RootExtendedPrivKey::new_master(&seed)?; // Compare with wallet's stored public key let derived_pub = root_key.to_root_extended_pub_key(); - let wallet_pub = wallet.root_extended_pub_key(); if derived_pub.root_public_key != wallet_pub.root_public_key { return Err(Error::InvalidParameter( diff --git a/key-wallet/src/wallet/mod.rs b/key-wallet/src/wallet/mod.rs index 7feebd889..91281bfa5 100644 --- a/key-wallet/src/wallet/mod.rs +++ b/key-wallet/src/wallet/mod.rs @@ -53,10 +53,23 @@ pub enum WalletType { }, /// Wallet from extended private key ExtendedPrivKey(RootExtendedPrivKey), - /// External signable wallet with extended public key (signing happens externally) - ExternalSignable(RootExtendedPubKey), - /// Watch-only wallet with extended public key (no signing capability) - WatchOnly(RootExtendedPubKey), + /// External signable wallet (signing happens externally via a hardware device + /// or remote signer). + /// + /// This variant carries no key material: the external device holds all signing + /// keys, and the host only needs the per-account xpubs carried by + /// [`AccountCollection`] plus derivation paths to request signatures. + /// [`Wallet::wallet_id`] identifies the wallet — see [`Wallet::new_external_signable`]. + ExternalSignable, + /// Watch-only wallet (no signing capability). + /// + /// This variant carries no key material: every Dash derivation path hits a + /// hardened level before the account index, so a host-side root xpub cannot + /// expand coverage to account xpubs that weren't already supplied. The + /// per-account xpubs carried by [`AccountCollection`] are the only state + /// needed for address generation and transaction tracking. + /// [`Wallet::wallet_id`] identifies the wallet — see [`Wallet::new_watch_only`]. + WatchOnly, } /// Complete wallet implementation @@ -101,9 +114,22 @@ impl Wallet { hash.to_byte_array() } - /// Compute wallet ID + /// Compute wallet ID. + /// + /// For wallet types that carry a root public key (directly or derivable from a + /// stored root private key), this recomputes the id from that key. For the + /// [`WalletType::WatchOnly`] and [`WalletType::ExternalSignable`] unit + /// variants there is no root key on hand, so the id fed in at construction + /// time (`self.wallet_id`) is returned as-is. pub fn compute_wallet_id(&self) -> [u8; 32] { - Self::compute_wallet_id_from_root_extended_pub_key(&self.root_extended_pub_key_cow()) + match &self.wallet_type { + WalletType::WatchOnly | WalletType::ExternalSignable => self.wallet_id, + _ => Self::compute_wallet_id_from_root_extended_pub_key( + &self + .root_extended_pub_key_cow() + .expect("signing wallet types always have a root public key"), + ), + } } } @@ -170,10 +196,8 @@ impl Zeroize for Wallet { root_extended_private_key.zeroize(); // Note: root_private_key (SecretKey) doesn't implement Zeroize } - WalletType::ExternalSignable(root_extended_public_key) - | WalletType::WatchOnly(root_extended_public_key) => { - // Public keys are not sensitive, but zeroize for consistency - root_extended_public_key.zeroize(); + WalletType::ExternalSignable | WalletType::WatchOnly => { + // Unit variants carry no key material; nothing sensitive to zeroize. } } @@ -412,32 +436,31 @@ mod tests { // ✓ Test watch-only wallet creation (high level) #[test] fn test_watch_only_wallet_basics() { - // Create a regular wallet first to get the root xpub - + // Create a regular wallet first to snapshot its id + accounts let wallet = Wallet::new_random( Network::Testnet, initialization::WalletAccountCreationOptions::Default, ) .unwrap(); - // Get the root extended public key - let root_xpub = wallet.root_extended_pub_key(); - let root_xpub_as_extended = root_xpub.to_extended_pub_key(Network::Testnet); + // Snapshot the id and a single BIP44 account so the watch-only wallet + // has something to track without needing a root xpub. + let wallet_id = wallet.wallet_id; + let account = wallet.get_bip44_account(0).unwrap(); + let account_xpub = account.extended_public_key(); - // Create watch-only wallet from root xpub + // Build a watch-only wallet via the unit-variant constructor. let mut watch_only = - Wallet::from_xpub(root_xpub_as_extended, AccountCollection::new(), false).unwrap(); + Wallet::new_watch_only(Network::Testnet, wallet_id, AccountCollection::new()); assert!(watch_only.is_watch_only()); assert!(!watch_only.has_mnemonic()); + assert_eq!(watch_only.wallet_id, wallet_id); // Watch-only wallets start with no accounts assert_eq!(watch_only.accounts.count(), 0); // But we can add accounts manually by providing their xpubs - let account = wallet.get_bip44_account(0).unwrap(); - let account_xpub = account.extended_public_key(); - watch_only .add_account( AccountType::Standard { @@ -482,8 +505,8 @@ mod tests { .unwrap(); // Different passphrases should generate different root keys - let root_xpub1 = wallet1.root_extended_pub_key(); - let root_xpub2 = wallet2.root_extended_pub_key(); + let root_xpub1 = wallet1.root_extended_pub_key().unwrap(); + let root_xpub2 = wallet2.root_extended_pub_key().unwrap(); assert_ne!(root_xpub1.root_public_key, root_xpub2.root_public_key); } diff --git a/key-wallet/src/wallet/root_extended_keys.rs b/key-wallet/src/wallet/root_extended_keys.rs index 7b6904147..4ef0a8f20 100644 --- a/key-wallet/src/wallet/root_extended_keys.rs +++ b/key-wallet/src/wallet/root_extended_keys.rs @@ -352,51 +352,62 @@ impl FromOnNetwork for ExtendedPubKey { } impl Wallet { - /// Get the root extended public key from the wallet type - pub fn root_extended_pub_key(&self) -> RootExtendedPubKey { + /// Get the root extended public key from the wallet type. + /// + /// The [`WalletType::WatchOnly`] and [`WalletType::ExternalSignable`] unit + /// variants carry no key material — they return an error here because there + /// is nothing to return. The wallet's identity is available via + /// [`Wallet::wallet_id`] for those cases. + pub fn root_extended_pub_key(&self) -> crate::Result { match &self.wallet_type { WalletType::Mnemonic { root_extended_private_key, .. - } => root_extended_private_key.to_root_extended_pub_key(), + } => Ok(root_extended_private_key.to_root_extended_pub_key()), WalletType::MnemonicWithPassphrase { root_extended_public_key, .. - } => root_extended_public_key.clone(), + } => Ok(root_extended_public_key.clone()), WalletType::Seed { root_extended_private_key, .. - } => root_extended_private_key.to_root_extended_pub_key(), - WalletType::ExtendedPrivKey(key) => key.to_root_extended_pub_key(), - WalletType::ExternalSignable(key) => key.clone(), - WalletType::WatchOnly(key) => key.clone(), + } => Ok(root_extended_private_key.to_root_extended_pub_key()), + WalletType::ExtendedPrivKey(key) => Ok(key.to_root_extended_pub_key()), + WalletType::ExternalSignable | WalletType::WatchOnly => Err(Error::InvalidParameter( + "Root extended public key is not available for watch-only or \ + external-signable wallets; use wallet.wallet_id for identity and \ + per-account xpubs for derivation" + .into(), + )), } } - /// Get the root extended public key from the wallet type as Cow - pub fn root_extended_pub_key_cow(&self) -> Cow<'_, RootExtendedPubKey> { + /// Get the root extended public key from the wallet type as Cow. + /// + /// See [`Wallet::root_extended_pub_key`] for the unit-variant behavior. + pub fn root_extended_pub_key_cow(&self) -> crate::Result> { match &self.wallet_type { WalletType::Mnemonic { root_extended_private_key, .. - } => Cow::Owned(root_extended_private_key.to_root_extended_pub_key()), + } => Ok(Cow::Owned(root_extended_private_key.to_root_extended_pub_key())), WalletType::MnemonicWithPassphrase { root_extended_public_key, .. - } => Cow::Borrowed(root_extended_public_key), + } => Ok(Cow::Borrowed(root_extended_public_key)), WalletType::Seed { root_extended_private_key, .. - } => Cow::Owned(root_extended_private_key.to_root_extended_pub_key()), + } => Ok(Cow::Owned(root_extended_private_key.to_root_extended_pub_key())), WalletType::ExtendedPrivKey(root_extended_priv_key) => { - Cow::Owned(root_extended_priv_key.to_root_extended_pub_key()) - } - WalletType::ExternalSignable(root_extended_public_key) => { - Cow::Borrowed(root_extended_public_key) - } - WalletType::WatchOnly(root_extended_public_key) => { - Cow::Borrowed(root_extended_public_key) + Ok(Cow::Owned(root_extended_priv_key.to_root_extended_pub_key())) } + WalletType::ExternalSignable | WalletType::WatchOnly => Err(Error::InvalidParameter( + "Root extended public key is not available for watch-only or \ + external-signable wallets; use wallet.wallet_id for identity and \ + per-account xpubs for derivation" + .into(), + )), } } @@ -417,10 +428,10 @@ impl Wallet { .. } => Ok(root_extended_private_key), WalletType::ExtendedPrivKey(key) => Ok(key), - WalletType::ExternalSignable(_) => { + WalletType::ExternalSignable => { Err(Error::InvalidParameter("External signable wallet has no private key".into())) } - WalletType::WatchOnly(_) => { + WalletType::WatchOnly => { Err(Error::InvalidParameter("Watch-only wallet has no private key".into())) } } @@ -453,10 +464,10 @@ impl Wallet { .. } => Ok(root_extended_private_key.clone()), WalletType::ExtendedPrivKey(key) => Ok(key.clone()), - WalletType::ExternalSignable(_) => { + WalletType::ExternalSignable => { Err(Error::InvalidParameter("External signable wallet has no private key".into())) } - WalletType::WatchOnly(_) => { + WalletType::WatchOnly => { Err(Error::InvalidParameter("Watch-only wallet has no private key".into())) } } diff --git a/key-wallet/src/wallet_comprehensive_tests.rs b/key-wallet/src/wallet_comprehensive_tests.rs index 69179c50f..93fd65d29 100644 --- a/key-wallet/src/wallet_comprehensive_tests.rs +++ b/key-wallet/src/wallet_comprehensive_tests.rs @@ -127,7 +127,7 @@ mod tests { .unwrap(); // Get the wallet's root extended public key - let root_xpub = wallet.root_extended_pub_key(); + let root_xpub = wallet.root_extended_pub_key().unwrap(); let root_xpub_as_extended = root_xpub.to_extended_pub_key(Network::Testnet); // Create watch-only wallet from the root xpub @@ -138,12 +138,11 @@ mod tests { assert!(!watch_only.has_mnemonic()); assert_eq!(watch_only.accounts.count(), 0); // None creates no accounts - // Both wallets should have the same root public key - let watch_root_xpub = watch_only.root_extended_pub_key(); - assert_eq!(root_xpub.root_public_key, watch_root_xpub.root_public_key); - assert_eq!(root_xpub.root_chain_code, watch_root_xpub.root_chain_code); - - // And they should have the same wallet ID since it's based on the root public key + // Watch-only wallets no longer retain the root xpub — the unit variant + // carries no key material. Identity is preserved through wallet_id: + // `from_xpub` hashes the provided xpub into the same id the full wallet + // computes from its root key. + assert!(watch_only.root_extended_pub_key().is_err()); assert_eq!(wallet.wallet_id, watch_only.wallet_id); } @@ -169,8 +168,8 @@ mod tests { .unwrap(); // Different passphrases should generate different root keys - let root_xpub1 = wallet1.root_extended_pub_key(); - let root_xpub2 = wallet2.root_extended_pub_key(); + let root_xpub1 = wallet1.root_extended_pub_key().unwrap(); + let root_xpub2 = wallet2.root_extended_pub_key().unwrap(); assert_ne!(root_xpub1.root_public_key, root_xpub2.root_public_key); } From 8a916179d6201065c0bc6926851735cd75c68650 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sat, 18 Apr 2026 05:09:49 +0700 Subject: [PATCH 2/2] docs(key-wallet): correct derive_*_public_key docstrings for unit variants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing docstrings on `derive_extended_public_key`, `derive_public_key`, and `derive_public_key_as_hex` still claim "For non-hardened paths, this works with watch-only wallets" — true before the unit-variant refactor, but now misleading: watch-only and external-signable wallets carry no root xpub, so non-hardened root-derivation fails. Rewrite `derive_extended_public_key`'s doc to spell out the per-wallet-type behavior explicitly (including that watch-only / external-signable callers must go through per-account xpubs) and point the two thin wrappers at it. At the non-hardened call site, replace the generic propagated "root pub key unavailable" message with path-specific guidance that names the actual escape hatch (`account.extended_public_key()`), so callers get actionable advice instead of a symptom. Add a regression test asserting the new error mentions the per-account xpub path for both unit variants. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/tests/unit_variant_wallet_tests.rs | 31 ++++++++++ key-wallet/src/wallet/helper.rs | 58 +++++++++++++------ 2 files changed, 72 insertions(+), 17 deletions(-) diff --git a/key-wallet/src/tests/unit_variant_wallet_tests.rs b/key-wallet/src/tests/unit_variant_wallet_tests.rs index 23a540e71..9234695cf 100644 --- a/key-wallet/src/tests/unit_variant_wallet_tests.rs +++ b/key-wallet/src/tests/unit_variant_wallet_tests.rs @@ -130,6 +130,37 @@ fn round_trip_full_to_watch_only_has_address_parity_per_account() { } } +// -------- derive_extended_public_key surfaces actionable guidance ----- + +#[test] +fn derive_extended_public_key_error_points_to_per_account_xpub() { + use crate::DerivationPath; + + let full = built_full_wallet(); + let watch = Wallet::new_watch_only(Network::Testnet, full.wallet_id, AccountCollection::new()); + let ext = + Wallet::new_external_signable(Network::Testnet, full.wallet_id, AccountCollection::new()); + + // Non-hardened path: the generic "root pub key unavailable" error should + // be replaced with call-site-specific guidance pointing at per-account + // xpubs, which is the actual escape hatch for watch-only callers. + let non_hardened: DerivationPath = "m/0/0".parse().unwrap(); + let err = watch.derive_extended_public_key(&non_hardened).unwrap_err().to_string(); + assert!( + err.contains("extended_public_key") && err.contains("account"), + "watch-only derive error should recommend per-account xpubs; got: {err}" + ); + let err = ext.derive_extended_public_key(&non_hardened).unwrap_err().to_string(); + assert!( + err.contains("extended_public_key") && err.contains("account"), + "external-signable derive error should recommend per-account xpubs; got: {err}" + ); + + // Hardened path: the existing "no private key" error shape is preserved. + let hardened: DerivationPath = "m/44'/1'/0'".parse().unwrap(); + assert!(watch.derive_extended_public_key(&hardened).is_err()); +} + // -------- bincode round-trip ------------------------------------------- #[test] diff --git a/key-wallet/src/wallet/helper.rs b/key-wallet/src/wallet/helper.rs index 2eeb41a9b..a67e58185 100644 --- a/key-wallet/src/wallet/helper.rs +++ b/key-wallet/src/wallet/helper.rs @@ -687,16 +687,27 @@ impl Wallet { Ok(dash_key.to_wif()) } - /// Derive an extended public key at a specific derivation path + /// Derive an extended public key at a specific derivation path from the wallet's root. /// - /// For hardened derivation paths, this requires private key access. - /// For non-hardened paths, this works with watch-only wallets. + /// # Behavior by wallet type + /// * `Mnemonic`, `Seed`, `ExtendedPrivKey`: works for both hardened and non-hardened paths. + /// * `MnemonicWithPassphrase`: works for non-hardened paths only (hardened paths require + /// the passphrase to reconstruct the root private key; use + /// [`Wallet::derive_extended_private_key_with_passphrase`] for those). + /// * `WatchOnly` / `ExternalSignable`: **cannot derive from the root at all** — these + /// unit variants carry no root key material. Hardened paths fail because there's no + /// private key on hand, and non-hardened paths fail because there's no root xpub + /// either. For these wallets, fetch the relevant account via + /// [`Wallet::get_bip44_account`] (or similar) and derive non-hardened paths from + /// [`Account::extended_public_key`](crate::account::Account::extended_public_key) + /// directly. /// /// # Arguments /// * `path` - The derivation path (e.g., "m/44'/5'/0'/0/0") /// /// # Returns - /// The extended public key, or an error if the path is invalid + /// The extended public key, or an error if the path cannot be derived for this + /// wallet type (see above). pub fn derive_extended_public_key( &self, path: &crate::DerivationPath, @@ -719,26 +730,36 @@ impl Wallet { let secp = Secp256k1::new(); Ok(ExtendedPubKey::from_priv(&secp, &extended_private)) } else { - // For non-hardened paths, derive directly from the root public key. - // For watch-only / external-signable unit variants there is no root - // key on hand, so this propagates the "root pub key unavailable" error - // from `root_extended_pub_key`. + // For non-hardened paths, derive from the root public key. Watch-only and + // external-signable unit variants have no root key on hand — surface a + // path-specific message pointing callers at per-account xpubs instead of + // just propagating the generic "root pub key unavailable" error. + let root_xpub = self.root_extended_pub_key().map_err(|_| { + Error::InvalidParameter( + "Cannot derive from the root xpub for watch-only or external-signable \ + wallets; fetch the relevant account (e.g. wallet.get_bip44_account(idx)) \ + and derive non-hardened paths from its extended_public_key() instead" + .to_string(), + ) + })?; let secp = Secp256k1::new(); - let xpub = self.root_extended_pub_key()?.to_extended_pub_key(self.network); + let xpub = root_xpub.to_extended_pub_key(self.network); xpub.derive_pub(&secp, path).map_err(|e| e.into()) } } - /// Derive a public key at a specific derivation path + /// Derive a public key at a specific derivation path from the wallet's root. /// - /// For hardened derivation paths, this requires private key access. - /// For non-hardened paths, this works with watch-only wallets. + /// See [`Wallet::derive_extended_public_key`] for the per-wallet-type behavior + /// (in particular, watch-only and external-signable wallets cannot derive from + /// the root — use per-account xpubs instead). /// /// # Arguments /// * `path` - The derivation path (e.g., "m/44'/5'/0'/0/0") /// /// # Returns - /// The public key (secp256k1::PublicKey), or an error if the path is invalid + /// The public key (secp256k1::PublicKey), or an error if the path cannot be + /// derived for this wallet type. pub fn derive_public_key(&self, path: &crate::DerivationPath) -> Result { // Check if the path contains hardened derivation let has_hardened = path.into_iter().any(|child| child.is_hardened()); @@ -762,16 +783,19 @@ impl Wallet { } } - /// Derive a public key at a specific derivation path and return as hex string + /// Derive a public key at a specific derivation path from the wallet's root + /// and return it as a hex string. /// - /// For hardened derivation paths, this requires private key access. - /// For non-hardened paths, this works with watch-only wallets. + /// See [`Wallet::derive_extended_public_key`] for the per-wallet-type behavior + /// (in particular, watch-only and external-signable wallets cannot derive from + /// the root — use per-account xpubs instead). /// /// # Arguments /// * `path` - The derivation path (e.g., "m/44'/5'/0'/0/0") /// /// # Returns - /// The public key as hex string, or an error if the path is invalid + /// The public key as a hex string, or an error if the path cannot be derived + /// for this wallet type. pub fn derive_public_key_as_hex(&self, path: &crate::DerivationPath) -> Result { let public_key = self.derive_public_key(path)?;