feat: manual DAPI node discovery, SDK update, and network config improvements

[lklimek]

Summary

Imagine you're a user whose testnet node addresses went stale after a network migration. Previously, you'd need to manually edit a .env file with new IP addresses. Now you open Network Settings, see "DAPI: 0/18 reachable", click "Fetch Node List", and the app fetches fresh addresses from the Dash network, saves them, and reconnects — all in one click.

Key changes

• Manual DAPI discovery: "Fetch Node List" button in Network Settings fetches fresh node addresses from a DCG-operated HTTPS service on demand. Available for Mainnet and Testnet only.
• No auto-discovery: The app always boots from configured addresses in the .env file. No magic, no silent network calls at startup.
• Health ratio display: "DAPI: {available}/{total} reachable" shown in the Node Addresses section, using the existing connection status tracking.
• Confirmation dialog: When addresses already exist, confirms before replacing.
• Trust disclosure: Info tooltip explains the DCG service trust model (convenience trust, not security trust).
• SDK update: dash-sdk + rs-sdk-trusted-context-provider track branch = "feat/mempool-support". initialize_sdk accepts AddressList directly.
• Network-aware RPC defaults: All 7 unwrap_or(9998) callsites replaced with NetworkConfig::rpc_port(network) — testnet defaults to 19998, devnet to 29998, regtest to 19898.
• Testnet seed nodes: Updated to new infrastructure at 68.67.122.1-29 (29 nodes).
• Network chooser: Mainnet and Testnet always visible; Devnet and Local only shown in developer mode.
• macOS accessibility: Eager AccessKit activation and labeled sidebar buttons.

Architecture

Boot:
  Read dapi_addresses from config → parse with AddressList::from_str()
  If empty → log error, context creation fails (user must configure addresses)

Refresh (user-triggered):
  Click "Fetch Node List" → BackendTask::DiscoverDapiNodes
  → try_discover_nodes() via HTTPS to DCG endpoint
  → Save addresses_csv to config
  → ReinitCoreClientAndSdk with new addresses
  → "Updated to {count} node addresses."

What was moved to PR #803

• Lazy network context creation (only active network at startup)
• BackendTask::SwitchNetwork for async network switching
• BackendTask::ReinitCoreClientAndSdk for async SDK reinit
• Devnet dynamic discovery support

Test plan

• cargo clippy --all-features --all-targets -- -D warnings passes
• cargo +nightly fmt --all passes
• App starts with configured .env DAPI addresses — uses them directly
• App starts with empty/missing DAPI addresses — graceful error, no crash
• Network Settings shows "DAPI: X/Y reachable" health ratio
• Click "Fetch Node List" on Mainnet/Testnet — fetches, saves, reconnects
• Click "Fetch Node List" with existing addresses — confirmation dialog appears
• Fetch failure — error banner, existing addresses untouched
• "Fetch Node List" button hidden for Devnet/Regtest
• Info tooltip explains DCG service trust model
• Testnet connects via new seed nodes (68.67.122.1-29 in .env.example)
• Network chooser: Mainnet + Testnet always visible, Devnet + Local in dev mode only
• Network-aware RPC port defaults work (testnet → 19998 when port omitted)

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds synchronous DAPI masternode discovery with fallback seeds, migrates legacy .env DAPI entries, makes DAPI/RPC config fields optional, resolves addresses before SDK init, exposes a public dapi_discovery module and error variant, and updates related call sites and tests.

Changes

