Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/backend_task/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,23 @@ pub enum TaskError {
/// Creating a network context failed during a network switch.
#[error("Could not connect to {network}. Check your network configuration and retry.")]
NetworkContextCreationFailed { network: Network, detail: String },

// ──────────────────────────────────────────────────────────────────────────
// Address-watch registration errors
// ──────────────────────────────────────────────────────────────────────────
/// An off-tree address (not covered by the standard BIP44 account watch)
/// could not be registered with the SPV subsystem. The running SPV loop
/// currently has no runtime registration API, so addresses outside the
/// wallet's BIP44 account may not receive their incoming transactions until
/// the wallet is reloaded.
///
/// The associated Base58 address lets users identify which handle is
/// affected.
#[error(
"Could not start watching address {address}. Please reload the wallet and retry, \
or switch to Dash Core RPC mode in Expert settings if the problem persists."
)]
SpvOffTreeAddressRegistrationUnsupported { address: String },
Comment on lines +1014 to +1018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: User-facing error uses 'RPC' jargon banned by CLAUDE.md

CLAUDE.md (User-facing message rules) explicitly bans 'RPC' as jargon for messages shown via Display in MessageBanner. The new variant says "switch to Dash Core RPC mode in Expert settings". The codebase already standardizes on the alternative phrasing — see SINGLE_KEY_REQUIRES_CORE at src/ui/wallets/wallets_screen/single_key_view.rs:14 and src/ui/components/tools_subscreen_chooser_panel.rs:150, which both say "switch to Expert mode, and select Local Dash Core node". Aligning keeps the message Everyday-User friendly and removes the banned term.

💡 Suggested change
Suggested change
#[error(
"Could not start watching address {address}. Please reload the wallet and retry, \
or switch to Dash Core RPC mode in Expert settings if the problem persists."
)]
SpvOffTreeAddressRegistrationUnsupported { address: String },
#[error(
"Could not start watching address {address}. Please reload the wallet and retry, \
or switch to Expert mode and connect to a local Dash Core node if the problem persists."
)]
SpvOffTreeAddressRegistrationUnsupported { address: String },

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/backend_task/error.rs`:
- [SUGGESTION] lines 1014-1018: User-facing error uses 'RPC' jargon banned by CLAUDE.md
  CLAUDE.md (User-facing message rules) explicitly bans 'RPC' as jargon for messages shown via `Display` in `MessageBanner`. The new variant says "switch to Dash Core RPC mode in Expert settings". The codebase already standardizes on the alternative phrasing — see `SINGLE_KEY_REQUIRES_CORE` at `src/ui/wallets/wallets_screen/single_key_view.rs:14` and `src/ui/components/tools_subscreen_chooser_panel.rs:150`, which both say "switch to Expert mode, and select Local Dash Core node". Aligning keeps the message Everyday-User friendly and removes the banned term.

}

/// Escapes control characters in a token name for safe display in error messages.
Expand Down
103 changes: 103 additions & 0 deletions src/context/address_watch.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
//! Typed address-watch registration for `AppContext`.
//!
//! The previous pair `ensure_address_imported` / `try_import_address` silently
//! no-opped in SPV mode and relied on an undocumented precondition — that the
//! address was already covered by the wallet's account-level watch. That was
//! true for standard BIP44 receive addresses derived from the wallet xprv, but
//! not for DashPay DIP-15 contact paths, imported single-key addresses, or
//! P2SH multisig outputs.
//!
//! [`AddressCoverage`] forces callers to declare how SPV coverage is provided
//! for an address so [`AppContext::ensure_address_watched`] can dispatch to
//! the right subsystem and never silently drop coverage.
//!
//! [`AppContext::ensure_address_watched`]: crate::context::AppContext::ensure_address_watched

use crate::model::wallet::DerivationPathReference;

/// How SPV coverage is provided for an address being registered.
///
/// Callers must pick the variant that matches how the address was obtained.
/// The wrong variant can cause incoming transactions to be missed (off-tree
/// address classified as BIP44) or waste a runtime SPV registration slot
/// (BIP44 address classified as off-tree).
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum AddressCoverage {
/// Address derived inside the wallet's standard BIP44 account
/// (`m/44'/coin'/0'/change/index`).
///
/// SPV coverage is automatic via the account-level watch set up at wallet
/// load; no explicit SPV registration is required. In Core RPC mode the
/// address is imported into the wallet as watch-only.
StandardBip44Account,

