refactor(context): typed address-watch registration (ensure_address_watched)#848
refactor(context): typed address-watch registration (ensure_address_watched)#848lklimek wants to merge 3 commits into
Conversation
…hed API Add a typed entry point for address-watch registration that forces callers to declare how SPV coverage is provided, so the function can dispatch to the right subsystem and never silently drop coverage. `AddressCoverage` has two variants: - `StandardBip44Account` — wallet's BIP44 account watch already covers the address; SPV mode is a true no-op, Core RPC mode imports as watch-only. - `OffTree` — DashPay contact paths, imported single-key wallets, identity paths (m/9'/…), and anything outside the BIP44 chains. Core RPC imports as watch-only; SPV mode currently returns a typed error because the running SpvManager has no runtime registration hook (TODO tracked). Legacy `ensure_address_imported` / `try_import_address` are marked `#[deprecated]`; callers migrate in follow-up commits and the legacy pair is removed once migration is complete. Adds `TaskError::SpvOffTreeAddressRegistrationUnsupported` with an actionable user-facing message that includes the Base58 address. Co-Authored-By: Claude Opus 4.1 <noreply@anthropic.com>
…tched Migrate every caller of the legacy `ensure_address_imported` and `try_import_address` pair to the new typed `ensure_address_watched` API: - `add_new_identity_screen/by_wallet_qr_code.rs` — BIP44 receive address for identity-creation funding (StandardBip44Account). - `top_up_identity_screen/by_wallet_qr_code.rs` — BIP44 receive address for identity top-up funding (StandardBip44Account). - `wallets/create_asset_lock_screen.rs` — BIP44 receive address for asset-lock funding (StandardBip44Account). - `model/wallet/mod.rs` — generic `register_address` now picks the coverage variant from the `DerivationPathReference`, so BIP44 entries stay standard-account and BlockchainIdentities / DashPay / Platform paths become OffTree. Removes the duplicate `try_import_address` in `receive_address` — `register_address` already registers via the typed API. Errors from `ensure_address_watched` in the generic wallet-registration helper are logged as warnings rather than surfaced to the caller, to preserve the previous best-effort behaviour while making failures visible in logs. Co-Authored-By: Claude Opus 4.1 <noreply@anthropic.com>
…dress All callers now go through `ensure_address_watched` with an explicit `AddressCoverage`, so the legacy pair is no longer needed. Drop both methods and the deprecation shims. Co-Authored-By: Claude Opus 4.1 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit a03514e) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR cleanly replaces the silently-failing ensure_address_imported/try_import_address pair with the typed AddressCoverage-aware ensure_address_watched API. Verified six valid findings: a CLAUDE.md jargon violation in the new TaskError variant, a real regression where the descriptive Core-wallet import label for BIP44 addresses is silently dropped, double-logging on the SPV/off-tree path, a Display-vs-Debug log idiom slip, the dispatch matrix lacking tests, and a design-discussion point about register_address swallowing the new typed error. No blocking issues.
Reviewed commit: a03514e
🟡 4 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🟡 suggestion: Descriptive Core-wallet import label is silently dropped for newly-discovered BIP44 addresses
src/model/wallet/mod.rs (lines 1001-1012)
Pre-PR, unused_bip_44_public_key imported the freshly-derived address into Core with a rich identifying label "Managed by Dash Evo Tool {alias} {derivation_path}" via try_import_address, and then called register_address. The PR consolidated registration into register_address — but register_address calls ensure_address_watched(..., None) (line 1200), so the Core wallet now records the address with no label. This is a user-visible regression for Power/Expert users who inspect the underlying Core wallet to map addresses back to a DET wallet.
Fix options: (a) extend register_address to accept an Option<&str> label and thread the formatted string through from this call site (other internal callers can pass None), or (b) re-introduce the labeled import_address_into_core-equivalent step here before register_address. Option (a) keeps a single registration entry point and matches the PR's design intent.
🤖 Prompt for all review comments 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.
In `src/model/wallet/mod.rs`:
- [SUGGESTION] lines 1001-1012: Descriptive Core-wallet import label is silently dropped for newly-discovered BIP44 addresses
Pre-PR, `unused_bip_44_public_key` imported the freshly-derived address into Core with a rich identifying label `"Managed by Dash Evo Tool {alias} {derivation_path}"` via `try_import_address`, and *then* called `register_address`. The PR consolidated registration into `register_address` — but `register_address` calls `ensure_address_watched(..., None)` (line 1200), so the Core wallet now records the address with no label. This is a user-visible regression for Power/Expert users who inspect the underlying Core wallet to map addresses back to a DET wallet.
Fix options: (a) extend `register_address` to accept an `Option<&str>` label and thread the formatted string through from this call site (other internal callers can pass `None`), or (b) re-introduce the labeled `import_address_into_core`-equivalent step here before `register_address`. Option (a) keeps a single registration entry point and matches the PR's design intent.
- [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.
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`.
| #[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 }, |
There was a problem hiding this comment.
🟡 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
| #[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.
| 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(), | ||
| }) | ||
| } | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 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`.
| 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." | ||
| ); |
There was a problem hiding this comment.
🟡 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.
| 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." | ||
| ); |
There was a problem hiding this comment.
💬 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:
AppContext::ensure_address_watchedatsrc/context/mod.rs:743-749before returningSpvOffTreeAddressRegistrationUnsupported.register_addressatsrc/model/wallet/mod.rs:1206-1213in 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']
| 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." | ||
| ); |
There was a problem hiding this comment.
💬 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
| 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']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Refactor cleanly introduces a typed AddressCoverage / ensure_address_watched dispatch matrix and a typed TaskError variant. Two real behavioral concerns confirmed: BIP44 receive addresses lose their previous descriptive Core RPC label, and SPV-mode wallet bootstrap now emits duplicate warning floods (each off-tree register_address logs both inside ensure_address_watched and at the caller, multiplied by 50+ bootstrap registrations). Three smaller architectural nits (typed Address in TaskError variant, catch-all match arm, missing dispatcher tests) round out the review.
Reviewed commit: a03514e
Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.
Code Review
Reviewed commit: a03514e
🟡 3 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments 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 1001-1012: BIP44 receive addresses lose their Core RPC label
Pre-refactor, `unused_bip_44_public_key` explicitly called `try_import_address(&address, core_wallet, Some("Managed by Dash Evo Tool {alias} {derivation_path}"))` before invoking `register_address`, so newly derived BIP44 receive addresses showed up in Core's wallet UI with a descriptive, DET-attributable label. After the refactor, the labelled call is gone and `register_address` (mod.rs:1196-1200) calls `ensure_address_watched(..., None)` — so the address is imported as watch-only with an empty label.
The QR-funding screens (`top_up_identity_screen/by_wallet_qr_code.rs:27-32`, `add_new_identity_screen/by_wallet_qr_code.rs`, `create_asset_lock_screen.rs`) still pass `Some("Managed by Dash Evo Tool")`, but `import_address_into_core` (mod.rs:761-777) short-circuits when `is_watchonly || is_mine`, so the second call never reaches `import_address` and the label is silently dropped.
Fix: either thread an optional label into `register_address` (synthesised from `self.alias` + `derivation_path` for the receive-address path) and forward it to `ensure_address_watched`, or have `import_address_into_core` overwrite the label even on already-imported addresses. Without one of these, every Core-RPC user loses operator-facing wallet labels they had before.
In `src/context/mod.rs`:
- [SUGGESTION] lines 743-753: Duplicate warn! per off-tree SPV registration, multiplied across bootstrap
`ensure_address_watched` emits `tracing::warn!` AND returns `Err(SpvOffTreeAddressRegistrationUnsupported)` for the off-tree+SPV branch. The fire-and-forget caller `Wallet::register_address` (`src/model/wallet/mod.rs:1206-1213`) catches that Err and emits its own warn with the same address (plus the extra `coverage` field). Every off-tree registration in SPV mode therefore produces two near-identical warnings, with the inner one missing the coverage context that makes the outer one useful.
This compounds with bootstrap call volume: `bootstrap_bip32_addresses` (16, BIP32→OffTree), `bootstrap_coinjoin_addresses` (16), identity-credit funding/topup/invitation bootstraps, and `identity_authentication_ecdsa_public_keys_data_map` (BlockchainIdentities) all pass through `register_address`. Pre-refactor `register_address` only attempted the import in RPC mode (`if core_backend_mode() == Rpc { try_import_address(...) }`), so SPV bootstrap was a silent no-op. Now a fresh SPV wallet load emits dozens of warn! lines for an expected, already-typed condition, which buries genuinely actionable warnings.
Fix in two parts: (1) drop the `tracing::warn!` inside `ensure_address_watched` — the typed error already carries the signal; let the caller log once at the policy boundary; (2) consider gating bootstrap-time `register_address` calls so only paths that genuinely need receive-side watching invoke the dispatcher in SPV mode (or downgrade those bootstrap call sites to `debug!` and reserve `warn!` for user-initiated registrations).
In `src/context/address_watch.rs`:
- [SUGGESTION] lines 1-103: Dispatcher behaviour itself is untested
The new tests cover only the trivial `AddressCoverage::from_derivation_path_reference` mapping. The dispatch matrix that is the whole point of this PR — `ensure_address_watched` returning `Ok(())` for `StandardBip44Account` + SPV, returning `TaskError::SpvOffTreeAddressRegistrationUnsupported { address }` for `OffTree` + SPV, and the warn-and-continue semantics in `Wallet::register_address` — is not pinned by any test. Since the stated goal is to prevent silent SPV no-ops from recurring, a small unit test that constructs a stub-mode `AppContext` (or mocks `core_backend_mode()` if practical) and asserts the four-cell dispatch matrix would lock the contract before it becomes the canonical pattern.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All five findings validated against head a03514e. Two real behavioral suggestions converge across both reviewers: BIP44 receive-address Core RPC labels regress because register_address now calls ensure_address_watched(..., None) and the labeled UI call is then short-circuited by import_address_into_core's is_watchonly || is_mine guard; and every off-tree SPV registration emits two near-identical warn! lines (one inside ensure_address_watched, one in the register_address catch), which compounds across wallet bootstrap. The remaining three findings are reasonable code-quality nits: the dispatch matrix the PR is built around is not pinned by tests, the new TaskError variant stores the address as a Base58 String instead of the typed Address (against CLAUDE.md guidance), and AddressCoverage::from_derivation_path_reference's catch-all silently absorbs future variants.
Reviewed commit: a03514e
Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.
Code Review
Reviewed commit: a03514e
🟡 3 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments 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 1001-1012: BIP44 receive addresses lose their descriptive Core RPC label
Pre-refactor, `unused_bip_44_public_key` called `try_import_address(&address, core_wallet, Some("Managed by Dash Evo Tool {alias} {derivation_path}"))` before `register_address`, so newly derived BIP44 receive addresses appeared in Core's wallet UI with a per-address, DET-attributable label (confirmed via `git show 3a7bc07c^:src/model/wallet/mod.rs`). After the refactor that explicit labeled call is gone (src/model/wallet/mod.rs:1001-1012) and address registration funnels through `register_address` → `ensure_address_watched(..., label = None)` (src/model/wallet/mod.rs:1196-1200), so Core receives the import with an empty label.
The QR-funding screens still pass `Some("Managed by Dash Evo Tool")` to `ensure_address_watched` (`top_up_identity_screen/by_wallet_qr_code.rs:27-32`, `add_new_identity_screen/by_wallet_qr_code.rs:33-49`, `create_asset_lock_screen.rs:111-128`), but `import_address_into_core` (src/context/mod.rs:761-777) short-circuits whenever `info.is_watchonly || info.is_mine`. Once `register_address` has done its own label-less import for a freshly-derived receive address, the screen's labeled call cannot overwrite it. Net effect for Core-RPC users: descriptive operator labels they had pre-refactor are silently replaced by an empty string.
Fix: thread an optional label through `register_address` → `ensure_address_watched` (synthesise the alias/derivation-path string at the BIP44 receive-address call site, mod.rs:1001-1012), or have `import_address_into_core` overwrite the label even when the address is already known. Either way, preserve the prior labeling behavior so existing wallets don't regress.
In `src/context/mod.rs`:
- [SUGGESTION] lines 743-753: Off-tree SPV registration emits duplicate warnings, multiplied across wallet bootstrap
`ensure_address_watched` emits `tracing::warn!` AND returns `Err(TaskError::SpvOffTreeAddressRegistrationUnsupported { address })` for the OffTree + SPV branch (src/context/mod.rs:743-753). The fire-and-forget caller `Wallet::register_address` (src/model/wallet/mod.rs:1196-1213) treats the error as best-effort, catches it, and emits its own `warn!` with the same address plus the extra `coverage = ?` field — so every off-tree registration in SPV mode produces two near-identical warning lines, with the outer one carrying strictly more context than the inner.
This compounds badly with wallet load: `bootstrap_known_addresses` (src/model/wallet/mod.rs:792-823) eagerly walks BIP32, CoinJoin, identity, provider, and platform-payment derivations, and `identity_authentication_ecdsa_public_keys_data_map` (BlockchainIdentities → OffTree) also passes through `register_address`. Pre-refactor `register_address` only attempted the import in RPC mode, so SPV bootstrap was a silent no-op; now a fresh SPV wallet load emits dozens of paired warn! lines for an expected, already-typed condition, drowning out genuinely actionable warnings.
Fix in two parts: (1) drop the inner `tracing::warn!` inside `ensure_address_watched` — the typed error already carries the signal; let the caller log once at the policy boundary; (2) consider downgrading bootstrap-time `register_address` call sites to `debug!` (or short-circuiting the OffTree + SPV path silently inside `ensure_address_watched`) so an expected typed condition during wallet bootstrap doesn't surface as `warn!`.
In `src/context/address_watch.rs`:
- [SUGGESTION] lines 64-103: Dispatcher behavior itself is untested
The new tests cover only the trivial `AddressCoverage::from_derivation_path_reference` mapping (src/context/address_watch.rs:64-103). The dispatch matrix that is the whole point of this PR — `ensure_address_watched` returning `Ok(())` for `StandardBip44Account` + SPV, returning `TaskError::SpvOffTreeAddressRegistrationUnsupported { address }` for `OffTree` + SPV, and the warn-and-continue best-effort semantics in `Wallet::register_address` — is not pinned by any test. Since the stated goal of the refactor is to prevent silent SPV no-ops from recurring, a small unit test that constructs a stub-mode `AppContext` (or factors the dispatch into a pure function over `(CoreBackendMode, AddressCoverage)`) and asserts the four-cell matrix would lock the contract before it becomes the canonical pattern. The API is intentionally using enum exhaustiveness and typed errors as a correctness boundary; that boundary should be exercised by tests.
2 participants
Summary
Replaces the pair
AppContext::ensure_address_imported/AppContext::try_import_addresswith a single typed entry pointensure_address_watched(address, coverage, …)that forces callers todeclare how SPV coverage is provided for the address and dispatches
accordingly. Eliminates the undocumented precondition that the old pair
silently relied on.
Motivation
The legacy pair:
PR #839's Copilot /
CodeRabbit threads and the minimum SPV guard added there).
by SPV's account-level watch — true for standard BIP44 receive
addresses derived from the wallet xprv; not true for DashPay
DIP-15 contact paths, imported single-key wallets, Blockchain-Identities
paths (
m/9'/…), Platform-Payment paths (DIP-17), or multisigoutputs.
would silently fail to be watched, and funds to that address would
never be discovered.
PR #839 patches the specific SPV-mode QR-funding symptom; this PR
addresses the underlying API shape so the bug class cannot recur.
Design
AddressCoverageenumA convenience constructor
AddressCoverage::from_derivation_path_reference(DerivationPathReference)maps
BIP44→StandardBip44Accountand every other variant →OffTree, so the wallet's internalregister_addresshelper picks theright coverage without burdening every caller with the classification.
Started at the simpler two-variant shape — richer variants
(
DashPayContact { our_identity, contact, index },RawImportedKey,…) can be added incrementally when they actually remove a string /
integer from downstream logic. None are needed today.
ensure_address_watcheddispatch matrixStandardBip44AccountOffTreeTaskError::SpvOffTreeAddressRegistrationUnsupportedwith the Base58 address.Error propagation replaces silent
let _: fire-and-forget call sitesmust now use
let _ = …or explicit matching, so the swallowing isvisible at the call site.
New
TaskErrorvarianti18n-ready, Everyday-User-friendly, actionable (reload wallet, or
switch to Expert-mode Core RPC), with the Base58 address as a
user-meaningful handle per CLAUDE.md rule 6.
Migration
5 call sites migrated, all standard-account funding addresses:
src/ui/identities/add_new_identity_screen/by_wallet_qr_code.rs:34,43— identity-creation QR fundingsrc/ui/identities/top_up_identity_screen/by_wallet_qr_code.rs:27— identity-top-up QR fundingsrc/ui/wallets/create_asset_lock_screen.rs:113,122— asset-lock fundingsrc/model/wallet/mod.rs:1196— genericregister_addresshelper (picks coverage fromDerivationPathReference)src/model/wallet/mod.rs:1001— duplicatetry_import_addressremoved;register_addressalready performs the registration via the typed APILegacy methods
ensure_address_importedandtry_import_addressareremoved in the final commit.
Not in scope / deferred
SpvManagerruntime registration hook. The running SPV loopcurrently has no API to add a single off-tree address (account-level
imports happen at wallet load). The typed API surfaces this as a
TaskErrorvariant and logs awarn!; a proper fix requires anupstream
SpvManager::register_addressesmethod, which belongs in aseparate PR. A
TODO(refactor)comment marks the spot insrc/context/mod.rs.try_import_addresspair — it maintains its own
wallet.known_addresses/wallet.watched_addressesbookkeeping insrc/backend_task/dashpay/incoming_payments.rs. So the suggested"commit 3" is a no-op; no DashPay code moves here. When the
SpvManager hook above lands, that path can start calling
ensure_address_watched(addr, AddressCoverage::OffTree, …)too.Test plan
cargo +nightly fmt --all -- --checkcargo clippy --all-features --all-targets -- -D warningscargo test --all-features --workspace— 467 + 72 + 10 + 3 tests pass, including two new unit tests onAddressCoverage::from_derivation_path_reference.cargo test --doc --all-features --workspace🤖 Co-authored by Claudius the Magnificent AI Agent