backport: feat(ui): add sync status panel to wallet screen#642
Open
backport: feat(ui): add sync status panel to wallet screen#642
Conversation
…ount Replace hardcoded 3000 duff fee with dynamic fee calculation that accounts for actual number of inputs. Estimates tx size using standard component sizes (P2PKH input ~148B, output ~34B, header ~10B, payload ~60B) and uses max(3000, estimated_size) to always meet the min relay fee. Properly handles fee shortfall when allow_take_fee_from_amount is set, and returns clear error messages for insufficient funds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds a detailed manual test plan for the Sync Status Panel; strengthens error handling in asset lock and signing code by replacing unwraps with propagated errors and cleanup on broadcast failure; tweaks numeric precision for fee logging; adjusts wallet fee/change handling flow; clarifies connection labels; and implements an SPV-aware sync status panel with per-wallet caching and time-ago formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Wallets Screen UI
participant Cache as platform_sync_info Cache
participant DB as Database
participant SPV as SPV Service
UI->>DB: request platform sync info (seed_hash)
DB-->>Cache: return (last_sync_timestamp, last_sync_height)
Cache-->>UI: cache platform_sync_info
UI->>SPV: query SPV status & progress
SPV-->>UI: return SpvStatus / SpvSyncProgress
UI->>UI: format time-ago & progress text
UI->>UI: render_sync_status (Core line + Platform line)
Note over UI,DB: on wallet refresh or platform-balance update
UI->>DB: refresh platform sync info
DB-->>Cache: update cached values
Cache-->>UI: re-render sync status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5 tasks
…d signed Previously, `asset_lock_transaction_from_private_key` called `take_unspent_utxos_for` which immediately removed selected UTXOs from `Wallet.utxos`. Since fee recalculation and signing happen afterward, any failure at those steps (fee shortfall, missing private key, change address derivation error) would permanently drop UTXOs — especially dangerous in SPV mode where there is no Core RPC reload fallback. Fix: - Add `select_unspent_utxos_for` (`&self`, non-mutating) that performs the same UTXO selection logic without removing anything. - Add `remove_selected_utxos` (`&mut self`) for explicit removal. - Refactor `take_unspent_utxos_for` to delegate to these two methods (no behavior change for existing callers). - In `asset_lock_transaction_from_private_key`, use `select_unspent_utxos_for` for selection and only call `remove_selected_utxos` after the full tx is built and signed. Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
# Conflicts: # docs/ai-design/2026-02-24-asset-lock-fee-fix/manual-test-scenarios.md # src/model/wallet/asset_lock_transaction.rs
…ce recalc into remove_selected_utxos Previously, every backend task caller had to manually: (1) remove UTXOs from the in-memory map, (2) drop them from the database, and (3) recalculate affected address balances. This was error-prone — the payment transaction builders were missing the balance recalculation entirely. Now `remove_selected_utxos` accepts an optional `&AppContext` and handles all three steps atomically. The redundant cleanup blocks in 5 backend task callers are removed. Also applies the safe select-then-commit UTXO pattern to `build_standard_payment_transaction` and `build_multi_recipient_payment_transaction`, fixing the same UTXO-loss-on-signing-failure bug that was previously fixed only for asset lock transactions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add checked arithmetic to UTXO selection (amount + fee overflow safety) - Replace hardcoded fee in single-UTXO path with calculate_asset_lock_fee - Add UTXO selection retry when real fee exceeds initial estimate - Document write-lock invariant on select_unspent_utxos_for - Replace .unwrap() with .map_err() on wallet write locks - Restrict Database::shared_connection visibility to pub(crate) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Network directly Replace Option<&AppContext> with concrete dependencies (&Database, Network), removing the need for take_unspent_utxos_for. Extract balance recalculation into a private helper reused by both remove_selected_utxos and the existing AppContext-based wrapper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…extract/sync-status-panel
- Refresh platform sync info cache after wallet refresh completes - Add broadcast failure cleanup in create_asset_lock (remove stale finality tracking entries, replace mutex .unwrap() with .map_err()) - Replace .expect() with proper error propagation in signing loops - Use i128 for fee logging subtraction to prevent overflow - Renumber step comments sequentially after refactoring Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a sync status panel to the wallet screen showing real-time Core and Platform synchronization status. The panel displays Core connection state (RPC connected/disconnected or SPV sync progress with phase-specific percentages) and Platform sync information (address count, sync height, and time since last sync). The PR also includes several bug fixes: improved error handling in asset lock transactions, more descriptive connection status text, corrected comment numbering, and a fix for integer overflow in fee calculation logging.
Changes:
- Adds a compact sync status panel to the wallets screen with Core and Platform status lines, showing RPC/SPV connection state and platform sync details with relative timestamps
- Improves error handling in asset lock transaction building by replacing panics with proper error propagation
- Includes comprehensive manual test scenarios documentation covering 25+ test cases and 6 edge cases
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/wallets/wallets_screen/mod.rs | Adds sync status panel rendering, platform sync info caching, time formatting helpers, and integrates with screen lifecycle |
| src/model/wallet/asset_lock_transaction.rs | Replaces expect() panics with proper error handling using ok_or_else() and map_err() |
| src/context/connection_status.rs | Updates status text to be more descriptive (e.g., "Ready" instead of "SPV synced", more detailed endpoint counts) |
| src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs | Fixes step comment numbering from 7-8 to 6-7-8 |
| src/backend_task/identity/top_up_identity.rs | Changes fee difference calculation to use i128 cast to handle negative differences |
| src/backend_task/core/create_asset_lock.rs | Adds broadcast error handling with cleanup of finality tracking and uses map_err for mutex locks |
| docs/ai-design/2026-02-24-sync-status-panel/manual-test-scenarios.md | Comprehensive manual test documentation with 25 test scenarios and 6 edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
In SPV mode, I keep having |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs (2)
82-86: Inconsistent error handling:lock().unwrap()vslock().map_err()?.Line 84 still uses
lock().unwrap()ontransactions_waiting_for_finality, while the same operation increate_asset_lock.rs(lines 36–39, 93–96) was updated in this PR to uselock().map_err(|e| e.to_string())?. For consistency and to avoid a panic on a poisoned lock, consider aligning this call.Proposed fix
- let mut proofs = self.transactions_waiting_for_finality.lock().unwrap(); + let mut proofs = self + .transactions_waiting_for_finality + .lock() + .map_err(|e| e.to_string())?;Based on learnings, error handling refactoring is needed across the codebase, particularly to avoid panics with
.expect()and instead propagate errors properly using the?operator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs` around lines 82 - 86, Replace the panic-prone call to transactions_waiting_for_finality.lock().unwrap() with fallible error propagation consistent with other modules: acquire the mutex with transactions_waiting_for_finality.lock().map_err(|e| e.to_string())? and then perform proofs.insert(tx_id, None); so the function (the block in fund_platform_address_from_wallet_utxos where the "Step 2" registration occurs) returns an Err instead of panicking when the lock is poisoned, matching the pattern used in create_asset_lock.rs.
109-118:try_lock()in cleanup may skip cleanup unnecessarily.Line 113 uses
try_lock()which will fail not only on a poisoned lock but also if another thread momentarily holds the mutex. The analogous cleanup increate_asset_lock.rs(lines 52, 105) useslock()which waits for the mutex. Since this is an error-recovery path, it's better to wait briefly rather than risk leaving a stale entry in the finality map.Proposed fix
- if let Ok(mut proofs) = self.transactions_waiting_for_finality.try_lock() { + if let Ok(mut proofs) = self.transactions_waiting_for_finality.lock() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs` around lines 109 - 118, The cleanup path after broadcast_raw_transaction currently uses transactions_waiting_for_finality.try_lock(), which can fail transiently and skip removing tx_id; change it to acquire the mutex with transactions_waiting_for_finality.lock().await (or blocking lock() as used in create_asset_lock.rs) so the code waits for the lock and reliably removes the entry, then proceed to call db.delete_asset_lock_transaction(tx_id.as_byte_array()) and return the error as before; ensure you reference the same mutex (transactions_waiting_for_finality) and tx_id removal logic to mirror the safe cleanup in create_asset_lock.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs`:
- Around line 82-86: Replace the panic-prone call to
transactions_waiting_for_finality.lock().unwrap() with fallible error
propagation consistent with other modules: acquire the mutex with
transactions_waiting_for_finality.lock().map_err(|e| e.to_string())? and then
perform proofs.insert(tx_id, None); so the function (the block in
fund_platform_address_from_wallet_utxos where the "Step 2" registration occurs)
returns an Err instead of panicking when the lock is poisoned, matching the
pattern used in create_asset_lock.rs.
- Around line 109-118: The cleanup path after broadcast_raw_transaction
currently uses transactions_waiting_for_finality.try_lock(), which can fail
transiently and skip removing tx_id; change it to acquire the mutex with
transactions_waiting_for_finality.lock().await (or blocking lock() as used in
create_asset_lock.rs) so the code waits for the lock and reliably removes the
entry, then proceed to call
db.delete_asset_lock_transaction(tx_id.as_byte_array()) and return the error as
before; ensure you reference the same mutex (transactions_waiting_for_finality)
and tx_id removal logic to mirror the safe cleanup in create_asset_lock.rs.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/ai-design/2026-02-24-sync-status-panel/manual-test-scenarios.mdsrc/backend_task/core/create_asset_lock.rssrc/backend_task/identity/top_up_identity.rssrc/backend_task/wallet/fund_platform_address_from_wallet_utxos.rssrc/context/connection_status.rssrc/model/wallet/asset_lock_transaction.rssrc/ui/wallets/wallets_screen/mod.rs
Member
Concept NACK. I don't think it makes sense to show this on the wallets page; this is relevant to most DET operations. Maybe instead we enhance the sync indicator in top left to also show progress, but I don't like this approach. |
The PlatformAddressBalances task result handler updated wallet balances but did not refresh the platform_sync_info cache, causing the UI to display "never synced" until the wallet was reselected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Core/Platform sync status panel is now hidden by default and only visible when developer mode is enabled. It uses a collapsible header so developers can expand/collapse it as needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 179-184: The current chain that builds platform_sync_info
(starting from selected_wallet.as_ref().and_then(|w| w.read().ok().map(|g|
g.seed_hash())).and_then(|hash|
app_context.db.get_platform_sync_info(&hash).ok()).map(|(ts, checkpoint,
_terminal)| (ts, checkpoint)).filter(|(ts, _)| *ts > 0)) throws away valid
cached checkpoint data when ts == 0; remove or change the final .filter(|(ts,
_)| *ts > 0) so that platform_sync_info preserves the (ts, checkpoint) tuple
even if ts == 0 (or replace the filter with a check that only discards when both
ts == 0 and checkpoint indicates no data), and update any downstream handling
(e.g., code that renders "Addresses: never synced") to consider checkpoint
presence rather than assuming ts > 0 is required for valid sync info; locate
this logic around selected_wallet, platform_sync_info, seed_hash and
app_context.db.get_platform_sync_info to apply the change.
- Around line 120-121: The platform_sync_info cache is only updated in
select_hd_wallet, causing stale sync state when selected_wallet is reassigned
elsewhere (e.g., post-removal or fallback reselection paths); add a single
helper (e.g., refresh_platform_sync_info or set_selected_wallet) that both
assigns selected_wallet and updates platform_sync_info (using the same logic as
in select_hd_wallet) and replace all direct assignments to selected_wallet
across code paths with calls to this helper so the sync panel always reflects
the newly selected wallet.
Contributor
Author
🔍 Audit Summary — PR #642Reviewed by: Claude Code with a 3-agent team:
Overall risk: Low. No critical or high-severity issues. The PR improves codebase robustness (panic removal, Mutex error handling) while adding a well-gated diagnostic UI panel. Findings
Pre-existing / Outside-diff observations
✅ Positive observations
Redundancy analysis3 agents produced 18 raw findings. After deduplication: 6 unique actionable findings + 1 pre-existing observation. Redundancy ratio: 39% (7 findings flagged by 2+ agents). Key overlap areas: Mutex handling strategy, SPV phase logic concerns, and time formatting edge cases. 🤖 Reviewed by Claudius the Magnificent AI Agent | |
lklimek
commented
Feb 25, 2026
src/ui/wallets/wallets_screen/mod.rs
Outdated
| } | ||
|
|
||
| /// Get a text summary of the active SPV sync phase. | ||
| fn spv_active_phase_text(sync_progress: &Option<SpvSyncProgress>) -> String { |
Contributor
Author
There was a problem hiding this comment.
🟡 Medium — Duplicated SPV phase logic + missing Masternodes phase
NetworkChooserScreen::format_sync_progress (network_chooser_screen.rs ~L1961) already interprets SpvSyncProgress phases and includes a Masternodes phase that this function omits. During a fresh sync where masternodes is the active phase, users see generic "syncing..." instead of a meaningful label.
Consider:
- Extract shared SPV phase interpretation into a utility (e.g.,
src/ui/spv_helpers.rs) that both screens call - Or at minimum, add masternodes handling here and document the intentional simplification
docs/ai-design/2026-02-24-sync-status-panel/manual-test-scenarios.md
Outdated
Show resolved
Hide resolved
- Centralize wallet selection via set_selected_hd_wallet() helper to keep platform_sync_info cache consistent across all code paths - Add tracing::warn! for Mutex poisoning in asset lock cleanup paths - Fix misleading comment about wallet refresh on broadcast failure - Remove TS-25 from manual test scenarios (not part of this PR) Refs: #657 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Consolidate duplicated SPV sync phase formatting into a shared `spv_phase_summary()` function in `connection_status.rs`. The wallet screen now uses this shared function instead of its own copy. The network screen retains its richer operator-facing formatter. The connection indicator tooltip now shows detailed sync progress (e.g. "SPV: Headers: 12345 / 27000 (45%)") instead of bare "SPV: Syncing" when in SPV mode. Also adjust refresh polling rates: 4s when connected, 1s when disconnected (was 10s/2s). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
format_unix_time_ago()/format_duration_ago()time formatting helpersTest plan
Manual test scenarios:
docs/ai-design/2026-02-24-sync-status-panel/manual-test-scenarios.md🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes
Documentation