fix: display balances for key-only addresses and load RPC transactions (#692)#752
Conversation
|
Warning Rate limit exceeded
⌛ 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 (3)
📝 WalkthroughWalkthroughAdds a non‑SPV RPC path to load and aggregate transaction history (list_transactions + getrawtransaction), persist RPC transactions to DB before updating in‑memory wallet state, tightens address canonicalization, updates address filtering/display logic, and adjusts UI messaging and SPV account metadata mapping. Changes
Sequence DiagramsequenceDiagram
participant Task as Refresh Task
participant RPC as Core RPC
participant DB as Database
participant Mem as In‑Memory Wallet
Task->>Task: Check SPV mode
alt SPV mode active
Task->>Task: Skip RPC history load
else SPV mode inactive
Task->>RPC: list_transactions()
RPC-->>Task: transactions[]
Task->>Task: Aggregate by txid (net_amount, fee, ts, height, block_hash, label, is_ours)
loop for each aggregated tx
Task->>RPC: getrawtransaction(txid)
RPC-->>Task: raw_tx / error
alt raw_tx fetched
Task->>Task: Build WalletTransaction entry
else fetch failed
Task->>Task: Abort RPC‑transaction load (preserve existing history)
end
end
Task->>DB: Persist rpc_transactions (replace_wallet_transactions)
DB-->>Task: persist result
alt persist success and all raw fetched
Task->>Mem: wallet_guard.set_transactions(txs)
else
Task->>Task: Keep existing in‑memory transactions
end
end
Task->>Task: Continue UTXO & asset lock processing (existing flow)
Task->>DB: Final wallet persistence
DB-->>Task: Done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
Blocked by #739 |
#692) - Remove N/A display for Identity System addresses in Balance, UTXOs, and Total Received columns so actual values are shown - Load transaction history from Core RPC via list_transactions when SPV transactions are not available - Remove duplicate address_total_received insert that used non-canonical address, causing inconsistent DB data - Update empty transactions message to be backend-agnostic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
force-pushed
the
fix/692-wallet-tab-no-data
branch
from
7cd8314 to
be689cb
Compare
There was a problem hiding this comment.
Pull request overview
Fixes wallet UI/data gaps in RPC mode by ensuring key-only addresses display real balance/UTXO/received values and by populating wallet transaction history from Dash Core RPC (instead of only via SPV reconciliation).
Changes:
- Remove UI rendering guards that showed “N/A” for key-only address balances/UTXOs/total-received, while keeping key-only addresses exempt from the zero-balance hide filter.
- Load and persist recent wallet transactions in RPC mode using Core RPC (
listtransactions+ raw tx fetch), and update the empty-state message to be backend-agnostic. - Remove a duplicate
total_receivedinsertion path when loading wallet addresses from the database.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/wallets/wallets_screen/mod.rs | Updates empty transactions message to be non-SPV-specific. |
| src/ui/wallets/wallets_screen/address_table.rs | Shows numeric values for key-only addresses; keeps key-only addresses visible under zero-balance filtering. |
| src/ui/wallets/account_summary.rs | Updates is_key_only() doc to reflect its new (filter/label) semantics. |
| src/database/wallet.rs | Removes duplicate total_received insert under non-canonical address key. |
| src/backend_task/core/refresh_wallet_info.rs | Adds Core RPC transaction loading and DB persistence during wallet refresh (RPC mode). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/backend_task/core/refresh_wallet_info.rs`:
- Around line 105-225: Change rpc_transactions from Vec<WalletTransaction> to a
discriminated type (Option<Vec<WalletTransaction>> or a small enum like
TransactionLoadState { Skipped, Failed, Loaded(Vec<WalletTransaction>) }) so SPV
skip, RPC errors, and a successful empty history are distinct; set it to Skipped
when is_spv_mode, Failed when client.list_transactions or
client.get_raw_transaction errors occur, and Loaded(wallet_txs) on successful
load; update any later logic that currently checks rpc_transactions.is_empty()
or replaces DB/in-memory state (references: rpc_transactions variable,
client.list_transactions match arm, get_raw_transaction handling,
WalletTransaction construction, and TxAggregate aggregation) to branch on the
new discriminant so you only replace stored state on Loaded(...) and handle
Skipped/Failed appropriately.
- Around line 189-198: The code currently calls
client.get_raw_transaction(&txid, None) and may skip confirmed wallet
transactions without txindex; update the call to pass the known block hash
(agg.block_hash) as the second argument so get_raw_transaction(&txid,
Some(agg.block_hash)) is used, or alternatively replace this logic to call the
wallet-scoped RPC (client.gettransaction or equivalent) which returns the hex in
the "hex" field and avoids relying on txindex; adjust error handling around
get_raw_transaction/gettransaction to log and process the returned raw hex
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 870a1d5a-3436-451a-8e6f-2be73a84b512
📒 Files selected for processing (5)
src/backend_task/core/refresh_wallet_info.rssrc/database/wallet.rssrc/ui/wallets/account_summary.rssrc/ui/wallets/wallets_screen/address_table.rssrc/ui/wallets/wallets_screen/mod.rs
💤 Files with no reviewable changes (1)
- src/database/wallet.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The PR delivers its stated goal — key-only addresses show real values instead of "N/A", and RPC transaction history is loaded in non-SPV mode. The architecture is solid: lock-minimized refresh, clean TxAggregate deduplication pattern, DB persistence before the write lock. The UI changes, database cleanup, and message update are all correct.
However, the new RPC transaction loading has two correctness issues that will cause silent data loss on most node configurations:
| ID | Severity | Finding |
|---|---|---|
| RUST-001 | 🟡 MEDIUM | get_raw_transaction(&txid, None) fails without -txindex — block_hash available but unused |
| RUST-002 | 🟡 MEDIUM | Stale transactions never cleared on successful empty RPC fetch |
| RUST-003 | 🟡 MEDIUM | Hardcoded 200-entry limit with silent truncation — warning field unused |
RUST-001 is the most impactful — it effectively breaks the feature on standard node configurations. The fix is a one-liner.
Positives: Lock-minimization strategy is well thought out. TxAggregate pattern is clean. Database canonicalization fix is correct. UI filter logic for key-only addresses is sound.
Full report: review-report/report.html
🤖 Review by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Review Summary (Round 2)
The three MEDIUM findings from the previous review (RUST-001, RUST-002, RUST-003) remain unaddressed in the current code. One new MEDIUM finding (send-entry filtering) is posted inline.
Unresolved from previous review
| ID | Severity | Finding | Status |
|---|---|---|---|
| RUST-001 | 🟡 MEDIUM | get_raw_transaction(&txid, None) fails without -txindex — agg.block_hash available but unused |
❌ Not fixed |
| RUST-002 | 🟡 MEDIUM | Stale transactions never cleared on successful empty RPC fetch | ❌ Not fixed |
| RUST-003 | 🟡 MEDIUM | Hardcoded 200-entry limit — warning field returned as None |
❌ Not fixed |
New finding
| ID | Severity | Finding |
|---|---|---|
| RUST-004 | 🟡 MEDIUM | Send-category entries admitted without address membership check |
What's good
The architecture is solid — lock-minimized refresh, clean TxAggregate deduplication, DB persistence before the write lock. The UI changes (removing N/A guards, updating empty-state message, key-only filter logic) are all correct. The database canonicalization fix is clean.
Priority: RUST-001 (block_hash) is the most impactful — it's a one-liner that determines whether this feature actually works on standard node configurations.
Full report: /tmp/claudius-WBXsja/report.html
🤖 Review by Claudius the Magnificent AI Agent
- RUST-001: Pass block_hash to get_raw_transaction so confirmed txs resolve without -txindex - RUST-002: Use Option<Vec> for rpc_transactions to distinguish skip (SPV/failure) from successful-empty, clearing stale data - RUST-003: Surface truncation warning to the user when list_transactions hits the 200-entry limit - RUST-004: Document why Send-category entries are safe to include without address check (wallet-scoped RPC endpoint) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Review Summary (Round 3) ✅
All 5 previous review threads (RUST-001 through RUST-004, plus Copilot's two findings) are now resolved in the current code. Nicely done.
Fresh review found 5 findings — all LOW/INFO severity. No MEDIUM or higher issues remain.
| ID | Sev | Finding |
|---|---|---|
| RUST-005 | 🟢 LOW | Duplicated address-ownership check (lines 143-162) — same expression computed twice |
| RUST-006 | 🟢 LOW | N+1 RPC calls for raw transactions — acceptable at 200-cap |
| RUST-007 | 🟢 LOW | HashMap iteration yields non-deterministic tx order |
| RUST-008 | ⚪ INFO | DB-memory consistency gap on failure — self-healing |
| RUST-009 | 🟢 LOW | Key-only addresses show 0.00000000 instead of N/A — intentional UX tradeoff |
What's good
The lock-minimized refresh pattern is correctly maintained. The Option<Vec> pattern cleanly distinguishes "skip" from "empty result". The truncation warning surfaces the 200-entry cap to users. The TxAggregate struct keeps aggregation readable. The database duplicate fix is correct. Security posture is clean — wallet-scoped RPC, parameterized queries, no sensitive data in user-facing messages.
Full report: /tmp/claudius-afB16v/report.html
🤖 Review by Claudius the Magnificent AI Agent
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/backend_task/core/refresh_wallet_info.rs (1)
105-233: Add inline tests for transaction-load state transitions.This path now has subtle state semantics (
NonevsSome(empty)vsSome(non-empty)) plus aggregation/filtering behavior. A few focused#[test]cases in this file would lock in expected behavior and prevent regressions.As per coding guidelines:
**/*.rs: Unit tests are inline in source files with#[test]attribute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/core/refresh_wallet_info.rs` around lines 105 - 233, Add inline unit tests in refresh_wallet_info.rs that exercise the transaction-load path's state transitions (None vs Some(empty) vs Some(non-empty)) and aggregation/filtering behavior: write tests that mock the client.list_transactions and client.get_raw_transaction results to produce (a) SPV mode -> rpc_transactions == None, (b) RPC returns no wallet-involving entries -> Some(vec![]) and (c) RPC returns multiple entries that aggregate into a single WalletTransaction with correct net_amount, fee, label, is_ours, timestamp, height, block_hash and that tx_truncated toggles when 200 entries are returned. Target the logic around client.list_transactions, TxAggregate construction/aggregation, and WalletTransaction creation so tests fail on regressions to aggregation/filtering semantics.
🤖 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/backend_task/core/refresh_wallet_info.rs`:
- Around line 197-205: The current logic in refresh_wallet_info.rs builds
wallet_txs by skipping txids when client.get_raw_transaction(&txid, ...) returns
Err, but still returns Some(wallet_txs) which later (via the code paths that
update DB/in-memory history) can overwrite existing history with a partial set;
change the error handling around the get_raw_transaction call (the match that
binds tx or logs ?e and continues) to instead abort the wallet fetch on any RPC
failure: propagate the error or return None immediately (i.e., do not continue
collecting and do not return Some(wallet_txs) when any get_raw_transaction
fails), so the callers that expect Option will retain the existing DB/in-memory
history; reference the get_raw_transaction call and the wallet_txs aggregation
to locate and modify the behavior.
---
Nitpick comments:
In `@src/backend_task/core/refresh_wallet_info.rs`:
- Around line 105-233: Add inline unit tests in refresh_wallet_info.rs that
exercise the transaction-load path's state transitions (None vs Some(empty) vs
Some(non-empty)) and aggregation/filtering behavior: write tests that mock the
client.list_transactions and client.get_raw_transaction results to produce (a)
SPV mode -> rpc_transactions == None, (b) RPC returns no wallet-involving
entries -> Some(vec![]) and (c) RPC returns multiple entries that aggregate into
a single WalletTransaction with correct net_amount, fee, label, is_ours,
timestamp, height, block_hash and that tx_truncated toggles when 200 entries are
returned. Target the logic around client.list_transactions, TxAggregate
construction/aggregation, and WalletTransaction creation so tests fail on
regressions to aggregation/filtering semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf845b98-4765-47da-8710-6d169f1e2579
📒 Files selected for processing (3)
src/backend_task/core/refresh_wallet_info.rssrc/ui/wallets/account_summary.rssrc/ui/wallets/wallets_screen/address_table.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/wallets/account_summary.rs
…ddresses in SPV Remove the stale developer-mode gate around the transactions section (transactions are now loaded via RPC, not just SPV). Add the four Provider account types (VotingKeys, OwnerKeys, OperatorKeys, PlatformKeys) to spv_account_metadata() so their addresses are registered with SPV for balance tracking. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When get_raw_transaction fails for some txids, the partial result set would replace the full DB/in-memory history, silently dropping previously-known transactions. Now tracks fetch failures and returns None (preserving existing data) instead of Some(partial_set). Addresses PR #752 review comment from coderabbitai. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend_task/core/refresh_wallet_info.rs (1)
375-382: Consider adding guidance for the user.The warning message explains what happened but doesn't suggest what the user can do. Per coding guidelines, user-facing messages should include an action. While there's no in-app solution, you could guide users to an external resource.
Suggested improvement
let warning = if tx_truncated { Some( - "Transaction history may be incomplete. Only the most recent 200 entries are shown." + "Transaction history may be incomplete. Only the most recent 200 entries are shown. \ + Use a block explorer to view your full transaction history." .to_string(), ) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/core/refresh_wallet_info.rs` around lines 375 - 382, The warning built in the refresh_wallet_info logic (the `warning` variable set when `tx_truncated` is true) currently informs the user the history is incomplete but offers no next step; update the message string produced for `warning` to include actionable guidance (e.g., suggest they view full history on a block explorer or contact support/adjust settings) so users know what to do next—modify the string created in the `if tx_truncated { ... }` branch to append a brief instruction or external resource pointer.
🤖 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/core/refresh_wallet_info.rs`:
- Around line 375-382: The warning built in the refresh_wallet_info logic (the
`warning` variable set when `tx_truncated` is true) currently informs the user
the history is incomplete but offers no next step; update the message string
produced for `warning` to include actionable guidance (e.g., suggest they view
full history on a block explorer or contact support/adjust settings) so users
know what to do next—modify the string created in the `if tx_truncated { ... }`
branch to append a brief instruction or external resource pointer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b338c5d-ab36-46b0-9299-b0a0dc80d771
📒 Files selected for processing (3)
src/backend_task/core/refresh_wallet_info.rssrc/context/wallet_lifecycle.rssrc/database/wallet.rs
💤 Files with no reviewable changes (1)
- src/database/wallet.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/context/wallet_lifecycle.rs
There was a problem hiding this comment.
Pull request overview
Fixes wallet UI/state so Identity System (and other key-only) addresses show balance-related fields instead of “N/A”, and enables loading/persisting transaction history when using Dash Core RPC (non-SPV) so the Transactions section is populated.
Changes:
- Always render the Transactions section and update the empty-state copy to be backend-agnostic.
- Remove “N/A” guards in the address table so key-only addresses display numeric balances/UTXO counts/total received.
- Load recent transactions via Core RPC (
list_transactions+get_raw_transaction), dedupe/aggregate by txid, and persist towallet_transactions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/wallets/wallets_screen/mod.rs | Always shows Transactions panel and updates the “no transactions” message. |
| src/ui/wallets/wallets_screen/address_table.rs | Displays numeric values for key-only addresses and adjusts zero-balance filtering logic. |
| src/ui/wallets/account_summary.rs | Updates is_key_only doc comment to reflect its UI usage. |
| src/database/wallet.rs | Removes duplicate address_total_received insertion under non-canonical address keys. |
| src/context/wallet_lifecycle.rs | Adds SPV metadata mapping for Provider* account types. |
| src/backend_task/core/refresh_wallet_info.rs | Adds RPC-mode transaction history loading + persistence and a truncation warning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Review Summary (Round 4) ✅
All 9 previous review threads remain resolved. Fresh review with 2 specialist agents (Bilby + Smythe) found 15 findings — all LOW/INFO. No MEDIUM or higher issues.
The latest commit (9c69d32) correctly handles partial get_raw_transaction failures by returning None when any fetch fails, preserving existing transaction history. Conservative but safe.
What's solid
Option<Vec>state model — clean skip/replace semantics, no stale-data bugs- Lock discipline — no I/O under locks, brief write lock for final update
block_hashpass-through —get_raw_transactionworks without-txindex- Truncation warning — users know when history is capped at 200 entries
- Security posture — wallet-scoped RPC, parameterized SQL, no sensitive data in logs/UI
- DB canonicalization fix — duplicate
total_receivedinsert correctly removed
Minor improvements for follow-up (all LOW)
| Sev | Finding |
|---|---|
| 🟢 LOW | N+1 sequential RPC calls — acceptable at 200-cap, consider batch/cache later |
| 🟢 LOW | Duplicated wallet_address_set.contains() expression — compute once, reuse |
| 🟢 LOW | HashMap iteration → non-deterministic tx order → unnecessary DB churn |
| 🟢 LOW | raw_fetch_failed all-or-nothing — safe but aggressive on transient failures |
| 🟢 LOW | saturating_add() for net_amount accumulation — defense-in-depth |
Full report: review-report/report.html
🤖 Review by Claudius the Magnificent AI Agent
📊 View full HTML review report
- Reset tx_truncated when raw_fetch_failed to avoid misleading truncation warning when history was not actually updated (CMT-003) - Reword warning to drop specific "200 entries" count that refers to RPC entries, not displayed transactions (CMT-004) - Add INTENTIONAL comment documenting key-only address filter behavior as accepted design decision (CMT-002) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ions section - Wallet list dropdown and balance display now sum Core + Platform balances instead of showing Core-only - Add "Total:" line in wallet overview when platform balance > 0 - Rename "Transactions" heading to "Dash Core Transactions" - Add TODO for future account-type transaction filtering Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fixes #692 — the Wallets tab showed "N/A" for Balance, UTXOs, and Total Received on Identity System addresses, and the Transactions section was always empty in RPC mode.
is_key_only()rendering guards in the address table that suppressed balance/UTXO/total-received display. These addresses now show real values like all other account types.list_transactions+get_raw_transactioncalls to the wallet refresh path. In RPC mode, up to 200 recent transactions are fetched, deduplicated by txid with proper net_amount aggregation, and persisted to DB. SPV mode continues using its own reconciliation path.get_raw_transactioncall fails, existing transaction history is preserved instead of being replaced with a partial set.address_total_receivedinsert: Deleted a duplicate code block indatabase/wallet.rs.User Story
Imagine you are a Dash Evo Tool user who has imported a wallet and navigated to the Wallets tab. Previously, you would see "N/A" for all balance columns on Identity System addresses, "0.0000 DASH" in the wallet selector even though you had platform funds, and an empty Transactions section. Now you see combined Core + Platform balance in the wallet selector, actual values for every address type, and your recent Dash Core transaction history after clicking Refresh.
Test plan
cargo test --all-features --workspacepassescargo clippy --all-features --all-targets -- -D warningsis clean🤖 Co-authored by Claudius the Magnificent AI Agent