/// Address derived outside the standard BIP44 account.
///
/// Covers DashPay DIP-15 contact paths, imported single-key wallets,
/// Blockchain-Identities paths (`m/9'/…`), platform-payment addresses
/// (DIP-17), multisig outputs, and anything else that does not belong to
/// the wallet's BIP44 receive/change chains.
///
/// In Core RPC mode the address is imported into the wallet as watch-only.
/// In SPV mode the running subsystem currently has no runtime registration
/// hook — see the `TODO(refactor)` note in
/// [`AppContext::ensure_address_watched`] and [`TaskError::SpvOffTreeAddressRegistrationUnsupported`].
///
/// [`AppContext::ensure_address_watched`]: crate::context::AppContext::ensure_address_watched
/// [`TaskError::SpvOffTreeAddressRegistrationUnsupported`]: crate::backend_task::error::TaskError::SpvOffTreeAddressRegistrationUnsupported
OffTree,
}

impl AddressCoverage {
/// Map a [`DerivationPathReference`] to the appropriate coverage variant.
///
/// Used by the wallet's internal address-registration helpers to pick the
/// correct variant without burdening every caller with the classification.
pub fn from_derivation_path_reference(reference: DerivationPathReference) -> Self {
match reference {
DerivationPathReference::BIP44 => Self::StandardBip44Account,
_ => Self::OffTree,
}
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn bip44_maps_to_standard_account() {
assert_eq!(
AddressCoverage::from_derivation_path_reference(DerivationPathReference::BIP44),
AddressCoverage::StandardBip44Account,
);
}

#[test]
fn non_bip44_references_map_to_off_tree() {
let off_tree_refs = [
DerivationPathReference::BIP32,
DerivationPathReference::BlockchainIdentities,
DerivationPathReference::ProviderFunds,
DerivationPathReference::ContactBasedFunds,
DerivationPathReference::ContactBasedFundsRoot,
DerivationPathReference::ContactBasedFundsExternal,
DerivationPathReference::BlockchainIdentityCreditRegistrationFunding,
DerivationPathReference::BlockchainIdentityCreditTopupFunding,
DerivationPathReference::BlockchainIdentityCreditInvitationFunding,
DerivationPathReference::ProviderPlatformNodeKeys,
DerivationPathReference::CoinJoin,
DerivationPathReference::PlatformPayment,
DerivationPathReference::Root,
DerivationPathReference::Unknown,
];
for reference in off_tree_refs {
assert_eq!(
AddressCoverage::from_derivation_path_reference(reference),
AddressCoverage::OffTree,
"reference {:?} should map to OffTree",
reference,
);
}
}
}
66 changes: 53 additions & 13 deletions src/context/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pub mod address_watch;
pub mod connection_status;
mod contract_token_db;
mod identity_db;
Expand All @@ -6,6 +7,8 @@ pub mod shielded;
mod transaction_processing;
mod wallet_lifecycle;

pub use address_watch::AddressCoverage;

pub(crate) use transaction_processing::get_transaction_info;

use crate::app_dir::core_cookie_path;
Expand Down Expand Up @@ -703,10 +706,59 @@ impl AppContext {
Self::create_core_rpc_client(&url, self.network, &cfg.devnet_name, &cfg)
}

/// Ensure SPV/Core is watching the given address, dispatching by
/// [`AddressCoverage`] and the active backend mode.
///
/// # Dispatch matrix
///
/// | Coverage | Core RPC mode | SPV mode |
/// |----------------------------|-------------------------------------------|------------------------------------------------------------|
/// | `StandardBip44Account` | Import into the targeted Core wallet. | No-op — wallet-level account watch already covers it. |
/// | `OffTree` | Import into the targeted Core wallet. | Returns [`TaskError::SpvOffTreeAddressRegistrationUnsupported`]. |
///
/// See [`crate::context::address_watch`] for rationale and the history
/// of the legacy `ensure_address_imported` / `try_import_address` pair
/// this method replaced.
///
/// Fire-and-forget callers must use `let _` explicitly so the type system
/// makes the error-swallowing visible at the call site.
pub fn ensure_address_watched(
&self,
address: &Address,
coverage: AddressCoverage,
core_wallet_name: Option<&str>,
label: Option<&str>,
) -> Result<(), TaskError> {
match self.core_backend_mode() {
CoreBackendMode::Rpc => self.import_address_into_core(address, core_wallet_name, label),
CoreBackendMode::Spv => match coverage {
AddressCoverage::StandardBip44Account => Ok(()),
AddressCoverage::OffTree => {
// TODO(refactor): SpvManager has no runtime API to register an
// extra address outside the wallet's BIP44 account watch.
// Until that upstream hook exists, surface a typed error so
// callers can decide (warn-and-continue vs abort). The wallet
// reload path covers these addresses once the user restarts
// the SPV loop.
tracing::warn!(
address = %address,
"Off-tree address registration requested in SPV mode; \
running SPV loop has no runtime registration hook. \
Incoming transactions to this address may be missed \
until the wallet is reloaded."
);
Err(TaskError::SpvOffTreeAddressRegistrationUnsupported {
address: address.to_string(),
})
}
},
}
}
Comment on lines +725 to +756

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Dispatch matrix of ensure_address_watched is untested

The whole point of this refactor is to make the SPV+OffTree case fail loudly rather than silently no-op. Yet the only added tests cover AddressCoverage::from_derivation_path_reference mapping (a near-trivial enum mapping). The dispatch logic itself — StandardBip44Account no-op in SPV, OffTree returning SpvOffTreeAddressRegistrationUnsupported in SPV — has no assertion, so a future refactor could re-introduce the original silent no-op without any test failing. Per CLAUDE.md ("new backend tasks should have corresponding test coverage"), at minimum the SPV branches should be locked in.

Because the SPV branches don't touch RPC, the cleanest path is extracting a pure helper such as enum WatchAction { ImportIntoCore, NoOp, SpvOffTreeUnsupported } from (CoreBackendMode, AddressCoverage), unit-testing it directly without spinning up an AppContext.

source: ['claude', 'codex']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/context/mod.rs`:
- [SUGGESTION] lines 725-756: Dispatch matrix of `ensure_address_watched` is untested
  The whole point of this refactor is to make the SPV+OffTree case fail loudly rather than silently no-op. Yet the only added tests cover `AddressCoverage::from_derivation_path_reference` mapping (a near-trivial enum mapping). The dispatch logic itself — `StandardBip44Account` no-op in SPV, `OffTree` returning `SpvOffTreeAddressRegistrationUnsupported` in SPV — has no assertion, so a future refactor could re-introduce the original silent no-op without any test failing. Per CLAUDE.md ("new backend tasks should have corresponding test coverage"), at minimum the SPV branches should be locked in.

Because the SPV branches don't touch RPC, the cleanest path is extracting a pure helper such as `enum WatchAction { ImportIntoCore, NoOp, SpvOffTreeUnsupported }` from `(CoreBackendMode, AddressCoverage)`, unit-testing it directly without spinning up an `AppContext`.


/// Import an address into the correct Core wallet if it's not already known.
/// Uses `core_wallet_name` to target the right wallet on multi-wallet nodes.
/// No-op if the address is already watched/mine.
pub fn ensure_address_imported(
fn import_address_into_core(
&self,
address: &Address,
core_wallet_name: Option<&str>,
Expand All @@ -724,18 +776,6 @@ impl AppContext {
Ok(())
}

/// Import address into Core, ignoring errors. For best-effort registration.
pub fn try_import_address(
&self,
address: &Address,
core_wallet_name: Option<&str>,
label: Option<&str>,
) {
if let Ok(client) = self.core_client_for_wallet(core_wallet_name) {
let _ = client.import_address(address, label, Some(false));
}
}

/// Convert an RPC error to `TaskError`, enriching connection failures with
/// the configured host:port so the user knows which address was unreachable.
pub(crate) fn rpc_error_with_url(&self, e: dash_sdk::dashcore_rpc::Error) -> TaskError {
Expand Down
34 changes: 22 additions & 12 deletions src/model/wallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1000,16 +1000,8 @@ impl Wallet {
known_public_key = Some(public_key);
if let Some(app_context) = register {
let address = Address::p2pkh(&public_key, network);
app_context.try_import_address(
&address,
self.core_wallet_name.as_deref(),
Some(&format!(
"Managed by Dash Evo Tool {} {}",
self.alias.clone().unwrap_or_default(),
derivation_path
)),
);

// `register_address` handles address-watch registration internally
// (dispatches by backend mode via `ensure_address_watched`).
self.register_address(
address,
&derivation_path,
Expand Down Expand Up @@ -1199,8 +1191,26 @@ impl Wallet {
},
);

if app_context.core_backend_mode() == crate::spv::CoreBackendMode::Rpc {
app_context.try_import_address(&address, self.core_wallet_name.as_deref(), None);
let coverage =
crate::context::AddressCoverage::from_derivation_path_reference(path_reference);
if let Err(e) = app_context.ensure_address_watched(
&address,
coverage,
self.core_wallet_name.as_deref(),
None,
) {
// Best-effort: we've already persisted the address in the wallet
// maps and database, so downstream lookups still work. Only the
// external watch (Core RPC import or SPV registration) failed;
// log loudly and continue so the caller isn't blocked.
tracing::warn!(
address = %address,
error = %e,
?coverage,
"Failed to register address for external watch; wallet-side \
bookkeeping succeeded but incoming transactions may be missed \
until the wallet is reloaded."
);
Comment on lines +1196 to +1213

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: register_address discards the new typed error before any caller can react

SpvOffTreeAddressRegistrationUnsupported was added so callers can decide whether to abort or warn-and-continue. register_address always logs and returns Ok(()), so the dominant SPV-mode callers (identity_registration_*, identity_top_up_*, identity_authentication_* — all OffTree) never surface the unsupported-registration condition to higher layers. The inline comment at lines 1202-1205 frames this as an intentional best-effort design (wallet bookkeeping persists, only external watch fails), and unconditionally propagating would block every identity flow in SPV mode. That trade-off is defensible.

Worth confirming intent: if the design goal is for users to see a one-time warning when entering an SPV/OffTree flow, the warning currently only goes to logs (tracing::warn!) — it doesn't reach the UI. If a user-visible cue is wanted, consider surfacing the first occurrence via MessageBanner from the screen-level caller rather than suppressing it inside this helper.

source: ['codex']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/model/wallet/mod.rs`:
- [SUGGESTION] lines 1196-1213: `register_address` discards the new typed error before any caller can react
  `SpvOffTreeAddressRegistrationUnsupported` was added so callers can decide whether to abort or warn-and-continue. `register_address` always logs and returns `Ok(())`, so the dominant SPV-mode callers (`identity_registration_*`, `identity_top_up_*`, `identity_authentication_*` — all OffTree) never surface the unsupported-registration condition to higher layers. The inline comment at lines 1202-1205 frames this as an intentional best-effort design (wallet bookkeeping persists, only external watch fails), and unconditionally propagating would block every identity flow in SPV mode. That trade-off is defensible.

Worth confirming intent: if the design goal is for users to see a one-time warning when entering an SPV/OffTree flow, the warning currently only goes to logs (`tracing::warn!`) — it doesn't reach the UI. If a user-visible cue is wanted, consider surfacing the first occurrence via `MessageBanner` from the screen-level caller rather than suppressing it inside this helper.

Comment on lines +1196 to +1213

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Off-tree SPV registration warns twice per call

When register_address is invoked with an OffTree path in SPV mode, two near-identical warnings are emitted for the same event:

