feat(swift-sdk): drive shielded sync from Rust platform-wallet#3601
Merged
Conversation
The shielded path was implemented twice: a feature-gated ShieldedWallet in platform-wallet (correct location, but never wired) and an opaque ShieldedPoolClient duplicated in rs-sdk-ffi (the one Swift actually drove). Per the swift-sdk architectural rule, all shielded orchestration belongs on the platform-wallet side and Swift should only persist/load/bridge. Demolish the duplicate so the next PR can wire the platform-wallet ShieldedWallet through a thin FFI translator instead: - Delete rs-sdk-ffi/src/shielded/ (pool_client, transitions, queries, crypto, types). Drop "shielded" from rs-sdk-ffi's dash-sdk feature list. - Delete rs-sdk-ffi/src/nullifier_sync/ (only consumer was the deleted Swift NullifierSyncService). - Delete swift-sdk/Sources/SwiftDashSDK/Shielded/ wholesale. - Delete unused SwiftExampleApp ZKSyncService (no consumers). - Stub ShieldedService to display state only and gate Send shielded flows with a clear placeholder error; the Rust-side coordinator and external-signable signer plumbing land in a follow-up. Drop the spending_key field from OrchardKeySet — it was only used to derive FVK/ASK at construction and was retained dead. The SK is now strictly a transient local in `from_seed`. Also drop the #[allow(dead_code)] on ShieldedWallet itself plus the four unused crate-private accessor methods that hung on for "future use"; the manager wiring will reintroduce what it needs. No new behavior. Send shielded is broken until the Rust-side coordinator + OrchardSpendSigner ship; sync was already broken on launch (initializeShieldedService was a TODO stub).
…+ FFI
Adds the platform-wallet shielded sync end-to-end on the Rust side
so the next PR can collapse Swift to a thin display-state surface.
All new code is feature-gated behind the existing `shielded` Cargo
feature; default builds are unaffected.
Wallet
- `FileBackedShieldedStore` — concrete `ShieldedStore` impl that
wraps `ClientPersistentCommitmentTree` (SQLite, shared per network)
for the commitment tree plus an in-memory note vec. Notes are
rediscovered via trial decryption on cold start, matching the
previous `ShieldedPoolClient` behavior.
- `PlatformWallet.shielded` slot + `bind_shielded`/`shielded_sync`
methods. `bind_shielded` takes a 32-252 byte BIP-39 seed,
derives the FVK / IVK / OVK / default address via ZIP-32, opens
the per-network commitment tree, and stores a `ShieldedWallet`
on the wallet handle. The seed is consumed and dropped before
return; nothing secret survives.
Manager
- `ShieldedSyncManager` — periodic shielded note + nullifier sync
coordinator that mirrors `PlatformAddressSyncManager`. Iterates
every wallet with a bound shielded sub-wallet on a 60s default
cadence. Wallets without `bind_shielded` having run are emitted
as `WalletShieldedOutcome::Skipped` rather than erroring the
whole pass — bind is the host's responsibility (it requires
keychain access via the mnemonic resolver).
- Wired into `PlatformWalletManager::new` and `shutdown`. Accessor
methods on the manager mirror `platform_address_sync()` /
`platform_address_sync_arc()`.
- New `on_shielded_sync_completed(&summary)` event on
`PlatformEventHandler` (default no-op) plus the dispatcher
method on `PlatformEventManager`.
FFI
- `platform_wallet_manager_shielded_sync_{start,stop,is_running,
is_syncing,last_sync_unix_seconds,set_interval,sync_now,
sync_wallet}` — same shape as `platform_address_sync_*`.
- `platform_wallet_manager_bind_shielded(handle, wallet_id_bytes,
mnemonic_resolver_handle, account, db_path_cstr)` — fires the
resolver to fetch the host's mnemonic, derives a 64-byte BIP-39
seed in `Zeroizing`, and forwards into `PlatformWallet::bind_shielded`.
Mnemonic / seed are scrubbed before return.
- `ShieldedSyncWalletResultFFI` — tri-state per-wallet result
(success / skipped / error) carrying note + balance counters.
Defined unconditionally so `EventHandlerCallbacks` keeps a
stable C struct layout across feature toggles; the
`on_shielded_sync_completed_fn` callback slot is populated only
when the `shielded` feature compiles in the dispatcher.
- `rs-platform-wallet-ffi/shielded` and
`rs-unified-sdk-ffi/shielded` features added (off by default).
The iOS build still ships transparent-only by default. PR 3 will
flip on `--features shielded` in `build_ios.sh` and replace the
SwiftExampleApp `ShieldedService` placeholder with a thin wrapper
around the new manager-handle-based sync surface.
Wires the Swift side onto the platform-wallet shielded coordinator landed in the previous commit. The iOS framework now ships with the `shielded` Cargo feature enabled and the SwiftExampleApp drives shielded balance sync end-to-end through the Rust manager handle — no Swift-side ShieldedPoolClient, no spending key marshaled across the FFI, no separate iOS-owned periodic sync loop. swift-sdk - New PlatformWalletManagerShieldedSync.swift mirroring the PlatformAddressSync wrapper: bindShielded(walletId:resolver: account:dbPath:), startShieldedSync, stopShieldedSync, isShieldedSyncRunning/isSyncing, lastShieldedSyncUnixSeconds, setShieldedSyncInterval, syncShieldedNow, syncShieldedWalletNow. ShieldedSyncEvent + per-wallet ShieldedWalletSyncResult published via the manager's @published lastShieldedSyncEvent and shieldedSyncIsSyncing. - Manager event-handler callbacks wire the new on_shielded_sync_completed_fn slot. Progress polling now mirrors shielded sync state alongside platform-address sync state. - bind_ios.sh passes --features shielded to all three target builds so the unified xcframework exposes the shielded FFI symbols. swift-example-app - ShieldedService is now a real consumer of the manager: bind() drives bind_shielded via the existing keychain-backed MnemonicResolver, then subscribes to the manager's published shielded sync events. Per-wallet result decoding distinguishes success / skipped (not yet bound) / error states. manualSync() forwards to syncShieldedNow. - App.bootstrap retires the old initializeShieldedService stub. rebindWalletScopedServices binds the shielded service alongside PlatformBalanceSyncService and starts the shielded sync loop whenever a wallet is active. CoreContentView's "Sync Now" button invokes manualSync as an async Task. Sends are still gated behind the placeholder rebuild banner — the spend-signer pipeline lands in a follow-up.
manualSync set isSyncing=true to gate the button and disable re-entry, but only reset it to false on error. On success it relied on the manager's 1 Hz \$shieldedSyncIsSyncing poll to flip the local state back via the Combine subscription. Empty-tree shielded syncs complete in ~25 ms (well under the 1 s poll cadence), so the manager's atomic is_syncing flag goes true→false inside one tick — the polled @published value never changes, the subscription never fires, and ShieldedService.isSyncing stays at true forever. UI shows "Syncing..." indefinitely with the Sync Now button disabled. Wrap the manualSync body with `defer { isSyncing = false }` so the local flag always relaxes on the way out, and the subscription remains the source of truth for any sync that does straddle a poll tick.
…anual sync Same shape as the shielded fix in the previous commit: performSync set isSyncing=true to gate re-entry but only reset it on error, relying on the manager's 1 Hz \$platformAddressSyncIsSyncing poll to flip it back through the Combine subscription. Empty-pool regtest BLAST syncs return inside tens of milliseconds — well under the 1 s poll cadence — so the polled @published value never observes is_syncing=true, the subscription never fires, and the local flag stays at true forever after a manual Sync Now tap. Background loop syncs aren't affected because they don't go through performSync; the bug only surfaces for the manual path. Wrap the body with `defer { isSyncing = false }` so the local flag always relaxes regardless of outcome, and the subscription remains the source of truth for any sync that does straddle a poll tick.
…n shielded card The shielded sync status panel only had a binary "Syncing..." vs "Ready" indicator and a stale balance row — once a pass completed there was no signal that anything had run, no count of how many passes had landed since launch, and no per-pass throughput. Mirror the Platform Sync Status surface's shape on the shielded side. ShieldedService now tracks: - syncCountSinceLaunch — successful (non-skipped) pass count - totalScanned — sum of every pass's total_scanned - totalNewNotes / totalNewlySpent — cumulative wallet-side outcomes CoreContentView's Shielded Sync Status section now renders: - "Last sync: <relative time>" with the green check, or "Not bound" when bind_shielded hasn't run yet, or "Not synced yet" when the bind succeeded but no pass has landed. - "Queries Since Launch: N syncs" plus three QueryCountBadge columns (Scanned / New / Spent), gated on at least one successful pass having been observed. The skipped/error states already feed through ShieldedService's existing fields; this just rounds out the success-path surface.
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds an opt-in Cargo feature ChangesPlatform-wallet Shielded feature & sync integration
SDK-FFI & Swift Shielded removal
Sequence Diagram(s)sequenceDiagram
participant App as SwiftUI App
participant PM as PlatformWalletManager
participant SC as ShieldedSyncManager
participant W as PlatformWallet
participant Store as FileBackedShieldedStore
participant EM as PlatformEventManager
participant Handler as EventHandler (C)
Note over App,Handler: Shielded Sync Workflow
App->>PM: startShieldedSync()
PM->>SC: start(Arc<Self>)
SC->>SC: spawn OS thread, loop every interval
loop per interval
SC->>W: shielded_sync() for each wallet
W->>Store: append commitments / mark spent
W-->>SC: ShieldedSyncSummary per wallet
SC->>EM: on_shielded_sync_completed(summary)
EM->>Handler: invoke C callback with results array
Handler->>PM: deliver ShieldedSyncEvent (MainActor)
end
App->>PM: syncShieldedNow() (manual) --> SC: sync_now()
App->>PM: stopShieldedSync() --> SC: stop()
sequenceDiagram
participant UI as App UI
participant PM as PlatformWalletManager
participant Bind as Bind Flow
participant PW as PlatformWallet
participant Store as FileBackedShieldedStore
UI->>PM: bindShielded(walletId, resolver, dbPath)
PM->>Bind: resolve mnemonic -> seed
PM->>PW: platform wallet lookup by id
PW->>Store: FileBackedShieldedStore::open_path(dbPath)
PW->>PW: ShieldedWallet::from_seed(seed, account, store)
PW-->>PM: bind completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Collaborator
Review GateCommit:
|
![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
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/keys.rs (1)
40-51:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftASK (and SK) remain unzeroed in memory; Zeroize wrapper not viable without upstream support.
The concern is valid:
spend_auth_key: SpendAuthorizingKeyis sensitive key material that persists for the wallet's lifetime.SpendingKeyisCopyso stack bytes are never cleared on drop. However, the suggested fix won't work—the upstreamorchardcrate (re-exported viagrovedb_commitment_tree) does not implement theZeroizetrait for eitherSpendingKeyorSpendAuthorizingKey, so wrapping them inZeroizing<T>will not enable zeroing onDrop.The code already acknowledges this in the comment at lines 89–95, deferring the issue ("revisit when the spend signer lands").
To address this, either:
- Implement
Zeroize(or a customDrop) for these types ingrovedb_commitment_tree, or- Use a custom
Dropimpl forOrchardKeySetto manually overwritespend_auth_keymemory, or- Escalate to the architecture team to determine if re-deriving ASK transiently at signing time (as noted in the comment) is the intended design and acceptable for the security model.
🤖 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/keys.rs` around lines 40 - 51, The OrchardKeySet currently holds sensitive spend_auth_key (type SpendAuthorizingKey / underlying SpendingKey) that isn't zeroed on drop because upstream types lack Zeroize; fix by either (A) adding a Zeroize (or explicit Drop) implementation for SpendAuthorizingKey/SpendingKey in the grovedb_commitment_tree/orchard upstream so zeroing happens automatically, or (B) add a custom Drop impl for OrchardKeySet that securely overwrites the memory of spend_auth_key (convert the key to its raw byte representation and overwrite with zeros before drop) and document this workaround, or (C) escalate to the architecture team to confirm re-deriving ASK at sign-time is the approved approach and adjust OrchardKeySet to avoid holding ASK persistently if agreed. Ensure you reference OrchardKeySet and spend_auth_key when making the change so reviewers can find the updated behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/manager/shielded_sync.rs`:
- Around line 294-307: sync_wallet bypasses the manager's synchronization
exclusion (is_syncing) and calls PlatformWallet::shielded_sync() which only
holds a read lock, allowing concurrent manual and periodic syncs; fix by
serializing this path: before calling PlatformWallet::shielded_sync() in
sync_wallet, acquire the same exclusion used by sync_now() (or a per-wallet
exclusive lock) so the manual per-wallet sync cannot run concurrently with the
periodic pass, e.g. reuse the manager-level sync exclusion (is_syncing) or
add/obtain a per-wallet Mutex/RwLock around the shielded slot to ensure
exclusive access during sync.
- Around line 191-238: The background_cleanup currently unconditionally clears
background_cancel on thread exit, which can erase a newly-installed token; fix
by guarding the clear so it only removes the token belonging to the exiting
thread: introduce a generation counter (e.g. background_generation: AtomicU64),
increment it in start() when you create the CancellationToken, capture that
generation in the spawned thread, and on exit check inside the cleanup block
(the if let Ok(mut guard) = this.background_cancel.lock() { *guard = None; }
section) that the stored generation still equals the captured generation before
setting None; alternatively, compare token identity if you prefer, but ensure
stop() semantics (stop(), start(), is_running()) remain correct by only clearing
the slot when it still belongs to the exiting thread.
In `@packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- Around line 91-96: save_note currently appends duplicate notes when the same
nullifier is saved again, causing double-counting in get_unspent_notes and
leaving the original copy unspent for mark_spent; change save_note to first
check nullifier_index for note.nullifier and return Ok(()) (no-op) if present,
otherwise insert the new index and push the note into notes—this makes save_note
idempotent and prevents stale duplicate entries.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- Around line 89-130: In bind(...), clear per-wallet published state immediately
after cancelling subscriptions and before attempting to bind: reset
shieldedBalance, lastSyncTime, and any launch counters (the per-wallet launch
counter properties) to their initial/empty values so the UI won't show stale
data; also ensure these fields are reset on the catch path when bind fails
(alongside lastError/isBound) so a failed rebind doesn't preserve the previous
wallet's shielded state.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/shielded/keys.rs`:
- Around line 40-51: The OrchardKeySet currently holds sensitive spend_auth_key
(type SpendAuthorizingKey / underlying SpendingKey) that isn't zeroed on drop
because upstream types lack Zeroize; fix by either (A) adding a Zeroize (or
explicit Drop) implementation for SpendAuthorizingKey/SpendingKey in the
grovedb_commitment_tree/orchard upstream so zeroing happens automatically, or
(B) add a custom Drop impl for OrchardKeySet that securely overwrites the memory
of spend_auth_key (convert the key to its raw byte representation and overwrite
with zeros before drop) and document this workaround, or (C) escalate to the
architecture team to confirm re-deriving ASK at sign-time is the approved
approach and adjust OrchardKeySet to avoid holding ASK persistently if agreed.
Ensure you reference OrchardKeySet and spend_auth_key when making the change so
reviewers can find the updated behavior.
🪄 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: 14190729-d7c0-4c00-bf24-75b36fa8ff88
📒 Files selected for processing (58)
packages/rs-platform-wallet-ffi/Cargo.tomlpackages/rs-platform-wallet-ffi/src/event_handler.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/shielded_sync.rspackages/rs-platform-wallet-ffi/src/shielded_types.rspackages/rs-platform-wallet/src/events.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/manager/shielded_sync.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/file_store.rspackages/rs-platform-wallet/src/wallet/shielded/keys.rspackages/rs-platform-wallet/src/wallet/shielded/mod.rspackages/rs-sdk-ffi/Cargo.tomlpackages/rs-sdk-ffi/src/lib.rspackages/rs-sdk-ffi/src/nullifier_sync/mod.rspackages/rs-sdk-ffi/src/nullifier_sync/types.rspackages/rs-sdk-ffi/src/shielded/crypto/address.rspackages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rspackages/rs-sdk-ffi/src/shielded/crypto/decrypt.rspackages/rs-sdk-ffi/src/shielded/crypto/mod.rspackages/rs-sdk-ffi/src/shielded/mod.rspackages/rs-sdk-ffi/src/shielded/pool_client/bundle.rspackages/rs-sdk-ffi/src/shielded/pool_client/mod.rspackages/rs-sdk-ffi/src/shielded/pool_client/sync.rspackages/rs-sdk-ffi/src/shielded/queries/anchors.rspackages/rs-sdk-ffi/src/shielded/queries/encrypted_notes.rspackages/rs-sdk-ffi/src/shielded/queries/mod.rspackages/rs-sdk-ffi/src/shielded/queries/most_recent_anchor.rspackages/rs-sdk-ffi/src/shielded/queries/nullifiers.rspackages/rs-sdk-ffi/src/shielded/queries/pool_state.rspackages/rs-sdk-ffi/src/shielded/transitions/broadcast.rspackages/rs-sdk-ffi/src/shielded/transitions/builders.rspackages/rs-sdk-ffi/src/shielded/transitions/mod.rspackages/rs-sdk-ffi/src/shielded/transitions/shield.rspackages/rs-sdk-ffi/src/shielded/transitions/shield_from_asset_lock.rspackages/rs-sdk-ffi/src/shielded/transitions/shielded_transfer.rspackages/rs-sdk-ffi/src/shielded/transitions/shielded_withdrawal.rspackages/rs-sdk-ffi/src/shielded/transitions/unshield.rspackages/rs-sdk-ffi/src/shielded/types.rspackages/rs-unified-sdk-ffi/Cargo.tomlpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ZKSyncService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (35)
- packages/rs-sdk-ffi/src/shielded/transitions/unshield.rs
- packages/rs-sdk-ffi/Cargo.toml
- packages/rs-sdk-ffi/src/shielded/crypto/decrypt.rs
- packages/rs-sdk-ffi/src/shielded/queries/most_recent_anchor.rs
- packages/rs-sdk-ffi/src/shielded/transitions/mod.rs
- packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift
- packages/rs-sdk-ffi/src/shielded/queries/encrypted_notes.rs
- packages/rs-sdk-ffi/src/shielded/pool_client/sync.rs
- packages/rs-sdk-ffi/src/shielded/queries/pool_state.rs
- packages/rs-sdk-ffi/src/lib.rs
- packages/rs-sdk-ffi/src/shielded/mod.rs
- packages/rs-sdk-ffi/src/shielded/queries/mod.rs
- packages/rs-sdk-ffi/src/shielded/pool_client/bundle.rs
- packages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncService.swift
- packages/rs-sdk-ffi/src/shielded/transitions/shield_from_asset_lock.rs
- packages/rs-sdk-ffi/src/shielded/queries/anchors.rs
- packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swift
- packages/rs-sdk-ffi/src/shielded/crypto/mod.rs
- packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs
- packages/rs-sdk-ffi/src/shielded/transitions/shielded_transfer.rs
- packages/rs-sdk-ffi/src/shielded/pool_client/mod.rs
- packages/rs-sdk-ffi/src/shielded/transitions/builders.rs
- packages/rs-sdk-ffi/src/nullifier_sync/mod.rs
- packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolClient.swift
- packages/rs-sdk-ffi/src/shielded/transitions/shielded_withdrawal.rs
- packages/rs-sdk-ffi/src/shielded/types.rs
- packages/rs-sdk-ffi/src/shielded/transitions/broadcast.rs
- packages/rs-sdk-ffi/src/nullifier_sync/types.rs
- packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoService.swift
- packages/rs-sdk-ffi/src/shielded/queries/nullifiers.rs
- packages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncTypes.swift
- packages/rs-sdk-ffi/src/shielded/transitions/shield.rs
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ZKSyncService.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift
- packages/rs-sdk-ffi/src/shielded/crypto/address.rs
…he bound wallet The Receive sheet's Shielded tab showed "Not available" because ShieldedService.orchardDisplayAddress was permanently nil — the manager had no way to surface the per-wallet Orchard payment address. Add a tiny accessor on the bound shielded sub-wallet: - `PlatformWallet::shielded_default_address() -> Option<[u8; 43]>` reads `ShieldedWallet`'s `default_address.to_raw_address_bytes()` under the existing read lock; returns None when bind_shielded hasn't run. - `platform_wallet_manager_shielded_default_address` FFI: takes manager handle + 32-byte wallet id, writes 43 raw bytes plus a presence flag. Unknown wallet → ErrorWalletOperation, known but unbound → out_present = false. - `PlatformWalletManager.shieldedDefaultAddress(walletId:) -> Data?` Swift wrapper. - `ShieldedService.bind` calls it post-bindShielded and runs the bytes through `DashAddress.encodeOrchard` (existing bech32m helper) to populate `orchardDisplayAddress`. Best-effort — failures here don't unbind the wallet. Receive Dash → Shielded now renders the wallet's default Orchard address as soon as the shielded bind completes, matching the Core / Platform tabs.
The Core Sync Status panel had no signal for "is the chain actually advancing?" — once SPV caught up to the tip and the four progress bars hit 100%, a stalled core (no new blocks) and a healthy core looked identical from the wallet's side. Plumb the tip header's block time through to the UI so a glance at the Core Sync card answers the question directly: - `SpvRuntime::tip_block_time() -> Option<u32>`: walks the running client → storage → block-headers store → `get_tip` and reads `header.time`. None when SPV isn't running or no headers are stored. - `platform_wallet_manager_spv_tip_unix_seconds(handle, *out_unix_seconds)`: FFI translator, writes 0 as the no-tip sentinel. - `PlatformWalletManager.currentSpvTipBlockTime() throws -> Date?` one-shot wrapper plus a `@Published var spvTipBlockTime: Date?` mirror fed by the existing 1 Hz progress poll. Distinct names to avoid the `@Published` / method shadowing that breaks `\.spvTipBlockTime` keypath access. - CoreContentView's Core Sync Status section renders a Block Time row with `<relative> ago` plus the wall-clock time, hidden when no tip is available. Mirrors the existing Block Time row on the Platform Sync card. When the row stops advancing across polls, core has stalled producing blocks even though the local SPV client is healthy.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift (1)
89-142:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the per-wallet snapshot on rebind.
bind(...)cancels subscriptions, swapswalletId, and togglesisBound = false, but leavesshieldedBalance,lastNewNotes,lastNewlySpent,lastSyncTime,syncCountSinceLaunch,totalScanned,totalNewNotes,totalNewlySpent, andorchardDisplayAddressuntouched. After a wallet switch (or a failed rebind), the UI keeps showing the previous wallet's shielded snapshot until the next event arrives — and on a failure path no event will arrive at all. Reset these fields up-front (and again on thecatchbranch alongsidelastError/isBound) so a rebind never surfaces stale data.🛡️ Suggested reset on rebind
self.walletManager = walletManager self.walletId = walletId self.syncStateCancellable?.cancel() self.syncEventCancellable?.cancel() self.isBound = false + self.shieldedBalance = 0 + self.lastNewNotes = 0 + self.lastNewlySpent = 0 + self.lastSyncTime = nil + self.syncCountSinceLaunch = 0 + self.totalScanned = 0 + self.totalNewNotes = 0 + self.totalNewlySpent = 0 + self.orchardDisplayAddress = nil🤖 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/Core/Services/ShieldedService.swift` around lines 89 - 142, The bind(...) method must clear the per-wallet shielded snapshot up-front and on error to avoid showing stale data; inside ShieldedService.bind before calling walletManager.bindShielded reset shieldedBalance, lastNewNotes, lastNewlySpent, lastSyncTime, syncCountSinceLaunch, totalScanned, totalNewNotes, totalNewlySpent, and orchardDisplayAddress to their empty/default values and set isBound = false/lastError = nil accordingly, and mirror the same resets in the catch block alongside setting lastError and isBound so failed rebinds also clear the previous wallet's state.
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/spv/runtime.rs (1)
249-257: ⚡ Quick winNarrow the
self.clientread-lock scope before awaited storage reads.Line 249 keeps the
self.clientread guard alive through multiple awaits; dropping it immediately after extracting the storage handle avoids unnecessary contention withstop()/writers.♻️ Proposed change
pub async fn tip_block_time(&self) -> Option<u32> { use dash_spv::storage::{BlockHeaderStorage, StorageManager}; - let client_guard = self.client.read().await; - let client = client_guard.as_ref()?; - let storage_arc = client.storage(); + let storage_arc = { + let client_guard = self.client.read().await; + let client = client_guard.as_ref()?; + client.storage() + }; let storage = storage_arc.lock().await; let block_headers = StorageManager::block_headers(&*storage); drop(storage); let bh = block_headers.read().await; let tip = BlockHeaderStorage::get_tip(&*bh).await?; Some(tip.header().time) }🤖 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/spv/runtime.rs` around lines 249 - 257, Grab the storage handle while the self.client read lock is held and then drop the read guard before any awaits: call self.client.read().await, get the Option client via client_guard.as_ref()? and extract/clone the storage Arc via client.storage().clone() (or otherwise clone the Arc returned by storage()), then drop the read guard (or let it go out of scope) and only after that await storage_arc.lock().await and call StorageManager::block_headers(&*storage) and BlockHeaderStorage::get_tip(&*bh).await; this narrows the lifetime of the client read lock and avoids holding it across await points.packages/rs-platform-wallet-ffi/src/shielded_sync.rs (1)
125-136: 💤 Low valueReject (or floor) a zero
interval_secondsto avoid a tight background loop.
Duration::from_secs(0)is a valid value and will be forwarded verbatim toShieldedSyncManager::set_interval. If the manager's loop simply waitsintervalbetween passes, an interval of0will busy-cycle through full shielded sync passes back-to-back on the dedicated thread. A defensive floor (or an explicit error code) at the FFI boundary would make this misuse harder to trigger from the host.♻️ One option — floor at 1s
pub unsafe extern "C" fn platform_wallet_manager_shielded_sync_set_interval( handle: Handle, interval_seconds: u64, ) -> PlatformWalletFFIResult { + let interval = Duration::from_secs(interval_seconds.max(1)); let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - manager - .shielded_sync() - .set_interval(Duration::from_secs(interval_seconds)); + manager.shielded_sync().set_interval(interval); });🤖 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/shielded_sync.rs` around lines 125 - 136, The FFI currently forwards Duration::from_secs(interval_seconds) verbatim which allows 0 and can cause a tight busy-loop; update platform_wallet_manager_shielded_sync_set_interval to reject or floor zero by ensuring interval_seconds is at least 1 before calling manager.shielded_sync().set_interval(Duration::from_secs(...)) (e.g. if interval_seconds == 0 then use 1 or return a PlatformWalletFFIResult error), referencing the function platform_wallet_manager_shielded_sync_set_interval and ShieldedSyncManager::set_interval so callers cannot set a 0s interval from the FFI boundary.
🤖 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.
Duplicate comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- Around line 89-142: The bind(...) method must clear the per-wallet shielded
snapshot up-front and on error to avoid showing stale data; inside
ShieldedService.bind before calling walletManager.bindShielded reset
shieldedBalance, lastNewNotes, lastNewlySpent, lastSyncTime,
syncCountSinceLaunch, totalScanned, totalNewNotes, totalNewlySpent, and
orchardDisplayAddress to their empty/default values and set isBound =
false/lastError = nil accordingly, and mirror the same resets in the catch block
alongside setting lastError and isBound so failed rebinds also clear the
previous wallet's state.
---
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/shielded_sync.rs`:
- Around line 125-136: The FFI currently forwards
Duration::from_secs(interval_seconds) verbatim which allows 0 and can cause a
tight busy-loop; update platform_wallet_manager_shielded_sync_set_interval to
reject or floor zero by ensuring interval_seconds is at least 1 before calling
manager.shielded_sync().set_interval(Duration::from_secs(...)) (e.g. if
interval_seconds == 0 then use 1 or return a PlatformWalletFFIResult error),
referencing the function platform_wallet_manager_shielded_sync_set_interval and
ShieldedSyncManager::set_interval so callers cannot set a 0s interval from the
FFI boundary.
In `@packages/rs-platform-wallet/src/spv/runtime.rs`:
- Around line 249-257: Grab the storage handle while the self.client read lock
is held and then drop the read guard before any awaits: call
self.client.read().await, get the Option client via client_guard.as_ref()? and
extract/clone the storage Arc via client.storage().clone() (or otherwise clone
the Arc returned by storage()), then drop the read guard (or let it go out of
scope) and only after that await storage_arc.lock().await and call
StorageManager::block_headers(&*storage) and
BlockHeaderStorage::get_tip(&*bh).await; this narrows the lifetime of the client
read lock and avoids holding it across await points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05550094-8c7f-4e6b-bc89-dbe60a4bc9ed
📒 Files selected for processing (9)
packages/rs-platform-wallet-ffi/src/shielded_sync.rspackages/rs-platform-wallet-ffi/src/spv.rspackages/rs-platform-wallet/src/spv/runtime.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rs-platform-wallet/src/wallet/platform_wallet.rs
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift
CI clippy gate (`cargo clippy --workspace --all-features --locked -- --no-deps -D warnings`) tripped after the new `shielded` feature on `rs-platform-wallet-ffi` / `rs-unified-sdk-ffi` made `--all-features` reach paths that were previously gated behind disabled feature combos. Three latent lints surfaced: - `manual_unwrap_or_default` on `try_queue_depth()` match - `unnecessary_cast` (`*reg_idx as u32` where `reg_idx` is u32) - `redundant_closure` (`.map(|info| addr_info_snapshot(info))`) Pure mechanical replacements; no behavior change.
…dStore concurrency Three issues raised on PR review: **`background_cancel` slot race.** `ShieldedSyncManager::start` spawned a worker thread whose cleanup unconditionally cleared `background_cancel` to None on exit. A tight `stop()` → `start()` reschedule had the prior thread overwrite the *new* generation's token, leaving the new loop running but unobservable via `is_running()` and unstoppable via `stop()`. Fix: bump a generation counter under the slot lock on every `start()`, and have the exiting thread skip the cleanup unless the active generation still matches its own. **`sync_wallet` bypassed exclusion.** The per-wallet on-demand entrypoint took only a read lock on `PlatformWallet.shielded` and ran independently of the manager's `is_syncing` flag, so a manual single-wallet sync could race the periodic `sync_now()` against the same `ShieldedWallet` / store and corrupt commitment-tree state. Reuse the same `compare_exchange` exclusion as `sync_now`; return `Ok(None)` when another pass is already in flight rather than serializing. **`save_note` double-counted on rescan.** Re-saving an already-known note (e.g. a cold-start trial-decrypt of the same chunk) appended a second `ShieldedNote` while overwriting the nullifier index. `get_unspent_notes` then returned both copies (double-counted balance) and `mark_spent` only flipped the second. Orchard nullifiers are globally unique, so an existing entry for the same nullifier means we already have the note — overwrite-in-place rather than append.
`bind(walletManager:walletId:network:resolver:)` swapped the target wallet id and re-attached the manager subscriptions, but left every per-wallet `@Published` field carrying the previous wallet's snapshot until a new sync event landed. After a wallet switch (or a failed rebind) the UI showed the wrong balance, counters, last-sync timer and orchard address for tens of seconds — or indefinitely if the new bind never produced a successful pass. Reset the per-wallet fields up front. Done inline instead of calling `reset()` because the manager subscriptions get re-attached just below — `reset()` would also nil out `walletManager` / `walletId` and require re-setting them.
`cargo machete` (running after the clippy gate in CI) flagged `rand = \"0.8\"` in `rs-sdk-ffi/Cargo.toml` as unused. The dep was only pulled in by the now-deleted `rs-sdk-ffi/src/shielded/` family — every remaining crate imports `rand` indirectly via dash-sdk where it's actually used.
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: "35258ccebf56c7f22db27a1e96cc60cff7da418932aea2c8c35d33045e4c8d3f"
)Xcode manual integration:
|
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 Orchard / ZK shielded code path was implemented twice and the
Swift app drove the wrong copy:
rs-platform-wallet/src/wallet/shielded/— feature-gatedShieldedWallet<S: ShieldedStore>, ZIP-32 derivation, sync,note-selection — written but never referenced outside its own
module (every type was tagged
#[allow(dead_code)]).rs-sdk-ffi/src/shielded/pool_client/— duplicateOrchardKeySet/ShieldedPoolStateplus a parallel syncimplementation. Used by Swift's
ShieldedPoolClient/ShieldedPoolService/NullifierSyncService. Took the first 32bytes of the wallet seed as the spending key directly (i.e. not
ZIP-32). Held the spending key in Swift memory.
Both violate the
swift-sdk/CLAUDE.md architectural
rule: "Swift SDK + FFI only persist/load/bridge; high-level flows go
through platform-wallet." On top of that,
SwiftExampleAppApp.initializeShieldedService()was a TODO stub thatnever ran, so on launch the user saw a permanent
Sync error: Invalid State: Shielded pool not initializedredbanner.
This PR consolidates around the architecturally-correct
platform-wallet implementation and wires Swift to it through a
thin manager-handle FFI translator. Mnemonic stays in iOS Keychain;
Rust pulls it via the existing
MnemonicResolverHandlepattern whenshielded keys need to be derived; only non-spending view keys
(FVK / IVK / OVK / default address) survive on the wallet handle.
What was done?
Demolition —
chore(swift-sdk,platform-wallet): rip duplicated Orchard FFI surfacedelete
rs-sdk-ffi/src/shielded/,rs-sdk-ffi/src/nullifier_sync/,swift-sdk/Sources/SwiftDashSDK/Shielded/, and the unusedZKSyncService.swift. Drop thespending_keyfield +#[allow(dead_code)]markers fromOrchardKeySet/ShieldedWallet. (~5k LOC removed.)Rust-side coordinator + FFI —
feat(platform-wallet,swift-sdk): Rust-side shielded sync coordinator + FFIFileBackedShieldedStoreimpl backing theShieldedStoretrait,per-network
ClientPersistentCommitmentTreeSQLite plus in-memorynotes.
PlatformWallet.shieldedfield +bind_shielded/shielded_syncmethods.ShieldedSyncManagermirroringPlatformAddressSyncManager(default 60 s cadence, dedicated OS thread for
!SendSDK futures).PlatformWalletManager::new/shutdownplus accessormethods. New
on_shielded_sync_completedevent + dispatcher.platform_wallet_manager_shielded_sync_*andplatform_wallet_manager_bind_shielded(resolver-driven).shieldedCargo feature on bothrs-platform-wallet-ffiand
rs-unified-sdk-ffi.Swift wiring —
feat(swift-sdk,swift-example-app): drive shielded sync from Rust managerSources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift:bindShielded,startShieldedSync,syncShieldedNow, etc.@Published shieldedSyncIsSyncing+lastShieldedSyncEventonPlatformWalletManager, withon_shielded_sync_completed_fnwired into the event-handler vtable.
ShieldedServicebecomes a real consumer:bind()runsbindShieldedagainst the keychain-backedMnemonicResolver,then subscribes to the manager's events.
SwiftExampleAppApp.bootstrapretires the TODO stub and binds /starts shielded sync alongside the platform-address sync.
build_ios.shpasses--features shieldedso the unifiedxcframework exposes the new symbols.
Polish — fast-sync UX —
fix(swift-example-app): clear shielded isSyncing after fast manual sync... clear platform-address isSyncing after fast manual syncempty-pool regtest syncs return inside ~25 ms, faster than the
manager's 1 Hz
isSyncingpoll cadence. The localisSyncingflag was set by
manualSyncbut only reset on error. Wrap eachbody with
defer { isSyncing = false }.Polish — counters + timer —
feat(swift-example-app): show last-sync timer + per-launch counters on shielded cardrelative
Last sync: N agoindicator plus per-launchScanned / New / Spent counter badges, mirroring the Platform Sync
Status card.
Address surface —
feat(swift-sdk,platform-wallet): expose Orchard default address for the bound walletPlatformWallet::shielded_default_address() -> Option<[u8; 43]>+platform_wallet_manager_shielded_default_addressFFI +PlatformWalletManager.shieldedDefaultAddress(walletId:) -> Data?Swift wrapper.
ShieldedService.bindruns the bytes throughDashAddress.encodeOrchardso the Receive sheet's Shielded tabrenders a usable bech32m address as soon as bind completes.
SPV tip block time —
feat(swift-sdk,platform-wallet): surface SPV chain-tip block timenew
SpvRuntime::tip_block_time()+platform_wallet_manager_spv_tip_unix_secondsFFI +
@Published var spvTipBlockTime: Date?on the manager (fedby the existing 1 Hz progress poll). CoreContentView's Core Sync
Status now renders a Block Time row with
<relative> agoplusthe wall-clock time, hidden when no tip is available. Doubles as
"is core producing blocks?" and "is SPV alive?" signal — a stale
stamp across polls means dashmate has stalled even though the
local SPV client is healthy.
How Has This Been Tested?
cargo check --workspace --exclude wasm-dpp --exclude wasm-sdk—default-feature build green.
cargo check -p rs-platform-wallet --features shieldedandcargo check -p rs-unified-sdk-ffi --features shielded—feature-on builds green.
cargo fmt --all.bash build_ios.sh --target sim --profile devregenerates theunified xcframework with
--features shieldedand buildsSwiftExampleApp end-to-end (
** BUILD SUCCEEDED **after everycommit).
dashmate node. Confirmed in logs and on screen:
Starting shielded note sync/Sync complete/Shielded sync finishedfires every 60 s (and on every Sync Now tap).address as soon as
bindShieldedruns.Time row update correctly.
empty-tree sync.
origin/v3.1-devmerges along the way, no conflicts.Spend operations (Send shielded) are intentionally still gated
behind a placeholder error — the
OrchardSpendSignertrait + spendFFI lands in a follow-up PR. UI surface for that path returns
"Shielded sending is being rebuilt — see follow-up PR". There isno Core → Shielded path in the SendViewModel either; the previous
ShieldFromAssetLockType-18 transition was demolished and willreturn alongside the spend signer.
Breaking Changes
None for the public Swift SDK API in steady state. The
rs-sdk-ffishielded surface (dash_sdk_shielded_*,dash_sdk_sync_nullifiers*) is removed wholesale; the only callerswere the now-deleted
Sources/SwiftDashSDK/Shielded/wrappers.Checklist:
🤖 Generated with Claude Code