fix: address unresolved PR #860 review feedback#902
Conversation
Co-Authored-By: Claude GPT-5 <noreply@anthropic.com>
Reconcile deposit state from wallet snapshots, prevent duplicate address requests, and preserve retry controls on derivation failures. Co-Authored-By: Claude GPT-5 Codex <noreply@anthropic.com>
Release scheduled-vote sweep guards after panicking task joins and require explicit network confirmation when legacy settings I/O cannot restore the saved network. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com>
Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Skip corrupt transaction-history rows instead of aborting wallet registration, and surface a skipped token-balance refresh as an info banner instead of a sticky error (the balance is very likely still fresh from the periodic sync loop that raced it). Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (21)
✨ 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 |
|
✅ Final review complete — no blockers (commit 90a719f) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The PR fixes several PR #860 follow-ups, but its network-safety invariant remains incomplete in two boot failure paths: unreadable current settings and unsupported legacy database versions can still silently select Mainnet. The new address-generation retry state is also updated from uncorrelated screen-wide errors, so concurrently dispatched tasks can falsely mark an active address request as failed. These issues are introduced by or directly undermine the behavior claimed by this PR.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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/app.rs`:
- [BLOCKING] src/app.rs:1119-1128: Require network selection when the settings blob cannot be read
`network_selection_required` is finalized from the legacy-import result before the authoritative `det:settings:v1` blob is read. If the import reports `AlreadyDone` but that blob is subsequently unreadable or truncated, this branch substitutes `AppSettings::default()` while leaving the safety gate disabled. The app can therefore initialize and potentially auto-start on Mainnet without explicit confirmation, contradicting the PR's stated guarantee that an unconfirmed network preference must route through network selection. Treat an `app_kv.get` error as requiring explicit network selection before using the fallback settings.
- [BLOCKING] src/app.rs:120-127: Require network selection for unsupported legacy versions
`LegacyDataTooOld` and `LegacyDataTooNew` are returned before the importer reads the legacy settings row, so the user's previous network is unconfirmed in exactly the same way as after `LegacyRead` or `Write`. Returning `false` for these variants allows boot to continue with `AppSettings::default()` when no usable current blob exists, silently selecting Mainnet despite the PR's stated handling of unrecoverable legacy-network migration failures. The test at lines 2774-2780 currently codifies this unsafe exception and should instead require the network chooser for every import error.
In `src/ui/identities/top_up_identity_screen/mod.rs`:
- [SUGGESTION] src/ui/identities/top_up_identity_screen/mod.rs:577-583: Correlate address failures with the originating task
`display_message` receives every unhandled warning or error routed to the visible screen, but this code treats any such message as a failure of the pending receive-address request. This PR deliberately dispatches `GenerateReceiveAddress` concurrently with multiple `ListTrackedAssetLocks` tasks, so an asset-lock failure can clear the address in-flight flag and expose the retry control while address derivation is still running; retrying can then dispatch a duplicate derivation. `AddNewIdentityScreen` has the same boolean-only correlation at `src/ui/identities/add_new_identity_screen/mod.rs:1317` and can batch address generation with asset-lock, key-warming, or WIF tasks. Extend `BackendTaskContext` to identify `GenerateReceiveAddress` and update address failure state only from `display_backend_task_error` when that matching operation fails.
Claudius-Maginificent
left a comment
Claudius-Maginificent
left a comment
There was a problem hiding this comment.
🧐 Grumpy review — 4-way pass (security + project-consistency + QA + Codex Sol)
Reviewed at e8236f5f. 32 findings (11 MEDIUM, 20 LOW, 1 INFO), 9 merge-blocking.
The PR closes several real PR #860 gaps with genuinely careful work — the crash-window import guard, the non-fatal wallet hydration, and the CHANGELOG are all above the bar — but four independent reviewers converge on the same structural criticism: most of the fixes are narrower than the gaps they claim to close, and three of them introduce new defects. Nine findings block merge.
Verdict: Request changes — 9 blocking findings
2 finding(s) overlap with existing review threads and were posted as thread replies instead of new comments: SEC-006, CODE-001.
Additional findings (no single-line anchor)
Location: Cargo.lock
[LOW / out_of_scope_follow_up] Pre-existing dependency advisories in the tree (NOT introduced by this PR) — cargo audit reports 4 vulnerabilities
Recorded for due diligence; this PR changes no dependency manifest, so none of these are attributable to #902 and none should block it.
cargo audit on the worktree:
| ID | Crate | Severity/Type | Relevance |
|---|---|---|---|
| RUSTSEC-2026-0185 | quinn-proto 0.11.14 | Remote memory exhaustion via unbounded out-of-order stream reassembly | Most relevant — QUIC transport under dash-sdk/DAPI is remote-attacker-adjacent. Fix: >=0.11.15 |
| RUSTSEC-2026-0195 | quick-xml 0.39.4 | Unbounded namespace-declaration allocation (memory-exhaustion DoS) | Fix: >=0.41.0 |
| RUSTSEC-2026-0194 | quick-xml 0.39.4 | Quadratic run time on duplicate attribute names | Fix: >=0.41.0 |
| RUSTSEC-2026-0204 | crossbeam-epoch 0.9.18 | Invalid pointer dereference in fmt::Pointer for Atomic/Shared |
Fix: >=0.9.20 |
Unmaintained warnings: RUSTSEC-2025-0141 (bincode 1.3.3 and 2.0.1), RUSTSEC-2025-0052 (async-std), RUSTSEC-2023-0089 (atomic-polyfill), RUSTSEC-2024-0388 (derivative). Yanked crates: base58ck 0.1.100, bitcoin-io 0.1.100, bitcoin_hashes 0.14.100, spin 0.9.8/0.10.0.
bincode was specifically cross-referenced against this PR's code because it touches bincode::serde::decode_from_slice on the settings blob (src/model/settings.rs) — bincode's config::standard() sets no allocation limit, and bincode is only 'safe for untrusted input' with Configuration::with_limit. Verified NOT applicable here: the blob's trust boundary is the local k/v store on the user's own disk, not remote input.
Recommendation: Out of scope for this PR — do not gate merge on it.
File separately: prioritise quinn-proto >=0.11.15, then quick-xml >=0.41.0 and crossbeam-epoch >=0.9.20. Many are pinned transitively via the dashpay/platform git deps, so bumps likely need to land upstream first. If bincode is ever used on genuinely untrusted input, switch to a limited configuration.
🤖 Co-authored by Claudius the Magnificent AI Agent
Additional findings (no resolvable diff anchor)
Location: src/ui/wallets/send_screen.rs:3062 (no-nearby-diff-line (wanted 3062, nearest 3001))
[MEDIUM / blocking] Advanced fee-deducting sends still omit the recipient's reduced amount
The simple Core-to-Platform confirmation now computes and displays recipient_receives_credits — a genuine what-you-see-is-what-you-sign improvement. The advanced Core-to-Platform path only says the recipient will receive less than the entered amount. It lists the entered output amount, advanced_fee_estimate_credits returns None for this mixed path, and no reduced amount is shown — even though this path supports exactly one Platform destination and dispatches fee_deduct_from_output.
This leaves the advanced-mode subset of the changelog promise incomplete: the user is told the number on screen is wrong, but not what the right number is.
The existing test asserts only the words 'receive less', which is why the gap survived — it pins the disclaimer, not the figure.
Recommendation: Build the advanced confirmation from the already-normalized action data and the Core-to-Platform address-funding fee estimator, then show the entered amount, estimated fee, and estimated reduced recipient amount for ReduceFirstOutput/ReduceLastOutput.
Add an exact-message/value test analogous to simple_confirmation_reports_recipient_amount_when_fee_is_deducted rather than asserting only the words 'receive less'.
Location: src/backend_task/tokens/query_my_token_balances.rs:244 (path-not-in-diff)
[MEDIUM / blocking] A PR860 Blocking item claimed as triaged has a zero-line diff — silently dropped from the backlog
qa-engineer verified this by diff, not by reading the description.
PR #902's own description claims to have triaged all 34 unresolved PR860 threads and closed 13 confirmed-valid findings. But git diff origin/v1.0-dev...HEAD -- src/backend_task/tokens/query_my_token_balances.rs is empty — this file is completely untouched.
The PR860 thread at this file (discussion_r3597497082, marked Blocking) reads: 'The maximum-lifetime timer clears only DET's single-flight flag; it does not stop the managed refresh or release upstream's synchronization slot... every later refresh can therefore report success while balances remain stale.'
Confirmed still present: begin_token_balance_refresh_with_timeout (query_my_token_balances.rs:190) spawns an independent tokio::time::sleep_until(deadline) that calls lease.release() on token_balance_refresh_in_flight with zero coupling to whether sync_token_balances_now() / upstream's is_syncing() guard ever actually completed or was ever claimed by this caller.
PR #902 only touches the sibling Blocking thread (wallet_backend/mod.rs:131, 'ownership cannot be inferred from before/after samples') — via a TODO comment and an unrelated severity downgrade of TokenBalanceRefreshSkipped to Info (verified: token_balance_refresh_skipped_uses_info_banner_severity passes). The PR description conflates these two related-but-distinct Blocking findings, silently dropping this one with no code change, no TODO, and no mention in the '13 distinct findings confirmed still valid' tally.
What blocks here is the silent drop, not the bug's severity. The underlying lease bug is pre-existing and would be an ordinary follow-up. Claiming it closed while leaving it untouched is what gates the merge.
Recommendation: Either implement the fix this thread specifically calls for — couple the timeout-driven lease release to actual completion of the upstream sync pass, or add a cancellation path — or explicitly re-open / document this thread as deferred with its own TODO in query_my_token_balances.rs, and correct the PR description's tally so it is not silently dropped from PR860's Blocking backlog.
An honest deferral is a perfectly good outcome here. An inaccurate 'closed' is not.
Location: src/app.rs:933 (no-nearby-diff-line (wanted 933, nearest 830))
[LOW / out_of_scope_follow_up] The same-frame dispatch bug is fixed locally; the AppAction |= footgun that caused it is untouched
impl BitOrAssign for AppAction silently overwrites self with rhs unless rhs is None. That is what discarded the asset-lock fetches when the deposit-address request was OR'd in on the same frame — the bug the changelog's first entry describes.
The fix (pending_backend_tasks_action) is a correct local workaround for one call site, but the operator keeps its silently-lossy semantics: an operator named |= that discards its left operand will keep biting.
Four other files perform 2+ |= AppAction::BackendTask(...) dispatches in one function — key_info_screen.rs (6), wallets_screen/mod.rs (4), network_chooser_screen.rs (4), dashpay/contact_requests.rs (3). project-reviewer explicitly does not claim those are broken (they may sit in mutually exclusive branches — confirming needs per-site analysis), but they are the exact shape that failed here.
Classified as a follow-up: the local fix in this PR is correct, and the operator's semantics are pre-existing. This PR is not required to fix it — but someone should.
Recommendation: File separately, do not gate this PR.
Audit the 2+ |= dispatch sites listed above for the same silent drop. Longer term, make the lossy merge impossible rather than avoidable: have the accumulator collect into a Vec<BackendTask> (as AddNewIdentityScreen already does via pending_tasks.push), or make BitOrAssign combine task-carrying actions instead of overwriting. Worth a look from architect-nagatha.
Location: src/backend_task/error.rs:48 (path-not-in-diff)
[LOW / non_blocking] WalletTransactionHistoryError::RecordMissing is now a dead variant — the fix removed its only producer
RecordMissing { txid } was constructed solely by the .ok_or(...) on the get_core_tx_record lookup, which this PR replaces with Ok(None) => record_skip(...). Grep across src/ finds the variant's definition and zero constructors.
Dead error surface left behind after a fix is how error enums rot: the next reader assumes the case is still reachable and writes handling for it.
(The WalletStorageError::HashDecode construction was also dropped, but that type is upstream, so nothing is orphaned there.)
Recommendation: Remove RecordMissing unless a producer is coming back, and drop WalletTransactionHistoryError's import if it ends up unused. If it is deliberately retained for a planned caller, say so in a comment.
| .snapshots | ||
| .register_wallet(seed_hash, wallet_id, pw); | ||
| self.hydrate_persisted_transactions_nonfatal(&wallet_id); | ||
| self.inner.snapshots.recompute(&wallet_id); |
There was a problem hiding this comment.
[LOW / non_blocking] Hydration now runs after the snapshot is published, opening a brief window where a wallet is registered with empty history
Walked via: grep -rn across all *.rs.
Both registration paths moved hydrate_persisted_transactions from before id_map insertion to after snapshots.register_wallet(...), immediately followed by recompute. The reordering is necessary and correct — hydration targets a registered snapshot, which is what keeps the wallet visible when a row is corrupt.
But register_wallet publishes to shared state (self.inner.snapshots), so between that call and hydrate_transactions a concurrent reader can observe a registered wallet with no history. The window is a few microseconds of synchronous work with no await, so this is a question about intent rather than an observed defect.
Recommendation: Confirm that snapshot readers tolerate a registered-but-unhydrated wallet (a transient empty history that fills on the next frame is fine; a cached 'empty' verdict is not). If it matters, hydrate into the snapshot before publishing it, or publish under the same lock as the hydration.
There was a problem hiding this comment.
Not fixed — assessed as a false positive: register_wallet only registers internal state; no empty snapshot is published before synchronous transaction hydration and recomputation runs, so the reported window doesn't exist as described. Left as-is; flag if you disagree.
🤖 Co-authored by Claudius the Magnificent AI Agent
| /// Restore persisted public transaction records before publishing a | ||
| /// wallet's first display snapshot. This path never touches wallet secrets. | ||
| fn hydrate_persisted_transactions(&self, wallet_id: &WalletId) -> Result<(), TaskError> { | ||
| fn hydrate_persisted_transactions( |
There was a problem hiding this comment.
[INFO / ?] Call-tree walk clean — the single signature change is self-contained, with no orphaned callers
Walked via: rg fallback (grep -rn across all *.rs).
Inventory of removed/changed function declarations in this diff (git diff origin/v1.0-dev...HEAD | grep -E '^-.*fn [a-z_]+\(') surfaces exactly one non-test signature change:
fn hydrate_persisted_transactions(&self, wallet_id: &WalletId) -> Result<(), TaskError> → Result<PersistedTransactionHydration, TaskError> (private, wallet_backend/mod.rs).
Chain: grep -rn "hydrate_persisted_transactions\b" --include="*.rs" . (excluding the _nonfatal wrapper) finds exactly one caller in the entire repo — the new hydrate_persisted_transactions_nonfatal wrapper added in this same commit, which already matches on the new Ok(PersistedTransactionHydration { records, skipped_rows }) shape and never propagates the error via ?. No other in-repo caller exists; the two former call sites in register_persisted_wallets/single-wallet registration now go through the wrapper.
Recorded as a positive verification result: the walk was performed and found nothing to fix.
Recommendation: No action. The single signature change is self-contained and its only caller was updated atomically in the same commit.
There was a problem hiding this comment.
Not fixed — this is an informational "call-tree walk clean" note from the reviewer itself, not a defect. No change needed.
🤖 Co-authored by Claudius the Magnificent AI Agent
Addresses bbca6608394e, e9bdf38afeeb, SEC-001, SEC-002, SEC-004, SEC-005, SEC-007, PROJ-002, PROJ-003, PROJ-004, CODE-002, CODE-003, CODE-004, CODE-005, CODE-007, CODE-008, CODE-009, CODE-010, CODE-012, CODE-013, CODE-014, and CODE-015. Co-Authored-By: Claude GPT-5 <noreply@anthropic.com>
…01..003)
- CHANGELOG: add the missing token-balance-refresh-status entry (DOC-002).
- docs/gui-testing/README.md + scenarios/TEMPLATE.md: resolve the test
binary via `cargo metadata`'s target_directory instead of assuming
`${CARGO_TARGET_DIR:-target}`, which silently breaks when the target dir
is redirected via .cargo/config.toml rather than the env var (DOC-001).
Also stop implying the desktop-gui automation skill is required (DOC-003).
Co-Authored-By: Codex Sol <noreply@openai.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
All three carried-forward prior findings are independently confirmed FIXED at head 1dad1f8: an unreadable AppSettings blob now sets network_selection_required before falling back to defaults, legacy_settings_import_requires_network_selection now unconditionally gates every SettingsImportError variant (including LegacyDataTooOld/TooNew, and the fresh-install path verifiably reaches NoLegacyData, not TooOld, refuting the GitHub reply that argued for excluding TooOld), and receive-address failures are now correlated via a typed, wallet-specific BackendTaskContext consumed identically by both funding screens. Two in-scope test-coverage/quality items remain from the combined Sonnet+Codex evidence: neither funding screen's display_backend_task_error context-matching transition has a direct unit test, and legacy_settings_import_requires_network_selection retains an unused-in-substance _error parameter that silently escapes clippy's unused_variables lint due to the leading underscore.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed)
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/identities/top_up_identity_screen/mod.rs`:
- [SUGGESTION] src/ui/identities/top_up_identity_screen/mod.rs:605-615: No direct test of the address-failure context-matching state transition at the screen boundary
The fix for e9bdf38afeeb (correlating receive-address failures via BackendTaskContext::GenerateReceiveAddress{seed_hash}) is unit-tested at the BackendTaskContext construction level (src/backend_task/mod.rs), but neither TopUpIdentityScreen::display_backend_task_error nor the duplicated handler in AddNewIdentityScreen::display_backend_task_error (src/ui/identities/add_new_identity_screen/mod.rs:1336-1346) has a focused test proving: (a) an unrelated backend-task error leaves funding_address_request_in_flight/funding_address_request_failed unchanged, and (b) a matching receive-address error for the selected wallet clears the in-flight flag and sets the failed flag. Confirmed via grep across both files: display_backend_task_error is never invoked from a #[test] in either module. Without this coverage, a future edit to either handler (e.g. reverting to a boolean-only check, or breaking the seed-hash comparison) would compile and pass every existing test while silently reintroducing the original duplicate-address-derivation race this PR fixes.
| fn display_backend_task_error(&mut self, context: &BackendTaskContext, _error: &TaskError) { | ||
| let selected_seed_hash = self | ||
| .wallet | ||
| .as_ref() | ||
| .and_then(|wallet| wallet.read().ok().map(|wallet| wallet.seed_hash())); | ||
| if self.funding_address_request_in_flight | ||
| && context.generated_receive_address_wallet() == selected_seed_hash | ||
| { | ||
| self.funding_address_request_in_flight = false; | ||
| self.funding_address_request_failed = true; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No direct test of the address-failure context-matching state transition at the screen boundary
The fix for e9bdf38afeeb (correlating receive-address failures via BackendTaskContext::GenerateReceiveAddress{seed_hash}) is unit-tested at the BackendTaskContext construction level (src/backend_task/mod.rs), but neither TopUpIdentityScreen::display_backend_task_error nor the duplicated handler in AddNewIdentityScreen::display_backend_task_error (src/ui/identities/add_new_identity_screen/mod.rs:1336-1346) has a focused test proving: (a) an unrelated backend-task error leaves funding_address_request_in_flight/funding_address_request_failed unchanged, and (b) a matching receive-address error for the selected wallet clears the in-flight flag and sets the failed flag. Confirmed via grep across both files: display_backend_task_error is never invoked from a #[test] in either module. Without this coverage, a future edit to either handler (e.g. reverting to a boolean-only check, or breaking the seed-hash comparison) would compile and pass every existing test while silently reintroducing the original duplicate-address-derivation race this PR fixes.
source: ['claude', 'codex']
| fn legacy_settings_import_requires_network_selection( | ||
| _error: &crate::backend_task::migration::legacy_settings::SettingsImportError, | ||
| ) -> bool { | ||
| true | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: legacy_settings_import_requires_network_selection is now a vestigial pass-through of a constant
Commit 3af5afc changed the function body to unconditionally return true but kept the _error: &SettingsImportError parameter. Verified locally that a leading-underscore parameter does not trigger clippy's unused_variables lint, so this dead parameter will never be caught by CI. The function's name and signature imply per-variant decision logic that no longer exists, which could mislead a future reader into assuming the function still discriminates on _error when deciding whether a new SettingsImportError variant needs special-casing — when in fact any new variant is already covered by the blanket true. Since the fail-closed behavior is deliberate (confirmed: the GitHub reply arguing for excluding LegacyDataTooOld was independently checked and found incorrect, since a genuine fresh install's stored_data_version resolves to DEFAULT_DB_VERSION=38 via Database::initialize, reaching the Supported/NoLegacyData path rather than TooOld) and unlikely to need per-variant exceptions again, either drop the parameter and inline true at the two call sites, or restore a match with an explicit _ => true arm to preserve the exhaustiveness signal for future variants.
source: ['claude']
…-followups # Conflicts: # src/app.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Independently reverified PR #902 at exact clean head 90a719f, cross-checking the convergent evidence from two completed Codex attempts and two completed Sonnet attempts (general + rust-quality) against the live source tree. All five prior-finding reconciliations hold: the three previously reported defects (settings-blob read failure not gating network selection, unsupported legacy versions not gating network selection, and address-request failures crossing wallet boundaries) are confirmed fixed with matching tests, and the two remaining quality items (missing screen-boundary test for the address-failure correlation fix, and a vestigial unused parameter on legacy_settings_import_requires_network_selection) are confirmed still present and unchanged by the v1.0-dev merge. No new blocking defect exists at this head.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
claude-sonnet-5— verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— rust-quality (completed)
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/identities/top_up_identity_screen/mod.rs`:
- [SUGGESTION] src/ui/identities/top_up_identity_screen/mod.rs:605-616: Address-failure correlation is not tested at the screen boundary
Verified at the current head: TopUpIdentityScreen::display_backend_task_error (here) and the sibling AddNewIdentityScreen::display_backend_task_error (src/ui/identities/add_new_identity_screen/mod.rs:1336-1346) only clear funding_address_request_in_flight and set funding_address_request_failed when context.generated_receive_address_wallet() matches the currently selected wallet's seed hash. I confirmed via repository-wide search that display_backend_task_error is referenced only at its trait definition and the implementing screens (src/app.rs, src/ui/mod.rs, src/ui/tokens/tokens_screen/mod.rs, src/ui/tokens/view_token_claims_screen.rs, src/ui/network_chooser_screen.rs, src/ui/contracts_documents/contracts_documents_screen.rs) — the only call sites in src/app.rs are production dispatch code inside AppState::update (lines 2310, 2341, 2345), not test code, and no file under tests/ references the method. BackendTaskContext::generated_receive_address_wallet() itself is unit-tested (src/backend_task/mod.rs:1793-1801), but the screen-level state transition that consumes it is not. A regression in either duplicated handler — e.g. dropping the seed-hash comparison or comparing against the wrong wallet — would compile and leave the existing suite green while reintroducing the task-misattribution bug this PR fixes.
| fn display_backend_task_error(&mut self, context: &BackendTaskContext, _error: &TaskError) { | ||
| let selected_seed_hash = self | ||
| .wallet | ||
| .as_ref() | ||
| .and_then(|wallet| wallet.read().ok().map(|wallet| wallet.seed_hash())); | ||
| if self.funding_address_request_in_flight | ||
| && context.generated_receive_address_wallet() == selected_seed_hash | ||
| { | ||
| self.funding_address_request_in_flight = false; | ||
| self.funding_address_request_failed = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Address-failure correlation is not tested at the screen boundary
Verified at the current head: TopUpIdentityScreen::display_backend_task_error (here) and the sibling AddNewIdentityScreen::display_backend_task_error (src/ui/identities/add_new_identity_screen/mod.rs:1336-1346) only clear funding_address_request_in_flight and set funding_address_request_failed when context.generated_receive_address_wallet() matches the currently selected wallet's seed hash. I confirmed via repository-wide search that display_backend_task_error is referenced only at its trait definition and the implementing screens (src/app.rs, src/ui/mod.rs, src/ui/tokens/tokens_screen/mod.rs, src/ui/tokens/view_token_claims_screen.rs, src/ui/network_chooser_screen.rs, src/ui/contracts_documents/contracts_documents_screen.rs) — the only call sites in src/app.rs are production dispatch code inside AppState::update (lines 2310, 2341, 2345), not test code, and no file under tests/ references the method. BackendTaskContext::generated_receive_address_wallet() itself is unit-tested (src/backend_task/mod.rs:1793-1801), but the screen-level state transition that consumes it is not. A regression in either duplicated handler — e.g. dropping the seed-hash comparison or comparing against the wrong wallet — would compile and leave the existing suite green while reintroducing the task-misattribution bug this PR fixes.
source: ['codex', 'claude']
…own-channel log (#905) * fix(shutdown): stop wallet backend on GUI close and clear stale shutdown-channel log Await every initialized wallet backend within the bounded GUI shutdown flow, including contexts completed by an in-flight network switch, and consume terminal shutdown receivers exactly once. The platform-wallet coordinator thread join race remains tracked upstream in dashpay/platform#3954 and is not fixed here. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com> * fix(shutdown): make wallet teardown race-safe Close task registration before draining managed work, include the final MCP context, clear remembered secrets, and bound both graceful and fallback wallet backend shutdown paths. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com> * fix(shutdown): wait for backend-task blocking work before wallet teardown Backend tasks run inside tokio::spawn_blocking closures wrapped by an abortable async watcher. On shutdown timeout, aborting the watcher only cancelled the cancellable wrapper — the underlying spawn_blocking closure cannot be forcibly stopped and kept running, unobserved, while wallet backend secrets were cleared and storage torn down underneath it. TaskManager now tracks blocking-task completion via a Shared future kept independent of the abortable watcher, so shutdown can genuinely await real completion (bounded by a new blocking-task timeout) instead of an abortable wrapper. Both the async shutdown path and the on_exit force-close fallback route through the same two-phase wait before shutdown_wallet_backends runs. A still-running blocking task past the bound now reports a distinct degraded ShutdownOutcome::BackendTasksTimedOut instead of silently proceeding as if nothing were wrong. shutdown_hard_deadline() extended to cover both phases. Addresses PR #905 review thread PRRT_kwDOM8GK3c6R6sht. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * fix(shutdown): close double-teardown and secret re-caching races found in review - Latch `shutdown_started` at the first close attempt (graceful or force-close) so on_exit()'s guard blocks fallback re-entry on every branch, not only the clean-finish path -- stops a second close signal from racing wallet teardown against a still-running first attempt (SEC-001). - Call forget_all_secrets() again after the bounded wallet-shutdown wait resolves, including the timeout branch, so a backend task that re-caches a secret mid-wait is still cleared before exit (SEC-002). - Register a SwitchNetwork-created wallet backend before its slow network call resolves, and race that call against shutdown's cancellation token, so an in-flight network switch is discoverable to teardown instead of being invisible to it (CODE-003). - Prune completed blocking-task entries, drop dead TaskManager::shutdown(), centralize the shutdown budget constant, and add a CHANGELOG entry (non-blocking cleanup from the same review). Adds regression tests for all three fixes; cargo fmt/clippy/tests all clean (verified independently, not from the fixing agent's own report). Co-Authored-By: Codex Sol <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> * fix: update merged test to new forward_backend_task_join_error signature The merge of origin/v1.0-dev (904ba0b) auto-merged src/app.rs cleanly at the text level, but left a semantic break: v1.0-dev's new test `panicking_scheduled_vote_sweep_clears_in_progress_guard` (added in #902) called `forward_backend_task_join_error(join_handle, ...)`, passing the raw `JoinHandle` — the pre-#905 signature. This branch's shutdown-race work changed the function to take an already-awaited `Result<(), JoinError>` (so the join can be observed via `tokio::select!` alongside shutdown), which every other call site in this file was updated for except this one new test that didn't exist when the signature changed. Add the missing `.await`, matching the two sibling tests immediately above and below it. No behavioral change to production code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: correct mislabeled platform PR reference in shutdown TODOs The commented-out ShutdownReport check and the ShieldedShutdownIncomplete bucket TODOs referenced platform-pr3968 (rs-platform-wallet-storage, an unrelated SQLite persistence PR). The actual upstream work that restores these types is the ThreadRegistry/shutdown-report PR, platform#3954. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(shutdown): keep wallet teardown behind task barrier Track timed network requests and MCP backend work before wallet teardown, and skip secret clearing whenever managed work fails to drain. Co-Authored-By: OpenAI Codex <noreply@openai.com> * fix(shutdown): close remaining barrier gaps Keep join-error callbacks and MCP network-switch lifecycle work inside the bounded shutdown barrier. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com> * fix(app): store the secret prompt host on AppState The base-branch merge and this PR's NetworkContextRegistered handler both landed changes to AppState without a textual conflict, but the handler referenced a `self.secret_prompt_host` field that never existed — the Arc<dyn SecretPrompt> built in AppState::new() was only installed on the initial network contexts and then dropped. This broke compilation (E0609) for both the Test Suite and Clippy CI jobs. Store the host as an AppState field so contexts registered later (via an in-flight network switch surviving into NetworkContextRegistered) can also have the secret prompt installed on them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(shutdown): forget wallet secrets on degraded shutdown outcomes Two independent reviewers (CodeRabbit and a Claude Code automated review) found that finish_shutdown_after_tasks (src/app.rs) and shutdown_app_context_wallet_backend (src/mcp/server.rs) only cleared session-cached wallet secrets on the Complete outcome. On BackendTasksTimedOut or a failed task manager, the wallet-backend Arc<AppContext> set was collected and then dropped unpolled, so forget_all_secrets() never ran — contradicting this PR's own CHANGELOG entry promising secrets are cleared on close. Split the two concerns: forget_all_secrets() now runs unconditionally on every collected wallet backend regardless of outcome (cheap, synchronous, no dependency on any task finishing), while the full coordinator shutdown() join stays gated on Complete only, since backend tasks may still be using those coordinators on a degraded outcome. Applies to both the async and blocking-fallback GUI shutdown paths, and to the standalone MCP shutdown helper. Adds regression tests proving secrets are forgotten with zero coordinator-shutdown calls on both degraded outcomes, in both src/app.rs::shutdown_tests and the new src/mcp/server.rs::tests. Co-Authored-By: Codex Sol <noreply@openai.com> --------- Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: OpenAI Codex GPT-5 <noreply@openai.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
3 participants
Why this PR exists
v1.0-dev) received substantial review feedback that was never addressed before merge. A bot auto-marked several threads "resolved" using a commit-diff heuristic rather than semantic verification — four of those are demonstrably still broken.AppAction::BitOrAssignreplaces rather than merges), leaving those wallets stuckLoadingfor the rest of the screen session.scheduled_vote_sweeps_in_progress, permanently blocking future sweep attempts for that network until app restart.What was done
Triaged all PR #860 review feedback fresh from GitHub (34 inline threads, 39 review summaries, 28 general comments) and verified each unresolved item against current code — not the bot's "resolved" heuristic. 13 distinct findings confirmed still valid; 9 unresolved threads confirmed already effectively addressed or no longer applicable; 7 threads already resolved on GitHub.
HIGH
top_up_identity_screen: dispatch asset-lock fetches and the receive-address request as a single task vector, matchingAddNewIdentityScreen's existing pattern, instead of two chainedaction |=assignments that silently drop the first.wallet_backend::hydrate_persisted_transactions: skip and log corrupt/invalid history rows instead of aborting the whole registration pass; hydration now runs after the wallet is registered so a bad row degrades history, not the wallet's visibility.MEDIUM
app.rs: scheduled-vote sweep guard is now cleared on any terminal task result, including a panicking join, not just the typed failure variant.wallet_backend::run_token_balance_sync_if_idle: documented the upstream race (is_syncing()before/after sampling) with a TODO pointing at the neededplatform-walletfix; softenedTokenBalanceRefreshSkippedto an info banner instead of a sticky error, since a skip doesn't mean the balance is wrong.send_screen.rs: simple-send confirmation now discloses the actual recipient amount (or that the fee is deducted from it) when the fee comes out of the output, mirroring the advanced confirmation.LOW
WaitingOnFundsstate (with its "Try again" control) instead of resetting to a control-lessReadyToCreate.Testing
cargo build -p dash-evo-tool --all-features— clean.cargo clippy -p dash-evo-tool --all-features --all-targets -- -D warnings— clean.cargo +nightly fmt --all -- --check— clean.Breaking changes
None.
Checklist
cargo fmt/cargo clippycleandocs/user-stories.mdnot updatedAttribution
🤖 Co-authored by Claudius the Magnificent AI Agent