  1. AppContext::ensure_address_watched at src/context/mod.rs:743-749 before returning SpvOffTreeAddressRegistrationUnsupported.
  2. register_address at src/model/wallet/mod.rs:1206-1213 in the error branch.

Identity helpers (BlockchainIdentities paths) trigger this every time keys are derived in SPV mode, so the duplication will be very visible. Pick one canonical log line: either drop the tracing::warn! inside ensure_address_watched (let the caller decide level/wording — this fits the typed-error design), or downgrade one to debug. The TODO comment can stay regardless.

source: ['claude']

Comment on lines +1206 to +1213

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Log the Debug representation of TaskError, not Display

error = %e formats TaskError via Display, which per CLAUDE.md is the user-facing rendering intended for MessageBanner. Logs should preserve the technical chain (which Debug provides via the #[source] field), so subsequent debugging of an off-tree-in-SPV failure can see the underlying cause rather than just the end-user copy. This matches how BannerHandle::with_details(e) is used elsewhere — Display goes to UI, Debug to logs.

💡 Suggested change
Suggested change
tracing::warn!(
address = %address,
error = %e,
?coverage,
"Failed to register address for external watch; wallet-side \
bookkeeping succeeded but incoming transactions may be missed \
until the wallet is reloaded."
);
tracing::warn!(
address = %address,
error = ?e,
?coverage,
"Failed to register address for external watch; wallet-side \
bookkeeping succeeded but incoming transactions may be missed \
until the wallet is reloaded."
);

source: ['claude']

}

tracing::trace!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::backend_task::error::TaskError;
use crate::backend_task::identity::{
IdentityRegistrationInfo, IdentityTask, RegisterIdentityFundingMethod,
};
use crate::context::AddressCoverage;
use crate::ui::MessageType;
use crate::ui::components::MessageBanner;
use crate::ui::identities::add_new_identity_screen::{
Expand Down Expand Up @@ -31,16 +32,18 @@ impl AddNewIdentityScreen {

if let Some(has_address) = self.core_has_funding_address {
if !has_address {
self.app_context.ensure_address_imported(
self.app_context.ensure_address_watched(
&receive_address,
AddressCoverage::StandardBip44Account,
core_wallet_name.as_deref(),
Some("Managed by Dash Evo Tool"),
)?;
}
self.funding_address = Some(receive_address);
} else {
self.app_context.ensure_address_imported(
self.app_context.ensure_address_watched(
&receive_address,
AddressCoverage::StandardBip44Account,
core_wallet_name.as_deref(),
Some("Managed by Dash Evo Tool"),
)?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::app::AppAction;
use crate::backend_task::BackendTask;
use crate::backend_task::error::TaskError;
use crate::backend_task::identity::{IdentityTask, IdentityTopUpInfo, TopUpIdentityFundingMethod};
use crate::context::AddressCoverage;
use crate::ui::MessageType;
use crate::ui::components::MessageBanner;
use crate::ui::identities::funding_common::{self, copy_to_clipboard, generate_qr_code_image};
Expand All @@ -23,8 +24,9 @@ impl TopUpIdentityScreen {
let core_wallet_name = wallet.core_wallet_name.clone();
drop(wallet);

self.app_context.ensure_address_imported(
self.app_context.ensure_address_watched(
&receive_address,
AddressCoverage::StandardBip44Account,
core_wallet_name.as_deref(),
Some("Managed by Dash Evo Tool"),
)?;
Expand Down
11 changes: 7 additions & 4 deletions src/ui/wallets/create_asset_lock_screen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::app::AppAction;
use crate::backend_task::core::{CoreItem, CoreTask};
use crate::backend_task::error::TaskError;
use crate::backend_task::{BackendTask, BackendTaskSuccessResult};
use crate::context::AppContext;
use crate::context::{AddressCoverage, AppContext};
use crate::model::amount::Amount;
use crate::model::qualified_identity::QualifiedIdentity;
use crate::model::wallet::Wallet;
Expand Down Expand Up @@ -106,19 +106,22 @@ impl CreateAssetLockScreen {
let core_wallet_name = wallet.core_wallet_name.clone();
drop(wallet);

// Import address to core if needed
// Ensure the funding address is watched (Core-RPC: imports into the
// targeted Core wallet; SPV: covered by the BIP44 account watch).
if let Some(has_address) = self.core_has_funding_address {
if !has_address {
self.app_context.ensure_address_imported(
self.app_context.ensure_address_watched(
&receive_address,
AddressCoverage::StandardBip44Account,
core_wallet_name.as_deref(),
Some("Managed by Dash Evo Tool - Asset Lock"),
)?;
}
self.funding_address = Some(receive_address);
} else {
self.app_context.ensure_address_imported(
self.app_context.ensure_address_watched(
&receive_address,
AddressCoverage::StandardBip44Account,
core_wallet_name.as_deref(),
Some("Managed by Dash Evo Tool - Asset Lock"),
)?;
Expand Down
Loading