fix(rs-sdk-ffi): stop labeling unclassified SDK errors as "failed to fetch balances"#3878
Merged
Merged
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>
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>
…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>
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>
- 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>
…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>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
- 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>
…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>
…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>
…ment Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
…_id) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ry_id) tuple Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…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>
…fetch balances" The catch-all arm of the FFIError::SDKError -> DashSDKError classifier prefixed every unclassified error with a copy-pasted "Failed to fetch balances: " string and mapped it to NetworkError. Any error that didn't match the timeout/connection/DAPI/protocol/not-found heuristics (e.g. a getDataContractHistory proof-verification failure) surfaced to the UI as "Internal Error: Failed to fetch balances: Proof verification error: ..." — both the prefix and the network classification were wrong. Pass the original message through unchanged and map to InternalError, matching the existing pass-through arms (protocol/not found/timeout). Added a regression test covering the catch-all. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR introduces end-to-end shielded activity logging spanning Rust domain models, live recording on transaction emit, scan-based backfill during sync, FFI persistence contracts, SwiftData storage, and SwiftUI display views. Activity entries track operation kind, direction, status, amounts, and optional fees/memo/block heights, with deterministic ID dedupe via cmx hashing and upsert-by-id persistence semantics. ChangesShielded Activity Logging (Live + Scan Derivation)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Collaborator
|
✅ Review complete (commit 123ebbd) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift (1)
34-40: 💤 Low valueAlign "withdrawal" terminology with Rust "withdraw" for cross-language consistency. Both Swift doc comments use "withdrawal" while the Rust error documentation (error.rs line 205) uses "withdraw". Matching the Rust wording maintains exact terminology alignment across the FFI boundary.
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift#L34-L40: change "withdrawal" to "withdraw" in theerrorShieldedSpendUnconfirmedcase documentation.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift#L174-L179: change "withdrawal" to "withdraw" in theshieldedSpendUnconfirmederror case documentation.🤖 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/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift` around lines 34 - 40, Update the doc comments to use "withdraw" instead of "withdrawal" for cross-language consistency: in packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift (lines 34-40) change "withdrawal" to "withdraw" in the documentation for the errorShieldedSpendUnconfirmed case, and in the same file (lines 174-179) change "withdrawal" to "withdraw" in the documentation for the shieldedSpendUnconfirmed error case.packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs (1)
692-697: ⚡ Quick winAvoid
_ => Vec::new()swallowing invariant breaks forShieldFromAssetLockactivity recording.In
packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs(lines 692-697), falling back toVec::new()can silently skip the live-activity row if the builder ever returns an unexpectedStateTransition/ShieldFromAssetLockTransitionvariant. TodayShieldFromAssetLockTransitiononly hasV0, so this arm is currently unreachable, but it’s fragile against future variant expansion—use an explicit exhaustive match/invariant guard instead of defaulting to empty actions.🤖 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 692 - 697, The code currently uses a fallback `_ => Vec::new()` when matching `StateTransition::ShieldFromAssetLock(ShieldFromAssetLockTransition::V0(v0))` to populate `actions`, which can silently swallow future variant changes; replace the fallback with an explicit invariant guard such as `_ => unreachable!("unexpected ShieldFromAssetLock transition variant in fund_from_asset_lock activity recording")` (or propagate an error) so that any new `ShieldFromAssetLockTransition` variants fail loudly; keep the extracted `v0.actions.clone()` for the V0 arm and remove the `Vec::new()` default.
🤖 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 1832-1846: The decoding currently constructs
ShieldedActivityKind::IdentityCreate for kind_tag == 6 regardless of
ffi.has_identity_id; update the match arm so it only returns
ShieldedActivityKind::IdentityCreate { identity_id: ffi.identity_id } when
ffi.has_identity_id == 1, and otherwise treat it as the fallback (e.g., return
ShieldedActivityKind::ShieldedSpend) so rows with has_identity_id == 0 do not
materialize a zeroed identity; adjust the arm handling for tag 6 in the match on
ffi.kind_tag accordingly.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 2865-2872: The code currently constructs a
ShieldedActivityRestoreFFI with per-field guards that emit nil+count==0 for
malformed blobs; instead, validate both blob lengths (cmxLen and nfLen) as exact
multiples of 32 before creating ShieldedActivityRestoreFFI in
PlatformWalletPersistenceHandler (the code that sets
note_cmxs_ptr/note_cmxs_count and spent_nullifiers_ptr/spent_nullifiers_count).
If either cmxLen or nfLen is non-zero and not divisible by 32, skip this
activity row (or return/propagate a load error) rather than substituting
nil/count==0; only construct ShieldedActivityRestoreFFI when cmxLen%32==0 and
nfLen%32==0, and populate the ptr/count fields as you already do when valid.
- Around line 2424-2470: The upsert currently keys PersistentShieldedActivity
only by walletId/accountIndex/entryId (in persistShieldedActivity) which allows
cross-network collisions; add a networkRaw (or equivalent) property to the
PersistentShieldedActivity model and include it in the predicate and
constructor: update the predicate in persistShieldedActivity to require
$0.networkRaw == snap.networkRaw (and set row.networkRaw when creating a new
PersistentShieldedActivity), and then update the corresponding
load/restore/delete predicates and constructors used elsewhere (the restore and
delete handlers referenced in this file) to also include networkRaw so each
network's rows are scoped and won’t be overwritten or removed by operations for
a different network.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs`:
- Around line 692-697: The code currently uses a fallback `_ => Vec::new()` when
matching
`StateTransition::ShieldFromAssetLock(ShieldFromAssetLockTransition::V0(v0))` to
populate `actions`, which can silently swallow future variant changes; replace
the fallback with an explicit invariant guard such as `_ =>
unreachable!("unexpected ShieldFromAssetLock transition variant in
fund_from_asset_lock activity recording")` (or propagate an error) so that any
new `ShieldFromAssetLockTransition` variants fail loudly; keep the extracted
`v0.actions.clone()` for the V0 arm and remove the `Vec::new()` default.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- Around line 34-40: Update the doc comments to use "withdraw" instead of
"withdrawal" for cross-language consistency: in
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
(lines 34-40) change "withdrawal" to "withdraw" in the documentation for the
errorShieldedSpendUnconfirmed case, and in the same file (lines 174-179) change
"withdrawal" to "withdraw" in the documentation for the shieldedSpendUnconfirmed
error case.
🪄 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: 8cffdb2c-c602-401d-bb06-1f1b91d11f74
📒 Files selected for processing (32)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/shielded_persistence.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/changeset/shielded_changeset.rspackages/rs-platform-wallet/src/changeset/shielded_sync_start_state.rspackages/rs-platform-wallet/src/error.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/seed_pool.rspackages/rs-platform-wallet/src/wallet/shielded/store.rspackages/rs-platform-wallet/src/wallet/shielded/sync.rspackages/rs-platform-wallet/src/wallet/shielded/sync/ovk_builder_roundtrip_tests.rspackages/rs-sdk-ffi/src/error.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentShieldedActivity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.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
Comment on lines
+1832
to
+1846
| let kind = match ffi.kind_tag { | ||
| 0 => ShieldedActivityKind::Shield, | ||
| 1 => ShieldedActivityKind::ShieldFromAssetLock, | ||
| 2 => ShieldedActivityKind::Received, | ||
| 3 => ShieldedActivityKind::Sent, | ||
| 4 => ShieldedActivityKind::Unshield, | ||
| 5 => ShieldedActivityKind::Withdrawal, | ||
| 6 => ShieldedActivityKind::IdentityCreate { | ||
| identity_id: ffi.identity_id, | ||
| }, | ||
| // 7 and any unknown tag fall back to the | ||
| // residual — a forward-compat tag we don't yet | ||
| // model still loads as an opaque spend rather | ||
| // than getting dropped. | ||
| _ => ShieldedActivityKind::ShieldedSpend, |
Contributor
There was a problem hiding this comment.
Honor has_identity_id when decoding IdentityCreate.
The load path reconstructs ShieldedActivityKind::IdentityCreate from kind_tag alone. A persisted row with kind_tag == 6 but has_identity_id == 0 is explicitly documented as having no meaningful identity payload, yet this arm still materializes a real identity-create entry for [0; 32].
Suggested fix
- 6 => ShieldedActivityKind::IdentityCreate {
- identity_id: ffi.identity_id,
- },
+ 6 if ffi.has_identity_id != 0 => {
+ ShieldedActivityKind::IdentityCreate {
+ identity_id: ffi.identity_id,
+ }
+ }
+ 6 => {
+ tracing::warn!(
+ "identity-create activity row missing identity_id; \
+ folding to ShieldedSpend"
+ );
+ ShieldedActivityKind::ShieldedSpend
+ }Based on field semantics documented in packages/rs-platform-wallet-ffi/src/shielded_persistence.rs, identity_id is only meaningful when has_identity_id == 1.
🤖 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-ffi/src/persistence.rs` around lines 1832 - 1846,
The decoding currently constructs ShieldedActivityKind::IdentityCreate for
kind_tag == 6 regardless of ffi.has_identity_id; update the match arm so it only
returns ShieldedActivityKind::IdentityCreate { identity_id: ffi.identity_id }
when ffi.has_identity_id == 1, and otherwise treat it as the fallback (e.g.,
return ShieldedActivityKind::ShieldedSpend) so rows with has_identity_id == 0 do
not materialize a zeroed identity; adjust the arm handling for tag 6 in the
match on ffi.kind_tag accordingly.
Comment on lines
+2424
to
+2470
| func persistShieldedActivity(walletId: Data, snapshots: [ShieldedActivitySnapshot]) { | ||
| onQueue { | ||
| for snap in snapshots { | ||
| let wid = snap.walletId | ||
| let acct = snap.accountIndex | ||
| let eid = snap.entryId | ||
| let predicate = #Predicate<PersistentShieldedActivity> { | ||
| $0.walletId == wid && $0.accountIndex == acct && $0.entryId == eid | ||
| } | ||
| var descriptor = FetchDescriptor<PersistentShieldedActivity>(predicate: predicate) | ||
| descriptor.fetchLimit = 1 | ||
| if let existing = try? backgroundContext.fetch(descriptor).first { | ||
| existing.kindTag = snap.kindTag | ||
| existing.direction = snap.direction | ||
| existing.status = snap.status | ||
| existing.amount = snap.amount | ||
| existing.fee = snap.fee | ||
| existing.hasFee = snap.hasFee | ||
| existing.blockHeight = snap.blockHeight | ||
| existing.hasBlockHeight = snap.hasBlockHeight | ||
| existing.createdAtMs = snap.createdAtMs | ||
| existing.identityId = snap.identityId | ||
| existing.counterparty = snap.counterparty | ||
| existing.memo = snap.memo | ||
| existing.noteCmxs = snap.noteCmxs | ||
| existing.spentNullifiers = snap.spentNullifiers | ||
| existing.lastUpdated = Date() | ||
| } else { | ||
| let row = PersistentShieldedActivity( | ||
| walletId: snap.walletId, | ||
| accountIndex: snap.accountIndex, | ||
| entryId: snap.entryId, | ||
| kindTag: snap.kindTag, | ||
| direction: snap.direction, | ||
| status: snap.status, | ||
| amount: snap.amount, | ||
| fee: snap.fee, | ||
| hasFee: snap.hasFee, | ||
| blockHeight: snap.blockHeight, | ||
| hasBlockHeight: snap.hasBlockHeight, | ||
| createdAtMs: snap.createdAtMs, | ||
| identityId: snap.identityId, | ||
| counterparty: snap.counterparty, | ||
| memo: snap.memo, | ||
| noteCmxs: snap.noteCmxs, | ||
| spentNullifiers: snap.spentNullifiers | ||
| ) |
Contributor
There was a problem hiding this comment.
Scope shielded activity rows by network.
This handler already supports the same walletId on multiple networks (see Lines 423-430), but the new activity rows are keyed only by (walletId, accountIndex, entryId). That lets sibling-network activity collide in one table, and the new restore/delete paths at Lines 2780-2883 and 3218-3223 cannot distinguish or preserve the other network's rows. Persist networkRaw (or an equivalent scope key) on PersistentShieldedActivity and include it in the upsert/load/delete predicates; otherwise deleting one network's wallet can wipe or rehydrate another network's activity.
🤖 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/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`
around lines 2424 - 2470, The upsert currently keys PersistentShieldedActivity
only by walletId/accountIndex/entryId (in persistShieldedActivity) which allows
cross-network collisions; add a networkRaw (or equivalent) property to the
PersistentShieldedActivity model and include it in the predicate and
constructor: update the predicate in persistShieldedActivity to require
$0.networkRaw == snap.networkRaw (and set row.networkRaw when creating a new
PersistentShieldedActivity), and then update the corresponding
load/restore/delete predicates and constructors used elsewhere (the restore and
delete handlers referenced in this file) to also include networkRaw so each
network's rows are scoped and won’t be overwritten or removed by operations for
a different network.
Comment on lines
+2865
to
+2872
| // A persisted blob that isn't a whole number of | ||
| // 32-byte elements is corrupt — drop the linkage | ||
| // (count 0, null ptr) rather than silently truncating | ||
| // trailing bytes into a wrong-but-plausible array. | ||
| note_cmxs_ptr: cmxLen > 0 && cmxLen % 32 == 0 ? UnsafePointer(cmxPtr) : nil, | ||
| note_cmxs_count: cmxLen % 32 == 0 ? UInt(cmxLen / 32) : 0, | ||
| spent_nullifiers_ptr: nfLen > 0 && nfLen % 32 == 0 ? UnsafePointer(nfPtr) : nil, | ||
| spent_nullifiers_count: nfLen % 32 == 0 ? UInt(nfLen / 32) : 0 |
Contributor
There was a problem hiding this comment.
Drop malformed activity rows instead of zeroing their linkage.
note_cmxs and spent_nullifiers are fixed-width [u8; 32] arrays upstream, not optional metadata. Emitting nil plus count == 0 for a non-multiple-of-32 blob turns a corrupt row into a valid-looking restored entry, which can still suppress fresh re-derivation by entry_id while losing the linkage used for confirmation/dedupe. Guard both blobs before constructing ShieldedActivityRestoreFFI and skip the row (or fail the load) once either length is invalid.
🤖 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/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`
around lines 2865 - 2872, The code currently constructs a
ShieldedActivityRestoreFFI with per-field guards that emit nil+count==0 for
malformed blobs; instead, validate both blob lengths (cmxLen and nfLen) as exact
multiples of 32 before creating ShieldedActivityRestoreFFI in
PlatformWalletPersistenceHandler (the code that sets
note_cmxs_ptr/note_cmxs_count and spent_nullifiers_ptr/spent_nullifiers_count).
If either cmxLen or nfLen is non-zero and not divisible by 32, skip this
activity row (or return/propagate a load error) rather than substituting
nil/count==0; only construct ShieldedActivityRestoreFFI when cmxLen%32==0 and
nfLen%32==0, and populate the ptr/count fields as you already do when valid.
Comment on lines
+19
to
+28
| switch tag { | ||
| case 0: return "Shielded" | ||
| case 1: return "Shielded from Asset Lock" | ||
| case 2: return "Received" | ||
| case 3: return "Sent" | ||
| case 4: return "Unshielded" | ||
| case 5: return "Withdrawn" | ||
| case 6: return "Identity Created" | ||
| default: return "Shielded Spend" | ||
| } |
Contributor
There was a problem hiding this comment.
Handle unknown kind/status/direction explicitly instead of re-labeling them as known values.
label(_:) maps any unknown kindTag to "Shielded Spend", and detail helpers map unknown status/direction to "Failed"/"Self". That can misreport future/corrupt values as legitimate states and mislead debugging.
Suggested fix
enum ShieldedActivityKindDisplay {
static func label(_ tag: Int) -> String {
switch tag {
case 0: return "Shielded"
case 1: return "Shielded from Asset Lock"
case 2: return "Received"
case 3: return "Sent"
case 4: return "Unshielded"
case 5: return "Withdrawn"
case 6: return "Identity Created"
- default: return "Shielded Spend"
+ case 7: return "Shielded Spend"
+ default: return "Unknown (\(tag))"
}
}
@@
private var statusText: String {
switch entry.status {
case 0: return "Pending"
case 1: return "Confirmed"
- default: return "Failed"
+ case 2: return "Failed"
+ default: return "Unknown (\(entry.status))"
}
}
private var directionText: String {
switch entry.direction {
case 0: return "In"
case 1: return "Out"
- default: return "Self"
+ case 2: return "Self"
+ default: return "Unknown (\(entry.direction))"
}
}Also applies to: 243-257
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Validated all agent findings against source. The headline rs-sdk-ffi error-classifier change is correct, well-tested, and consistent with the Swift error mapping (code 99 → internal). One in-scope performance suggestion remains in new shielded sync code: each sync iteration calls the display-pagination API twice on the full activity history. No blocking issues.
🟡 1 suggestion(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/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:789-830: Avoid display-pagination API for the sync dedupe index
`derive_activity_into_changeset` invokes `store.get_activity(*id, 0, usize::MAX)` twice per sync pass — once to build the `existing_cmxs` snapshot before the lock-free classification, and again as `current_cmxs` after re-acquiring the write lock. The `get_activity` implementations clone every activity entry and run `sort_activity_for_display` before slicing, so each sync pass does two O(n log n) clone-and-sort passes over the full activity history just to construct an unsorted `cmx → entry_id` map. The display ordering is wasted work here, and the second pass holds the write lock for the duration of the sort. Expose a dedicated indexed view (e.g. `store.activity_cmx_index(*id)` returning an unsorted `BTreeMap<[u8;32],[u8;32]>`), or maintain the index alongside `activity` so sync stays O(n) and doesn't compete with UI ordering work under the write lock.
Comment on lines
+789
to
+830
| let existing_cmxs: BTreeMap<[u8; 32], [u8; 32]> = store | ||
| .get_activity(*id, 0, usize::MAX) | ||
| .map_err(|e| { | ||
| crate::error::PlatformWalletError::ShieldedStoreError(e.to_string()) | ||
| })? | ||
| .into_iter() | ||
| .flat_map(|entry| entry.note_cmxs.into_iter().map(move |c| (c, entry.id))) | ||
| .collect(); | ||
|
|
||
| ( | ||
| ScanDeriveInput { | ||
| notes, | ||
| outgoing, | ||
| own_addresses, | ||
| }, | ||
| existing_cmxs, | ||
| ) | ||
| }; | ||
|
|
||
| // Lock-free classification. | ||
| let derived = derive_activity_from_scan_data(&input, &existing_cmxs); | ||
| if derived.new_entries.is_empty() && derived.confirmations.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| // Write pass: only the upserts hold the write lock. | ||
| let mut store = self.store.write().await; | ||
| // Re-check cmx overlap against the CURRENT activity rows | ||
| // before inserting: a live recorder may have written a richer | ||
| // row (kind / fee / memo / created identity id) for the same | ||
| // cmx set between the read snapshot and here. Saving the | ||
| // scan-derived entry anyway would either clobber that row | ||
| // (id collision) or duplicate it (id mismatch), and scan-only | ||
| // data can never reconstruct the lost live fields. Overlapped | ||
| // clusters degrade to confirmation sightings instead — same | ||
| // treatment the classifier gives overlaps it can see. | ||
| let current_cmxs: BTreeMap<[u8; 32], [u8; 32]> = store | ||
| .get_activity(*id, 0, usize::MAX) | ||
| .map_err(|e| crate::error::PlatformWalletError::ShieldedStoreError(e.to_string()))? | ||
| .into_iter() | ||
| .flat_map(|entry| entry.note_cmxs.into_iter().map(move |c| (c, entry.id))) | ||
| .collect(); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Avoid display-pagination API for the sync dedupe index
derive_activity_into_changeset invokes store.get_activity(*id, 0, usize::MAX) twice per sync pass — once to build the existing_cmxs snapshot before the lock-free classification, and again as current_cmxs after re-acquiring the write lock. The get_activity implementations clone every activity entry and run sort_activity_for_display before slicing, so each sync pass does two O(n log n) clone-and-sort passes over the full activity history just to construct an unsorted cmx → entry_id map. The display ordering is wasted work here, and the second pass holds the write lock for the duration of the sort. Expose a dedicated indexed view (e.g. store.activity_cmx_index(*id) returning an unsorted BTreeMap<[u8;32],[u8;32]>), or maintain the index alongside activity so sync stays O(n) and doesn't compete with UI ordering work under the write lock.
source: ['codex']
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3878 +/- ##
=========================================
Coverage 87.22% 87.22%
=========================================
Files 2641 2641
Lines 328569 328569
=========================================
Hits 286597 286597
Misses 41972 41972
🚀 New features to boost your workflow:
|
Contributor
|
✅ 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: "205351a603c9929dc73a0c9845cefde7226c157927b40e72e5f7f25f0e1f26d9"
)Xcode manual integration:
|
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
The catch-all arm of the
FFIError::SDKError -> DashSDKErrorclassifier inpackages/rs-sdk-ffi/src/error.rsprefixed every unclassified error with a copy-pasted"Failed to fetch balances: "string and mapped it toNetworkError. That prefix came from a balance-fetch context but was applied as the generic fallback for all errors that didn't match the earliertimeout/connection/DAPI/protocol/not foundheuristics.Observed on testnet 2026-06-12: a
getDataContractHistoryquery failed and surfaced to the UI as:Both the "Failed to fetch balances" text and a
NetworkErrorclassification are nonsense for a contract-history proof failure.What was done?
DashSDKErrorCode::InternalErrorinstead ofNetworkError. This matches the existing pass-through arms (protocol/not found/timeout) and correctly classifies genuinely-unknown failures.unclassified_error_maps_to_internal_error_without_balance_prefix) that drives a proof-verification error through the catch-all and asserts the code isInternalError, the message is the SDK error'sDisplayverbatim, and it contains no "Failed to fetch balances" text.Audit of the surrounding match arms: all other arms are correctly scoped — their messages only fire behind the matching condition that makes them accurate (
"Network connection failed: …","DAPI error: …", the "No DAPI addresses configured" text). The buggy literal appeared in exactly one place repo-wide, and this is the onlyFrom<…> for DashSDKErrorimpl in the crate.No Swift changes are required: the Swift side maps the numeric code to a display prefix only and never branches on it, and its retry logic is message-based, so switching the catch-all code from
3(NetworkError) to99(InternalError) is safe.How Has This Been Tested?
cargo test -p rs-sdk-ffi— all 5 error-module tests pass, including the new one.cargo check -p rs-sdk-ffi --all-targets --all-features --tests— clean.cargo clippy -p rs-sdk-ffi --all-targets— no warnings.cargo fmt --allapplied.Breaking Changes
None. This is not a consensus-breaking change; it only adjusts the human-readable message and error code returned across the FFI boundary for previously-unclassified errors.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes