feat(platform-wallet): rebuild tracked asset locks after restore; honest scan-derived shielded history - #4342
Conversation
…records Tracked asset locks (and the host's persisted mirror, e.g. the swift-sdk PersistentAssetLock store) were only recorded live at build/broadcast time, so they did not survive a wipe & recover: a restored wallet's historical asset-lock funding txs had no tracked entry and hosts could not classify them (dashwallet-ios rendered "Internal Transfer — 0 DASH" until it grew a client-side fallback in dashwallet-ios#939). The classification signal survives restore: every asset-lock credit output pays a one-time address from a purpose-specific funding account, and the restore scan already files a TransactionRecord under that account. The wallet-event adapter now reconstructs missing tracked entries from those records (TransactionDetected + BlockProcessed inserted/updated) and persists them in the same store() as the core rows. Insert-if-absent: live build-pipeline entries always win. Reconstructed entries carry a new status, RecoveredFromChain (raw 5, label "recovered_from_chain"): core finality is known (a ChainAssetLockProof is attached when the record context is chain-locked), but Platform-side consumption is unknown after a restore — neither ChainLocked (UIs read 1…3 as in-flight) nor Consumed (claims success) would be truthful. The new status is outside both windows, so restored historical locks neither render as stuck nor feed auto-resume sweeps. An explicit resume_asset_lock may still consume one — Platform arbitrates and rejects an already-spent outpoint. identity_index recovery is exact for IdentityTopUp (the account key is the registration index) and reports 0 for the singleton families, whose credit-output address does not encode the destination index. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e artifacts The restore-scan activity deriver stamped every reconstructed entry (and the batch key it clusters by) with the discovering chunk's proof-anchor height — the chain tip the response was proven at — and created_at_ms = the scan wall clock. Observed on the same wallet restored on two simulators: the identical entry (same entryId) showed 'Aug 7 07:20 / block 411495' on one device and 'Aug 8 11:13 / block 412108' on the other, and weeks-old restored transfers grouped under 'Today'. The real inclusion height is unknowable client-side: the note-fetch proof carries no per-note mined height, nullifier items are stored with empty values, and the on-chain anchors-by-height index is pruned to a recent retention window — so for restored history there is nothing honest to stamp. Scan-derived entries now carry block_height: None and created_at_ms: 0 (documented unknown sentinel), both device-independent, so two restores of the same wallet produce byte-identical entries. The display sort gains a fourth band: unknown-age scan-derived rows (no height AND no record time) sink below every dated/heighted row — unknown age must not read as 'newest' — while fresh live successes whose height the scan hasn't backfilled yet keep floating on top. Live rows are untouched: the live recorder keeps its genuine record time, and the Pending→Confirmed sighting flip still backfills the observed-at height (near-tip for the live flows it serves, documented as an at-or-before bound). Scan-derived rows are exempted from that backfill (keyed off the created_at_ms == 0 marker) so the next pass's sighting for a restored row's own cluster can't smuggle the scan-tip height back onto it. Exposing real per-note inclusion heights (and block times) would need a node-side change — per-note heights in the shielded note items and their fetch proof. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe wallet reconstructs tracked asset locks from historical transaction records and persists them with core changes. It adds ChangesAsset-lock recovery
Shielded activity provenance
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CoreBridge
participant Reconstruction
participant WalletManager
participant PlatformPersister
CoreBridge->>Reconstruction: select asset-lock transaction records
Reconstruction->>WalletManager: reconstruct missing tracked locks
WalletManager-->>Reconstruction: return AssetLockChangeSet
Reconstruction-->>CoreBridge: merge recovered asset-lock changes
CoreBridge->>PlatformPersister: persist wallet changes atomically
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
… inclusion height Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕓 Ready for review — 4 ahead in queue (commit 0a2031a) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-platform-wallet/src/manager/accessors.rs (1)
649-656: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the
statusfield doc to list codes 4 and 5.The match now produces 6 codes. The doc on
TrackedAssetLockSnapshot.status(Line 154) still says0=Built, 1=Broadcast, 2=InstantSendLocked, 3=ChainLocked. Hosts read this doc to decode theu8. Add4=Consumed, 5=RecoveredFromChain.📝 Proposed doc fix (Line 154)
- /// 0=Built, 1=Broadcast, 2=InstantSendLocked, 3=ChainLocked. + /// 0=Built, 1=Broadcast, 2=InstantSendLocked, 3=ChainLocked, + /// 4=Consumed, 5=RecoveredFromChain. pub status: u8,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/manager/accessors.rs` around lines 649 - 656, Update the documentation for TrackedAssetLockSnapshot.status to include the decoding entries 4=Consumed and 5=RecoveredFromChain, preserving the existing descriptions for codes 0 through 3.packages/rs-platform-wallet/src/changeset/core_bridge.rs (1)
444-461: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftA rejected asset-lock store is not retried in this process run.
reconstruct_tracked_asset_locksinserts the entry intoinfo.tracked_asset_locksbeforecommit_batchruns (packages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rs, Line 241). Ifpersister.storethen fails at Line 468, the durable row is absent but the in-memory entry remains.The core-record path recovers from this: the next scan re-emits the record and the idempotent upsert lands it, as documented at Lines 469-471. The asset-lock path does not. Reconstruction is insert-if-absent (
reconstruction.rsLines 185-187), so the re-emitted record now finds the in-memory entry and produces no changeset. The row is only re-offered after a process restart clears the in-memory map.Two options:
- Drop the reconstructed outpoints from
info.tracked_asset_lockswhenstorerejects, so the next scan re-emits them.- Move the in-memory insert after a successful
store, and keep the changeset as the only pre-persistence artifact.The second option is the more direct fix, but it changes the reconstruct/commit split. Confirm which behavior you intend before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/changeset/core_bridge.rs` around lines 444 - 461, Ensure reconstructed asset-lock entries are not retained in info.tracked_asset_locks before persistence succeeds. Update the reconstruction/commit flow around reconstruct_tracked_asset_locks and the persister.store result so entries are inserted only after a successful store, or removed when store rejects, allowing the next scan to re-emit them while preserving atomic changeset persistence.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rs (1)
565-607: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the final status in
recovered_lock_resumes_from_attached_proof.The doc at Lines 566-567 states that the status advances to
ChainLockedon the way out. The test asserts only the returned proof.resume_asset_lockperforms the status advance at step 3, so the transition fromRecoveredFromChaintoChainLockedis currently untested. Add the assertion so the documented behavior is covered.💚 Proposed test addition
match proof { dpp::prelude::AssetLockProof::Chain(chain) => { assert_eq!(chain.core_chain_locked_height, 77); } other => panic!("expected the reconstructed chain proof, got {other:?}"), } + + let wm = wallet_manager.read().await; + assert_eq!( + wm.get_wallet_info(&wallet_id) + .expect("wallet") + .tracked_asset_locks + .get(&out_point) + .expect("tracked entry") + .status, + AssetLockStatus::ChainLocked, + "an explicit resume must advance the recovered lock past RecoveredFromChain" + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rs` around lines 565 - 607, Update recovered_lock_resumes_from_attached_proof to assert that the reconstructed asset lock’s final status is ChainLocked after resume_asset_lock completes. Preserve the existing proof validation and use the available wallet/asset-lock lookup state to verify the transition from RecoveredFromChain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/asset_lock/manager.rs`:
- Line 79: Update the ABI documentation for TrackedAssetLockFFI.status to list
every exposed status value, including Consumed (4) and RecoveredFromChain (5),
while preserving the existing mappings for values 0 through 3.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- Line 88: Add a new SQLite migration for the existing asset-locks schema that
replaces the `asset_locks.status` CHECK constraint with one including
`recovered_from_chain`, without modifying only the initial `V001__initial.rs`
schema. Add an upgrade test that starts from the prior schema, applies the
migration, and verifies persisting the new status succeeds.
In `@packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs`:
- Around line 310-331: Update the RecoveredFromChain handling in the asset-lock
sync method so the None branch reuses the same best-effort transaction
re-broadcast behavior as the Broadcast arm before calling wait_for_proof, using
the transaction held by lock.transaction and preserving existing error handling.
Also update that method’s documentation to include RecoveredFromChain among the
statuses it handles.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- Around line 444-461: Ensure reconstructed asset-lock entries are not retained
in info.tracked_asset_locks before persistence succeeds. Update the
reconstruction/commit flow around reconstruct_tracked_asset_locks and the
persister.store result so entries are inserted only after a successful store, or
removed when store rejects, allowing the next scan to re-emit them while
preserving atomic changeset persistence.
In `@packages/rs-platform-wallet/src/manager/accessors.rs`:
- Around line 649-656: Update the documentation for
TrackedAssetLockSnapshot.status to include the decoding entries 4=Consumed and
5=RecoveredFromChain, preserving the existing descriptions for codes 0 through
3.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rs`:
- Around line 565-607: Update recovered_lock_resumes_from_attached_proof to
assert that the reconstructed asset lock’s final status is ChainLocked after
resume_asset_lock completes. Preserve the existing proof validation and use the
available wallet/asset-lock lookup state to verify the transition from
RecoveredFromChain.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6aef6218-1f60-4ca2-a022-67dab3a9bdd3
📒 Files selected for processing (17)
packages/rs-platform-wallet-ffi/src/asset_lock/manager.rspackages/rs-platform-wallet-ffi/src/asset_lock_persistence.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rspackages/rs-platform-wallet/src/changeset/core_bridge.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-platform-wallet/src/wallet/asset_lock/mod.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/mod.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rspackages/rs-platform-wallet/src/wallet/asset_lock/tracked.rspackages/rs-platform-wallet/src/wallet/shielded/activity.rspackages/rs-platform-wallet/src/wallet/shielded/coordinator.rspackages/rs-platform-wallet/src/wallet/shielded/store.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAssetLock.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentShieldedActivity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The shielded-history changes are coherent, but the asset-lock reconstruction path has four release-blocking compatibility and recoverability defects: existing SQLite schemas diverge, mempool records are frozen into an on-chain status, nonzero credit outputs resume with the wrong key, and Kotlin drops the new status. The public FFI status documentation also needs a small update.
Source: reviewer backends gpt-5.6-sol (general, rust-quality, ffi-engineer); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking | 💬 1 nitpick(s)
2 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 `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:82-88: Append a migration instead of changing V001's generated CHECK constraint
`V001__initial.rs` builds its `asset_locks.status` CHECK clause from `ASSET_LOCK_STATUS_LABELS`, so adding `recovered_from_chain` here changes the generated V001 SQL and its Refinery checksum. The storage runner uses Refinery's default `abort_divergent = true`; any database that already applied the previous V001 therefore fails in `SqlitePersister::open` with a divergent-migration error before it can be used. Disabling that check would not solve the schema mismatch because the old CHECK constraint would still reject reconstructed rows. Keep V001 byte-identical to the prior release and append a migration that rebuilds `asset_locks` with the expanded domain, plus an upgrade test beginning from the prior V003 schema.
In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rs:100-114: Do not mark mempool detections as recovered from chain
This function assigns `RecoveredFromChain` to every context, including the `TransactionContext::Mempool` records deliberately accepted from `TransactionDetected`. That contradicts the status invariant that Core finality is known and changes behavior: `resume_asset_lock` treats this status as already on-chain, skips the defensive rebroadcast, and may wait indefinitely after the transaction is evicted or never propagated. The insert-if-absent check at lines 185-187 also means the later `BlockProcessed.updated` record cannot replace that proof-less entry with the chain proof. Represent non-final detections with broadcast/rebroadcast semantics (or restrict this status to finalized records), and allow a later finalized record to enrich entries created by this reconstruction path without overwriting live build-pipeline entries.
In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:407-411: Resume uses the wrong key for reconstructed credit outputs after vout zero
Reconstruction now creates a tracked lock for every wallet-owned credit output and records its DIP-0027 credit-output index in `out_point.vout`. This resume helper still always selects the first output, so a reconstructed lock at vout 1 or later either fails to find the wallet's path or returns output 0's one-time key. The proof identifies the later outpoint, and the resulting Platform transition is therefore signed with the wrong key. Select the payload output by the tracked vout with bounds checking and add a reconstruction/resume test containing multiple credit outputs.
In `packages/rs-platform-wallet/src/manager/accessors.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/accessors.rs:655: Kotlin drops every recovered asset lock at the JNI boundary
This new arm emits raw status 5 through `TrackedAssetLockEntryFFI`, and the JNI bridge forwards that byte unchanged. Kotlin's `TrackedAssetLock.Status` defines only 0 through 3, while `eligibleFromNative` resolves with `firstOrNull` and then `mapNotNull`; every reconstructed row is therefore silently discarded. Android's registration and top-up recovery screens obtain their selectable outpoints exclusively from `trackedIdentityRecoveryAssetLocks`, so restored locks cannot reach `resumeWithExistingAssetLock` or `resumeTopUpWithExistingAssetLock`. Add `RECOVERED_FROM_CHAIN(5)` to the Kotlin model and update the eligibility tests/policy, while continuing to exclude this status from any automatic stuck-lock sweep that should not retry historical locks blindly.
| "is_locked", | ||
| "chain_locked", | ||
| "consumed", | ||
| "recovered_from_chain", |
There was a problem hiding this comment.
🔴 Blocking: Append a migration instead of changing V001's generated CHECK constraint
V001__initial.rs builds its asset_locks.status CHECK clause from ASSET_LOCK_STATUS_LABELS, so adding recovered_from_chain here changes the generated V001 SQL and its Refinery checksum. The storage runner uses Refinery's default abort_divergent = true; any database that already applied the previous V001 therefore fails in SqlitePersister::open with a divergent-migration error before it can be used. Disabling that check would not solve the schema mismatch because the old CHECK constraint would still reject reconstructed rows. Keep V001 byte-identical to the prior release and append a migration that rebuilds asset_locks with the expanded domain, plus an upgrade test beginning from the prior V003 schema.
source: ['codex']
There was a problem hiding this comment.
Addressed in a6cfb13: V001 is frozen back to its original five-label CHECK (byte-identical generated SQL, so existing databases' Refinery checksums verify), and the domain widens via an appended V004__asset_lock_recovered_status.rs table rebuild that preserves rows and the wallet_metadata FK. tc045_v004_widens_asset_lock_status_on_existing_db drives the exact upgrade an existing install experiences (V003 schema + data → latest), asserting the old CHECK rejected the new label, the pre-upgrade row survives the rebuild, the widened domain admits recovered_from_chain, garbage stays rejected, and ON DELETE CASCADE survives. A new pin test (asset_lock_status_labels_frozen_in_latest_migration) fails with append-a-migration instructions if the live const ever drifts from V004's frozen list.
| fn recovered_status( | ||
| record: &TransactionRecord, | ||
| out_point: OutPoint, | ||
| ) -> (AssetLockStatus, Option<dpp::prelude::AssetLockProof>) { | ||
| use dpp::identity::state_transition::asset_lock_proof::chain::ChainAssetLockProof; | ||
| let proof = match &record.context { | ||
| TransactionContext::InChainLockedBlock(_) => record.height().map(|height| { | ||
| dpp::prelude::AssetLockProof::Chain(ChainAssetLockProof { | ||
| core_chain_locked_height: height, | ||
| out_point, | ||
| }) | ||
| }), | ||
| _ => None, | ||
| }; | ||
| (AssetLockStatus::RecoveredFromChain, proof) |
There was a problem hiding this comment.
🔴 Blocking: Do not mark mempool detections as recovered from chain
This function assigns RecoveredFromChain to every context, including the TransactionContext::Mempool records deliberately accepted from TransactionDetected. That contradicts the status invariant that Core finality is known and changes behavior: resume_asset_lock treats this status as already on-chain, skips the defensive rebroadcast, and may wait indefinitely after the transaction is evicted or never propagated. The insert-if-absent check at lines 185-187 also means the later BlockProcessed.updated record cannot replace that proof-less entry with the chain proof. Represent non-final detections with broadcast/rebroadcast semantics (or restrict this status to finalized records), and allow a later finalized record to enrich entries created by this reconstruction path without overwriting live build-pipeline entries.
source: ['codex']
There was a problem hiding this comment.
Addressed in a6cfb13: RecoveredFromChain is now reserved for InChainLockedBlock records (finality proven, chain proof attached by construction). Mempool / unconfirmed-block detections enter with the live pipeline's own pre-finality statuses — Broadcast / InstantSendLocked, mirroring resolve_status_with_in_memory — so resume_asset_lock keeps its defensive re-broadcast for evictable txs. And a later finalized record now enriches a still-unproven Broadcast/InstantSendLocked entry in place (attaches the chain proof, advances to ChainLocked — the same terminal state every live path produces on observing the same finality) instead of being dropped by insert-if-absent; Built (owned by an in-flight build) and Consumed (terminal) are never touched. Covered by unconfirmed_record_reconstructs_as_broadcast, finalized_record_enriches_unproven_tracked_entry, and enrichment_leaves_built_and_consumed_entries_alone.
| AssetLockStatus::InstantSendLocked => 2, | ||
| AssetLockStatus::ChainLocked => 3, | ||
| AssetLockStatus::Consumed => 4, | ||
| AssetLockStatus::RecoveredFromChain => 5, |
There was a problem hiding this comment.
🔴 Blocking: Kotlin drops every recovered asset lock at the JNI boundary
This new arm emits raw status 5 through TrackedAssetLockEntryFFI, and the JNI bridge forwards that byte unchanged. Kotlin's TrackedAssetLock.Status defines only 0 through 3, while eligibleFromNative resolves with firstOrNull and then mapNotNull; every reconstructed row is therefore silently discarded. Android's registration and top-up recovery screens obtain their selectable outpoints exclusively from trackedIdentityRecoveryAssetLocks, so restored locks cannot reach resumeWithExistingAssetLock or resumeTopUpWithExistingAssetLock. Add RECOVERED_FROM_CHAIN(5) to the Kotlin model and update the eligibility tests/policy, while continuing to exclude this status from any automatic stuck-lock sweep that should not retry historical locks blindly.
source: ['codex']
There was a problem hiding this comment.
Addressed in a6cfb13: TrackedAssetLock.Status gains RECOVERED_FROM_CHAIN(5) and eligibleFromNative now surfaces those rows, so the registration / top-up recovery screens (the only consumers of trackedIdentityRecoveryAssetLocks — both user-driven pickers, no automatic sweep exists on this path) can feed them to resumeWithExistingAssetLock / resumeTopUpWithExistingAssetLock. Consumed (4) stays deliberately unmapped as a terminal tombstone, and the class doc now states explicitly that this status must never enter an automatic stuck-lock retry sweep. The eligibility test pins a status-5 row surviving the filter. (The Kotlin/workspace CI compile failure — the JNI restore path missing the new shielded-activity FFI fields — is fixed in the same commit.)
Scan-derived restored entries deliberately carry no height and no timestamp (their real values are unknowable client-side), which left the 'unknown age' display band ordered by entry id — random, though identical across devices. The exact chain order IS knowable: commitment tree positions are append-only chain order, and the entry's received notes carry theirs. ShieldedActivityEntry gains min_note_position (Option<u64>, serde default for previously-persisted entries): the smallest tree position among the entry's own received notes, set by the scan deriver. None on live entries (which order by their real record time) and on the rare outgoing-only cluster (OVK-recovered sends don't persist a position). The display sort's unknown-age band now orders by it descending, so restored history reads newest-first like the rest of the list — in its true on-chain sequence, identically on every device. Plumbed through the persist + restore FFI structs (min_note_position + has_min_note_position) and the swift-sdk snapshot/model (PersistentShieldedActivity.minNotePosition/hasMinNotePosition, with defaults covering pre-existing rows). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4342 +/- ##
============================================
- Coverage 87.62% 87.60% -0.02%
============================================
Files 2704 2704
Lines 345206 345268 +62
============================================
+ Hits 302473 302489 +16
- Misses 42733 42779 +46
🚀 New features to boost your workflow:
|
- Storage: V001's asset-lock status CHECK is frozen at the original five labels again — interpolating the live ASSET_LOCK_STATUS_LABELS const changed V001's generated SQL and its Refinery checksum, which bricks every already-migrated database (abort_divergent). The domain now widens by APPENDING V004, a table rebuild that preserves rows and the wallet_metadata FK. A V003→V004 upgrade test drives the exact sequence an existing install experiences, and a pin test fails with append-a-migration instructions if the live const ever drifts from V004's frozen list. - Reconstruction: RecoveredFromChain is reserved for records with proven Core finality (InChainLockedBlock), matching the variant's 'finality known, consumption unknown' invariant — the chain proof is attached by construction. Non-final detections (mempool sightings from a same-seed device, unconfirmed blocks) now enter with the live pipeline's own pre-finality statuses (Broadcast / InstantSendLocked), keeping resume's defensive re-broadcast for evictable txs. A later finalized record enriches a still-unproven Broadcast/IS entry in place (chain proof + ChainLocked) instead of being dropped by insert-if-absent; Built and Consumed entries are never touched. - Kotlin: TrackedAssetLock.Status gains RECOVERED_FROM_CHAIN(5) so restored locks reach the user-driven registration / top-up recovery screens instead of being silently dropped by eligibleFromNative (Consumed stays deliberately unmapped). rs-unified-sdk-jni's restore path initializes the new shielded-activity chain-order FFI fields as honestly absent (the Kotlin store doesn't persist them yet) — this was the workspace/Kotlin CI compile failure. - FFI: TrackedAssetLockFFI.status doc covers the full 0–5 domain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/TrackedAssetLock.kt (1)
43-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCite the Swift counterpart in the KDoc.
This status mirrors the iOS asset-lock model. Add a reference to the corresponding Swift source file, for example
PersistentAssetLock.swiftorManagedAssetLockManager.swift, in the KDoc forRECOVERED_FROM_CHAIN.As per coding guidelines: "When porting behavior from iOS, cite the corresponding Swift source file in KDoc."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/TrackedAssetLock.kt` around lines 43 - 46, The KDoc for RECOVERED_FROM_CHAIN must cite its Swift counterpart. Add a reference to the corresponding PersistentAssetLock.swift or ManagedAssetLockManager.swift source file while preserving the existing recovery-status description.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/rs-platform-wallet-storage/migrations/V004__asset_lock_recovered_status.rs`:
- Around line 37-38: Before the INSERT that rebuilds asset_locks in the
migration, run PRAGMA foreign_key_check and apply an explicit policy for any
orphan rows so legacy violations cannot abort the copy with a foreign-key error.
Update the migration’s asset_locks rebuild flow to detect and handle these
violations before copying into asset_locks_v4, while preserving the normal copy
behavior when no violations exist.
In `@packages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rs`:
- Around line 121-127: Update the InChainLockedBlock arm in the record-context
match so it returns RecoveredFromChain only when record.height() produces a
chain proof; when the height is absent, bind the status to the
non-final/recoverable state used for records without proof. Preserve the
existing proof construction for records with a height and ensure status and
proof availability remain consistent.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/TrackedAssetLock.kt`:
- Around line 43-46: The KDoc for RECOVERED_FROM_CHAIN must cite its Swift
counterpart. Add a reference to the corresponding PersistentAssetLock.swift or
ManagedAssetLockManager.swift source file while preserving the existing
recovery-status description.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 95b485ce-ad99-416c-8399-8a622bf5aef4
📒 Files selected for processing (11)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/TrackedAssetLock.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/TrackedAssetLockTest.ktpackages/rs-platform-wallet-ffi/src/asset_lock/manager.rspackages/rs-platform-wallet-storage/migrations/V001__initial.rspackages/rs-platform-wallet-storage/migrations/V004__asset_lock_recovered_status.rspackages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rspackages/rs-platform-wallet-storage/tests/sqlite_migrations.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/reconstruction.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rspackages/rs-platform-wallet/src/wallet/asset_lock/tracked.rspackages/rs-unified-sdk-jni/src/persistence.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rs-platform-wallet-ffi/src/asset_lock/manager.rs
- packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs
- packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs
…ed status to proof availability - V004 drops legacy orphan asset_locks rows (created while an old connection had FK enforcement off) before the copy — the FK-declared twin would otherwise abort the whole rebuild with 'FOREIGN KEY constraint failed'. Dropping matches what the declared ON DELETE CASCADE would have done, and the upgrade test now plants exactly such an orphan and asserts the migration survives it. - The clippy dead-code failure: ASSET_LOCK_STATUS_LABELS is test-only now that migrations freeze their own domain copies — scope it #[cfg(test)] (it remains the drift guard pinning writer ⇔ latest migration). - reconstruction: a chain-locked record that somehow lacked a height (unreachable via the current context shape, but the API allows it) no longer yields a permanently proof-less RecoveredFromChain entry — status stays bound to proof availability, entering as Broadcast so enrich_from_record can still upgrade it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e storage blob A proof-carrying AssetLockEntry written to the SQLite lifecycle_blob could never be read back: AssetLockProof's serde impl requires deserialize_any, which bincode-serde rejects (AnyNotSupported). Latent until now — the only roundtrip test used proof: None, and the restore reconstruction is the first writer that persists chain proofs through this path. The proof field now serializes as opaque dpp-bincode bytes via a serde(with) adapter — the same proof wire encoding the FFI layer and swift-sdk's PersistentAssetLock.proofBytes already use, so every persistence surface speaks one format. None encodes identically to the old derive (Option tag byte), and old Some blobs were undecodable to begin with, so no readable data changes meaning. Adds tc010b: a RecoveredFromChain lock with its ChainAssetLockProof round-trips through the widened V004 CHECK, the writer's TEXT status mapping, and the blob codec. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed 3f28a74 for the codecov/patch failure. Two things in it:
Remaining uncovered patch lines are the multi-line failure-message literal inside a passing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/changeset/serde_adapters.rs`:
- Around line 137-139: Update the persisted asset-lock proof deserialization
around decode_from_slice to validate that the consumed-byte count equals
b.len(), returning serde::de::Error::custom when trailing bytes remain; preserve
successful decoding for exact-input payloads and add a regression test covering
appended bytes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6276cdf8-775a-4074-a83b-299fefb7b741
📒 Files selected for processing (3)
packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/serde_adapters.rs
…verable space The migration-pin test's multi-line assert message is a never-taken panic branch, so llvm-cov reported its six literal lines as the bulk of this file's uncovered patch lines. The guidance now lives in the test's doc comment (comments aren't coverable) and the assert compares against a named array — same failure signal, no phantom misses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
What
Two fixes for restored-wallet history fidelity in
rs-platform-wallet, from the dashwallet-ios restored-internal-transfers investigation (dashwallet-ios#939):1. Tracked asset locks repopulate after a wallet restore
tracked_asset_locks(and the host's persisted mirror — the swift-sdkPersistentAssetLockstore) was only recorded live at build/broadcast time, so it did not survive a wipe & recover: restored wallets rendered asset-lock funding txs as "Internal Transfer — 0 DASH" until dashwallet-ios grew a client-side parsing fallback.The classification signal survives restore: every asset-lock credit output pays a one-time address from a purpose-specific funding account, and the restore scan already files a
TransactionRecordunder that account (key-wallet's router checks all six funding families for AssetLock txs). The wallet-event adapter (core_bridge) now reconstructs missing tracked entries from those records —TransactionDetected+BlockProcessedinserted/updated, behind a lock-free pre-filter — and persists them in the samestore()round-trip as the core rows. Insert-if-absent: live build-pipeline entries always win.Reconstructed entries carry a new status,
RecoveredFromChain(raw 5, labelrecovered_from_chain): core finality is known (aChainAssetLockProofis attached when the record context is chain-locked), but Platform-side consumption is unknown after a restore. NeitherChainLocked(UIs read 1…3 as in-flight — restored consumed shields would render "stuck" and feed the app's stuck-shield recovery sweep) norConsumed(claims success) would be truthful; raw 5 sits outside both windows. An explicitresume_asset_lockmay still consume one — Platform arbitrates and rejects an already-spent outpoint with a typed error.identity_indexrecovery is exact forIdentityTopUp(the account key is the registration index) and reports 0 for the singleton families, whose credit-output address does not encode the destination index.2. Scan-derived shielded activity entries carry no scan-time artifacts
The restore-scan activity deriver stamped reconstructed entries with
created_at_ms =the scan wall clock andblock_height =the discovering chunk's proof-anchor (scan-tip) height. Observed on the same wallet restored on two simulators: the identical entry showed "Aug 7 07:20 / block 411495" on one device and "Aug 8 11:13 / block 412108" on the other, and weeks-old transfers grouped under "Today".The real inclusion height is unknowable client-side (no per-note mined height in the note items or their fetch proof, nullifier items stored with empty values, anchors-by-height pruned to a recent retention window), so scan-derived entries now carry
block_height: Noneandcreated_at_ms: 0(documented unknown sentinel) — device-independent, so two restores produce byte-identical entries. The display sort gains a fourth band so unknown-age rows sink below dated history instead of reading as "newest". Live rows are untouched (genuine record time; the Pending→Confirmed sighting flip still backfills its near-tip observed-at height, with scan-derived rows exempted so the artifact can't sneak back).Exposing real per-note inclusion heights/times would need a node-side change (per-note heights in the shielded note items + fetch proof) — deliberately not attempted here.
Consumers
dashwallet-ios needs no changes for the asset-lock fix (store-backed rows win in its fallback; statusRaw 5 falls outside its pending/consumed windows). A small app-side change renders the unknown-date shielded entries honestly (separate dashwallet-ios PR).
Testing
swift-sdk/build_ios.sh --target ios --target simbuilds clean (xcframework + example app).🤖 Generated with Claude Code
Summary by CodeRabbit
3. Chain-order key for the unknown-age band (follow-up commit)
Scan-derived entries' real dates/heights are unknowable, but their exact chain order is: commitment-tree positions are append-only chain order.
ShieldedActivityEntrygainsmin_note_position(smallest received-note position,Nonefor live entries and outgoing-only clusters;serde(default)for previously-persisted entries), the display sort orders the unknown-age band by it descending, and it's plumbed through the persist/restore FFI structs andPersistentShieldedActivity.minNotePosition/hasMinNotePosition(defaults cover pre-existing rows) so hosts can order restored history in its true on-chain sequence.