Cohort / File(s)	Summary
Environment example \.env\.example	Removed explicit MAINNET/TESTNET DAPI lists; replaced with commented guidance and optional override examples.
Config & migration src/config.rs	Made DAPI/RPC fields Option<...>; added known-old address constants; added Config::migrate_env_file_if_needed() to comment-out legacy .env entries; adjusted save/parse to skip absent options; updated tests.
DAPI discovery src/dapi_discovery.rs, src/lib.rs	New public dapi_discovery module with DapiDiscoveryError and resolve_dapi_addresses_sync(...): parses explicit addresses, runs async discovery on mainnet/testnet via trusted provider, and falls back to hardcoded seed lists.
Context, SDK & wrapper src/context/..., src/sdk_wrapper.rs	Resolve DAPI addresses synchronously before SDK init; initialize_sdk now accepts an AddressList; core RPC host/port accessed via new rpc_host()/rpc_port() helpers.
App init & UI src/app.rs, src/ui/network_chooser_screen.rs	Call Config::migrate_env_file_if_needed() during AppState::new(); show a global banner when switching to an unavailable network context; tolerate optional RPC credentials via unwrap_or_default(); network selection gating simplified.
Error plumbing src/backend_task/error.rs	Added TaskError::DapiDiscovery(#[from] crate::dapi_discovery::DapiDiscoveryError).
Core & provider callers src/backend_task/core/mod.rs, src/context_provider.rs, src/spv/manager.rs	RPC host/port and credentials treated as optional; callers use rpc_host()/rpc_port() and unwrap_or_default() for user/password; chain-lock RPC error messages format URL using network-aware host/port.
Tests src/spv/tests.rs	Updated test helper to wrap previously-required RPC/DAPI fields in Some(...).
Dependency Cargo.toml	Added rs-sdk-trusted-context-provider dependency from Dashpay platform branch.

Sequence Diagram(s)

sequenceDiagram
    participant App as AppState::new()
    participant Config as Config::migrate_env_file_if_needed()
    participant AppCtx as AppContext::new()
    participant Discovery as resolve_dapi_addresses_sync()
    participant Provider as TrustedHttpContextProvider
    participant SDK as initialize_sdk()

    App->>Config: migrate_env_file_if_needed(.env)
    Config-->>App: (legacy entries commented if matched)

    App->>AppCtx: create with NetworkConfig
    AppCtx->>Discovery: resolve_dapi_addresses_sync(network, dapi_addresses?, devnet_name)

    alt explicit addresses present
        Discovery-->>AppCtx: AddressList (parsed from config)
    else no explicit addresses (mainnet/testnet)
        Discovery->>Provider: init & fetch masternode DAPI URLs (async via runtime)
        Provider-->>Discovery: list of URLs / error
        alt discovery succeeds
            Discovery-->>AppCtx: AddressList (discovered)
        else discovery fails
            Discovery-->>AppCtx: AddressList (from hardcoded seed nodes)
        end
    else other network without explicit addresses
        Discovery-->>AppCtx: AddressesRequired error
    end

    AppCtx->>SDK: initialize_sdk(address_list, network, ...)
    SDK-->>AppCtx: SDK instance or error
    AppCtx-->>App: context initialized or None

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nibbled old envs and tucked them away,

I asked the net kindly, then fell back to seeds,
I fetched a list of nodes and fed them to the SDK,
I patched the banners and let the networks play,
A rabbit hops on — discovery hums and leads.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main changes: dynamic DAPI node discovery, config migration, and SDK updates are the central themes across all modified files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch chore/update-dash-sdk-b70f39dc

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR updates Dash Platform connectivity to prefer dynamic DAPI node discovery when .env doesn’t explicitly configure nodes, while also bumping dash-sdk and centralizing healthcheck start/stop to the currently active network context.

Changes:

• Add DAPI address resolution with a 3-tier fallback (explicit .env → discovery → hardcoded fallback).
• Make NetworkConfig.dapi_addresses optional and add .env migration to comment out known legacy hardcoded lists.
• Update SDK initialization call sites and manage SDK healthchecks on app startup and network switches.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/spv/tests.rs	Updates test config to the new optional dapi_addresses field type.
src/sdk_wrapper.rs	Changes SDK init to accept a pre-resolved AddressList and disables built-in healthcheck config.
src/lib.rs	Exposes the new dapi_discovery module.
src/dapi_discovery.rs	Implements synchronous DAPI resolution with dynamic discovery + hardcoded fallback lists.
src/context/mod.rs	Resolves DAPI addresses before SDK init and restarts healthchecks on SDK rebuild.
src/config.rs	Makes dapi_addresses optional, adds env migration logic, adjusts save/validation/tests.
src/app.rs	Runs env migration at startup and toggles healthchecks on network changes.
Cargo.toml	Bumps dash-sdk rev and adds rs-sdk-trusted-context-provider.
Cargo.lock	Locks updated dependency graph for the SDK bump and new dependency.
.env.example	Removes hardcoded mainnet/testnet DAPI addresses and documents optional overrides.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[thepastaclaw]

✅ Review complete (commit aa2f0bb)

[Copilot]

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[thepastaclaw]

Code Review

DAPI discovery PR has two regressions: removing Testnet guard crashes app, saving settings blocks UI.

Reviewed commit: dd5fe9f

🔴 1 blocking | 🟡 1 suggestion(s)

2 additional findings

🔴 blocking: Selecting Testnet crashes when no context exists

src/ui/network_chooser_screen.rs (line 386)

Removed is_some() guard. AppState panics on missing context.

🟡 suggestion: Save blocks UI during DAPI discovery

src/ui/network_chooser_screen.rs (line 522)

Synchronous block_on(try_discover_nodes) freezes egui thread.

🤖 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/ui/network_chooser_screen.rs`:
- [BLOCKING] line 386: Selecting Testnet crashes when no context exists
  Removed is_some() guard. AppState panics on missing context.
- [SUGGESTION] line 522: Save blocks UI during DAPI discovery
  Synchronous block_on(try_discover_nodes) freezes egui thread.

[thepastaclaw]

Code Review

Well-structured PR with clean error types and proper trust model documentation. The main real bug is in the discovery result handler: discovered DAPI addresses are silently discarded when the target network has no existing config block, which is the exact scenario the feature is meant to address. The first_dapi_host trailing-slash issue is real but currently unreachable due to calling context guards. Several architectural suggestions are valid.

Reviewed commit: d6d90e5

🔴 1 blocking | 🟡 7 suggestion(s)

1 additional finding

🟡 suggestion: context_for_network silently falls back to mainnet — mutating actions may target wrong network

src/ui/network_chooser_screen.rs (lines 219-233)

context_for_network() returns &mainnet_app_context for any network without a context (the _ => catch-all on line 231). With Testnet now always selectable, code that calls context_for_network(Network::Testnet) without checking context existence will silently operate on mainnet. The discovery handler at line 2265 does check network_context_exists before using the context, which is good — but this guard pattern is opt-in and easy to miss in future code. Consider returning Option<&Arc<AppContext>> or adding a separate try_context_for_network so the compiler enforces handling.

🤖 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/ui/network_chooser_screen.rs`:
- [BLOCKING] lines 2248-2306: Discovered DAPI addresses silently discarded when target network has no config block
  The `if let` chain at lines 2248-2249 requires `config_for_network(network)` to return `Some(...)`. For Testnet — which is now always selectable in the network dropdown — this returns `None` when no `TESTNET_*` vars exist in `.env`. The entire discovery result is silently dropped: `discovery_in_progress` resets to false on line 2245, but no banner is shown and no addresses are saved.

This is the primary use case for the feature: a user selects Testnet, clicks "Refresh DAPI endpoints", discovery succeeds against the DCG endpoint, but the result vanishes with no feedback.

The fix should create a default `NetworkConfig` when the existing one is `None`, then populate it with the discovered addresses. An `else` branch should also handle the `Config::load_from` failure case with an error banner.
- [SUGGESTION] lines 219-233: context_for_network silently falls back to mainnet — mutating actions may target wrong network
  `context_for_network()` returns `&mainnet_app_context` for any network without a context (the `_ =>` catch-all on line 231). With Testnet now always selectable, code that calls `context_for_network(Network::Testnet)` without checking context existence will silently operate on mainnet. The discovery handler at line 2265 does check `network_context_exists` before using the context, which is good — but this guard pattern is opt-in and easy to miss in future code. Consider returning `Option<&Arc<AppContext>>` or adding a separate `try_context_for_network` so the compiler enforces handling.
- [SUGGESTION] line 2291: reinit_core_client_and_sdk called synchronously in display_task_result (UI thread)
  `reinit_core_client_and_sdk()` rebuilds the RPC client and SDK from the UI thread's `display_task_result` path. While `Client::new()` and SDK init are likely fast (no network I/O), they do hold and replace write locks. If SDK initialization ever adds blocking work, the UI would freeze. Consider dispatching as a `BackendTask` for consistency with the app's async architecture.

In `src/spv/manager.rs`:
- [SUGGESTION] lines 1500-1508: first_dapi_host does not strip trailing path/slash from DAPI URLs
  For URLs like `https://45.135.180.114/` (the exact format in `.env.example` mainnet addresses), `first_dapi_host` returns `45.135.180.114/` because it splits on `:` but never strips the path component. This would produce `45.135.180.114/:9999` in `primary_peer_socket()`, which `to_socket_addrs()` cannot resolve.

**Currently unreachable in practice**: `primary_peer_socket()` is only called for Devnet/Regtest (which use `:port` format) or when `use_local_node()` is true (which always sets `core_host` to `127.0.0.1` first). So `first_dapi_host` is never reached with trailing-slash URLs today. However, the function's contract promises to extract a host from any DAPI URL, and the latent bug will surface if any new code path calls it without the `core_host` guard.
- [SUGGESTION] lines 1500-1508: No unit tests for first_dapi_host URL parsing
  `first_dapi_host` manually parses URL strings with multiple edge cases (trailing slashes, missing schemes, ports, empty strings). A few focused unit tests would catch the trailing-slash bug and prevent regressions.

In `src/config.rs`:
- [SUGGESTION] lines 72-78: Regtest default RPC port (19898) doesn't match the app's docs and .env.example (20302)
  `default_rpc_port(Network::Regtest)` returns `19898` (standard Dash Core regtest port), but `.env.example`, `docs/local-network.md`, and all test fixtures use `20302` (dashmate's port). Since this app's setup guide assumes dashmate for local networks, the default is inconsistent. The mismatch only manifests when a user configures regtest without explicitly setting `core_rpc_port`, which the docs/example always include — so practical impact is low. But the default should match the app's own conventions to avoid confusion.

In `src/context/mod.rs`:
- [SUGGESTION] lines 164-184: Missing mainnet DAPI addresses prevent app launch — Refresh button is unreachable
  `AppContext::new()` returns `None` when DAPI addresses are absent (line 177-183), and `AppState::new()` treats a missing mainnet context as fatal (line 257-258). If a user's mainnet addresses are cleared or their `.env` is corrupted, the app won't start and the "Refresh DAPI endpoints" recovery button is unreachable. This is a pre-existing constraint (not introduced by this PR), but worth considering as the discovery feature matures — e.g., allowing the app to start in a degraded state with just the network settings screen.

In `src/backend_task/dapi_discovery.rs`:
- [SUGGESTION] lines 1-135: No unit tests for dapi_discovery module
  The module has no tests. While `try_discover_nodes` requires network access, the error construction paths and `discover_and_format` CSV formatting are testable locally with mocked inputs.

[thepastaclaw]

Code Review

Solid PR adding DAPI node discovery and making NetworkConfig fields optional. Two blocking issues: (1) the network combo box desyncs from AppState when the switch is rejected, silently falling back to mainnet context for screen-local operations, and (2) reinit_core_client_and_sdk() bypasses cookie-based auth used at startup, breaking RPC for cookie-auth setups. Several lower-severity issues around silent error handling and missing user feedback.

Reviewed commit: 0002ede

🔴 2 blocking | 🟡 5 suggestion(s) | 💬 1 nitpick(s)

2 additional findings

🔴 blocking: Network selector desyncs from AppState when switch is rejected, enabling cross-network config corruption

src/ui/network_chooser_screen.rs (lines 379-435)

selectable_value(&mut self.current_network, Network::Testnet, ...) mutates self.current_network immediately on click. AppAction::SwitchNetwork is then dispatched to AppState::change_network(), which checks context_available_for_network() and returns early with an error banner when the context doesn't exist — without updating AppState::chosen_network.

The screen now believes it's on Testnet while the app is still on Mainnet. Since context_for_network() (line 219-232) silently falls back to mainnet_app_context when the requested context is missing, all screen-local operations (Save RPC password at line 527, DAPI result handler at line 2295) that call context_for_network(self.current_network) will silently operate on mainnet.

Concrete scenario: User selects Testnet (no context) → screen shows Testnet UI → user enters an RPC password → Save writes it to mainnet's in-memory config and triggers mainnet SDK reinit.

Fix: Don't let selectable_value mutate self.current_network directly. Use a local variable and only commit the network change after verifying the context exists, or restore prev_network (already captured at line 379) when the context is unavailable.

🟡 suggestion: Saving RPC password is a silent no-op when the network has no config block

src/ui/network_chooser_screen.rs (lines 489-500)

The let Some(network_cfg) = config.config_for_network(self.current_network).clone() guard (line 492-493) means that if the selected network has no config section in .env (e.g. Testnet with no TESTNET_* vars), the entire save block is skipped. No file is written, no banner is shown — the user clicks Save and nothing happens.

This is reachable: the combo box now shows Testnet unconditionally, and newly defaulted networks won't have a persisted config block. Add an else branch with a user-facing banner explaining that the network needs to be configured first.

🤖 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/ui/network_chooser_screen.rs`:
- [BLOCKING] lines 379-435: Network selector desyncs from AppState when switch is rejected, enabling cross-network config corruption
  `selectable_value(&mut self.current_network, Network::Testnet, ...)` mutates `self.current_network` immediately on click. `AppAction::SwitchNetwork` is then dispatched to `AppState::change_network()`, which checks `context_available_for_network()` and returns early with an error banner when the context doesn't exist — **without** updating `AppState::chosen_network`.

The screen now believes it's on Testnet while the app is still on Mainnet. Since `context_for_network()` (line 219-232) silently falls back to `mainnet_app_context` when the requested context is missing, all screen-local operations (Save RPC password at line 527, DAPI result handler at line 2295) that call `context_for_network(self.current_network)` will silently operate on mainnet.

Concrete scenario: User selects Testnet (no context) → screen shows Testnet UI → user enters an RPC password → Save writes it to mainnet's in-memory config and triggers mainnet SDK reinit.

Fix: Don't let `selectable_value` mutate `self.current_network` directly. Use a local variable and only commit the network change after verifying the context exists, or restore `prev_network` (already captured at line 379) when the context is unavailable.
- [SUGGESTION] lines 2296-2300: Poisoned config lock silently ignored — success banner shows with stale config
  `if let Ok(mut cfg_lock) = app_context.config.write()` silently discards a poisoned-lock error. If the write fails, `network_cfg` is not applied, but `reinit_core_client_and_sdk()` still runs and reads the old config. The success banner at line 2312 still shows, telling the user addresses were updated when they were not.

Note: the Save-password path at line 529 uses `.unwrap()` on the same lock — inconsistent handling.

A poisoned lock is rare (requires a prior panic), but when it happens the silent success is misleading.
- [SUGGESTION] lines 489-500: Saving RPC password is a silent no-op when the network has no config block
  The `let Some(network_cfg) = config.config_for_network(self.current_network).clone()` guard (line 492-493) means that if the selected network has no config section in `.env` (e.g. Testnet with no `TESTNET_*` vars), the entire save block is skipped. No file is written, no banner is shown — the user clicks Save and nothing happens.

This is reachable: the combo box now shows Testnet unconditionally, and newly defaulted networks won't have a persisted config block. Add an `else` branch with a user-facing banner explaining that the network needs to be configured first.
- [SUGGESTION] lines 2227-2232: Any unrelated error banner resets discovery in-progress flag
  `display_message()` clears `discovery_in_progress` on every `MessageType::Error`, not just DAPI discovery errors. If an unrelated backend task (RPC timeout, SPV error) completes with an error while DAPI discovery is in flight, the Refresh button re-enables prematurely, allowing the user to spawn a duplicate discovery task.

The comment on line 2228-2229 acknowledges that non-error messages can be unrelated, but doesn't account for unrelated errors. Consider gating the reset on a task-specific flag or only resetting in `display_task_result`.

In `src/context/mod.rs`:
- [BLOCKING] lines 565-576: reinit_core_client_and_sdk() drops cookie-based Core RPC auth
  Startup uses `create_core_rpc_client()` (line 640-663), which tries cookie auth first via `Auth::CookieFile` and falls back to `Auth::UserPass` only when the cookie path is unavailable.

`reinit_core_client_and_sdk()` bypasses this and unconditionally builds `Client::new(&addr, Auth::UserPass(...))`. When `core_rpc_user` and `core_rpc_password` are both `None` (the norm for cookie-auth setups), this sends `Auth::UserPass("", "")` — which Core will reject.

Triggered after DAPI discovery completes (line 2302 in network_chooser_screen.rs) or after saving an RPC password. Users on cookie auth will see "reconnection failed" after every DAPI refresh.

In `src/backend_task/core/mod.rs`:
- [SUGGESTION] lines 496-501: RPC connection error details lost — source: None discards root cause
  `TaskError::CoreRpcConnectionFailed { url, source: None }` at line 501 discards the original `dashcore_rpc::Error`. The error variant has a `source: Option<Box<dashcore_rpc::Error>>` field specifically for preserving diagnostics (connection refused vs. timeout vs. DNS failure), but it's passed as `None`. The user sees "Could not connect to Dash Core at host:port" with no technical details available in the collapsible panel.

If `dashcore_rpc::Error` implements `Clone`, pass `Some(Box::new(e.clone()))`. Otherwise, consider restructuring to take ownership of the error.

In `src/app.rs`:
- [SUGGESTION] lines 257-258: Recovery flow unreachable when mainnet DAPI addresses are missing or malformed
  This PR adds DAPI node discovery to recover from stale addresses, but the recovery button is only reachable after successful startup. `AppContext::new()` returns `None` when `dapi_addresses` is `None`, empty, or unparseable (lines 165-183 in context/mod.rs). Since mainnet context is required for startup (line 257-258), a completely broken mainnet DAPI config prevents the app from launching — the user can never reach Refresh DAPI Endpoints.

This is pre-existing behavior, not a regression from this PR. The PR still helps for the common case (stale but parseable addresses). Consider a follow-up that allows startup with a degraded mainnet context when DAPI addresses are missing, enabling the recovery flow for this edge case.

[thepastaclaw]

🟡 Suggestion: Any unrelated error banner resets discovery in-progress flag

display_message() clears discovery_in_progress on every MessageType::Error, not just DAPI discovery errors. If an unrelated backend task (RPC timeout, SPV error) completes with an error while DAPI discovery is in flight, the Refresh button re-enables prematurely, allowing the user to spawn a duplicate discovery task.

The comment on line 2228-2229 acknowledges that non-error messages can be unrelated, but doesn't account for unrelated errors. Consider gating the reset on a task-specific flag or only resetting in display_task_result.

_{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/ui/network_chooser_screen.rs`:
- [SUGGESTION] lines 2227-2232: Any unrelated error banner resets discovery in-progress flag
  `display_message()` clears `discovery_in_progress` on every `MessageType::Error`, not just DAPI discovery errors. If an unrelated backend task (RPC timeout, SPV error) completes with an error while DAPI discovery is in flight, the Refresh button re-enables prematurely, allowing the user to spawn a duplicate discovery task.

The comment on line 2228-2229 acknowledges that non-error messages can be unrelated, but doesn't account for unrelated errors. Consider gating the reset on a task-specific flag or only resetting in `display_task_result`.

[thepastaclaw]

🟡 Suggestion: RPC connection error details lost — source: None discards root cause

TaskError::CoreRpcConnectionFailed { url, source: None } at line 501 discards the original dashcore_rpc::Error. The error variant has a source: Option<Box<dashcore_rpc::Error>> field specifically for preserving diagnostics (connection refused vs. timeout vs. DNS failure), but it's passed as None. The user sees "Could not connect to Dash Core at host:port" with no technical details available in the collapsible panel.

If dashcore_rpc::Error implements Clone, pass Some(Box::new(e.clone())). Otherwise, consider restructuring to take ownership of the error.

_{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/core/mod.rs`:
- [SUGGESTION] lines 496-501: RPC connection error details lost — source: None discards root cause
  `TaskError::CoreRpcConnectionFailed { url, source: None }` at line 501 discards the original `dashcore_rpc::Error`. The error variant has a `source: Option<Box<dashcore_rpc::Error>>` field specifically for preserving diagnostics (connection refused vs. timeout vs. DNS failure), but it's passed as `None`. The user sees "Could not connect to Dash Core at host:port" with no technical details available in the collapsible panel.

If `dashcore_rpc::Error` implements `Clone`, pass `Some(Box::new(e.clone()))`. Otherwise, consider restructuring to take ownership of the error.

[thepastaclaw]

Code Review

Clean incremental addressing two prior review findings. The cookie auth regression is fully resolved by delegating to create_core_rpc_client(). The hardcoded mainnet_app_context references in the DAPI discovery handler are replaced with current_app_context(). The network selector desync (prior blocking finding about selectable_value mutating current_network before AppState confirms the switch) is not yet addressed in this push.

Reviewed commit: aa2f0bb

🟡 1 suggestion(s)

1 additional finding

🟡 suggestion: Network selector desync still present (prior blocking finding)

src/ui/network_chooser_screen.rs (lines 379-420)

The prior review flagged that selectable_value(&mut self.current_network, ...) mutates self.current_network immediately on click, before AppState::change_network() validates whether the target context exists. If the switch is rejected, the screen believes it's on the new network while the app stays on the old one.

This push fixes the hardcoded mainnet_app_context references (good — reduces impact of the desync), but the root cause remains: self.current_network is still mutated by the combo box before validation. The context_for_network() fallback-to-mainnet still applies to all screen-local operations when the selected network has no context.

The fix from the prior review still applies: use a local variable for the combo box, and only commit the change to self.current_network after confirming the context exists (or restore prev_network on rejection).

🤖 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/ui/network_chooser_screen.rs`:
- [SUGGESTION] lines 379-420: Network selector desync still present (prior blocking finding)
  The prior review flagged that `selectable_value(&mut self.current_network, ...)` mutates `self.current_network` immediately on click, before `AppState::change_network()` validates whether the target context exists. If the switch is rejected, the screen believes it's on the new network while the app stays on the old one.

This push fixes the hardcoded `mainnet_app_context` references (good — reduces impact of the desync), but the root cause remains: `self.current_network` is still mutated by the combo box before validation. The `context_for_network()` fallback-to-mainnet still applies to all screen-local operations when the selected network has no context.

The fix from the prior review still applies: use a local variable for the combo box, and only commit the change to `self.current_network` after confirming the context exists (or restore `prev_network` on rejection).