Repository navigation
fix(platform-wallet): carry a resumed asset lock's change into the store - #5716
Conversation
An asset lock whose InstantSend lock was not persisted before the app stopped is restored at load as a synthetic record with no output details. When the lock or the block arrives, key-wallet confirms the change in memory and re-emits that record; the FFI stores derive coin flags only from a record's output details, so the change row kept is_confirmed=false / is_instantlocked=false for good and the coin never became spendable after the next launch. The restored record now carries the coins its account holds from the transaction, classified with key-wallet's own account match (held_output_details), so the re-emit writes the change's new flags. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
✅ Final review complete — no blockers (commit 8510ed2) · triage: normal |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
At head 462f524, the fix correctly reconstructs held output details in platform-wallet and leaves the FFI adapter to consume them; the upstream confirmation behavior and linked rust-dashcore#1291 support its premise. No blocking defect was found; one combined suggestion addresses diagnostics and coverage for inconsistent restored address pools. This was a static review: current CI reports successful Rust wallet, Swift SDK, frozen-schema, and Kotlin checks, with PR Hygiene pending.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The production change is a small, contained reconstruction of held output details during wallet restore, with extensive regression tests and no changes to coin selection, consensus, or storage schemas. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 98% left, weekly 99% left; passed overgemini-3.8-flash-high(antigravity quota lookup failed) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort medium); agentphase2-reviewer
🤖 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 `packages/rs-platform-wallet/src/wallet/asset_lock/restored_record.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/restored_record.rs:78-80: Report and test held outputs missing from restored address pools
The warning above covers a complete transaction mismatch, but this branch silently skips a held output when the transaction matches through another output or a tracked input and this output's address is missing from the restored pools. That state is representable: the persistence loader inserts UTXOs without requiring their addresses to exist in a pool, and key-wallet's matcher can return a match independently of this output. Excluding an unclassifiable output is appropriate, but its confirmation flags will be absent from the record's store projection without the diagnostic provided for the complete-mismatch case. Log the txid and output index here, and add focused tests for both partial mismatch and the existing held-coin/no-match warning; the current restoration tests exercise neither diagnostic case. This is an observability and coverage improvement for inconsistent persisted state, not a blocker for the normal restore path.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Track recovery for wallets whose asset-lock change is already stuck — The existing load path restores synthetic funding records only for unresolved asset locks. A wallet that previously persisted the lock as resolved but left its change unconfirmed therefore does not enter the repaired path. This is a concrete recovery gap for already-affected wallets, but the diff neither introduces nor worsens it.
- Follow-up: Track a separate targeted recovery or rescan for stale confirmation flags on change from already-resolved asset locks.
|
Bots are done — your move: post |
held_output_details left out, without a log, a held output whose address the account's match does not name as receive or change, or whose script has no address. Each now logs a warning with the txid, vout and account. The whole-transaction no-match warning names the held vouts. The held outputs are collected once, and the account is matched only when it holds one. Tests in platform-wallet capture both warnings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete diff at 30697bf and verified the restoration, upstream confirmation, and persistence-projection paths; no in-scope findings remain. The prior diagnostic finding is fixed: excluded held outputs are now reported, and focused tests cover partial and complete classification mismatches. Validation was static only; current CI still has “Select compatible runner image” and “Determine changed packages” pending, so head-specific build and test success is not yet established.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff changes wallet restore and confirmation propagation logic with substantial tests, but does not itself introduce intricate funds movement, coin selection, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
Bots are done — your move: post |
…ange-confirmation # Conflicts: # packages/rs-platform-wallet-ffi/src/persistence.rs
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete diff at 8510ed2 and found no actionable defects. Restored output classification stays in platform-wallet, follows key-wallet's role precedence, and includes only coins the account holds; the broader upstream repair is tracked in dashpay/rust-dashcore#1291. Validation was static only: current CI shows Rust wallet tests and Kotlin build/tests passing, with Swift SDK build/tests still pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff adds contained but nontrivial wallet restoration and output-classification logic with substantial tests, without intricate changes to funds movement, coin selection, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
Bots are done — your move: post |
|
/self-reviewed |
|
Missing human approval. Automatic reviewer requests are paused and this PR is excluded from reviewers' queues. Merge, close, or draft another active PR by this author to free a review slot. Required human review can be satisfied without a slot; once all merge requirements are met, PR Hygiene passes. |
Basic explanation
What this does: When the app stops between sending an asset lock and receiving its InstantSend lock, the wallet resumes that lock on the next launch from a stripped-down copy of the transaction. That copy did not list the wallet's own change, so when the network later confirmed the lock, the change was confirmed in memory but never in the app's database. This change makes the resumed copy list the change, so the confirmation reaches the database.
Value: The change of such an asset lock stays spendable after the next launch. Before, it was counted in the balance but excluded from every send: Max from Core used only part of the Core balance, and a larger amount was refused as "Insufficient balance". The only way out was a reset and restore.
Risks: Low. No consensus or protocol code. The change only affects a record that is re-created at load for an unresolved asset lock, and it lists only coins that account holds at that moment, so it cannot bring back a spent coin. Wallets that already hit the bug are repaired by #5719, not by this change.
Issue being fixed or feature implemented
The FFI stores (SwiftData, and Kotlin through
rs-unified-sdk-jni) take a coin'sis_confirmed/is_instantlocked/ height only from theoutput_detailsof an emitted record (record_new_utxos_ffi).restore_unresolved_asset_lock_tx_recordsre-inserts a Built/Broadcast asset lock's funding transaction as a synthetic record with emptyoutput_details. When its InstantSend lock or block arrives, key-wallet finds that record, updates its context and the engine's UTXO, and re-emits it asupdated. With no details, nothing reaches the store, so the change row keeps the flags it had in the mempool. The next launch restores the coin from that row as unconfirmed, and the block is never scanned again.What was done?
rs-platform-wallet/src/wallet/asset_lock/restored_record.rs(new):held_output_details(account, tx)returns the outputs oftxthat the account holds as coins, with the role from key-wallet's own match (check_transaction_for_match): receive address →Received, change address →Change, inrecord_transaction's order. Outputs not in the account'sutxos(spent, or another account's) are left out. It logs a warning when the account holds a coin of the transaction but does not match it.rs-platform-wallet-ffi/src/persistence.rs: the restored record gets theseoutput_detailsinstead of an empty list. The UTXOs are restored earlier in the same load, so they are in place. Swift and Kotlin share this restore and the same projection, so both get the fix.Not in this PR:
confirm_transactionand the InstantSend path update a known record's context without rebuilding itsoutput_details. Tracked in key-wallet: confirming or InstantSend-locking a known record does not refresh its output_details rust-dashcore#1291.The SQLite persister is not affected: it never restores this synthetic record.
How Has This Been Tested?
New tests in
rs-platform-wallet-ffi/src/persistence.rs. They build a real asset-lock transaction (burn output at vout 0, credit output in the payload, change at vout 1), see it in the mempool, drop the in-memory record as a reload does, restore it, and read the result throughWalletChangeSetFFI::from_changeset, the projection the stores apply:should_name_the_held_change_on_a_restored_lock_recordshould_store_the_change_confirmed_when_a_restored_lock_is_minedshould_store_the_change_locked_when_a_restored_lock_gets_its_instant_lock(throughmark_instant_send_utxos, the path for a lock on a known transaction)should_leave_a_coin_no_longer_held_off_a_restored_lock_recordshould_store_another_accounts_change_confirmed_when_a_restored_lock_is_minedThe first three fail on
v5.0-devwithout the change. The last two are boundaries and pass both ways.cargo test -p platform-wallet-ffi --all-features: 444 passedcargo test -p platform-wallet --all-features: 1521 passedcargo clippy -p platform-wallet -p platform-wallet-ffi --all-features --all-targets -- -D warnings: cleancargo check -p rs-unified-sdk-jni: buildsBreaking Changes
None.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
8510ed2rs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/src/persistence.rs) — HashEngineering or ZocoLini or llbartekllrs-platform-wallet(packages/rs-platform-wallet/src/wallet/asset_lock/mod.rs,packages/rs-platform-wallet/src/wallet/asset_lock/restored_record.rs) — HashEngineering or ZocoLini or llbartekllWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.