feat(platform): shielded transaction history#3870
Conversation
The wallet persisted shielded materials (own notes, OVK-recovered sends) but no user-facing history. This derives a per-operation activity log — client-side only, by explicit privacy decision: no node/DAPI queries or note→transition indexes were added, so a rescanning wallet's traffic stays indistinguishable from a syncing one. Two derivation paths share one id (sha256 of the bundle's wallet-visible output cmxs), so a live entry and a later rescan of the same bundle dedupe to one row, and a richer entry upgrades a coarser one in place: - Live recorder: each operation path (shield, fund-from-asset-lock, transfer, unshield, withdraw, identity-create) records a Pending entry at build time — exact type, amount, fee, recipient, memo, identity id — and flips it Confirmed/Failed after broadcast. - Scan deriver (restore path): clusters persisted notes / outgoing notes by block height and classifies by where value landed. A change note's rho equals the nullifier of the action that consumed our note, so a rho∈own-nullifiers match is zero-false-positive proof of an own spend, recovering exact amounts (and exact fees for transfers). The link is probabilistic in recall (~50% per 2-action bundle — Orchard shuffles spends and outputs independently before pairing), so unlinked self-pay clusters surface as honest self-transfers rather than a spend masquerading as "Received"; true third-party receives (no OVK pairing) stay Received. Stack: ShieldedActivityEntry + deriver + recorder + store/changeset plumbing in rs-platform-wallet; persist/load callbacks in rs-platform-wallet-ffi; PersistentShieldedActivity SwiftData model + persister wiring in swift-sdk; Shielded Activity list + detail view in SwiftExampleApp. Verified on devnet paloma: backfill over a wallet with 45 notes classified a week of history with no false receives; a live transfer recorded Sent with the exact pinned fee (162,851,200 credits) and a round-tripped memo; the scan-derived entry for a pre-feature transfer recovered recipient, amount, and memo via OVK. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
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:
📝 WalkthroughWalkthroughAdds a derived shielded activity system: Rust domain types and derivation, live recorder integration into shielded operations, store persistence and startup restore, FFI marshalling for host persistence/load, SwiftData model and persistence handler, and Swift example UI to list and inspect activity. ChangesShielded Activity Persistence & Display System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
✅ Review complete (commit e3cddf8) |
check-storage-explorer requires every DashModelContainer model in the explorer's tile/count, list, and detail views — adds all three for the new activity model, following the shielded-outgoing-note pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two convergent, in-scope correctness bugs in the new shielded activity log: the live ShieldFromAssetLock recorder ignores its own documented IVK-match contract and silently selects the wrong account in multi-account wallets, and the scan deriver's dedupe set unconditionally suppresses confirmation of Pending live rows that ambiguous post-broadcast paths explicitly rely on scans to upgrade. Lower-severity nits around FFI tag forward-compat and empty-buffer pointer contracts were noted but are not blocking.
🔴 2 blocking | 💬 2 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 `packages/rs-platform-wallet/src/wallet/shielded/activity.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/activity.rs:398-402: Pending live entries are never confirmed by later scans
`derive_activity_into_changeset` (coordinator.rs:776) collects `existing_ids` via `store.get_activity_ids`, which returns ids for ALL persisted entries regardless of status. The scan deriver then `continue`s on any id in that set (activity.rs:399-401). The `ShieldedSpendUnconfirmed` and `ShieldedBroadcastUnconfirmed` paths (operations.rs:645-649 and 782-785) intentionally leave the activity row `Pending` with the documented expectation that 'a later scan that finds the spend will flip the row to Confirmed (its id matches).' That promise cannot be kept: the scan recomputes the same id, sees it in `existing_ids`, and discards the cluster before any update can be emitted. The unit test `live_entry_then_scan_of_same_cluster_yields_one_entry` codifies the current (broken) behavior — the scan must instead diff against existing entries' status/block_height and emit a status/height-only upsert when a Pending row's cluster reappears in scan data.
In `packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs:529-557: ShieldFromAssetLock activity recorder ignores its own IVK-match contract
The function's doc comment (lines 507-517) states it 'Finds the keyset whose IVK recognizes the funded note's recipient (the row then lands under that account), falling back to the lowest bound account — mirrors the `sender_ovk` selection above.' The actual code at line 535 just does `keys_map.iter().next()`, picking the lowest-indexed bound account unconditionally. The `sender_ovk` selection at lines 304-316 already shows the correct pattern (try `incoming_viewing_key.diversifier_index(recipient.inner())` first, then fall back). In a multi-account wallet where account 0 is bound but a different bound account actually received the funding, this falls through to `build_pending_entry` with the wrong IVK/OVK: `visible_output_cmxs` returns empty (the funded output decrypts under another account's keys), so `build_pending_entry` returns `None` and the rich ShieldFromAssetLock row is silently dropped. The shared-id dedupe contract this feature relies on is also violated: if a future code path did persist under the fallback account, the eventual scan-derived entry under the IVK-matched account would land at a different `(wallet_id, account_index, entry_id)` natural key, producing two unrelated rows for the same on-chain event.
…rder account match)
- The scan deriver now returns on-chain sightings of clusters whose id
already has a row (DerivedActivity::confirmations) instead of
discarding them; the coordinator upgrades still-Pending / height-less
stored rows to Confirmed at the observed height via with_status, so
the ambiguous post-broadcast paths' documented promise ("a later scan
flips the row") actually holds — and the live entry's richer fields
survive the upgrade untouched.
- The ShieldFromAssetLock recorder now selects the bound account whose
keyset actually recognizes a visible output in the landed bundle
(falling back to the lowest), matching its documented contract — a
multi-account wallet no longer silently drops the entry or lands it
under the wrong natural key.
- FFI: empty cmx/nullifier buffers marshal real nulls per the
documented "valid or null" pointer contract; unknown direction/status
bytes on load warn loudly, with unknown status folding to Pending
(re-confirmable) instead of the materially-distinct Failed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review at 1fc46d9. Latest delta is Swift-only (StorageExplorer browser + ShieldedActivityView for PersistentShieldedActivity), so all four prior findings were re-verified at the current HEAD and remain STILL VALID — two blocking Rust lifecycle bugs and two FFI hygiene nits, none touched by the delta. Codex surfaced one additional in-scope bug in the new Swift list: it partitions and badges by hasBlockHeight, but the Rust live recorder marks successful broadcasts Confirmed with block_height=None, so confirmed shielded operations render as Pending until a later scan backfills the height. Five validated in-scope findings total (4 carried + 1 new); blocking issues remain — request changes.
🔴 1 blocking
4 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 `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ShieldedActivityView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ShieldedActivityView.swift:127-204: Confirmed rows without block height are displayed as Pending
`pending` / `settled` partition by `!hasBlockHeight` / `hasBlockHeight` (lines 127-140) and the row badge at line 196 shows "Pending" whenever `!hasBlockHeight`, falling through to "Failed" only when `entry.status == 2`. But the Rust live recorders mark successful broadcasts `Confirmed` while passing `block_height: None` (see `operations.rs` lines 481-487, 601-607, 757-763, 906-908, 1240-1242), explicitly expecting the height to be filled by a later scan (which today is itself blocked by the dedupe issue above). Until that scan lands, every successful Shield / Unshield / Transfer / Withdrawal / IdentityCreate sits in the "Pending" section with a "Pending" badge despite already being Confirmed in the underlying data — the user-facing history reports the wrong state for the most common success path. The detail view at line 223-228 already uses `entry.status` correctly; the list/badge code should do the same. Partition by `entry.status == 0` (or by a `pending` computed property), and gate the badge on the same condition.
The live recorders mark a successful broadcast Confirmed with block_height=None (the scan backfills the height later), so a height-less row is the COMMON success shape. Partitioning/badging on hasBlockHeight rendered every fresh success as "Pending"; both now key on entry.status (matching the detail view), with just-confirmed height-less rows floating to the top of History. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift (1)
1885-1889: ⚡ Quick winDecode discriminants to human-readable labels for storage-explorer usability.
The Classification section displays
kindTag,direction, andstatusas raw integers (e.g.,0,1,2). Other detail views in this file decode enum discriminants into display names—PublicKeyStorageDetailView(lines 574–587) uses computed properties, andAssetLockStorageDetailView(lines 2113–2122) uses a local helper.Add three small helper functions to decode these discriminants so the storage explorer shows labels like
"Shield (0)","In (0)", and"Pending (0)"instead of bare integers.📖 Proposed helpers
Add these helpers to the
ShieldedActivityStorageDetailViewstruct (after thebodyproperty):+ private func kindDisplay(_ tag: Int) -> String { + let name: String + switch tag { + case 0: name = "Shield" + case 1: name = "ShieldFromAssetLock" + case 2: name = "Received" + case 3: name = "Sent" + case 4: name = "Unshield" + case 5: name = "Withdrawal" + case 6: name = "IdentityCreate" + case 7: name = "ShieldedSpend" + default: return "Unknown(\(tag))" + } + return "\(name) (\(tag))" + } + + private func directionDisplay(_ raw: Int) -> String { + let name: String + switch raw { + case 0: name = "In" + case 1: name = "Out" + case 2: name = "Self" + default: return "Unknown(\(raw))" + } + return "\(name) (\(raw))" + } + + private func statusDisplay(_ raw: Int) -> String { + let name: String + switch raw { + case 0: name = "Pending" + case 1: name = "Confirmed" + case 2: name = "Failed" + default: return "Unknown(\(raw))" + } + return "\(name) (\(raw))" + }Then update the Classification section:
Section("Classification") { - FieldRow(label: "Kind Tag", value: "\(record.kindTag)") - FieldRow(label: "Direction", value: "\(record.direction)") - FieldRow(label: "Status", value: "\(record.status)") + FieldRow(label: "Kind Tag", value: kindDisplay(record.kindTag)) + FieldRow(label: "Direction", value: directionDisplay(record.direction)) + FieldRow(label: "Status", value: statusDisplay(record.status)) }🤖 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift` around lines 1885 - 1889, Add three helper methods inside the ShieldedActivityStorageDetailView struct (after the body property) that map the numeric discriminants for kindTag, direction, and status into human-readable labels (e.g., "Shield (0)", "In (0)", "Pending (0)"); then update the Classification Section to call these helpers instead of interpolating raw integers for record.kindTag, record.direction, and record.status (replace the FieldRow value arguments for "Kind Tag", "Direction", and "Status" to use the new helper methods).
🤖 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/persistence.rs`:
- Around line 2148-2157: The function decode_cmx_array trusts the host-supplied
count and multiplies it by 32 before calling slice::from_raw_parts, which can
overflow; guard the multiplication by using checked_mul (or an equivalent check)
on count and return early (e.g., Vec::new()) if it would overflow or if byte_len
is unreasonably large, and only then call slice::from_raw_parts with the safe
byte_len; keep the existing null-pointer and zero-count checks and locate this
change in decode_cmx_array to replace the direct count * 32 expression with a
checked multiplication and early-fail path.
In `@packages/rs-platform-wallet/src/wallet/shielded/activity_recorder.rs`:
- Around line 106-117: build_pending_entry currently treats note_cmxs.is_empty()
as a degenerate case and returns None, which drops exact-input exit bundles that
legitimately have zero visible Orchard outputs; instead, when
visible_output_cmxs(params.actions, keys) is empty, still construct and return a
Pending ShieldedActivityEntry that contains no visible outputs but includes a
deterministic fallback identifier/linkage so the row can be reconstructed later.
Implement this by deriving a stable unique id from bundle-level data available
in LiveEntryParams (e.g., bundle commitment/txid/anchor/actions hash) and
populate the ShieldedActivityEntry with that fallback id and an explicit
flag/empty outputs list; update build_pending_entry (and ensure the scan path
uses the same derivation) so exact-input unshield/withdraw/identity-create flows
handled by reserve_unspent_notes / reserve_unspent_notes_for_denomination are
recorded rather than skipped.
In `@packages/rs-platform-wallet/src/wallet/shielded/activity.rs`:
- Around line 505-545: The classifier incorrectly marks backfilled shield
clusters as SelfTransfer because it only checks cluster.outgoing.is_empty();
change the branch to also consult the subwallet-level "has spent shielded"
signal (e.g., a boolean like wallet_has_spent_shielded or
subwallet.has_any_shielded_spend available in context) before choosing
ShieldedActivityKind::ShieldedSpend / ShieldedDirection::SelfTransfer; if the
wallet_has_spent_shielded flag is false, treat the cluster as Received (keep
ShieldedActivityKind::Received / ShieldedDirection::In and existing fields),
otherwise keep the existing SelfTransfer behavior; update the comment to reflect
the new guard and ensure the flag is passed into the function that constructs
these ShieldedActivityEntry values.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 2856-2858: The activity marshal/restore path uses cmxLen and nfLen
to set note_cmxs_count and spent_nullifiers_count (via cmxLen / 32 and nfLen /
32) and later multiplies those counts by 32 without bounds checks, which can
truncate trailing bytes or overflow; update the restore and marshal code in
PlatformWalletPersistenceHandler (the functions handling note_cmxs_count,
spent_nullifiers_count, cmxLen, nfLen, and their pointer conversions) to first
validate that cmxLen % 32 == 0 and nfLen % 32 == 0 and reject/return an error
for malformed lengths, and replace plain arithmetic with checked arithmetic
(e.g., use safe multiply operations or bounds-checked UInt conversions) before
allocating buffers or indexing pointers to prevent overflow and silent
truncation.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift`:
- Around line 1885-1889: Add three helper methods inside the
ShieldedActivityStorageDetailView struct (after the body property) that map the
numeric discriminants for kindTag, direction, and status into human-readable
labels (e.g., "Shield (0)", "In (0)", "Pending (0)"); then update the
Classification Section to call these helpers instead of interpolating raw
integers for record.kindTag, record.direction, and record.status (replace the
FieldRow value arguments for "Kind Tag", "Direction", and "Status" to use the
new helper methods).
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1db88d0-00d0-4269-af27-a2bec50c592a
📒 Files selected for processing (23)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/shielded_persistence.rspackages/rs-platform-wallet/src/changeset/shielded_changeset.rspackages/rs-platform-wallet/src/changeset/shielded_sync_start_state.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/activity.rspackages/rs-platform-wallet/src/wallet/shielded/activity_recorder.rspackages/rs-platform-wallet/src/wallet/shielded/coordinator.rspackages/rs-platform-wallet/src/wallet/shielded/file_store.rspackages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/shielded/mod.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/store.rspackages/rs-platform-wallet/src/wallet/shielded/sync/ovk_builder_roundtrip_tests.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentShieldedActivity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ShieldedActivityView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletsContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageExplorerView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageModelListViews.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
- decode_cmx_array guards the host-supplied count with checked_mul before from_raw_parts (corrupt rows drop the linkage instead of UB). - Scan classifier: a self-pay cluster in a subwallet with ZERO spent notes is necessarily a shield-to-self (change-from-spend is impossible), so it now reads as Received/In; the SelfTransfer residual is reserved for wallets that have spends. Tests pin both. - Swift trampoline uses checked multiplication for the Rust-supplied linkage counts; the restore marshal rejects blobs that aren't whole 32-byte multiples (null/0) instead of silently truncating. - Storage-explorer activity detail decodes kind/direction/status discriminants to labeled values. Not changed: build_pending_entry's None branch (review suggested a fallback id for zero-visible-output bundles) — build_spend_bundle_with adds the change/recipient output unconditionally, OVK-keyed to the wallet's own external OVK, so even an exact-input spend carries a wallet-visible output; the branch guards a shape our builders cannot produce and warns loudly if it ever fires. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta cleanly resolves all five prior review findings: the post-broadcast Pending→Confirmed promise is now backed by DerivedActivity::confirmations plus the coordinator upgrade loop; ShieldFromAssetLock now picks the account whose IVK actually recognizes a visible output; empty cmx/nullifier buffers cross the FFI boundary as real null pointers; and unknown direction/status discriminants now log a tracing::warn! with status folding to Pending. One genuinely PR-introduced correctness blocker remains: the live recorder writes activity entries only to the host persister and never to the in-memory ShieldedStore, so intra-session scans miss them via existing_ids and silently upsert coarser scan-derived rows over the rich live entries. A couple of smaller items (Failed-no-height upgrade gate, two whitespace-mangled log strings, and an unchecked count*32 multiplication at the restore FFI boundary) are worth fixing alongside.
🔴 1 blocking | 🟡 2 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 `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:219-245: Live activity entries are never inserted into the in-memory ShieldedStore — intra-session scans clobber rich live rows
`record_pending_activity` (and the sibling `queue_shielded_activity` used by `record_shield_from_asset_lock_activity`) only push the entry through `queue_shielded_changeset` → `persister.store(...)`. They never call `store.save_activity(id, entry)` on the in-memory `ShieldedStore`. The scan deriver, however, gates dedupe on `store.get_activity_ids(*id)` at coordinator.rs:776, which reads exclusively from the in-memory store. The in-memory store is only populated with activity at bind/restore time (coordinator.rs:421–425) and at scan time (lines 787, 818).
Concrete failure path (single-op block, the common case): the user does an Unshield → live recorder writes a `Pending`/`Confirmed` Unshield with cmxs = [change_cmx] only to the persister. The next sync pass finds the change note at height H, builds a cluster with cmxs = [change_cmx], computes `compute_activity_id(cmxs)` — the same id as the live entry. `existing_ids` does NOT contain it (the store hasn't seen it), so the scan classifies it as a `Received` (or rho-linked `ShieldedSpend`) and falls into `new_entries`. `save_activity` upserts by id, so both the in-memory store AND the persisted row end up with the coarser scan-derived row — the Unshield/Withdrawal/IdentityCreate kind, fee, counterparty, and memo are silently destroyed. This directly violates the doc-comment invariant at activity.rs:370–378 ("a live `IdentityCreate` / `Unshield` / `Withdrawal` is never clobbered by a coarser scan-derived `Sent` / `ShieldedSpend`") and the same-line existing_ids contract.
Mirror flow exists for multi-op same-block: two live entries with disjoint cmxs A and B end up with cluster cmxs = A∪B and id = compute_activity_id(A∪B), which does not match either live id, so the scan emits a third spurious aggregate row even when the live entries are eventually restored. Both flows have the same root cause — the live recorder must also write to the in-memory store so the dedupe set stays consistent across the session.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:807-812: Confirmation upgrade gate silently flips a Failed-no-height row to Confirmed
The gate is `stored.status == Pending || stored.block_height.is_none()`. The `Failed` arms at operations.rs:651–660 call `record_activity_status(..., Failed, None)`, which produces exactly a `Failed` row with `block_height = None`. If a scan ever sees the cluster on-chain (e.g. a broadcast misclassified as definitively failed that actually landed, or a future caller marks `Failed` while still not knowing the height), this gate will silently rewrite that user-visible `Failed` to `Confirmed` at the observed height.
The inline comment frames the scope as "the ambiguous post-broadcast paths leave their live row Pending" — Failed is not in that scope. Either tighten the gate to `status == Pending` (or `status == Pending && block_height.is_none()`) so an explicitly-Failed row stays Failed, or update the doc-comment to make the "chain truth always wins, including for Failed" policy explicit. As written, the encoded rule is broader than the rationale.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:2148-2158: decode_cmx_array multiplies host-supplied count by 32 without overflow check
`count * 32` runs inside the unsafe `slice::from_raw_parts` call. `from_raw_parts` requires the byte length to not exceed `isize::MAX`, and in release builds an unchecked multiplication wraps silently. A corrupted or malicious host row with a large `count` would either truncate the slice or produce an invalid view of host memory at the activity restore boundary — neither failure mode is caught by the subsequent `chunks_exact(32)` filter (it operates on whatever the slice ends up being). Since this is the only place host-provided counts feed the persisted activity linkage on restore, the multiplication should be a checked op with a bounds clamp.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental verification against head 97e190c. The PR's stated goal — shielded transaction history with live-recorded entries reconciled against scan-derived rows — has one validated blocking issue carried over from the prior review: live activity entries are persisted to the host but never inserted into the in-memory ShieldedStore, so the dedupe set (existing_ids) the scan deriver reads is incomplete within a session and the scan can silently overwrite a rich live row with a coarser derived row. Two carry-forward smaller items also remain (Failed-no-height upgrade gate; whitespace-mangled warn strings). The earlier decode_cmx_array count*32 overflow was partially addressed (checked_mul added), but the isize::MAX precondition for slice::from_raw_parts is still unenforced.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
3 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 `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:2148-2168: decode_cmx_array still misses the isize::MAX precondition for slice::from_raw_parts
The patch turned `count * 32` into `checked_mul(32)`, which closes the wraparound. But `slice::from_raw_parts` additionally requires the total byte length to be no greater than `isize::MAX` — any larger length is UB even though the multiplication didn't wrap. A corrupted host-persisted `count` can still produce a `byte_len` between `isize::MAX + 1` and `usize::MAX`. Reject those before constructing the slice; this is the same FFI-boundary distrust rationale the new overflow guard already adopts.
…d scan dedupe Addresses the remaining review findings on the shielded activity log: - Live recorders (record_pending_activity / record_activity_status / queue_shielded_activity) now save_activity into the in-memory ShieldedStore before queueing to the host persister, so the scan deriver's dedupe sees intra-session live entries and can no longer clobber a rich live row with a coarser scan-derived one. The Shield (T15) and ShieldFromAssetLock/seed paths gained the store/coordinator plumbing this requires (FFI extern "C" signatures unchanged). - The deriver dedupes by cmx OVERLAP against stored entries (cmx → entry-id map) instead of exact id equality: a same-block cluster merging two live ops now confirms BOTH entries at the observed height instead of synthesizing a spurious aggregate row. New test pins it. - Confirmation-upgrade gate keeps upgrading Failed-no-height rows, now with the chain-truth-wins policy stated explicitly (observed on devnet: a client-side result-proof fetch error marked an actually-landed identity-create as Failed). - decode_cmx_array also enforces slice::from_raw_parts' isize::MAX bound, and its stale "multiple of 32" doc clause now describes the real guards; the unknown-tag warn messages are single-line literals. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta (ae31193) resolves the prior blocking issue: queue_shielded_activity now upserts into the in-memory ShieldedStore before the persister queue, closing the same-session scan-clobbers-live-row hazard. The prior decode_cmx_array isize::MAX, warn! whitespace, and doc-claim nits are also fixed. The Failed-no-height upgrade gate is unchanged but now carries an explicit 'Chain truth wins' justification tied to an observed devnet incident — accepting as intentionally deferred. One small carry-forward remains: a stale doc fragment sits above decode_cmx_array.
💬 1 nitpick(s)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
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/wallet/shielded/operations.rs (1)
466-518:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftShield still treats ambiguous wait failures as definitive failure.
This path now persists a live activity row, but any
broadcast_and_wait(...)error immediately flips it toFailed. A timeout or result-proof fetch failure after relay admission is still ambiguous: the shield can land later, and a host retry can top up the pool a second time with fresh address nonces/balances. Please stage shield like the other spend flows: separatebroadcast()fromwait_for_response(), keep the rowPendingon no-verdict wait failures, and avoid surfacing a definitive failure in that case.🤖 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/shielded/operations.rs` around lines 466 - 518, Split the combined call to state_transition.broadcast_and_wait::<StateTransitionProofResult> into two steps: call a non-waiting broadcast (e.g., broadcast()) first and only call wait_for_response()/the equivalent waiting/fetching logic afterwards; update the error handling so that ambiguous/timeout/fetch errors from the wait step do NOT call record_activity_status(..., ShieldedActivityStatus::Failed, ...), leaving the persisted activity row as Pending, while only definitive failures (e.g., explicit insufficient-funds/rich error detected from the broadcast response) should map to PlatformWalletError::ShieldedBroadcastFailed and trigger record_activity_status(..., ShieldedActivityStatus::Failed, ...); keep the final record_activity_status(..., ShieldedActivityStatus::Confirmed, ...) on successful confirmation. Ensure you reference state_transition.broadcast_and_wait::<StateTransitionProofResult>, any new state_transition.broadcast()/wait_for_response() helpers, record_activity_status, and ShieldedActivityStatus variants when making the change.packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs (1)
345-413:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftType 18 never emits Pending or Failed activity rows.
build_and_broadcast_shielded(...)only gives this method control back afterbroadcast_and_waitsucceeds, andrecord_shield_from_asset_lock_activity(...)is only called on that success path. That meansshielded_fund_from_asset_lock, its resume path, and everyshielded_seed_pool_notesbatch stay invisible while in flight and disappear entirely on definitive failure. Please split this like the other live-recorded flows: build the bundle, upsert a Pending entry immediately, then classify the broadcast result into Confirmed/Failed (or keep Pending on ambiguous outcomes).🤖 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/shielded/fund_from_asset_lock.rs` around lines 345 - 413, The current flow in shielded_fund_from_asset_lock only records activity after build_and_broadcast_shielded returns success, so in-flight and failed operations never get Pending/Failed rows; change the sequence to first build the bundle (call the builder logic that currently lives inside the submit closure) and immediately upsert a Pending activity via record_shield_from_asset_lock_activity for the to-be-broadcast landed_actions placeholder (or metadata), then perform submit_with_cl_height_retry + build_and_broadcast_shielded as before (including the is_instant_lock_proof_invalid retry that calls asset_locks.upgrade_to_chain_lock_proof / advance_asset_lock_status / queue_asset_lock_changeset), and finally reconcile the Pending row: mark Confirmed on success (update the same activity entry) or mark Failed (or leave Pending for ambiguous outcomes) on error; ensure both the instant-lock and chain-lock paths use the same pre-broadcast Pending upsert and post-broadcast classification.
🤖 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.
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs`:
- Around line 345-413: The current flow in shielded_fund_from_asset_lock only
records activity after build_and_broadcast_shielded returns success, so
in-flight and failed operations never get Pending/Failed rows; change the
sequence to first build the bundle (call the builder logic that currently lives
inside the submit closure) and immediately upsert a Pending activity via
record_shield_from_asset_lock_activity for the to-be-broadcast landed_actions
placeholder (or metadata), then perform submit_with_cl_height_retry +
build_and_broadcast_shielded as before (including the
is_instant_lock_proof_invalid retry that calls
asset_locks.upgrade_to_chain_lock_proof / advance_asset_lock_status /
queue_asset_lock_changeset), and finally reconcile the Pending row: mark
Confirmed on success (update the same activity entry) or mark Failed (or leave
Pending for ambiguous outcomes) on error; ensure both the instant-lock and
chain-lock paths use the same pre-broadcast Pending upsert and post-broadcast
classification.
In `@packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- Around line 466-518: Split the combined call to
state_transition.broadcast_and_wait::<StateTransitionProofResult> into two
steps: call a non-waiting broadcast (e.g., broadcast()) first and only call
wait_for_response()/the equivalent waiting/fetching logic afterwards; update the
error handling so that ambiguous/timeout/fetch errors from the wait step do NOT
call record_activity_status(..., ShieldedActivityStatus::Failed, ...), leaving
the persisted activity row as Pending, while only definitive failures (e.g.,
explicit insufficient-funds/rich error detected from the broadcast response)
should map to PlatformWalletError::ShieldedBroadcastFailed and trigger
record_activity_status(..., ShieldedActivityStatus::Failed, ...); keep the final
record_activity_status(..., ShieldedActivityStatus::Confirmed, ...) on
successful confirmation. Ensure you reference
state_transition.broadcast_and_wait::<StateTransitionProofResult>, any new
state_transition.broadcast()/wait_for_response() helpers,
record_activity_status, and ShieldedActivityStatus variants when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 073912ab-1893-41bb-9705-d750d6bb1f69
📒 Files selected for processing (8)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/activity.rspackages/rs-platform-wallet/src/wallet/shielded/coordinator.rspackages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/seed_pool.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-platform-wallet-ffi/src/persistence.rs
Shield (T15) now mirrors broadcast_shielded_spend's staging: broadcast first (a definitive CheckTx verdict marks the activity row Failed with the enriched address-funds diagnostics), then wait_for_response — a consensus rejection in the result is Failed, while an ambiguous wait failure (timeout / result-proof fetch error) leaves the row Pending and surfaces ShieldedSpendUnconfirmed, since the shield may still land and the scan's cmx-overlap confirmation flips the row when its note appears on-chain. Type 18 deliberately keeps recording only the landed bundle: the CL-height retry loop re-randomizes the bundle (different cmxs => a different activity id) per attempt, so a pre-broadcast Pending row would orphan whenever a retry lands; in-flight/failed T18s are surfaced via the tracked asset-lock lifecycle, which owns the recoverable L1 value. Rationale documented on the recorder. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Re the two outside-diff CodeRabbit findings (run 073912ab): Shield ambiguous wait failures — fixed in 60b7259. Shield (T15) now mirrors Type 18 Pending/Failed staging — deliberately not implemented, rationale now documented on the recorder (60b7259): |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta is a one-line doc cleanup that resolved the prior stale fragment above decode_cmx_array. Cumulative review surfaces one PR-introduced ordering bug (live Confirmed entries without height collide with the pending sort band) and three architectural/maintenance suggestions in the new code (FFI parallel-iteration coupling flagged by three reviewers, duplicated non_zero_memo helper, and write-lock held across the per-subwallet derivation loop). No blockers.
🟡 3 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 `packages/rs-platform-wallet/src/wallet/shielded/activity.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/activity.rs:616-631: sort_activity_for_display ignores status, mixing live Confirmed rows into the pending band
This PR introduces a live recorder that flips successful ops to ShieldedActivityStatus::Confirmed before any block_height is known — see record_activity_status calls such as operations.rs:1287-1295 which pass status=Confirmed with block_height=None, expecting a later scan to backfill the height. The sorter here treats every block_height==None as 'pending' and floats it above all heighted rows, so a live-confirmed (or failed-without-height) row will appear in the pending band until the scan deriver lands. The trait contract at store.rs:207 (and the Swift UI's recent partition-by-status workaround in a35bc9ad) both already model pending as a status, not as 'no height'. Switch the primary sort key to status==Pending so the canonical Rust sorter matches the documented contract and the Swift partition; height becomes the secondary key as before.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:1336-1430: Persist-side FFI batch builds backing buffers and entry pointers in two separate passes
The activity persist path iterates shielded_cs.activity_entries.values() to build a `backing: Vec<(Vec<u8>, Vec<u8>)>` of owned cmx/nullifier bytes, then iterates shielded_cs.activity_entries.iter() a second time and pairs each (id, e) with `backing_iter.next().expect("backing has one entry per activity entry")` to populate the FFI struct's note_cmxs_ptr / spent_nullifiers_ptr. This works today because BTreeMap iteration order is deterministic and the two passes walk the same map, but the FFI pointer-validity invariant (each `*_ptr` must point into the matching backing slot) is load-bearing on that implicit ordering. A future change that filters one pass, switches the container type, or sorts only one side would silently mis-pair pointers with the wrong entry — lengths would still line up so the expect wouldn't catch it, and the host would receive cmx/nullifier bytes from a different row. Build backing and the FFI entries in a single pass (or zip after constructing both vecs) so the parallel-array invariant is a compile-time property instead of a runtime assertion in an FFI hot path.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:749-842: derive_activity_into_changeset holds the store write lock across every subwallet's CPU-bound classification
The function grabs self.store.write().await once at the top and holds it across the entire per-subwallet loop: get_all_notes, get_outgoing_notes, get_activity, derive_activity_from_scan_data, save_activity, and per-confirmation get_activity_by_entry_id. With N bound subwallets and 1k+ notes each, the deriver serializes every other consumer of the store for the full classification window even though only save_activity strictly needs the write guard. The deriver is already idempotent (the overlap dedupe + upsert-by-id contract tolerates re-runs), so a read pass to gather inputs followed by a focused write pass for the upserts would cut contention without changing correctness.
- sort_activity_for_display keys the pending band on STATUS (matching the Swift partition and the store contract): a live-Confirmed row whose height the scan hasn't backfilled sorts at the top of the settled band instead of masquerading as pending. Test extended. - The activity persist FFI builds each entry and its owned cmx / nullifier buffers in one structurally-paired pass, so the pointer-into-backing invariant can't silently mis-pair if either side is ever filtered or reordered. - derive_activity_into_changeset snapshots inputs under a read lock, classifies lock-free, and takes the write lock only for the upserts — the derivation is idempotent, so anything landing between passes reconciles on the next one. - non_zero_memo exists once (the deriver's slice-keyed helper); the recorder's fixed-array version is a thin adapter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest push cleanly resolves all four prior d64de16 findings. New activity-derivation logic introduces one blocking correctness issue: incoming notes are stored at the pass-wide max height while outgoing notes keep per-batch heights, so cluster_events cannot correctly cluster their joint events in multi-batch syncs. Two suggestion-level concurrency races and a minor error-message wording issue are also flagged.
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(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 `packages/rs-platform-wallet/src/wallet/shielded/activity.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/activity.rs:321-336: cluster_events clusters on heights that incoming notes do not preserve
`cluster_events` keys on `ShieldedNote::block_height` (incoming) and `ShieldedOutgoingNote::block_height` (outgoing). However, the sync path stores incoming notes with the pass-wide `max_block_height` (sync.rs:399 + 609 — `max_block_height = max_block_height.max(batch.block_height)` accumulated across all batches, then stamped on every decoded note in every batch), while outgoing notes preserve the per-batch height (sync.rs:635 uses `r.block_height`). In any multi-batch sync — including initial sync, backfills, or a resume — an OVK-recovered outgoing note from an earlier batch will key on its real height while its matching incoming change note from the same bundle gets pulled up to the run-wide max. The deriver then splits one bundle across two clusters: the cluster containing only the outgoing note will classify as `Sent` with no change, and the later max-height cluster will mix the bundle's change with unrelated incoming notes from other batches, producing wrong amounts/fees/IDs and breaking the rho-link/OVK shape the classifier relies on. Either store incoming notes with their per-batch `batch.block_height` (matching what outgoing notes already do) or pass per-note heights through to the deriver so the cluster key is the true block of each event.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:815-822: Scan-derived entries are saved against a stale snapshot
The lock split snapshots `existing_cmxs` under a read lock, releases it for classification, then under a fresh write lock unconditionally saves every `derived.new_entries`. A live recorder writing a richer Pending/Confirmed row for the same cmx set between the two phases will not appear in the snapshot, so the classifier still emits a `new_entries` row for that cluster. Depending on whether the scan-derived id collides with the live id, the write either overwrites the richer live row with the coarser scan-derived one (losing fee/kind/memo/counterparty/created identity id) or leaves a duplicate row in the store; the next pass's `existing_cmxs` will then dedupe future passes but cannot recover the lost fields. The in-code comment claims idempotence is sufficient, but neither failure mode is reconciled by a subsequent pass since scan-only data cannot reconstruct live-recorder fields. Re-check current activity cmx overlap (or entry id) under the write lock and treat overlaps as confirmations/no-ops instead of unconditional `save_activity` calls.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:274-288: record_activity_status flips status on a stale captured pending entry
`record_activity_status` always calls `with_status(entry, ...)` on the `pending_entry` captured before broadcast. If a concurrent sync pass observes the operation's outputs and has already upgraded the stored row to `Confirmed` with a real `block_height` between broadcast and the wait_for result, the post-wait `Confirmed`-without-height write will overwrite the row in the store and clobber the height the scan just learned. The scan-backfill contract (height comes from chain) is then broken for any operation whose result-wait races a scan that already saw the outputs. Read the current stored entry by id inside `record_activity_status` and apply the status change to that row (preserving any block_height the scan has already populated) before queueing the upsert.
…rding - Stamp incoming notes with their own chunk's proven height (matching OVK-recovered outgoing notes) instead of the pass-wide max, so a bundle's change and send agree on the cluster key in multi-batch syncs - Re-check cmx overlap under the write lock before inserting scan-derived entries; overlaps with rows a live recorder landed mid-derivation degrade to confirmation sightings instead of clobbering/duplicating - record_activity_status now flips the CURRENT stored row (read under the save's write lock), never the stale pre-broadcast capture; a row the scan already confirmed with a height is left untouched Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/shielded/sync.rs (1)
587-613:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHistorical notes never pick up the new per-batch height.
Line 589 still skips every note below the existing watermark, so rows written under the old pass-wide height scheme never get rewritten with
d.block_heightfrom Line 613.derive_activity_into_changesetclusters on the persisted note height, so previously synced history can stay split or misclassified until the user clears and resyncs. Please add a migration/update path for stored notes whose height differs instead of gating all pre-watermark rows.🤖 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/shielded/sync.rs` around lines 587 - 613, The loop currently skips any discovered note with d.position < sub_watermark so existing stored notes never get their block_height updated; change the logic in the loop that uses watermarks.get(id).copied().unwrap_or(0) and the discovered iteration to still load the existing stored note for positions below the watermark and compare its stored block_height with d.block_height, and if they differ call the store update path (e.g., via your store module functions that insert/update ShieldedNote) to rewrite the record with the new ShieldedNote { note_data, position: d.position, cmx: d.cmx, nullifier: ..., block_height: d.block_height } so historical rows are migrated in-place rather than being skipped; keep the original insertion behavior for notes that do not exist.
🤖 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.
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- Around line 587-613: The loop currently skips any discovered note with
d.position < sub_watermark so existing stored notes never get their block_height
updated; change the logic in the loop that uses
watermarks.get(id).copied().unwrap_or(0) and the discovered iteration to still
load the existing stored note for positions below the watermark and compare its
stored block_height with d.block_height, and if they differ call the store
update path (e.g., via your store module functions that insert/update
ShieldedNote) to rewrite the record with the new ShieldedNote { note_data,
position: d.position, cmx: d.cmx, nullifier: ..., block_height: d.block_height }
so historical rows are migrated in-place rather than being skipped; keep the
original insertion behavior for notes that do not exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df3fe1d3-315d-42bb-80ca-441900306876
📒 Files selected for processing (7)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/wallet/shielded/activity.rspackages/rs-platform-wallet/src/wallet/shielded/activity_recorder.rspackages/rs-platform-wallet/src/wallet/shielded/coordinator.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/store.rspackages/rs-platform-wallet/src/wallet/shielded/sync.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-platform-wallet/src/wallet/shielded/store.rs
- packages/rs-platform-wallet/src/wallet/shielded/activity_recorder.rs
- packages/rs-platform-wallet-ffi/src/persistence.rs
- packages/rs-platform-wallet/src/wallet/shielded/activity.rs
- packages/rs-platform-wallet/src/wallet/shielded/operations.rs
|
Re CodeRabbit's outside-diff finding (historical notes never pick up the new per-batch height): declining the migration path. This code is pre-release — the only stores written under the old pass-wide-height scheme are internal devnet test devices, which recover via the standard clear-and-resync path. An in-place compare-and-rewrite would also be mostly inert: normal syncs fetch from the aligned network-min watermark, so pre-watermark positions are never re-discovered and the migration would only fire opportunistically on lagging-subwallet rewinds. Not worth the complexity for data no production user holds. |
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "ef8e93511ed2232772e2cbc77d4d72c3cc3a8bbfd51734b80a008a10ccdaf8dc"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Three prior findings (#1 per-batch note heights, #2 cmx overlap re-check, #3 record_activity_status race) are resolved at 50b98ae. Prior finding #4 (ShieldedSpendUnconfirmed text) still applies. The cumulative PR introduces three new in-scope blockers: the shield FFI flattens the new ShieldedSpendUnconfirmed variant into a generic error code (losing the do-not-retry signal hosts rely on), and the Swift Clear Shielded State and wallet-deletion paths both miss the new PersistentShieldedActivity table, so stale activity rows can rehydrate into Rust via the load callback after a user wipes shielded state or deletes/reimports a wallet.
🔴 3 blocking | 🟡 2 suggestion(s)
4 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-ffi/src/shielded_send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_send.rs:719-725: Shield FFI flattens ShieldedSpendUnconfirmed to generic ErrorWalletOperation
`shielded_shield_from_account` now returns `PlatformWalletError::ShieldedSpendUnconfirmed { operation: "shield", ... }` for the ambiguous post-broadcast path (operations.rs:574). But the shield FFI wrapper at shielded_send.rs:719 maps every `Err(e)` straight to `ErrorWalletOperation` instead of going through `map_spend_result` like transfer/unshield/withdraw do. As a result Swift cannot distinguish `ErrorShieldedSpendUnconfirmed` (do NOT retry — broadcast may already have executed) from any other local failure, and `ShieldedBroadcastFailed` (definitive failure, safe to retry) is also collapsed into the generic code. The whole point of the new typed variant is the retry-safety split; this wrapper silently throws it away. Route the shield result through the same helper so the three retry-relevant codes (`ErrorShieldedSpendUnconfirmed`, `ErrorShieldedBroadcastFailed`, `ErrorWalletOperation`) are preserved.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:619-628: Clear Shielded State leaves PersistentShieldedActivity rows behind
The cumulative PR adds a `PersistentShieldedActivity` SwiftData table that is loaded through `on_load_shielded_activity_fn` on wallet startup and copied into the Rust-side `ShieldedSyncStartState`. `ShieldedService.clearLocalState` deletes `PersistentShieldedNote`, `PersistentShieldedOutgoingNote`, and `PersistentShieldedSyncState` but does not delete `PersistentShieldedActivity`. After a user uses the global Clear button — whose stated semantics are 'blow away shielded persistence' — old activity rows survive and rehydrate into Rust on the next bind, where they will continue to dedupe/suppress scan-derived rows (via the cmx-overlap path) and surface deleted history in the UI. Delete the activity table here too.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3178-3203: Wallet deletion does not purge PersistentShieldedActivity rows
The wallet-deletion path explicitly enumerates the per-wallet shielded tables that need a hand-rolled cascade because they are keyed by raw `walletId` with no relationship to `PersistentWallet` — note, outgoing-note, and sync-state. The new `PersistentShieldedActivity` table is keyed the same way but is not deleted here, so deleting a wallet (or deleting then reimporting one with the same id) leaves activity rows behind. On the next load those rows flow through `loadShieldedActivityCallback` into `ShieldedSyncStartState` and resurface as ghost history. The block comment already documents the pattern; extend it to the activity table.
In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:216-224: ShieldedSpendUnconfirmed text says 'spent notes' but is also raised for shield
Carried forward from the 41964622 review. operations.rs:574-577 returns `ShieldedSpendUnconfirmed { operation: "shield", ... }` for the ambiguous shield path, but the `#[error(...)]` template still reads "the spend may already be executed on chain — do not re-submit (the next sync reconciles the spent notes)". For an `operation == "shield"` value this renders as 'Shielded shield broadcast … the spend may already be executed … reconciles the spent notes', which is misleading: shielding from platform addresses does not spend shielded notes, so there is nothing for the scan to reconcile on the spent-note side, and callers (CLI + Swift surface, which prints the Display) get the wrong recovery guidance. Either soften the wording to be neutral about what kind of on-chain effect is pending, or split a shield-specific variant. Since the variant is otherwise the right shape, softening the template is the smaller change.
In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:602-616: Per-batch note height is the response metadata height, not per-note mined height
`ShieldedNote.block_height` is now stamped from `d.block_height`, which is `batch.block_height` — and `batch.block_height` is the response metadata height (`fetch_chunk.rs:44-49` returns `metadata.height`, the chain tip the proof was anchored against), not the height each individual note was mined at. A single chunk covers up to `mmr_chunk_size` (≈2048) commitment-tree positions that can span many historical blocks of activity, so on a cold restore every note surfaced in one fetch batch — incoming receipts and OVK-recovered sends across unrelated bundles — receives the same anchor height, and `cluster_events` (activity.rs:324) then merges those unrelated wallet operations into a single synthetic activity row with combined amount/kind/memo. The per-batch change correctly fixes the bundle-split issue (a single bundle's incoming change and OVK send now agree on the key), but the documented goal of clustering 'notes landing in the *same block*' (activity.rs:260) is not what the data actually carries. The deeper fix needs a per-note mined height from the SDK; until then please at least update the cluster_events comments to call out that 'same height' means 'same fetch batch' and document the cold-restore aggregation as a v1 limitation.
…ws on wipe - Route the shield FFI result through map_spend_result so hosts keep the ErrorShieldedSpendUnconfirmed (do-not-retry) / ErrorShieldedBroadcastFailed (safe-to-retry) split instead of a generic ErrorWalletOperation - Delete PersistentShieldedActivity in Clear Shielded State and in the wallet-deletion cascade so wiped history can't rehydrate into Rust via the load callback as ghost entries - Soften ShieldedSpendUnconfirmed wording to cover the shield path (no spent notes to reconcile) and document that cluster heights are fetch proof anchor heights (same fetch batch), incl. cold-restore aggregation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review of head f1ce3f7 against prior baseline 50b98ae. All five prior findings are resolved: shield FFI now routes through map_spend_result, both wipe paths purge PersistentShieldedActivity, the ShieldedSpendUnconfirmed message reads operation-neutral, and the per-batch height limitation is now explicitly documented as a v1 restore restriction. No new in-scope blocking issues were validated in the f1ce3f7 delta; the codex-ffi-engineer 'changeset_end swallows save failure' finding is a pre-existing persistence-handler behavior on a path unchanged by this PR and out of scope. One small in-scope nit confirmed: a stale inline comment in map_spend_result still describes spend-only semantics after shield was rewired through it.
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
…ment Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review of ac2f2b4 against prior baseline f1ce3f7. The prior nitpick about the stale spend-only comment in map_spend_result is FIXED. Cumulative review surfaces two new findings, both about the broadened ShieldedSpendUnconfirmed/activity model: a wallet-level activity list keys ForEach rows by entryId only despite the persistence model deliberately scoping uniqueness by (walletId, accountIndex, entryId), and the Swift mirror of result code 18 still documents spend-only semantics even though Rust now routes shield through the same helper.
🔴 1 blocking | 💬 1 nitpick(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 `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ShieldedActivityView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ShieldedActivityView.swift:162-171: Wallet-level activity ForEach collides rows from different shielded accounts
`ShieldedActivityListView` queries `PersistentShieldedActivity` for one `walletId` (no `accountIndex` predicate) but both `ForEach` sections identify rows only by `\.entryId`. The persistence model in `PersistentShieldedActivity.swift` is uniqued on `(walletId, accountIndex, entryId)` precisely because the same visible-output `cmx` legitimately appears under two accounts of one wallet — e.g. an intra-wallet shielded transfer from account 0 to account 1 writes a Sent row on acct 0 and a Received row on acct 1, both with `entryId = sha256(cmx)`. Once both rows land in the same wallet-level `pending`/`settled` array, SwiftUI sees duplicate ForEach identifiers and will drop or reuse rows, leading to missing entries in the history list and potentially the wrong detail sheet binding. Key by a composite that includes `accountIndex` (or use `\.persistentModelID`) instead of `\.entryId` alone.
The activity table is uniqued on (walletId, accountIndex, entryId), so a wallet-level list can hold the same entryId under two accounts (e.g. an intra-wallet transfer's Sent + Received rows). Keying ForEach by entryId alone made SwiftUI drop/reuse rows. Also neutralize the spend-only wording on the Swift mirrors of result code 18 now that shield routes through the same path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both prior findings are FIXED in head 75406eb — the SwiftUI activity ForEach now keys on \.persistentModelID (with an explanatory comment) and the Swift mirror for code 18 now correctly enumerates shield/unshield/transfer/withdrawal with the shield-reserves-nothing carve-out. One small doc nit remains in the FFI struct comment, which still tells hosts to key activity rows on entry_id alone — a non-Swift host following this guidance would recreate the cross-account collision that prior finding 1 fixed.
💬 1 nitpick(s)
…_id) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding on ShieldedActivityFFI struct doc is FIXED at 433f099. However, two adjacent public docs in the same PR series still describe the old entry_id-only key contract: the FFI callback-level doc in persistence.rs and the SwiftData model-level doc in PersistentShieldedActivity.swift. Both are doc-only nitpicks; the actual code paths (composite #Unique in Swift, composite predicate in PlatformWalletPersistenceHandler) are correct.
💬 2 nitpick(s)
…ry_id) tuple Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both prior nitpicks from the 433f099 review are FIXED in current HEAD — the FFI persist-activity callback doc (persistence.rs:357-362) and the Swift PersistentShieldedActivity doc (PersistentShieldedActivity.swift:8-35) now correctly key on (wallet_id/walletId, account_index/accountIndex, entry_id/entryId) and explain the intra-wallet-transfer collision case. Verified three new findings against the source: a stale implementation comment that wasn't updated alongside the public docs, a SwiftData→FFI tag conversion that truncates instead of validating, and a Result-swallowing fallback in record_activity_status that defeats the function's own anti-clobber invariant. Dropped Codex's blocking ABI-versioning concern as speculative for an internal monorepo FFI where Rust and Swift ship from the same SHA.
🟡 2 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 `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:2840-2842: Activity-load truncates Swift `Int` tags into valid-looking u8 discriminants
`PersistentShieldedActivity` stores `kindTag`, `direction`, and `status` as Swift `Int` (PersistentShieldedActivity.swift:50-54), but the FFI restore struct takes `u8`. `UInt8(truncatingIfNeeded:)` silently wraps an out-of-range stored value (e.g. corruption or a future tag that wasn't migrated) into a different valid discriminant — `256` becomes `0` (Shield/In/Pending), so Rust's intentional unknown-tag fallback path (persistence.rs:1832-1880) never fires. Use `UInt8(exactly:)` and skip the row when conversion fails, so corrupt persisted state is dropped rather than reinterpreted as a different valid activity classification.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:296-299: `record_activity_status` swallows store read errors, defeating its anti-clobber invariant
The doc comment (lines 275-281) explicitly states the design: the flip must be applied to the CURRENT stored row to avoid erasing a scan-learned `Confirmed` height. But `get_activity_by_entry_id(...).unwrap_or_default()` collapses any storage `Err` into `None`, so the code then falls back to the captured pre-broadcast `entry` and queues a persister upsert. With any fallible `ShieldedStore` impl, that turns a transient read error into exactly the clobber the function was written to prevent — writing a stale `Failed`/`Confirmed-without-height` row over richer state precisely when the invariant could not be checked. Skip the flip (log and return) on read error so the result-wait can't overwrite scan truth blindly.
…n read error - Activity restore now converts kind/direction/status with UInt8(exactly:) and drops the row on overflow, so corrupt persisted tags can't wrap into a different valid discriminant and bypass Rust's unknown-tag fallback - record_activity_status skips the flip (warn + return) when the store read fails instead of falling back to the stale pre-broadcast capture, which would have been exactly the clobber the merge exists to prevent - Align the dispatch call-site comment with the composite upsert key Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The incremental delta (3 files, 31 insertions) cleanly fixes all three prior PastaClaw findings: Swift activity-load now uses UInt8(exactly:) and skips out-of-range rows before any per-row buffer allocation (no leak); record_activity_status now matches the store read and bails on Err preserving the anti-clobber invariant; and the dispatch_shielded_changeset comment correctly names the (wallet_id, account_index, entry_id) upsert key. No new in-scope defects observed. The single codex-ffi-engineer suggestion (Swift String.withCString silently truncates memos at embedded NUL) is real but applies to a pre-existing shielded_send path untouched by this PR — out of scope.
|
Reviewed |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3870 +/- ##
============================================
+ Coverage 70.73% 71.20% +0.47%
============================================
Files 20 20
Lines 2788 2837 +49
============================================
+ Hits 1972 2020 +48
- Misses 816 817 +1
🚀 New features to boost your workflow:
|
2 participants
Issue being fixed or feature implemented
The wallet stack persists shielded materials — own notes and OVK-recovered sends — but no user-facing history: nothing shows "what happened in this wallet" the way the core side's transaction list does. Privacy makes this nontrivial: the chain has no per-wallet transaction list to fetch (notes and nullifiers are anonymous, and the note→transition linkage subsystem was deliberately removed in #3823), so history must be derived.
Privacy decision (deliberate): classification is client-side only. No node/DAPI queries or note→transition indexes were added — a richer node-side index would institutionalize note↔transition correlation, and its query pattern (fetching transitions at exactly the wallet's heights) would fingerprint wallet activity to serving nodes. Every query this feature relies on is traffic the wallet already emits, so a rescanning wallet is indistinguishable from a syncing one.
What was done?
One activity log, two derivation paths, one shared id —
sha256(sorted wallet-visible output cmxs)— so a live entry and a later rescan of the same bundle dedupe to a single row, and richer entries upgrade coarser ones in place.rs-platform-wallet
ShieldedActivityEntry(kind / direction / amount / fee / counterparty / 36-byte memo / height / Pending-Confirmed-Failed status / cmx + nullifier linkage), store + file-store methods,ShieldedChangeSet.activity_entriesriding the existing persister flush.live id == scan idby construction. The skip-branch (no visible cmx) logs loudly instead of silently dropping history.rhoequals the nullifier of the action that consumed our note, sorho ∈ own nullifiersis zero-false-positive proof of an own spend — recovering the exact amount that left the pool, and the exact fee for transfers (spent − sent − change). Recall is probabilistic (~50% per 2-action bundle: Orchard shuffles spends and outputs independently before pairing —indexed_spends.shuffle/indexed_outputs.shuffleinorchard::builder), and a missed link degrades honestly, never wrongly.Sent(recipient/value/memo); own notes with no OVK pairing →Received(a true third-party receive has exactly this shape); unlinked self-pay clusters → self-transferShieldedSpend(a shield-to-self and an unlucky-shuffle change are indistinguishable — labeling eitherReceivedwould show a spend as money arriving).FFI / Swift: persist + load callbacks (
on_persist_shielded_activity_fn+ load/free pair) following the existing one-way changeset→SwiftData pattern;PersistentShieldedActivitymodel (upsert by entry id); "Shielded Activity" list + detail sheet in SwiftExampleApp (Pending section on top, kind icons, signed amounts, memo snippets;ShieldedSpendrows carry an explainer footnote).How Has This Been Tested?
cargo test -p platform-wallet --features shielded: 272 passed. fmt/clippy/ffi checks clean;./build_ios.sh --target sim --profile releasegreen.Sentwith the exact pinned fee (162,851,200 credits) and a round-tripped UTF-8 memo, Pending→Confirmed; the scan-derived entry for a pre-feature transfer recovered recipient, amount, and memo via OVK; UI list and detail render as designed.Breaking Changes
None. New changeset field defaults empty for old flows; FFI additions are additive; SwiftData model is new (pre-release: no migration story needed). No consensus, serialization, or protocol-version changes.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI
Tests