feat: working SPV mode, with identity creation support

[PastaPastaPasta]

Summary by CodeRabbit

• New Features

  • Added SPV finality event handling for asset lock proofs (instant and chain locks)
  • Integrated DAPI transaction queries for status verification
  • Display connected peer count in SPV sync status
  • Enhanced UI labels for keys, identities, and contracts

• Bug Fixes

  • Fixed transaction broadcast synchronization
  • Improved balance synchronization after asset locks
  • Enhanced address balance refresh in receive dialog

• Improvements

  • Optimized asset lock timeout based on backend mode (SPV vs RPC)
  • Better wallet state and UTXO management
  • Improved BIP32 account naming

Summary by CodeRabbit

• New Features

  • SPV mode now handles finality events for improved transaction confirmation tracking.
  • Added unified transaction broadcasting across RPC and SPV backends.
  • Introduced UI helpers for clearer identity, contract, and key labeling.

• Bug Fixes

  • Improved wallet balance accuracy after spending UTXOs.
  • Fixed core address balance refresh in wallet receive dialogs.

• Refactoring

  • Streamlined transaction handling and asset lock proof waiting logic.
  • Centralized wallet balance recalculation across operations.

[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
• ✅ Review completed - (🔄 Check again to review again)

📝 Walkthrough

Walkthrough

Updated dash-sdk dependency and added SPV-driven asset-lock finality handling. Asset-lock creation methods were converted to async and now broadcast transactions via a unified broadcast path. Identity registration/top-up and wallet balance/UTXO reconciliation were made backend-mode (RPC vs SPV) aware.

Changes

Cohort / File(s)	Summary
Dependency Update Cargo.toml	Bumped dash-sdk rev and expanded features (added core_quorum-validation, core_verification, core_rpc_client, core_spv).
Asset Lock Creation (Async) src/backend_task/core/create_asset_lock.rs	Converted registration/top-up asset-lock creators to async fn returning Result<BackendTaskSuccessResult, String>; now use broadcast_raw_transaction and trigger wallet balance recalculation after broadcast.
Core Task Async Handling src/backend_task/core/mod.rs	run_core_task now awaits asset-lock creation futures. SPV height for unsigned transaction building now uses spv_manager.status().sync_progress.header_height fallback.
Incoming Payments Comment src/backend_task/dashpay/incoming_payments.rs	Updated comment to reference WalletEvent::TransactionReceived (no behavior change).
Identity Registration (Mode-Aware) src/backend_task/identity/register_identity.rs	Branch flows by CoreBackendMode (Rpc vs Spv); replaced direct RPC sends with broadcast_raw_transaction, centralized proof waiting via wait_for_asset_lock_proof, and added mode-specific error/fallback handling and balance recalculation calls.
Top-Up Identity (Mode-Aware) src/backend_task/identity/top_up_identity.rs	Similar mode-aware adjustments: use broadcast_raw_transaction, wait_for_asset_lock_proof, get_transaction_info, mode-specific retries/propagation, and wallet balance recalculation helpers.
Context & SPV Finality src/context.rs	Added broadcast_raw_transaction, wait_for_asset_lock_proof, get_transaction_info and DapiTransactionInfo. Added SPV finality wiring (spv_setup_finality_listener, handle_spv_finality_event), SPV reconcile enhancements, and exported DerivationPathHelpers.
Wallet Balance Recalculation APIs src/model/wallet/mod.rs	Added DerivationPathHelpers::is_bip32() and wallet methods recalculate_affected_address_balances and recalculate_address_balance to recompute/persist balances from UTXO state.
QualifiedIdentity / Decode Generics src/model/qualified_identity/...	Made Decode/BorrowDecode impls generic over context type C for WalletDerivationPath and QualifiedIdentity; QualifiedIdentity decode now initializes several fields from DB defaults.
SPV Manager & Events src/spv/manager.rs, src/spv/mod.rs	Introduced AssetLockFinalityEvent (InstantLock/ChainLock), register_finality_channel, expected_wallet_count startup coordination, connected_peers in status, and refactored event handlers; module visibility set to pub(crate) and crate-private re-export added.
Backend Wallet Flows src/backend_task/wallet/*.rs	Replaced get_transaction_info_via_dapi calls with get_transaction_info; top-up/funding flows now use broadcast path and wallet balance recalculation helpers.
UI Helpers & Wallet Screens src/ui/helpers.rs, src/ui/wallets/*	Added label helpers, token authorization helpers, compute_allowed_security_levels, and UI balance cache-refresh that reads SPV-derived address balances for display; minor label/description updates.

Sequence Diagram(s)

sequenceDiagram
    participant App as AppContext / Wallet
    participant SPV as SPV Manager
    participant Core as Core/RPC (DAPI)
    participant DB as Database
    participant UI as UI

    App->>SPV: broadcast_raw_transaction(tx)
    alt SPV backend active
        SPV->>SPV: accept & monitor tx (InstantLock/ChainLock)
    else RPC backend active
        SPV->>Core: send raw tx via RPC
        Core-->>SPV: tx accepted/confirmed
    end

    SPV->>DB: persist asset-lock transaction
    SPV->>DB: update UTXOs and address balances
    SPV-->>App: AssetLockFinalityEvent (InstantLock/ChainLock)
    App->>DB: recalculate_affected_address_balances(used_utxos)
    App->>UI: notify balance update
    UI->>DB: read updated balances
    DB-->>UI: return balances

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped a patch across the chain,

Async hops make balance sane,
Instant locks and chain‑lock song,
RPC or SPV — both belong,
Wallets hum, UTXOs align.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main feature: SPV mode with identity creation support, matching the extensive SPV and identity-related changes throughout the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/working-spv

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

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.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/backend_task/identity/register_identity.rs (1)

165-194: ⚠️ Potential issue | 🟠 Major

The TODO accurately describes a real data integrity risk.

The UTXO removal timing issue (lines 165-169) is correctly flagged. If the asset lock proof times out or identity creation fails after line 180, the UTXOs will be permanently "lost" from wallet tracking even though they weren't actually spent. This could cause user confusion and balance discrepancies.

The current code stores the asset lock transaction in the DB (lines 155-163) which provides some recovery path, but the UTXO restoration logic would need to be implemented.

Would you like me to help design a recovery mechanism or open an issue to track this for Phase 2.2 as mentioned in the comment?

🤖 Fix all issues with AI agents

In `@src/backend_task/core/create_asset_lock.rs`:
- Around line 60-71: The calls that currently drop the Result from
wallet_guard.update_address_balance(&address, new_balance, self) must not be
ignored: check the Result and either log the error with context (address,
new_balance) using the project's logger/tracing macro or propagate the error up
by returning Err from the surrounding function; apply the same change to the
other occurrence around the 133-144 block. Locate the calls to
update_address_balance and replace the unused let _ = ... with a match or ?
handling that logs error details (including address and new_balance) or returns
the error so DB/state failures are visible.

In `@src/context.rs`:
- Around line 1575-1589: The broadcast_raw_transaction function currently calls
self.core_client.read().expect(...), which can panic on a poisoned lock; replace
this with proper error handling: attempt to acquire the RwLock via
self.core_client.read().map_err(|e| format!("core client lock poisoned: {}",
e))? (or match the Result and return Err(String) on poison) and then call
send_raw_transaction on the acquired guard, propagating send_raw_transaction
errors via map_err(|e| e.to_string()). Update the CoreBackendMode::Rpc arm to
return Err(String) instead of panicking while keeping the Spv branch unchanged;
references: broadcast_raw_transaction, core_client, core_backend_mode,
CoreBackendMode::Rpc, send_raw_transaction.

In `@src/spv/manager.rs`:
- Around line 1105-1121: The finality event forwarding currently uses
ftx.try_send(...) which can silently drop SyncEvent::InstantLockReceived and
SyncEvent::ChainLockReceived when the finality_tx buffer is full; update the
code in the block referencing finality_tx,
SyncEvent::InstantLockReceived/ChainLockReceived and AssetLockFinalityEvent to
use ftx.send(...).await (or otherwise await a send on the same channel) and
handle Err by logging the failure (include context like event type and
identifiers such as instant_lock.txid or chain_lock.block_height);
alternatively, if awaiting is unacceptable for backpressure reasons, increase
the finality_tx buffer capacity and still log try_send failures so these
critical events are never silently dropped.

🧹 Nitpick comments (6)

src/model/wallet/mod.rs (1)

143-147: Clarify BIP32 path shape and add a small test matrix.

is_bip32 currently matches any 2–3 component path starting with m/0', which can misclassify other 0'‑prefixed paths and would ignore non‑zero accounts if you ever expand BIP32 account usage. If the wallet only uses m/0'/index (or m/0'/change/index), tighten the match and add inline tests to lock in the intended structure.

🔧 Suggested tightening (aligns with current bootstrap_bip32_addresses shape)

-        let components = self.as_ref();
-        matches!(components.len(), 2..=3)
-            && components[0] == ChildNumber::Hardened { index: 0 }
+        match self.as_ref() {
+            [ChildNumber::Hardened { index: 0 }, ChildNumber::Normal { .. }] => true,
+            [ChildNumber::Hardened { index: 0 }, ChildNumber::Normal { .. }, ChildNumber::Normal { .. }] => true,
+            _ => false,
+        }

Optional inline tests:

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

    #[test]
    fn is_bip32_matches_expected_paths() {
        let p1 = DerivationPath::from(vec![
            ChildNumber::Hardened { index: 0 },
            ChildNumber::Normal { index: 0 },
        ]);
        assert!(p1.is_bip32());

        let p2 = DerivationPath::from(vec![
            ChildNumber::Hardened { index: 0 },
            ChildNumber::Normal { index: 0 },
            ChildNumber::Normal { index: 1 },
        ]);
        assert!(p2.is_bip32());

        let p3 = DerivationPath::from(vec![
            ChildNumber::Hardened { index: 1 },
            ChildNumber::Normal { index: 0 },
        ]);
        assert!(!p3.is_bip32());
    }
}

As per coding guidelines: “Write unit tests inline in source files using #[test] attribute”.

src/model/qualified_identity/mod.rs (1)

273-291: Add inline tests for the new decode defaults

Please add #[test] coverage for decode defaults (e.g., wallet_index, top_ups, status, network) to lock in the new behavior.

As per coding guidelines: Write unit tests inline in source files using #[test] attribute.

src/ui/helpers.rs (1)

14-205: Add inline tests for the new helper logic.

compute_allowed_security_levels and check_token_authorization are core policy helpers; inline unit tests will lock in behavior across refactors. As per coding guidelines: Write unit tests inline in source files using #[test] attribute.

src/backend_task/identity/top_up_identity.rs (1)

210-242: Consider extracting duplicated timeout/wait logic into a helper function.

The asset lock proof waiting logic with timeout is duplicated between FundWithWallet (lines 210-242) and FundWithUtxo (lines 322-354). This includes identical timeout configuration, polling loop, and error handling.

♻️ Suggested helper function

async fn wait_for_asset_lock_proof(
    &self,
    tx_id: Txid,
    timeout_duration: Duration,
) -> Result<AssetLockProof, String> {
    match tokio::time::timeout(timeout_duration, async {
        loop {
            {
                let proofs = self.transactions_waiting_for_finality.lock().unwrap();
                if let Some(Some(proof)) = proofs.get(&tx_id) {
                    return proof.clone();
                }
            }
            tokio::time::sleep(Duration::from_millis(200)).await;
        }
    })
    .await
    {
        Ok(proof) => Ok(proof),
        Err(_) => {
            let mut proofs = self.transactions_waiting_for_finality.lock().unwrap();
            proofs.remove(&tx_id);
            Err(format!(
                "Timeout waiting for asset lock proof after {} seconds. \
                 The transaction may not have been confirmed by the network.",
                timeout_duration.as_secs()
            ))
        }
    }
}

Also applies to: 322-354

src/backend_task/identity/register_identity.rs (1)

196-225: Duplicated timeout/wait logic - same refactor applies here.

This is the same code duplication pattern as in top_up_identity.rs. The timeout logic for waiting on asset lock proofs could be extracted into a shared helper method on AppContext.

Also applies to: 336-365

src/context.rs (1)

817-836: Consider adding error logging when chain lock finality processing fails.

In the ChainLock branch (lines 817-836), errors from received_asset_lock_finality are silently ignored with let _ = ... (line 823). While individual failures shouldn't halt processing of other transactions, logging would help diagnose issues.

💡 Suggested improvement

                             if let Ok(Some((tx, ..))) =
                                 self.db.get_asset_lock_transaction(txid.as_byte_array())
                             {
-                                let _ = self.received_asset_lock_finality(
+                                if let Err(e) = self.received_asset_lock_finality(
                                     &tx,
                                     None,
                                     Some(tx_info.height),
-                                );
+                                ) {
+                                    tracing::debug!(
+                                        %txid,
+                                        error = %e,
+                                        "Failed to process chain lock finality for asset lock"
+                                    );
+                                }
                             }

[thephez]

@PastaPastaPasta this PR summary isn't accurate, is it?

[PastaPastaPasta]

Seems like it :) maybe it thought "master" was the base for the purpose of seeing what was included not v1.0-dev

[thephez]

I've had it do that. Can you fix please?

[thephez]

Identity create using Core worked @ 96412b6
https://testnet.platform-explorer.com/identity/J6WMURtVGoPQKbnyvdRzMvpew8TB8GC2X3MttvtNz7qR

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@src/backend_task/identity/top_up_identity.rs`:
- Around line 208-237: The success branch currently leaves entries in
transactions_waiting_for_finality after returning the cloned asset_lock_proof,
causing an unbounded map growth; modify the code handling the
tokio::time::timeout loops (the block that awaits the asset_lock_proof using
tx_id and transactions_waiting_for_finality) so that once you obtain Ok(proof)
you acquire the same mutex, remove the entry for tx_id from
transactions_waiting_for_finality, then return the cloned proof; apply the same
removal change to the second identical loop (around the asset lock proof
consumption at the later occurrence) to ensure entries are cleaned up on success
as well as on timeout.
- Around line 118-138: The code panics on a poisoned RwLock read via
self.core_client.read().expect(...); replace the expect with a non-panicking
propagation: attempt to read the lock (self.core_client.read()) and map the
PoisonError into the function's error return (using map_err or ? after mapping)
so the error is returned rather than aborting; apply this change in the match
arm handling CoreBackendMode::Rpc where reload_utxos and
top_up_asset_lock_transaction are called (referenced symbols: self.core_client,
core_backend_mode / CoreBackendMode::Rpc, wallet.reload_utxos,
wallet.top_up_asset_lock_transaction) so that a poisoned lock yields a
propagated error instead of panic.

In `@src/spv/manager.rs`:
- Around line 818-821: The SPV client can miss historical transactions when all
wallets are locked because expected_wallet_count only counts open wallets;
update logic in the flow that computes expected_wallet_count (used before
calling build_client) to include wallets that exist but are locked so
start_height is not set to u32::MAX, or alternatively add logic in the wallet
unlock handler (where load_wallet_from_seed is called and reconciliation is
triggered) to call the DashSpvClient rescan/restart routine (or invoke
build_client again/trigger a filter rescan) when the first wallet transitions
from locked -> unlocked; modify the code around expected_wallet_count,
build_client, and the wallet event handler to implement one of these fixes and
ensure start_height and DashSpvClient state are updated to scan historical
filters for newly unlocked wallets.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/backend_task/identity/register_identity.rs`:
- Around line 110-139: Replace the panic-causing .expect("Core client lock was
poisoned") in register_identity.rs with graceful error propagation like in
top_up_identity.rs: call self.core_client.read().map_err(|e| format!("core
client lock poisoned: {:?}", e))? and pass the resulting guard into
wallet.reload_utxos so the function returns an Err instead of panicking; update
the two places using self.core_client.read() in the Err(e) match branch (the
call in reload_utxos and any subsequent uses) to use the mapped error + ?
operator.

[Copilot]

Pull request overview

This PR advances SPV mode to support identity registration/top-ups by wiring SPV finality events (instant/chain locks) into the app flow, improving SPV wallet reconciliation, and polishing related UI labeling.

Changes:

• Add SPV finality event forwarding and DAPI-based chain-lock verification to unblock asset-lock proof creation for identity flows.
• Refactor SPV runtime event subscriptions (sync/wallet/network), expose peer count, and adjust client startup behavior.
• Improve UI labeling helpers (keys/identities/contracts) and tweak legacy BIP32 account naming/description.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/ui/wallets/wallets_screen/mod.rs	Refresh per-address receive balances from cached wallet state so SPV updates reflect in UI.
src/ui/wallets/account_summary.rs	Improve legacy BIP32 account labeling and description text.
src/ui/helpers.rs	Centralize UI label formatting and shared security-level / token-authorization logic.
src/spv/mod.rs	Expose SPV manager module and re-export finality event type within the crate.
src/spv/manager.rs	Rework SPV runtime subscriptions (sync/wallet/network/progress), add finality forwarding + connected peer count, change start behavior.
src/model/wallet/mod.rs	Add DerivationPathHelpers::is_bip32 helper.
src/model/qualified_identity/mod.rs	Update bincode Decode generic context signature.
src/model/qualified_identity/encrypted_key_storage.rs	Update bincode Decode / BorrowDecode generic context signatures.
src/context.rs	Add SPV finality listener handling, improve SPV reconcile (UTXO/balance/tx history), and add backend-agnostic tx broadcast helper.
src/backend_task/identity/top_up_identity.rs	Use backend-agnostic tx broadcast + backend-specific proof timeout.
src/backend_task/identity/register_identity.rs	Use backend-agnostic tx broadcast + backend-specific proof timeout and SPV-mode behavior.
src/backend_task/dashpay/incoming_payments.rs	Update comment to reflect new wallet event source naming.
src/backend_task/core/mod.rs	Await asset-lock creation tasks and adjust SPV tx building height source.
src/backend_task/core/create_asset_lock.rs	Make asset-lock creation async, use unified broadcast, and update affected address balances after UTXO drops.
Cargo.toml	Bump dash-sdk git revision.
Cargo.lock	Update dependency lock entries for new dash-sdk / dashcore versions and related crates.

Comments suppressed due to low confidence (1)

src/context.rs:1038

• During SPV reconcile, per-address balances are only updated for addresses present in per_address_sum (i.e., those with at least one UTXO). Addresses that previously had UTXOs but now have none will keep a stale non-zero balance in wallet.address_balances (and the Receive dialog reads from this cache). After building per_address_sum, consider iterating over the wallet’s known/watched addresses (or existing address_balances keys) and explicitly setting missing addresses to 0 (and optionally clearing entries no longer applicable).

            // Write per-address balances and UTXOs into wallet model
            if let Some(wref) = wallets_guard.get(seed_hash)
                && let Ok(mut w) = wref.write()
            {
                // Update in-memory UTXOs map
                w.utxos = new_utxos;

                for (addr, sum) in per_address_sum.into_iter() {
                    // Update wallet and DB through model helper
                    let _ = w.update_address_balance(&addr, sum, self);
                }

---

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

[lklimek]

Creating a new thread here and starting second runtime doesn't look correct (but maybe a task for separate PR)

[lklimek]

@copilot create an issue to review thread model of spv and resolve this comment.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs (2)

88-99: ⚠️ Potential issue | 🟡 Minor

Finality map entry leaks on broadcast failure.

If send_raw_transaction at line 98 fails, the entry inserted at line 91 is never removed from transactions_waiting_for_finality. Over repeated failures this map will grow unboundedly.

🛡️ Suggested fix

         // Step 3: Broadcast the transaction
-        self.core_client
+        if let Err(e) = self.core_client
             .read()
             .expect("Core client lock was poisoned")
             .send_raw_transaction(&asset_lock_transaction)
-            .map_err(|e| format!("Failed to broadcast asset lock transaction: {}", e))?;
+        {
+            // Clean up the finality tracking entry on broadcast failure
+            let mut proofs = self.transactions_waiting_for_finality.lock().unwrap();
+            proofs.remove(&tx_id);
+            return Err(format!("Failed to broadcast asset lock transaction: {}", e));
+        }

---

126-137: ⚠️ Potential issue | 🟠 Major

Unbounded wait loop with no timeout.

The loop at lines 128–137 polls transactions_waiting_for_finality every 200ms with no timeout. If the proof never arrives (e.g., transaction dropped, network issue), this function will hang indefinitely, blocking the backend task executor.

Other paths in this PR use wait_for_asset_lock_proof which presumably includes a timeout. Consider using the same centralized helper here for consistency and safety.

src/context.rs (1)

1035-1045: ⚠️ Potential issue | 🟠 Major

Reset balances for known addresses with zero UTXOs.
Currently only addresses present in per_address_sum are updated, so a spent-out address keeps a stale balance in memory/DB.

✅ Suggested fix

-                for (addr, sum) in per_address_sum.into_iter() {
-                    // Update wallet and DB through model helper
-                    let _ = w.update_address_balance(&addr, sum, self);
-                }
+                for addr in known_addresses.iter() {
+                    let sum = per_address_sum.get(addr).copied().unwrap_or(0);
+                    let _ = w.update_address_balance(addr, sum, self);
+                }

🤖 Fix all issues with AI agents

In `@src/backend_task/identity/register_identity.rs`:
- Around line 181-182: The call to
wallet.recalculate_affected_address_balances(&used_utxos, self) currently
ignores its Result which can hide failures and leave DB/wallet out of sync;
change the call sites (the one around the shown line and the similar call at the
later occurrence) to handle the Result instead of dropping it—either propagate
the error by returning a Result (use ? from the enclosing function) or
explicitly log the error (e.g., tracing::error! or your crate logger) and
return/abort as appropriate; update the enclosing function signature if you
choose propagation so the compiler surfaces the failure from
recalculate_affected_address_balances.

In `@src/model/wallet/mod.rs`:
- Around line 1816-1845: Add inline unit tests in this module using #[test] that
exercise recalculate_address_balance and recalculate_affected_address_balances:
create wallet state with addresses having zero UTXOs and with multiple addresses
sharing used_utxos, call the functions and assert address_balances (and
persisted DB via AppContext mock or in-memory test context) reflect zero for
empty-address and correct sums for multi-UTXO/multi-address cases; name tests
clearly (e.g., test_recalculate_address_balance_zero_utxos,
test_recalculate_affected_address_balances_multi_address) and ensure they seed
self.utxos and used_utxos appropriately and clean up any test context.

🧹 Nitpick comments (3)

src/ui/wallets/wallets_screen/mod.rs (1)

1893-1909: Platform address balances are not refreshed alongside Core addresses.

The new refresh block updates core_addresses balances from the wallet on every render, but platform_addresses in the same dialog are not similarly refreshed. If SPV updates platform balances while the Receive dialog is open, those won't be reflected until the dialog is reopened.

Additionally, parsing each address from its string representation on every frame is a minor overhead. Consider caching the typed Address alongside the string in core_addresses (e.g., Vec<(String, Address, u64)>) to avoid repeated parsing.

src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs (1)

123-123: Silently discarding the balance recalculation result.

let _ = wallet.recalculate_affected_address_balances(...) swallows any error. If recalculation fails, the UI will show stale balances with no indication of the problem. Consider logging a warning on failure so that issues are at least visible in traces.

🛡️ Suggested fix

-            let _ = wallet.recalculate_affected_address_balances(&used_utxos, self);
+            if let Err(e) = wallet.recalculate_affected_address_balances(&used_utxos, self) {
+                tracing::warn!("Failed to recalculate address balances after UTXO spend: {}", e);
+            }

src/backend_task/identity/top_up_identity.rs (1)

189-189: Silently discarding balance recalculation results (same pattern as other files).

Both recalculate_affected_address_balances (line 189) and recalculate_address_balance (line 263) discard their Result. Consider logging a warning on failure for debuggability, same as suggested for fund_platform_address_from_wallet_utxos.rs.

🛡️ Suggested fix (line 189)

-                        let _ = wallet.recalculate_affected_address_balances(&used_utxos, self);
+                        if let Err(e) = wallet.recalculate_affected_address_balances(&used_utxos, self) {
+                            tracing::warn!("Failed to recalculate address balances: {}", e);
+                        }

🛡️ Suggested fix (line 263)

-                        let _ = wallet.recalculate_address_balance(&input_address, self);
+                        if let Err(e) = wallet.recalculate_address_balance(&input_address, self) {
+                            tracing::warn!("Failed to recalculate address balance: {}", e);
+                        }

Also applies to: 263-263

[lklimek]

Please review and resolve Copilot/Rabbit comments.

[lklimek]

@copilot create an issue to review thread model of spv and resolve this comment.

[Copilot]

@lklimek I've opened a new pull request, #544, to work on those changes. Once the pull request is ready, I'll request review from you.

[coderabbitai]

Actionable comments posted: 5

🤖 Fix all issues with AI agents

In `@src/backend_task/identity/top_up_identity.rs`:
- Around line 189-190: The call to
wallet.recalculate_affected_address_balances(&used_utxos, self) currently drops
its Result (let _ = ...); change this so errors are not ignored: either
propagate the error with the ? operator from the enclosing function (if it
returns a Result) or match the Result and log and return/handle the Err (e.g.,
tracing::error! or return Err(...)) to keep wallet/DB state consistent; apply
the same fix to the other identical call site (the second
recalculate_affected_address_balances invocation).

In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs`:
- Line 127: The call to
wallet.recalculate_affected_address_balances(&used_utxos, self) currently
swallows errors (let _ = ...); change this to handle the Result properly: either
propagate the error by returning it (replace let _ = with
wallet.recalculate_affected_address_balances(&used_utxos, self)? if the
enclosing function returns Result) or explicitly log the error and context
(e.g., capture Err(e) and call error!(...) including used_utxos/self details) so
address-balance recalculation failures are not silently ignored.

In `@src/context.rs`:
- Around line 806-887: The function
spv_setup_finality_listener/handle_spv_finality_event uses
transactions_waiting_for_finality.lock().unwrap() twice (pending check and
pending_txids build) which can panic on poisoning and also silently drops errors
returned by received_asset_lock_finality; replace both unwrap() calls with
proper error handling (e.g., let transactions =
self.transactions_waiting_for_finality.lock().map_err(|_|
"transactions_waiting_for_finality lock poisoned".to_string())? ) to propagate
lock-poisoning as Err from handle_spv_finality_event, and change the calls to
self.received_asset_lock_finality(...) in both the InstantLock and ChainLock
branches to capture the Result and log failures via tracing::debug!("Finality
processing error: {}", e) (or propagate if appropriate) instead of ignoring them
so finality-processing failures are surfaced consistently with the overall error
handling.

In `@src/spv/manager.rs`:
- Around line 1033-1177: Add an inline test module (#[cfg(test)] mod tests) in
the same file and implement focused unit tests for determine_sync_stage and the
progress-mapping behavior used by spawn_progress_watcher: 1) write tests that
construct WatchSyncProgress instances (or minimal mocks/builders) with each
manager in SyncState::Syncing and assert determine_sync_stage returns the
expected SyncStage variant and that fields (e.g., pending, completed/total,
current/target, batch_size, start/end) are computed correctly, verifying the
pipeline priority (blocks > filters > filter_headers > masternodes > headers);
2) test the synced vs connecting edge cases by asserting determine_sync_stage
returns SyncStage::Complete when watch.is_synced() is true and
SyncStage::Connecting otherwise; 3) add an async test (#[tokio::test]) that
drives a tokio::sync::watch channel through spawn_progress_watcher (using the
same WatchSyncProgress values), waits for the task to process a change, and
asserts that sync_progress_state, detailed_progress_state, and status are
updated to Some(...) and the expected SpvStatus (Running when is_synced(),
Syncing otherwise); reference the functions/fields determine_sync_stage,
spawn_progress_watcher, WatchSyncProgress, SyncStage, SyncState, SyncProgress,
DetailedSyncProgress, SpvStatus, sync_progress_state, detailed_progress_state,
and status to locate code.

In `@src/ui/helpers.rs`:
- Around line 14-19: Add inline unit tests in src/ui/helpers.rs using #[test]
that exercise compute_allowed_security_levels and check_token_authorization;
create at least one test for compute_allowed_security_levels covering typical
inputs and edge cases (e.g., empty input, full access, and restricted inputs)
and assert the returned security level set/ordering, and create tests for
check_token_authorization covering authorized and unauthorized token scenarios
(including boundary cases like missing token or mismatched scopes) by calling
check_token_authorization directly and asserting Ok/Err or boolean results as
appropriate; place these tests in the same file below the helper functions and
use simple constructed inputs/mocks for any required structs so tests run
isolated and fast.

🧹 Nitpick comments (1)

src/model/qualified_identity/encrypted_key_storage.rs (1)

99-132: Consider extracting shared decoding logic.

The Decode and BorrowDecode implementations share identical logic (lines 59–92 vs 99–132). You could extract a generic helper to reduce duplication:

fn decode_wallet_derivation_path<D: Decoder>(decoder: &mut D) -> Result<WalletDerivationPath, DecodeError> {
    // shared logic here
}

This is a minor improvement and can be deferred.