feat(swift-sdk,platform-wallet): wire shielded send end-to-end (all 4 transitions)#3603
feat(swift-sdk,platform-wallet): wire shielded send end-to-end (all 4 transitions)#3603QuantumExplorer wants to merge 77 commits into
Conversation
…draw end-to-end
Shielded send was stubbed out behind a "rebuilt in follow-up PR"
placeholder for the four send flows even though
`ShieldedWallet::transfer` / `unshield` / `withdraw` already exist
on the Rust side and need only the bound shielded wallet's cached
`SpendAuthorizingKey` (no host signer). This commit threads them
through to the Swift Send sheet.
platform-wallet
- New `PlatformWalletError::ShieldedNotBound` so the wrapper can
distinguish "wallet has no shielded sub-wallet" from a build /
broadcast failure.
- New `PlatformWallet` wrappers under the existing `shielded`
feature: `shielded_transfer_to(recipient_raw_43, amount, prover)`,
`shielded_unshield_to(to_platform_addr_bytes, amount, prover)`,
`shielded_withdraw_to(to_core_address, amount, core_fee_per_byte,
prover)`. Each takes the prover by value because `OrchardProver`
is impl'd on `&CachedOrchardProver` (not the bare struct), and
forwards `&prover` into the underlying `ShieldedWallet` op.
Address parsing is inline — Orchard 43-byte raw → `PaymentAddress`,
bincode `PlatformAddress::from_bytes`, `dashcore::Address` from
string with network-match check.
platform-wallet-ffi
- New module `shielded_send` (feature-gated `shielded`):
- `platform_wallet_shielded_warm_up_prover()` —
fire-and-forget global, no manager handle.
- `platform_wallet_shielded_prover_is_ready()` — bool getter
for a UI affordance.
- `platform_wallet_manager_shielded_transfer/unshield/withdraw`
— manager-handle FFIs that resolve the wallet, instantiate
a `CachedOrchardProver`, and forward to the wallet wrappers
via `runtime().block_on(...)`.
swift-sdk
- New `PlatformWalletManager` async methods:
`shieldedTransfer(walletId:recipientRaw43:amount:)`,
`shieldedUnshield(walletId:toPlatformAddress:amount:)`,
`shieldedWithdraw(walletId:toCoreAddress:amount:coreFeePerByte:)`.
All run on a `Task.detached(priority: .userInitiated)` so the
~30 s first-call proof build doesn't block the main actor.
- Static helpers `PlatformWalletManager.warmUpShieldedProver()`
and `PlatformWalletManager.isShieldedProverReady`.
swift-example-app
- `SendViewModel.executeSend` gains a `walletManager` parameter
and replaces three of the four shielded placeholder branches
with the real FFI calls (Shielded → Shielded, Shielded →
Platform, Shielded → Core). The Platform → Shielded branch
retains a clearer placeholder because Type 15 still needs the
per-input nonce fetch the Rust spend builder stubs to zero.
- `SwiftExampleAppApp.bootstrap` kicks off
`warmUpShieldedProver()` on a background task at app start so
the first user-initiated shielded send doesn't pay the build
cost inline.
Verified:
- `cargo fmt --all`, `cargo clippy --workspace --all-features
--locked -- --no-deps -D warnings` clean.
- `bash build_ios.sh --target sim --profile dev` green
(** BUILD SUCCEEDED **).
The end-to-end story is still missing Platform → Shielded
(blocked on the spend builder's nonce TODO) and a host
`Signer<PlatformAddress>` adapter, plus the optional Type 18
`shield_from_asset_lock`. Wallets that already have shielded
balance can now move it freely.
|
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 feature-gated multi-account Orchard (shielded) support across FFI, core Rust, and Swift: new FFI modules and re-exports, multi-account ShieldedWallet and sync/store changes, per-subwallet persistence and restore plumbing, prover warm-up APIs, and Swift UI/wrapping and persistence model additions. (50 words) ChangesFFI ↔ PlatformWallet ↔ Swift (shielded public surface and wiring)
Shielded internals: multi-account, storage, sync, proofs
Sequence Diagram(s)sequenceDiagram
autonumber
participant SwiftApp as Swift App
participant FFI as rs-platform-wallet-ffi
participant Wallet as PlatformWallet (Rust)
participant Network as Platform Network
participant Prover as CachedOrchardProver
SwiftApp->>FFI: platform_wallet_manager_shielded_transfer(walletId, account, recipient, amount)
FFI->>Wallet: resolve_wallet(handle) & spawn worker task
Wallet->>Network: fetch AddressInfo for input addresses (nonces & balances)
Network-->>Wallet: address nonces & balances
Wallet->>Prover: request proving key / build proof
Prover-->>Wallet: proof result
Wallet->>Network: broadcast state transition
Network-->>Wallet: broadcast success / error (maybe AddressesNotEnoughFunds)
Wallet-->>FFI: return mapped PlatformWalletFFIResult
FFI-->>SwiftApp: return to caller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Review GateCommit:
|
|
✅ 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: "4976c511fb7492450afd824831ce2c4e71534be0f467fd1df77207ee4ccaf360"
)Xcode manual integration:
|
…pe 15) Completes the four shielded send flows by lighting up Type 15. The Rust spend pipeline already had `ShieldedWallet::shield` but stubbed every input's nonce to 0, which drive-abci rejected on broadcast. This commit: platform-wallet - `ShieldedWallet::shield` now fetches per-input nonces from Platform via `AddressInfo::fetch_many` and increments them before handing to `build_shield_transition`. Removes the long-standing `nonce=0` placeholder + TODO. - New `PlatformWallet::shielded_shield_from_account` helper with auto input selection: walks the chosen Platform Payment account's addresses in ascending derivation order and picks enough to cover `amount + 0.01 DASH` fee buffer (the on-chain fee comes off input 0 via `DeductFromInput(0)`). Returns `ShieldedInsufficientBalance` if the account total can't cover the request. rs-platform-wallet-ffi - New `platform_wallet_manager_shielded_shield(handle, wallet_id, account_index, amount, signer_address_handle)` in `shielded_send.rs`. Takes a `*mut SignerHandle` (Swift's `KeychainSigner.handle`) and casts to `&VTableSigner` — same shape `platform_address_wallet_transfer` uses, since `VTableSigner` already implements `Signer<PlatformAddress>`. swift-sdk - New async method `PlatformWalletManager.shieldedShield( walletId:accountIndex:amount:addressSigner:)`. Threads the `KeychainSigner` keepalive through the detached task the same way `topUpFromAddresses` does. swift-example-app - `SendViewModel.executeSend`'s `.platformToShielded` branch now constructs a `KeychainSigner` and calls `walletManager.shieldedShield(...)`. Replaces the last of the four shielded placeholder errors. The full Send Dash matrix is now real: | Source | Destination | Status | |------------|--------------|------------| | Core | Core | works | | Platform | Shielded | works (this PR) | | Shielded | Shielded | works | | Shielded | Platform | works | | Shielded | Core | works | Type 18 (`shield_from_asset_lock`) — direct Core L1 → Shielded without going through Platform first — is still unwired; tracked separately.
QuantumExplorer
changed the title
… restore + send credits at credits scale Two adjacent bugs that surfaced together when sending Platform → Shielded immediately after a fresh app launch: **`shielded_shield_from_account` reported `available 0`** even though the wallet detail showed 1.005 DASH on the Platform Payment account. `PlatformAddressWallet::initialize_from_persisted` was only seeding the *provider*'s `found` map — the source it hands to the SDK's incremental sync — but never pushing those balances into the in-memory `ManagedPlatformAccount.address_balances` map. Spend paths that enumerate funded addresses (`shielded_shield_from_account`, `PlatformAddressWallet::addresses_with_balances`, `account.address_credit_balance`) all read from `address_balances`, so they returned 0 until the first BLAST sync finished and `provider::on_address_found` repopulated it. Walk `persisted.per_account` at restore time and call `set_address_credit_balance(addr, balance, None)` on the matching `ManagedPlatformAccount` for each entry, mirroring the same `apply_changeset` path the steady-state sync writes through. New public accessor `PerAccountPlatformAddressState::persisted_balances()` exposes the iteration without leaking the inner `found` map. **Send screen sent at duffs scale.** `SendViewModel.amount` unconditionally multiplied the typed DASH value by 1e8 (L1 duffs). Right for `coreToCore` but wrong for the four flows that touch the credits ledger (1 DASH = 1e11), which underpaid by 1000×. Typing 0.5 DASH for a Platform → Shielded shield turned into 50_000_000 credits (~0.0005 DASH) on the wire — error-message gave it away as `required 1050000000 = amount + fee_buffer`. Split into `amountDuffs` and `amountCredits`. `executeSend` picks `amountCredits` for `shieldedToShielded`, `shieldedToPlatform`, `shieldedToCore`, `platformToShielded`; `coreToCore` still uses `amountDuffs`. The legacy `amount` property aliases `amountDuffs` so any caller that hadn't been audited still gets Core-correct semantics. Verified: `cargo clippy --workspace --all-features --locked -- --no-deps -D warnings` clean, `bash build_ios.sh --target sim --profile dev` green.
Halo 2 circuit synthesis recurses past the ~512 KB iOS dispatch-thread stack and crashes with EXC_BAD_ACCESS on the first `synthesize(config.clone(), V1Pass::<_, CS>::measure(pass))?` call when the future is polled directly on the calling thread. Switch the four shielded spend FFI entry points (transfer/unshield/withdraw/shield) from `runtime().block_on(...)` to `block_on_worker(...)` so the proof runs on a tokio worker with the configured 8 MB stack — the exact case `runtime.rs` was set up for. For `shield`, transmute the borrowed `&VTableSigner` to `&'static` inside the FFI call: the caller retains ownership of the signer handle and we block until the worker future completes, so the painted lifetime never actually escapes the call. `VTableSigner` is `Send + Sync` per its `unsafe impl` in rs-sdk-ffi, so the resulting reference is `Send + 'static` — exactly what `block_on_worker` needs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…adcast failure
`AddressesNotEnoughFundsError` from drive-abci already carries
`addresses_with_info: BTreeMap<PlatformAddress, (AddressNonce, Credits)>`
— Platform's actual per-address nonce and remaining balance after
the bundle's `DeductFromInput(0)` strategy deducts the shield
amount. Stringifying with `e.to_string()` discarded everything but
`required_balance` (the fee), leaving the host with no way to tell
*which* input fell short or whether the local-cache balance
disagreed with Platform.
Pattern-match the broadcast `dash_sdk::Error` for the structured
consensus error (via `Error::Protocol(ProtocolError::ConsensusError)`
or `Error::StateTransitionBroadcastError { cause }`), then format
both the local claim list and Platform's view side-by-side. Add a
per-input `tracing::info!`/`warn!` before broadcast so the same
data is visible in logs even on success — and hosts can spot
local-cache drift by comparing claimed_credits vs platform_balance.
Co-Authored-By: Claude Opus 4.7 (1M context) <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: 1
🧹 Nitpick comments (5)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift (2)
407-438: 💤 Low valueConsider validating
toCoreAddressis non-empty.Other methods explicitly reject empty inputs; an empty
toCoreAddresshere would be passed straight towithCStringand across the FFI as a zero-length C string. Rust will reject it, but a host-side guard produces a clearer error and avoids the detached-task hop.♻️ Suggested guard
guard walletId.count == 32 else { throw PlatformWalletError.invalidParameter( "walletId must be exactly 32 bytes" ) } + guard !toCoreAddress.isEmpty else { + throw PlatformWalletError.invalidParameter( + "toCoreAddress is empty" + ) + }🤖 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/PlatformWalletManagerShieldedSync.swift` around lines 407 - 438, Add a precondition that toCoreAddress is non-empty in shieldedWithdraw before spawning the detached Task: check that toCoreAddress.isEmpty == false and if empty throw PlatformWalletError.invalidParameter with a clear message like "toCoreAddress must be non-empty"; place this validation alongside the existing walletId and handle guards at the top of the function so the invalid input is rejected on the host side rather than passed into withCString/platform_wallet_manager_shielded_withdraw.
374-378: 💤 Low valueTighter validation on
toPlatformAddresswould catch host-side mistakes earlier.The doc says the address is "bincode-encoded
PlatformAddress—0x00 ‖ 20-byte hashfor P2PKH", which implies a 21-byte payload for P2PKH. The current guard only rejects empty buffers, so a malformed length (e.g. raw 20-byte hash without the discriminant byte) gets passed to FFI and produces a less-actionable error from Rust. Consider rejecting clearly invalid lengths up-front.♻️ Suggested validation
- guard !toPlatformAddress.isEmpty else { - throw PlatformWalletError.invalidParameter( - "toPlatformAddress is empty" - ) - } + // Bincode-encoded `PlatformAddress`: 1-byte discriminant + payload. + // P2PKH today is `0x00 ‖ 20 bytes` = 21 bytes. Reject anything that + // can't possibly be a valid encoding before crossing the FFI boundary. + guard toPlatformAddress.count >= 2 else { + throw PlatformWalletError.invalidParameter( + "toPlatformAddress is too short to be a bincode PlatformAddress" + ) + }🤖 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/PlatformWalletManagerShieldedSync.swift` around lines 374 - 378, The guard that only rejects an empty toPlatformAddress is too weak; in PlatformWalletManagerShieldedSync validate the bincode-encoded PlatformAddress length (for P2PKH expect exactly 21 bytes: leading discriminant 0x00 + 20-byte hash) before calling the FFI. Replace the empty check on parameter toPlatformAddress with a length check and throw PlatformWalletError.invalidParameter with a clear message like "toPlatformAddress must be 21 bytes (0x00 || 20-byte hash)" when the size is invalid so malformed 20-byte raw hashes are rejected earlier.packages/rs-platform-wallet/src/wallet/shielded/operations.rs (1)
73-90: 💤 Low valueHoist the
usestatements to module scope.The three
useimports (FetchMany,AddressInfo,BTreeSet) are placed inside the function body. While valid, this departs from the existing pattern in this file (all other imports are at the top). Moving them up keeps the import surface discoverable.♻️ Proposed move
@@ top of file use std::collections::BTreeMap; +use std::collections::BTreeSet; use dash_sdk::platform::transition::broadcast::BroadcastStateTransition; +use dash_sdk::platform::FetchMany; +use dash_sdk::query_types::AddressInfo; @@ inside shield() - // Fetch the current address nonces from Platform. Each - // input address has a per-address nonce that the next - // state transition must use as `last_used + 1`. - // ... - use dash_sdk::platform::FetchMany; - use dash_sdk::query_types::AddressInfo; - use std::collections::BTreeSet; - let address_set: BTreeSet<PlatformAddress> = inputs.keys().copied().collect();🤖 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 73 - 90, Move the three local imports used in this block up to module scope to match the file's import pattern: remove the in-function uses of FetchMany, AddressInfo, and BTreeSet and add them to the top-level use statements for the module so callers like AddressInfo::fetch_many(&self.sdk, ...) and references to PlatformAddress and inputs.keys() keep working; ensure you import dash_sdk::platform::FetchMany, dash_sdk::query_types::AddressInfo, and std::collections::BTreeSet at the file top and then delete the redundant in-function use lines in operations.rs.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift (2)
116-118: 💤 Low value
canSendkeys offamountDuffseven for credits-based flows.Today
amountDuffs != nil⇔amountCredits != nilbecause both parsers gate on the sameDouble > 0predicate, so this is correct in practice. It will silently break the moment one parser gains stricter validation (e.g., theDecimalswitch suggested elsewhere, or an upper-bound check). Consider keying off the right unit perdetectedFlowso the invariant is local rather than implicit.🤖 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/ViewModels/SendViewModel.swift` around lines 116 - 118, The computed property canSend currently checks amountDuffs regardless of flow; change it to validate the correct amount field based on detectedFlow (e.g., if detectedFlow indicates a credits-based flow require amountCredits != nil, otherwise require amountDuffs != nil) while still checking detectedFlow != nil and !isSending; locate and update the canSend property and use detectedFlow’s discriminator (enum case or helper like isCredits) to pick the right amount variable (amountCredits or amountDuffs) so the invariant is explicit and local.
92-108: ⚡ Quick winConsider parsing amounts via
Decimalto avoid float rounding.
Double(amountString) * 100_000_000_000is binary-floating-point multiplication, so user-friendly inputs that aren't representable in base-2 quietly truncate by one or more credit. Example:"1.23"⇒Double≈1.2299999999999999⇒* 1e11 ≈ 122999999999.99998⇒UInt64(...)⇒122999999999(intent:123_000_000_000). For credits this is a one-credit dust loss per send; for any amount whose decimal string has >15.95 significant digits the rounding gets larger.♻️ Decimal-based parsing
- var amountDuffs: UInt64? { - guard let double = Double(amountString), double > 0 else { return nil } - return UInt64(double * 100_000_000) - } + var amountDuffs: UInt64? { + guard let dash = Decimal(string: amountString), dash > 0 else { return nil } + let duffs = (dash * Decimal(100_000_000)) as NSDecimalNumber + return duffs.uint64Value + } @@ - var amountCredits: UInt64? { - guard let double = Double(amountString), double > 0 else { return nil } - return UInt64(double * 100_000_000_000) - } + var amountCredits: UInt64? { + guard let dash = Decimal(string: amountString), dash > 0 else { return nil } + let credits = (dash * Decimal(100_000_000_000)) as NSDecimalNumber + return credits.uint64Value + }(Optionally round explicitly via
NSDecimalNumberHandlerif you want banker's rounding rather thanuint64Value's default.)🤖 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/ViewModels/SendViewModel.swift` around lines 92 - 108, The parsing in amountDuffs and amountCredits uses Double which causes binary-floating rounding loss; update both computed properties (amountDuffs and amountCredits) to parse amountString with Decimal (or NSDecimalNumber), perform the multiplication using Decimal (100_000_000 and 100_000_000_000 respectively), then convert to UInt64 using an explicit rounding mode (via NSDecimalNumberHandler or Decimal's rounded(_:)) to avoid off-by-one dust; keep the same guard for positive values and return nil on parse failure.
🤖 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/wallet/shielded/operations.rs`:
- Around line 91-103: The code currently treats a None AddressInfo as an error;
change handling so that when infos.get(&addr) returns Some(None) (proof of
absence) you treat the starting nonce as 0 and compute nonce = 0 + 1, instead of
failing; keep an error only if the map lacks the key entirely (infos.get(&addr)
is None). Concretely, update the inputs loop that populates inputs_with_nonce
(and the lookup of infos.get(&addr) / info.nonce) to accept opt.as_ref().map(|i|
i.nonce).unwrap_or(0) and then insert (nonce + 1, credits) for that addr;
preserve the PlatformWalletError only for a truly missing map entry.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- Around line 73-90: Move the three local imports used in this block up to
module scope to match the file's import pattern: remove the in-function uses of
FetchMany, AddressInfo, and BTreeSet and add them to the top-level use
statements for the module so callers like AddressInfo::fetch_many(&self.sdk,
...) and references to PlatformAddress and inputs.keys() keep working; ensure
you import dash_sdk::platform::FetchMany, dash_sdk::query_types::AddressInfo,
and std::collections::BTreeSet at the file top and then delete the redundant
in-function use lines in operations.rs.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift`:
- Around line 407-438: Add a precondition that toCoreAddress is non-empty in
shieldedWithdraw before spawning the detached Task: check that
toCoreAddress.isEmpty == false and if empty throw
PlatformWalletError.invalidParameter with a clear message like "toCoreAddress
must be non-empty"; place this validation alongside the existing walletId and
handle guards at the top of the function so the invalid input is rejected on the
host side rather than passed into
withCString/platform_wallet_manager_shielded_withdraw.
- Around line 374-378: The guard that only rejects an empty toPlatformAddress is
too weak; in PlatformWalletManagerShieldedSync validate the bincode-encoded
PlatformAddress length (for P2PKH expect exactly 21 bytes: leading discriminant
0x00 + 20-byte hash) before calling the FFI. Replace the empty check on
parameter toPlatformAddress with a length check and throw
PlatformWalletError.invalidParameter with a clear message like
"toPlatformAddress must be 21 bytes (0x00 || 20-byte hash)" when the size is
invalid so malformed 20-byte raw hashes are rejected earlier.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift`:
- Around line 116-118: The computed property canSend currently checks
amountDuffs regardless of flow; change it to validate the correct amount field
based on detectedFlow (e.g., if detectedFlow indicates a credits-based flow
require amountCredits != nil, otherwise require amountDuffs != nil) while still
checking detectedFlow != nil and !isSending; locate and update the canSend
property and use detectedFlow’s discriminator (enum case or helper like
isCredits) to pick the right amount variable (amountCredits or amountDuffs) so
the invariant is explicit and local.
- Around line 92-108: The parsing in amountDuffs and amountCredits uses Double
which causes binary-floating rounding loss; update both computed properties
(amountDuffs and amountCredits) to parse amountString with Decimal (or
NSDecimalNumber), perform the multiplication using Decimal (100_000_000 and
100_000_000_000 respectively), then convert to UInt64 using an explicit rounding
mode (via NSDecimalNumberHandler or Decimal's rounded(_:)) to avoid off-by-one
dust; keep the same guard for positive values and return nil on parse failure.
🪄 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: 013dd0c5-e525-4073-932a-7dd9f3ff4d1e
📒 Files selected for processing (11)
packages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/platform_addresses/provider.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift
The shield transition uses `DeductFromInput(0)` as its fee strategy, which drive-abci interprets as "after each input has had its claim deducted, take the fee out of input 0's *remaining* balance" (see the doc comment on `deduct_fee_from_outputs_or_remaining_balance_of_inputs_v0` in rs-dpp). "Input 0" is the BTreeMap-smallest key. The previous selection code claimed the full balance of every picked input, so every input's remaining was 0, and `DeductFromInput(0)` had nothing to bite into. Platform rejected the broadcast with `AddressesNotEnoughFundsError` showing "total available is less than required <fee>". Sort candidates by address bytes (BTreeMap order), skip leading dust addresses whose balance can't reserve the fee buffer (so the next funded address becomes the bundle's input 0), then claim only what's needed to cover `amount` — capping input 0's claim at `balance - FEE_RESERVE_CREDITS` so its post-claim remaining stays ≥ FEE_RESERVE for the network's fee deduction step. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR wires the four shielded send flows end-to-end. One blocking issue: the Shielded→Platform path passes bech32m-payload bytes (type byte 0xb0/0x80) to a Rust entry point that decodes via bincode (expects 0x00/0x01), so unshield will fail to decode any real platform recipient. Several real suggestions around nonce overflow, fee-reserve fallback selection, and the unusual &'static transmute. 4 lower-priority findings dropped.
Reviewed commit: 6c72239
🔴 1 blocking | 🟡 7 suggestion(s) | 💬 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift`:
- [BLOCKING] lines 240-249: Shielded→Platform sends pass the wrong platform-address byte format to Rust
`DashAddress.parse` returns the 21-byte bech32m payload — the type byte is 0xb0 (P2PKH) or 0x80 (P2SH) per `PlatformAddress::to_bech32m_string` in `packages/rs-dpp/src/address_funds/platform_address.rs:222-242`. Those bytes are passed straight through to `platform_wallet_manager_shielded_unshield`, which calls `PlatformAddress::from_bytes` (`platform_wallet.rs:413`). `from_bytes` is bincode-decoded and expects the storage variant index — 0x00 for P2pkh, 0x01 for P2sh (see the test at `platform_address.rs:1386-1387` and the explicit `to_bytes`/`from_bytes` doc-comments at 311-319/333-337). So a normal user-entered address fails to decode and the unshield broadcast can't proceed. The fix is to either translate the type byte at the Swift→Rust boundary (0xb0→0x00, 0x80→0x01), or to expose an FFI entry point that accepts the bech32m-encoded string and goes through `PlatformAddress::from_bech32m_string` instead.
- [SUGGESTION] lines 221-242: shielded→shielded and shielded→platform branches re-parse the untrimmed recipient text
`detectAddressType()` (line 153) trims whitespace before calling `DashAddress.parse`, so a pasted address with a trailing newline is recognised and the send button is enabled. The shielded→shielded branch (lines 221) and shielded→platform branch (line 240) then re-parse `recipientAddress` without trimming, so the same input hits a `Recipient is not …` error at submit time. The Core branch (line 205) and Shielded→Core branch (line 261) already trim — these two should match.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] line 167: Address nonce increment can wrap silently at u32::MAX
`AddressNonce` is a `u32`, and `info.nonce + 1` on the line that builds `inputs_with_nonce` will panic in debug and wrap to 0 in release once an address reaches the ceiling. Drive treats `u32::MAX` as exhausted, so wrapping submits a transition with nonce 0 — drive-abci then rejects it as a replay, after the wallet has spent ~30 s building the Halo 2 proof. Practically unreachable today, but a `checked_add(1).ok_or(PlatformWalletError::ShieldedBuildError(...))` keeps the failure mode legible and matches the conservative style used elsewhere in this crate.
- [SUGGESTION] lines 117-145: Reimplements rs-sdk's canonical address-nonce fetch instead of reusing it
rs-sdk has `fetch_inputs_with_nonce`, `nonce_inc`, and `ensure_address_balance` in `packages/rs-sdk/src/platform/transition/address_inputs.rs:12-40` that encapsulate exactly this fetch-and-increment dance plus a hard balance check. They are `pub(crate)` today, so platform-wallet can't reach them directly, but a single-line visibility change would let this code re-use the canonical helpers. As written, the new shield path will silently drift from the SDK's behaviour — for example the SDK enforces a balance check that this implementation only `warn!`s on.
- [SUGGESTION] lines 108-168: Concurrent shields on the same wallet TOCTOU on the fetched address nonce
Nonces are fetched via `AddressInfo::fetch_many`, incremented locally, then handed to the builder. Two concurrent calls to `ShieldedWallet::shield` for the same wallet (e.g. user double-taps Send, or app retries while the first is still proving) both observe the same `info.nonce`, both build with `info.nonce + 1`, and the second to land at drive-abci is rejected with a nonce conflict. Not exploitable, but produces an opaque user-facing failure after a ~30 s proof. Either serialise shield-class operations on a per-wallet mutex inside `ShieldedWallet`, or document at the FFI boundary that hosts must enforce single-flight.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [SUGGESTION] lines 553-556: Fall-through input selection can pick a tiny address as input 0 with no real fee headroom
When no candidate has `balance > FEE_RESERVE_CREDITS`, `viable_input_0` falls through to 0 and `usable` becomes the entire candidate slice. The total-balance check still requires `total_usable >= amount + FEE_RESERVE_CREDITS`, so practical broadcasts usually still succeed — actual mempool fees on Type 15 are ~20M credits, well below any candidate that can contribute. But in pathological dust scenarios (every funded address holds < actual fee) the chosen input 0's remaining balance can be smaller than the fee, and the broadcast will fail only after the ~30 s proof. Since the comment at lines 547-552 already acknowledges this case will be rejected by the network, it's cheaper to short-circuit here with `ShieldedInsufficientBalance { available: total_usable, required: amount + FEE_RESERVE_CREDITS }` when no candidate exceeds the reserve, instead of producing a bundle that's known to be on the boundary.
- [SUGGESTION] lines 469-610: shielded_shield_from_account selection logic has no Rust unit coverage
`shielded_shield_from_account` carries non-trivial selection rules that directly determine whether shield broadcasts succeed: skipping leading addresses below `FEE_RESERVE_CREDITS`, reserving fee headroom only on input 0, walking BTreeMap order, and accumulating to `amount`. None of this is covered by a focused Rust test, so a future refactor can reintroduce the original `viable_input_0`-style failure without tripping CI. Worth a deterministic unit test against a synthetic managed account covering: dust-first-address case, exact-reserve case, and amount-equal-to-total case.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] lines 268-271: Use the established usize round-trip pattern instead of transmuting the signer borrow to 'static
`block_on_worker` requires `F: 'static`, and the new shield path satisfies this with `mem::transmute::<&VTableSigner, &'static VTableSigner>(...)`. It is sound today only because `block_on_worker` (`runtime.rs`) parks on the spawned future to completion — any future change that lets it return early (timeout, cancellation, shutdown select!) silently turns this into a use-after-free. Other call sites in this crate (e.g. `identity_top_up.rs:117-122`) solve the same `Send + 'static` constraint by round-tripping the signer pointer through `usize` and re-materializing the `&VTableSigner` *inside* the future, which captures only `Send + 'static` data and avoids the lifetime fiction entirely. Aligning the shield path to that pattern would remove a sharp edge from the FFI surface at zero behavioural cost.
- unshield FFI now takes the bech32m string and parses Rust-side via `PlatformAddress::from_bech32m_string`, with a network check. The previous byte-based path passed the 21-byte bech32m payload (type byte 0xb0/0x80) into bincode `from_bytes`, which expects the storage variant tag 0x00/0x01 and rejected real user-entered addresses (thepastaclaw c8873f6312ef). - shield: nonce increment now `checked_add(1)` so a u32 wrap surfaces as `ShieldedBuildError` instead of replaying with nonce 0 after a 30 s proof (cb50b774985e). - shield input selection: when no candidate clears FEE_RESERVE_CREDITS, fail fast with `ShieldedInsufficientBalance` instead of producing a known-boundary bundle (2b28ee4ac2f4). - SendViewModel: trim recipient in the shielded→shielded and shielded→platform branches (68c36dcd4fe0). Forward the trimmed bech32m string to `shieldedUnshield` directly — the Swift side no longer extracts payload bytes. - format_addresses_with_info now renders via `to_bech32m_string` and takes the wallet's network — diagnostics match what the UI shows so log greps line up (6b82603320bd). - platform_wallet_shielded_warm_up_prover dispatches the build via `runtime().spawn_blocking(...)` so it actually returns immediately as the doc claims (a575d0f7eb0f). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/wallet/platform_wallet.rs`:
- Around line 377-459: The three new methods (shielded_transfer_to,
shielded_unshield_to, shielded_withdraw_to) call ShieldedWallet::{transfer,
unshield, withdraw} which still rely on the shared spend helper that errors out
with "Spending operations require a ShieldedStore that provides MerklePath
witnesses. Not yet implemented."; fix by wiring/implementing a ShieldedStore
that returns MerklePath witnesses (or update the shared spend helper to support
a witness-less code path), ensuring the store used by ShieldedWallet (the guard
in self.shielded) implements the witness provider used during note selection so
the calls from shielded_transfer_to / shielded_unshield_to /
shielded_withdraw_to succeed at runtime.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift`:
- Around line 92-118: The amount parsers currently accept any positive Double
and truncate to UInt64, but canSend only checks amountDuffs so tiny values
become zero after scaling or the wrong unit is validated for shielded flows;
update amountDuffs and amountCredits to parse the Double, compute the scaled
UInt64 and only return it if the scaled integer is > 0 (so UInt64(double *
scale) must be > 0), then replace the canSend check to use the active unit based
on detectedFlow (use amountDuffs for Core flows and amountCredits for
Platform/shielded flows) — reference the existing computed properties
amountDuffs, amountCredits, amount (shim) and the canSend and detectedFlow logic
when making the change.
🪄 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: db812a17-166a-4e0c-a217-a3f4b8cf1387
📒 Files selected for processing (5)
packages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift
…ieldedStore
`extract_spends_and_anchor` returned `ShieldedBuildError("Spending
operations require a ShieldedStore that provides MerklePath
witnesses. Not yet implemented.")` for every note, so shielded
transfer / unshield / withdraw failed at runtime even when the
store had a real commitment tree. The persistent tree's
`ClientPersistentCommitmentTree::witness(position, depth) ->
Option<MerklePath>` was already available — the trait was just
sitting on a `Vec<u8>` placeholder.
Change `ShieldedStore::witness()` to return
`Result<Option<MerklePath>, _>` directly, wire
`FileBackedShieldedStore::witness` through
`tree.witness(Position::from(position), 0)` (depth 0 matches the
`tree_anchor()` that the same builder consumes), and have
`extract_spends_and_anchor` build real `SpendableNote { note,
merkle_path }` entries.
Side effects (deliberate):
- `InMemoryShieldedStore::witness` keeps its existing `Err`; that
store has no tree state, only a flat `Vec<[u8; 32]>` of
commitments. Spend paths require a real store.
- Trait module-doc was updated: the "no orchard types" claim was
already partially false (notes deserialize to `orchard::Note` at
the call site) and is now plainly false.
Tests: 11 existing shielded unit tests pass; clippy clean; iOS
xcframework + SwiftExampleApp rebuild succeeds.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`dbPath(for:)` was keyed only on network, so two wallets on the
same network bound `bind_shielded` to the *same* SQLite file.
`FileBackedShieldedStore`'s notes table has no `wallet_id`
column, so `store.get_unspent_notes()` returned every wallet's
notes — wallet B saw wallet A's shielded balance under its own
name even though B's seed (and FVK) is unrelated.
User reproduced this with two wallets on regtest, distinct
mnemonics: a freshly created Wallet2 with empty Core/Platform
balances reported the same 0.6 DASH shielded balance as the
funded Reg wallet.
Include the wallet id hex in the dbPath. Each wallet now has
its own commitment-tree file and will re-sync from genesis on
first bind. Per project memory ("pre-release: schema migrations
aren't a concern; dev DBs rebuild"), the resulting one-time
re-sync is acceptable. Long-term the right fix is to add a
`wallet_id` column to the notes table inside `FileBackedShieldedStore`
so wallets can share the tree but filter their own notes; that's
a bigger change tracked separately.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… detail `ShieldedService` is a singleton bound by `rebindWalletScopedServices()` to `walletManager.firstWallet`. The detail-view code path never re-bound it, so opening any wallet other than `firstWallet` showed `firstWallet`'s shielded balance under the wrong wallet's name. The previous per-wallet dbPath fix correctly isolated each wallet's notes in Rust, but the published `shieldedBalance` on the UI side stayed pinned to the first-bound wallet. `ShieldedService` now stashes `walletManager` / `resolver` / `network` on first `bind(...)` and exposes `switchTo(walletId:)` that reuses them — cheap and idempotent (the Rust-side `bind_shielded` already replaces its slot). `WalletDetailView` calls it from `.onAppear` and `.onChange(of: wallet.walletId)`, and grew the `@EnvironmentObject var shieldedService` it was missing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift (2)
187-208: 💤 Low valueConsider clearing stashed
network/resolverinreset()for symmetry.
reset()(lines 241-258) nils outwalletManagerandwalletIdbut leaves the newnetworkandresolverfields populated. It's not a correctness bug today sinceswitchTo(walletId:)guards onwalletManagerbeing non-nil before using them, but the asymmetry will be a footgun if a future change re-checks any of these fields independently ofwalletManager.♻️ Proposed tweak
func reset() { syncStateCancellable?.cancel() syncEventCancellable?.cancel() walletManager = nil walletId = nil + network = nil + resolver = nil isSyncing = false🤖 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 187 - 208, The reset() method currently nils walletManager and walletId but leaves network and resolver set, creating asymmetry with switchTo(walletId:) which relies on those being cleared only when walletManager is nil; update reset() to also clear the network and resolver properties (nil out network and resolver) so that reset() fully clears state and remains symmetric with the guard in switchTo(walletId:), ensuring bind(walletManager:walletId:network:resolver:) cannot be called with stale network/resolver values.
290-308: 💤 Low valueStale per-network DB files from prior installs are left behind.
The path scheme changed from
shielded_tree_<network>.sqlitetoshielded_tree_<network>_<walletHex>.sqlite. Existing users upgrading will keep the old per-network file orphaned in the documents directory forever (it's no longer referenced by any wallet). Low-impact disk leak but worth either a one-time cleanup pass or a brief note in the comment so it isn't forgotten.🤖 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 290 - 308, Existing per-network DB files named "shielded_tree_<network>.sqlite" are orphaned after switching to dbPath(for:network:walletId:) which names files "shielded_tree_<network>_<walletHex>.sqlite"; add a one-time cleanup to remove legacy files (or at least document it) by detecting the old filename pattern and deleting any matching file before/when creating the per-wallet DB. Implement this cleanup in the same initialization flow that opens/creates the shielded DB (e.g., in FileBackedShieldedStore init or just inside dbPath caller) so you remove "shielded_tree_\(network.networkName).sqlite" if present, and then proceed to return the new per-wallet path.packages/rs-platform-wallet/src/wallet/shielded/file_store.rs (1)
163-175: ⚡ Quick winUpdate the module docs to reflect that witness generation is live.
This implementation makes the header note at Lines 13-15 stale. Leaving "not implemented yet" in the file docs will send future debugging in the wrong direction.
🤖 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/file_store.rs` around lines 163 - 175, Update the module-level documentation to remove the "not implemented yet" note and instead state that witness generation is implemented and live; mention that the FileShieldedStore::witness method locks the tree (tree.lock), converts the position with Position::from, and calls tree.witness with checkpoint_depth = 0 (producing a grovedb_commitment_tree::MerklePath) and that errors are wrapped in FileShieldedStoreError, so future readers know how witnesses are produced and where to look for failures.
🤖 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.
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- Around line 163-175: Update the module-level documentation to remove the "not
implemented yet" note and instead state that witness generation is implemented
and live; mention that the FileShieldedStore::witness method locks the tree
(tree.lock), converts the position with Position::from, and calls tree.witness
with checkpoint_depth = 0 (producing a grovedb_commitment_tree::MerklePath) and
that errors are wrapped in FileShieldedStoreError, so future readers know how
witnesses are produced and where to look for failures.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- Around line 187-208: The reset() method currently nils walletManager and
walletId but leaves network and resolver set, creating asymmetry with
switchTo(walletId:) which relies on those being cleared only when walletManager
is nil; update reset() to also clear the network and resolver properties (nil
out network and resolver) so that reset() fully clears state and remains
symmetric with the guard in switchTo(walletId:), ensuring
bind(walletManager:walletId:network:resolver:) cannot be called with stale
network/resolver values.
- Around line 290-308: Existing per-network DB files named
"shielded_tree_<network>.sqlite" are orphaned after switching to
dbPath(for:network:walletId:) which names files
"shielded_tree_<network>_<walletHex>.sqlite"; add a one-time cleanup to remove
legacy files (or at least document it) by detecting the old filename pattern and
deleting any matching file before/when creating the per-wallet DB. Implement
this cleanup in the same initialization flow that opens/creates the shielded DB
(e.g., in FileBackedShieldedStore init or just inside dbPath caller) so you
remove "shielded_tree_\(network.networkName).sqlite" if present, and then
proceed to return the new per-wallet path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c749d03-a814-4258-bdef-f71935e6ec37
📒 Files selected for processing (5)
packages/rs-platform-wallet/src/wallet/shielded/file_store.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/store.rspackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
…n B)
Refactor shielded internals so a single PlatformWallet can hold
multiple ZIP-32 Orchard accounts that share the network's
commitment tree but keep their decrypted notes / nullifiers /
sync watermarks scoped per-(wallet_id, account_index).
This replaces the per-wallet shielded SQLite path that was
shipped earlier — that change isolated wallets at the cost of a
duplicate tree per wallet, and didn't help with same-wallet
multi-account at all. The on-chain commitment stream is
chain-wide, so the tree should be too.
## What changes
**`ShieldedStore` trait** (rs-platform-wallet):
- New `SubwalletId { wallet_id: [u8; 32], account_index: u32 }`.
- Note + sync-state methods (`save_note`, `get_unspent_notes`,
`mark_spent`, `last_synced_note_index`,
`nullifier_checkpoint`, …) take `id: SubwalletId`. Tree
methods (`append_commitment`, `checkpoint_tree`,
`tree_anchor`, `witness`) stay scope-free.
- `InMemoryShieldedStore` and `FileBackedShieldedStore` now hold
a `BTreeMap<SubwalletId, SubwalletState>` and lazily allocate
per-subwallet entries.
**`ShieldedWallet`**:
- Holds `accounts: BTreeMap<u32, AccountState>` (per-account
keyset). New constructors `from_keysets`, `from_seed_accounts`;
`add_account_from_seed` for live add. New `account_indices`,
`keys_for(account)`, `default_address(account)`,
`balance(account)`, `balances`, `balance_total`. Per-wallet
`wallet_id` field threaded through every store call as
`SubwalletId`.
**Sync** (`shielded/sync.rs`):
- One sync pass covers every bound account: fetch raw chunks
via `sync_shielded_notes` once with the lowest-keyed
account's IVK, then locally trial-decrypt each chunk with
every other account's IVK via `dash_sdk::platform::shielded::
try_decrypt_note`. Append each cmx to the shared tree once
with `marked = (any account decrypted this position)`.
- `SyncNotesResult` and `ShieldedSyncSummary` carry per-account
maps; `total_new_notes`, `total_newly_spent`, `balance_total`
helpers fold them for the flat FFI surface.
**Operations** (`shielded/operations.rs`):
- `transfer`, `unshield`, `withdraw`, `shield`, `shield_from_asset_lock`
all take `account: u32` and route through the corresponding
`OrchardKeySet` and per-subwallet note set. Spends never
cross account boundaries.
**`PlatformWallet`**:
- `bind_shielded(seed, accounts: &[u32], db_path)` derives all
listed accounts at once. New `shielded_add_account(seed,
account)` for live add (with a docstring caveat that
historical retroactive marking requires a tree wipe + resync).
- `shielded_default_address(account)`, `shielded_balances()`,
`shielded_account_indices()`, plus the four spend helpers
(`shielded_transfer_to`, `shielded_unshield_to`,
`shielded_withdraw_to`, `shielded_shield_from_account`) all
take `account: u32`.
- `shielded_shield_from_account` now takes both
`shielded_account` and `payment_account` — they're distinct
concepts (Orchard recipient account vs Platform Payment funding
account) that previously shared one `account_index` parameter.
**FFI** (`rs-platform-wallet-ffi`):
- `platform_wallet_manager_bind_shielded` takes
`accounts_ptr: *const u32, accounts_len: usize` (1..=64).
- All four spend entry points + `shielded_default_address` take
`account: u32`. `shielded_shield` takes both
`shielded_account` and `payment_account`.
- `ShieldedSyncWalletResultFFI::ok` flattens per-account sums.
**Swift SDK + example app**:
- `bindShielded` takes `accounts: [UInt32] = [0]`; passes the
C buffer through.
- All shielded send wrappers take `account: UInt32 = 0`.
- `shieldedDefaultAddress(walletId:account:)` per-account.
- `ShieldedService.dbPath(for:network:)` reverts to per-network
(the per-(wallet,network) workaround is no longer needed —
notes are scoped at the column level inside the store).
## Persistence (deferred)
This commit ships the multi-account refactor with notes still
held only in memory (`Vec<ShieldedNote>` on `SubwalletState`).
Cold start = re-sync from genesis, same as before. SwiftData
persistence (`PersistentShieldedNote` keyed by
`(walletId, accountIndex, position)` driven through the
existing changeset model) is the planned next step but is its
own substantial slice — splitting it out keeps this commit
reviewable.
## Tests
11 existing shielded unit tests pass. New
`test_save_and_retrieve_notes`, `test_mark_spent`,
`test_sync_state_per_subwallet` cover SubwalletId scoping in
the in-memory store. iOS xcframework + SwiftExampleApp rebuild
green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…bind
Adds the Rust-side persistence wiring for the multi-account
shielded refactor. Sync passes and spend operations now emit
`ShieldedChangeSet` deltas to the wallet's persister, and
`bind_shielded` rehydrates the in-memory `SubwalletState` from
the persister's `ClientStartState` snapshot before kicking off
the first sync.
This is the Rust half of the deferred persistence slice; the
FFI callback that surfaces these changesets to the host
(SwiftData on iOS) and the matching Swift handler land in
follow-up commits in this same PR.
## What changes
**`changeset/`**:
- `ShieldedChangeSet` — per-`SubwalletId` `notes_saved`,
`nullifiers_spent`, `synced_indices`, `nullifier_checkpoints`.
Implements `Merge` (LWW on watermarks; append on note vecs).
Carried as a new `Option<ShieldedChangeSet>` field on
`PlatformWalletChangeSet` (feature-gated `shielded`).
- `ShieldedSyncStartState` — restore snapshot keyed by
`SubwalletId`. Lives on `ClientStartState.shielded`.
- Existing destructure sites in `apply.rs`, `manager/load.rs`,
`manager/wallet_lifecycle.rs`, and `platform_wallet.rs` updated
to drop the new field with a `#[cfg(feature = "shielded")]` arm.
**`wallet/shielded/mod.rs`**:
- `ShieldedWallet` grows an optional `WalletPersister` handle and a
`set_persister(...)` setter.
- New `queue_shielded_changeset(cs)` helper that wraps a
`ShieldedChangeSet` in a `PlatformWalletChangeSet` and pushes
it to the persister. No-op when no persister is attached.
- New `restore_from_snapshot(&ShieldedSyncStartState)` consumes
per-subwallet entries that match `(self.wallet_id, account)`
for any bound account, save_note's their notes, marks spent
ones, and replays the sync watermarks.
**`wallet/shielded/sync.rs`**:
- `sync_notes` accumulates a `ShieldedChangeSet` as it saves
decrypted notes / advances watermarks, then queues it on the
persister at the end of the pass (after dropping the store
write lock so the persister callback isn't nested under it).
- `check_nullifiers` does the same for spent marks +
nullifier checkpoints.
**`wallet/shielded/operations.rs`**:
- `mark_notes_spent` queues a changeset for each freshly-marked
nullifier so spend events propagate to durable storage
immediately rather than waiting for the next nullifier-sync
pass to rediscover them.
**`wallet/platform_wallet.rs`**:
- `bind_shielded` attaches the wallet's persister to the
`ShieldedWallet`, then calls `restore_from_snapshot` against
`self.persister.load()?.shielded` so the freshly-bound wallet
starts pre-populated with whatever the host already has on
disk for `(self.wallet_id, account)` for each requested account.
## Tests
11 existing shielded unit tests still pass. Clippy clean. The
load-side end-to-end flow ("host writes → cold start →
restore_from_snapshot → spend works") is exercised once the FFI
+ SwiftData sides land in the next commits.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-review at 3ffce1a confirms the prior blocking unshield address-format bug is fixed and most earlier suggestions (checked_add nonce, fee-reserve early return, trim-before-parse, bech32m diagnostic, fire-and-forget warm-up) are addressed. One new blocking issue stands: shielded spend paths (unshield/transfer/withdraw) call mark_notes_spent immediately after broadcast() — which only submits, not confirms — so a rejected/dropped transition strands notes locally. Six remaining suggestions/nitpicks: the &'static signer transmute (convergent across all reviewers), TOCTOU on per-address nonces in concurrent shields, TOCTOU on note selection in concurrent spends, missing Rust unit coverage on shielded_shield_from_account, drift from rs-sdk's canonical address-nonce helper, an amount==0 edge case, and a mut→const SignerHandle ergonomic.
Reviewed commit: 3ffce1a
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 2 nitpick(s)
2 additional findings
🔴 blocking: Shielded spends mark notes spent on broadcast acceptance, before chain confirmation
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (lines 355-362)
unshield, transfer, and withdraw all call mark_notes_spent(&selected_notes) immediately after state_transition.broadcast(...) returns Ok (operations.rs:355-362, 425-431, 498-504). BroadcastStateTransition::broadcast (rs-sdk/src/platform/transition/broadcast.rs:36-93) only submits the transition — wait_for_response and broadcast_and_wait are separately exposed for confirmation. A transition that is mempool-accepted and later rejected, dropped, or replaced will silently flip those notes to spent in the local ShieldedStore, which has no reconciliation path to clear a false spend. The user then sees the funds permanently unavailable locally even though the chain never consumed them. Fix is to either (a) use broadcast_and_wait here so notes are only marked once the proof result is observed, or (b) add a mark_notes_pending step under a write lock before broadcast and only promote to spent on observed inclusion (with a clearing path on rejection). The same write-after-broadcast shape interacts with finding #4 below — both are root-caused in the same fetch→broadcast→mutate sequence.
🟡 suggestion: Note selection → broadcast → mark_spent is non-atomic across concurrent spends
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (lines 312-511)
unshield, transfer, and withdraw each (1) take a read lock on the store to select unspent notes, (2) build+broadcast the transition, then (3) take a write lock to mark notes spent. The read lock is released before broadcast, so two concurrent calls (user-initiated send + retry, or two flows from the UI) can both observe the same notes as unspent. The first transition wins; the second's nullifiers are already on chain, drive-abci rejects the duplicate, and the user sees a generic broadcast error after 30 s of proof work. Same shape and remedy as the shield-side TOCTOU above — single-flight at the wallet level (or a tentative mark_in_flight step under a write lock before broadcast) prevents the failure. This also dovetails with finding #1: a mark_in_flight / mark_pending step would naturally provide the missing reconciliation hook.
🤖 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] lines 355-362: Shielded spends mark notes spent on broadcast acceptance, before chain confirmation
`unshield`, `transfer`, and `withdraw` all call `mark_notes_spent(&selected_notes)` immediately after `state_transition.broadcast(...)` returns `Ok` (operations.rs:355-362, 425-431, 498-504). `BroadcastStateTransition::broadcast` (rs-sdk/src/platform/transition/broadcast.rs:36-93) only submits the transition — `wait_for_response` and `broadcast_and_wait` are separately exposed for confirmation. A transition that is mempool-accepted and later rejected, dropped, or replaced will silently flip those notes to spent in the local `ShieldedStore`, which has no reconciliation path to clear a false spend. The user then sees the funds permanently unavailable locally even though the chain never consumed them. Fix is to either (a) use `broadcast_and_wait` here so notes are only marked once the proof result is observed, or (b) add a `mark_notes_pending` step under a write lock before broadcast and only promote to spent on observed inclusion (with a clearing path on rejection). The same write-after-broadcast shape interacts with finding #4 below — both are root-caused in the same fetch→broadcast→mutate sequence.
- [SUGGESTION] lines 117-180: Concurrent shields on the same wallet TOCTOU on the fetched address nonces
`ShieldedWallet::shield` reads each input's last-used nonce via `AddressInfo::fetch_many`, locally increments with `checked_add(1)`, and hands the result to `build_shield_transition`. The `shielded` slot uses `RwLock` and only a read guard is taken, so two overlapping `shield` invocations for the same wallet (UI double-tap, retry while the first proof is still building, two host threads racing) both observe the same `info.nonce`, both build with `info.nonce + 1`, and the second to land at drive-abci is rejected with a nonce conflict — but only after each has spent ~30 s on a Halo 2 proof. Not a memory-safety or consensus issue, but the user-visible failure is opaque and expensive. Either serialise shield-class operations on a per-wallet `tokio::sync::Mutex` taken across fetch+build+broadcast, or document at the FFI boundary (`shielded_send.rs:251-299`) that hosts must enforce single-flight per wallet.
- [SUGGESTION] lines 312-511: Note selection → broadcast → mark_spent is non-atomic across concurrent spends
`unshield`, `transfer`, and `withdraw` each (1) take a read lock on the store to select unspent notes, (2) build+broadcast the transition, then (3) take a write lock to mark notes spent. The read lock is released before broadcast, so two concurrent calls (user-initiated send + retry, or two flows from the UI) can both observe the same notes as unspent. The first transition wins; the second's nullifiers are already on chain, drive-abci rejects the duplicate, and the user sees a generic broadcast error after 30 s of proof work. Same shape and remedy as the shield-side TOCTOU above — single-flight at the wallet level (or a tentative `mark_in_flight` step under a write lock before broadcast) prevents the failure. This also dovetails with finding #1: a `mark_in_flight` / `mark_pending` step would naturally provide the missing reconciliation hook.
- [SUGGESTION] lines 117-180: Reimplements rs-sdk's canonical address-nonce fetch instead of reusing it
rs-sdk has `fetch_inputs_with_nonce`, `nonce_inc`, and `ensure_address_balance` in `packages/rs-sdk/src/platform/transition/address_inputs.rs` that encapsulate exactly this fetch-and-increment dance plus a hard balance check. They are `pub(crate)` today, so `platform-wallet` can't reach them directly, but a single-line visibility change would let this code re-use the canonical helpers. As written, the new shield path will silently drift from the SDK's behaviour — for example the SDK enforces a balance check while this implementation only `warn!`s on `info.balance < credits` (operations.rs:150-157).
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] lines 269-291: Use the established usize round-trip pattern instead of transmuting the signer borrow to &'static
`block_on_worker` requires `F: Send + 'static` (runtime.rs:49-56), and the new shield path satisfies that with `mem::transmute::<&VTableSigner, &'static VTableSigner>(...)` at lines 276-279. It is sound today only because `block_on_worker` is `rt.block_on(async move { rt.spawn(future).await.expect(...) })` — the calling thread parks until the spawned task completes, so the host-owned `SignerHandle` outlives the borrow. Any future change that lets `block_on_worker` return early — a shutdown `select!`, a timeout, a cancellation token, or replacing `.expect` with a `?`-style return — silently turns this into a use-after-free across the FFI boundary, since Swift's `KeychainSigner.deinit` is free to destroy the handle as soon as the FFI call returns. The same crate already solves the identical constraint without the lifetime fiction: `identity_top_up.rs:113-126` round-trips the pointer through `usize` and re-materializes `&VTableSigner` *inside* the spawned future (capturing only `Send + 'static` data). Aligning the shield path with that precedent removes the `unsafe { transmute }` from the FFI surface at zero behavioural cost.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [SUGGESTION] lines 480-627: shielded_shield_from_account selection rules have no Rust unit coverage
The selector carries non-trivial behaviour that directly determines whether shield broadcasts succeed: skipping leading addresses with `balance <= FEE_RESERVE_CREDITS` (only `>` is viable), reserving fee headroom only on input 0 via `balance.saturating_sub(FEE_RESERVE_CREDITS)`, walking BTreeMap key order so input 0 is the smallest-key entry, and accumulating until claim ≥ amount. None of this is covered by a focused Rust test, so a future refactor could regress any of these invariants — including reintroducing the original `viable_input_0` fall-through bug or the input-0 reserve regression — without tripping CI. Worth deterministic unit coverage against a synthetic `PlatformWalletInfo`/account: dust-first-address case, exact-reserve case (`balance == FEE_RESERVE`), single-address insufficient-with-reserve case, amount-equal-to-`(total_usable - FEE_RESERVE)`, and amount=0 (see #7).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift (2)
273-291:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBlock arbitrary Orchard recipients here until this flow can actually honor them.
This branch ignores
recipientAddressand callsshieldedShield(...), which only shields into the bound wallet's default Orchard address. Right now the UI can report success for a send to someone else's Orchard address even though the credits stay in the sender's own shielded pool.🤖 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/ViewModels/SendViewModel.swift` around lines 273 - 291, The platformToShielded case is ignoring recipientAddress and always calls walletManager.shieldedShield(...) which only deposits into the wallet's own default Orchard pool; block/validate arbitrary external Orchard recipients by checking recipientAddress in the platformToShielded branch and short-circuiting (returning an error or setting error state) when recipientAddress is present and not the wallet's own bound Orchard address. Update the platformToShielded branch (referencing platformToShielded, recipientAddress, and shieldedShield) to validate the recipient before calling shieldedShield and ensure only shielding into the bound wallet's default address is allowed.
92-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGate
canSendon the active unit, and reject values that round down to zero.Both parsers accept any positive
Doubleand truncate, so inputs smaller than 1 duff / 1 credit still yield0, andcanSendcurrently only checksamountDuffs. That enables shielded/platform submits with the wrong scale or a zero-credit amount.♻️ Minimal fix
var amountDuffs: UInt64? { guard let double = Double(amountString), double > 0 else { return nil } - return UInt64(double * 100_000_000) + let scaled = UInt64(double * 100_000_000) + return scaled > 0 ? scaled : nil } @@ var amountCredits: UInt64? { guard let double = Double(amountString), double > 0 else { return nil } - return UInt64(double * 100_000_000_000) + let scaled = UInt64(double * 100_000_000_000) + return scaled > 0 ? scaled : nil } @@ var canSend: Bool { - detectedFlow != nil && amountDuffs != nil && !isSending + let hasValidAmount: Bool + switch detectedFlow { + case .coreToCore: + hasValidAmount = amountDuffs != nil + case .platformToShielded, .shieldedToShielded, .shieldedToPlatform, .shieldedToCore: + hasValidAmount = amountCredits != nil + case nil: + hasValidAmount = false + } + return detectedFlow != nil && hasValidAmount && !isSending }🤖 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/ViewModels/SendViewModel.swift` around lines 92 - 118, The canSend logic currently only checks amountDuffs and allows values that round to zero; update both amountDuffs and amountCredits so they return nil when the scaled UInt64 would be 0 (i.e. compute let value = UInt64(double * scale) and guard value > 0 else { return nil }), and change canSend to gate on the active unit by checking detectedFlow and requiring the correct non-nil amount (for flows platformToShielded/shieldedToShielded/shieldedToPlatform/shieldedToCore use amountCredits != nil, otherwise use amountDuffs != nil) while still ensuring !isSending. This uses the existing symbols amountDuffs, amountCredits, detectedFlow and canSend to locate and fix the code.packages/rs-platform-wallet/src/wallet/platform_wallet.rs (1)
595-606:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject
amount == 0before building a shield selection.With zero here, the selector falls through to an empty
inputsmap and the failure happens much later in the shield builder/broadcast path. A small guard keeps the error legible for FFI/SDK callers that bypass the UI.♻️ Minimal fix
pub async fn shielded_shield_from_account<S, P>( &self, shielded_account: u32, payment_account: u32, amount: u64, signer: &S, prover: P, ) -> Result<(), PlatformWalletError> where S: dpp::identity::signer::Signer<dpp::address_funds::PlatformAddress> + Send + Sync, P: dpp::shielded::builder::OrchardProver, { + if amount == 0 { + return Err(PlatformWalletError::ShieldedBuildError( + "amount must be > 0".to_string(), + )); + } + // The shield transition uses `DeductFromInput(0)` as its fee🤖 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/platform_wallet.rs` around lines 595 - 606, In shielded_shield_from_account, add an early guard that rejects amount == 0 before any shield selection or builder work: check the amount at the top of the function (in pub async fn shielded_shield_from_account) and return a clear PlatformWalletError (e.g., InvalidAmount or a new variant indicating zero amount) so the function exits immediately instead of proceeding to the selector/inputs creation and later failing in the shield builder/broadcast path.packages/rs-platform-wallet/src/wallet/shielded/operations.rs (1)
116-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccept proof-of-absence nonce lookups for first-time shield inputs.
AddressInfo::fetch_manycan return the requested address with noAddressInfopayload for a never-before-used address. This branch collapses that intoinput address not found, so the first shield from a freshly funded payment address fails instead of using nonce1.♻️ Minimal fix
- let info = infos - .get(&addr) - .and_then(|opt| opt.as_ref()) - .ok_or_else(|| { - PlatformWalletError::ShieldedBuildError(format!( - "input address not found on platform: {:?}", - addr - )) - })?; - if info.balance < credits { + let maybe_info = infos.get(&addr).ok_or_else(|| { + PlatformWalletError::ShieldedBuildError(format!( + "input address missing from nonce lookup: {:?}", + addr + )) + })?; + if let Some(info) = maybe_info.as_ref() { + if info.balance < credits { + warn!( + address = ?addr, + claimed_credits = credits, + platform_balance = info.balance, + platform_nonce = info.nonce, + "Shield input claims more credits than Platform reports — broadcast will likely fail" + ); + } else { + info!( + address = ?addr, + claimed_credits = credits, + platform_balance = info.balance, + platform_nonce = info.nonce, + "Shield input" + ); + } + } - let next_nonce = info.nonce.checked_add(1).ok_or_else(|| { + let next_nonce = maybe_info + .as_ref() + .map(|info| info.nonce) + .unwrap_or(0) + .checked_add(1) + .ok_or_else(|| { PlatformWalletError::ShieldedBuildError(format!( "input address nonce exhausted on platform: {:?}", addr )) })?;Dash SDK `AddressInfo::fetch_many` semantics for requested platform addresses that have never been used: does the response keep the key with `None`, and should the next nonce for the first state transition be `1`?Also applies to: 147-153
🤖 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 116 - 124, The code treats a fetched entry of Some(None) from AddressInfo::fetch_many as a missing address and returns ShieldedBuildError; instead accept a present key with None payload and treat it as a first-time address with next nonce = 1. Modify the logic around infos.get(&addr) (where info is currently derived via .and_then(|opt| opt.as_ref()).ok_or_else(...)) to distinguish three cases: missing map entry -> error, Some(None) -> construct/use an AddressInfo-equivalent or set next_nonce = 1 for the shield input, and Some(Some(info)) -> use the existing info as before; apply the same change to the other similar branch (the code around the second occurrence that mirrors lines 147-153). Ensure any later code that reads info.nonce uses the computed next_nonce when info payload is None.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/changeset/shielded_sync_start_state.rs`:
- Around line 28-29: The field last_synced_index in the ShieldedSyncStartState
struct cannot represent "never scanned" and should be made explicit (e.g.,
Option<u64> or a small enum) so cold-start vs already-scanned-0 is unambiguous;
update the struct definition to change last_synced_index: u64 ->
last_synced_index: Option<u64> (or an enum like Never/Index(u64)), update
constructors/builders that set or read last_synced_index, and adjust any
(de)serialization/serde derives and consumers that assume a raw u64 to handle
None/enum variants accordingly so the absent state is encoded/decoded
explicitly.
In `@packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- Around line 314-315: The call to mark_notes_spent currently returns an error
that bubbles up and can make a successful broadcast appear as a send failure;
change the call sites (the mark_notes_spent invocations in operations.rs) to
treat the write as best-effort: call self.mark_notes_spent(...).await and on
Err(e) do not return the error but log it (e.g., tracing::error! /
self.logger.error) with context like "failed to mark notes spent after
broadcast" so the function proceeds normally and relies on the next sync to heal
local state drift.
In `@packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- Around line 201-205: Use a single authoritative resume index derived from
result.next_start_index for both the tree checkpoint and per-account watermark
to avoid drift: compute let resume_index = result.next_start_index as u32 (or an
appropriately typed variable) and pass resume_index to
store.checkpoint_tree(resume_index) and to calls to
set_last_synced_note_index(...) instead of using aligned_start +
result.total_notes_scanned; apply the same change to the similar block handling
lines ~246-255 so both checkpoint_tree and set_last_synced_note_index use the
same resume_index in both places.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift`:
- Around line 60-65: The docstring for PlatformWalletManagerShieldedSync says
"accounts" must be non-empty and at most 64 entries, but the Swift
implementation only checks for emptiness; before calling the Rust FFI you should
also enforce the 64-account cap. In the PlatformWalletManagerShieldedSync
initializer / method that accepts the accounts array (referencing the accounts
parameter and the code paths at lines around 60–65 and 89–93), add a guard that
validates accounts.count <= 64 and return or throw the same error path used for
the empty check so the contract is upheld in Swift and invalid input never
crosses the FFI boundary.
---
Duplicate comments:
In `@packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- Around line 595-606: In shielded_shield_from_account, add an early guard that
rejects amount == 0 before any shield selection or builder work: check the
amount at the top of the function (in pub async fn shielded_shield_from_account)
and return a clear PlatformWalletError (e.g., InvalidAmount or a new variant
indicating zero amount) so the function exits immediately instead of proceeding
to the selector/inputs creation and later failing in the shield
builder/broadcast path.
In `@packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- Around line 116-124: The code treats a fetched entry of Some(None) from
AddressInfo::fetch_many as a missing address and returns ShieldedBuildError;
instead accept a present key with None payload and treat it as a first-time
address with next nonce = 1. Modify the logic around infos.get(&addr) (where
info is currently derived via .and_then(|opt| opt.as_ref()).ok_or_else(...)) to
distinguish three cases: missing map entry -> error, Some(None) -> construct/use
an AddressInfo-equivalent or set next_nonce = 1 for the shield input, and
Some(Some(info)) -> use the existing info as before; apply the same change to
the other similar branch (the code around the second occurrence that mirrors
lines 147-153). Ensure any later code that reads info.nonce uses the computed
next_nonce when info payload is None.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift`:
- Around line 273-291: The platformToShielded case is ignoring recipientAddress
and always calls walletManager.shieldedShield(...) which only deposits into the
wallet's own default Orchard pool; block/validate arbitrary external Orchard
recipients by checking recipientAddress in the platformToShielded branch and
short-circuiting (returning an error or setting error state) when
recipientAddress is present and not the wallet's own bound Orchard address.
Update the platformToShielded branch (referencing platformToShielded,
recipientAddress, and shieldedShield) to validate the recipient before calling
shieldedShield and ensure only shielding into the bound wallet's default address
is allowed.
- Around line 92-118: The canSend logic currently only checks amountDuffs and
allows values that round to zero; update both amountDuffs and amountCredits so
they return nil when the scaled UInt64 would be 0 (i.e. compute let value =
UInt64(double * scale) and guard value > 0 else { return nil }), and change
canSend to gate on the active unit by checking detectedFlow and requiring the
correct non-nil amount (for flows
platformToShielded/shieldedToShielded/shieldedToPlatform/shieldedToCore use
amountCredits != nil, otherwise use amountDuffs != nil) while still ensuring
!isSending. This uses the existing symbols amountDuffs, amountCredits,
detectedFlow and canSend to locate and fix the code.
🪄 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: bbcc5df6-9a2b-4863-9677-4a9842a32775
📒 Files selected for processing (19)
packages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet-ffi/src/shielded_sync.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/client_start_state.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/rs-platform-wallet/src/changeset/shielded_changeset.rspackages/rs-platform-wallet/src/changeset/shielded_sync_start_state.rspackages/rs-platform-wallet/src/manager/load.rspackages/rs-platform-wallet/src/manager/wallet_lifecycle.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/file_store.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.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift
| /// Highest global note index that the subwallet has scanned. | ||
| pub last_synced_index: u64, |
There was a problem hiding this comment.
Encode the unsynced state explicitly.
last_synced_index: u64 cannot represent "never scanned". That makes cold-start resume ambiguous between a fresh subwallet and one that has already processed note index 0, so consumers either need an undocumented sentinel or risk an off-by-one at the first note boundary.
Suggested shape
- pub last_synced_index: u64,
+ pub last_synced_index: Option<u64>,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Highest global note index that the subwallet has scanned. | |
| pub last_synced_index: u64, | |
| /// Highest global note index that the subwallet has scanned. | |
| pub last_synced_index: Option<u64>, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet/src/changeset/shielded_sync_start_state.rs`
around lines 28 - 29, The field last_synced_index in the ShieldedSyncStartState
struct cannot represent "never scanned" and should be made explicit (e.g.,
Option<u64> or a small enum) so cold-start vs already-scanned-0 is unambiguous;
update the struct definition to change last_synced_index: u64 ->
last_synced_index: Option<u64> (or an enum like Never/Index(u64)), update
constructors/builders that set or read last_synced_index, and adjust any
(de)serialization/serde derives and consumers that assume a raw u64 to handle
None/enum variants accordingly so the absent state is encoded/decoded
explicitly.
…ed notes Wires the Rust-side `ShieldedChangeSet` persister hook from the previous commit through the FFI to SwiftData, so decrypted shielded notes / nullifier-spent flags / per-subwallet sync watermarks survive across app launches. Cold start re-loads the state into the in-memory `ShieldedWallet` so spending and balance reads work without re-decrypting the chain. ## What changes **rs-platform-wallet-ffi**: - `shielded_persistence.rs` — new C-ABI types `ShieldedNoteFFI` / `ShieldedNullifierSpentFFI` / `ShieldedSyncedIndexFFI` / `ShieldedNullifierCheckpointFFI` for the persist path, and `ShieldedNoteRestoreFFI` / `ShieldedSubwalletSyncStateFFI` for the load path. - `PersistenceCallbacks` grows four `on_persist_shielded_*_fn` fields and four `on_load_shielded_*_fn` / free pairs. Inlined function signatures (rather than `pub type` aliases) so cbindgen walks into the referenced struct definitions and emits their full field layout in the generated header. - `FFIPersister::store` fans `changeset.shielded` out across the four persist callbacks. `FFIPersister::load` calls the two load callbacks and folds the results into `ClientStartState.shielded` keyed by `SubwalletId`. **swift-sdk**: - `PersistentShieldedNote` / `PersistentShieldedSyncState` SwiftData models. Notes keyed by `nullifier` (globally unique); sync states uniquely keyed by `(walletId, accountIndex)`. Both registered in `DashModelContainer.modelTypes`. - `PlatformWalletPersistenceHandler` grows handler methods + trampolines for the four persist callbacks (upserts / spent-flag flips / watermark advances / nullifier-checkpoint upserts) and the two load callbacks (host-allocated arrays with deferred free under `ShieldedLoadAllocation` / `ShieldedSyncStateLoadAllocation`). - `makeCallbacks()` wires every new callback into the `PersistenceCallbacks` struct handed to Rust. ## End-to-end flow Per-spend / per-sync passes on the Rust side build a `ShieldedChangeSet` and queue it on the persister. The FFI flushes that into the four typed callback batches, and the Swift handler upserts SwiftData rows. On cold start `bind_shielded` calls `persister.load()` which fires the load callbacks; the host streams every persisted row back as flat FFI arrays, Rust assembles a `ShieldedSyncStartState`, and `ShieldedWallet::restore_from_snapshot` rehydrates the in-memory `SubwalletState` before the first sync runs. ## Tests Existing 11 shielded unit tests pass. iOS xcframework + the SwiftExampleApp build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ync state Adds two read-only browsers next to the existing "TXOs" / "Pending Inputs" / etc. rows in the Storage Explorer: "Shielded Notes" (per-(wallet, account) decrypted notes, spent/unspent filterable) and "Shielded Sync State" (per- subwallet `last_synced_index` + nullifier checkpoint). Both scoped to the active network via the `walletId` denorm on the row, matching the pattern `TxoStorageListView` uses. Also wires the matching count entries into `loadCounts()` so the row counts on the Storage Explorer index page reflect the new tables. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`ShieldedService.bind(...)` now takes `accounts: [UInt32]` (default `[0]`); after a successful Rust-side `bindShielded` it populates `boundAccounts` and `addressesByAccount` by calling `shieldedDefaultAddress` per bound account. The legacy `orchardDisplayAddress` is preserved as the lowest-bound account's address so the existing single-account Receive sheet keeps working. `AccountListView` grows a "Shielded" section that mirrors the existing Core / Platform Payment account rows. One row per bound ZIP-32 account showing `Shielded #N` plus the truncated bech32m address, driven by `shieldedService.boundAccounts` / `addressesByAccount`. The whole-wallet "Shielded Balance" row on the balance card stays as-is for now since the FFI sync event still flattens balance to the wallet level; per-account balance breakdown needs a follow-up FFI lookup (`platform_wallet_manager_shielded_balance(walletId, account)`). `reset()` clears the new published fields so wallet switches don't leak the prior wallet's accounts/addresses into the new detail view. This is the third leg of the multi-account refactor (Rust internals + persistence + UI); the "Add account" affordance itself is deferred — it needs a new `shielded_add_account` FFI that re-uses the bind path's mnemonic resolver. Hosts can already bind multiple accounts up front by passing `accounts: [0, 1, …]` to `bind`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Orchard spend builder rejected proofs with `AnchorMismatch: failed to add spend` because the anchor we passed in (read via `store.tree_anchor()` → `ClientPersistentCommitmentTree::anchor()` → `root_at_checkpoint_depth(None)`) reflected the latest tree state, while each witness was generated by `witness_at_checkpoint_depth(0)` — the root of the most recent checkpoint. Whenever the two depths diverged (e.g. commitments appended after the last checkpoint, or any sequencing where "latest" got ahead of "depth 0") the builder rejected the bundle. Derive the anchor from the witness paths themselves via `MerklePath::root(extracted_cmx)`. By construction that's the root the witness will verify against inside the Halo 2 proof, so it can't disagree with the bundle. Also catches the case where multiple selected notes' witnesses came from different checkpoints (returns `ShieldedBuildError` immediately instead of letting the spend builder surface `AnchorMismatch` after the ~30 s proof generation). `store.tree_anchor()` is no longer called from the spend pipeline; the trait method stays in place for diagnostics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shardtree's `checkpoint(id)` silently dedups duplicate ids — a second `checkpoint(N)` call when checkpoint `N` already exists returns false (no-op) and the depth-0 view of the tree stays pinned at the first call's state. Sync was passing `result.next_start_index as u32` as the id, which the SDK rewinds to the last partial chunk's start so it can re-fetch that chunk on the next sync. Consecutive syncs that all ended on a partial chunk passed the SAME id; only the first checkpoint took, every subsequent one was a no-op even though each sync DID append fresh commitments. The witness computed at depth 0 then reflected an old tree state — its root was a snapshot Platform never recorded as a block-end anchor, and broadcast failed with `Anchor not found in the recorded anchors tree`. Switch to the high-water position (`aligned_start + total_notes_scanned` — one past the last appended) as the checkpoint id. Each sync that appends gets a strictly-greater id than the previous, depth 0 advances to the latest tree state, the witness's root tracks Platform's most recent recorded anchor, and broadcast validates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (1)
313-315:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLocal spent-state error after a successful broadcast still aborts the call.
After
state_transition.broadcast(...)returnsOk, the transition is on Platform; failing the whole call whenmark_notes_spenterrors makes a successful send look failed to the host (and invites the user to retry, double-spending the same notes once the next sync sees them as spent). The next nullifier-sync pass already heals local drift, so this should be best-effort + warn.♻️ Proposed pattern (apply at all three call sites)
- self.mark_notes_spent(id, &selected_notes).await?; + if let Err(e) = self.mark_notes_spent(id, &selected_notes).await { + warn!( + account, + error = %e, + "Broadcast succeeded but local spent-state update failed; \ + state will be repaired on the next nullifier sync" + ); + }Also applies to: 377-379, 447-449
🤖 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 313 - 315, After a successful state_transition.broadcast(...) the subsequent call to mark_notes_spent(...) must be best-effort only; change the three call sites where you call self.mark_notes_spent(id, &selected_notes).await? (and similar invocations at the other two sites) so that you do not propagate errors when broadcast returned Ok—capture the Result, log a warning with context (including the error and the associated id/nullifiers) and continue without returning Err; only propagate mark_notes_spent failures if the broadcast itself failed, otherwise treat failures as non-fatal and let the normal nullifier-sync heal the local state.
🧹 Nitpick comments (3)
packages/rs-platform-wallet/src/wallet/shielded/sync.rs (3)
215-219: 💤 Low valueCompute
new_indexonce and reuse for both checkpoint id and watermarks.
new_index = aligned_start + result.total_notes_scannedis computed twice — once for the checkpoint id at Line 215 and again at Line 262 for the per-account watermark loop. Hoisting it above theif appended > 0block keeps the two values in lockstep by construction (which is exactly the invariant the past review comment was about) and avoids any future drift if one site is touched without the other.♻️ Proposed refactor
- if appended > 0 { + let new_index = aligned_start + result.total_notes_scanned; + if appended > 0 { // ... (existing comment) ... - let new_index = aligned_start + result.total_notes_scanned; let checkpoint_id: u32 = new_index.try_into().unwrap_or(u32::MAX); store .checkpoint_tree(checkpoint_id) .map_err(|e| PlatformWalletError::ShieldedTreeUpdateFailed(e.to_string()))?; } @@ - // Update every account's watermark to the same global - // tree position so the next sync resumes coherently. - let new_index = aligned_start + result.total_notes_scanned; + // Update every account's watermark to the same global + // tree position so the next sync resumes coherently. for &account in &account_indices {Also applies to: 260-269
🤖 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 215 - 219, Compute new_index once and reuse it for both the checkpoint id and the per-account watermark updates: hoist let new_index = aligned_start + result.total_notes_scanned above the if appended > 0 block, derive checkpoint_id from that single new_index (as you already do for checkpoint_tree via store.checkpoint_tree(checkpoint_id)), and use the same new_index when computing watermarks in the per-account watermark loop so both checkpoint and watermarks are guaranteed to stay in sync.
17-17: ⚡ Quick winDrop the dead-code stub by removing the unused
PaymentAddressimport.
PaymentAddressis no longer referenced in this module; the only thing keeping it alive is the dummy_unused_payment_addresshelper. Removing both is cleaner than carrying a no-op function with#[allow(dead_code)].♻️ Proposed cleanup
-use grovedb_commitment_tree::{Note as OrchardNote, PaymentAddress, PreparedIncomingViewingKey}; +use grovedb_commitment_tree::{Note as OrchardNote, PreparedIncomingViewingKey}; @@ -// Suppress dead_code on `address` field — kept for future use -// (e.g. surfacing diversifier index per discovered note). -#[allow(dead_code)] -fn _unused_payment_address(_pa: PaymentAddress) {} -Also applies to: 406-409
🤖 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` at line 17, Remove the unused PaymentAddress import and its dead-code helper: delete PaymentAddress from the use list in sync.rs (the grovedb_commitment_tree import) and remove the helper function named _unused_payment_address (and its #[allow(dead_code)] attribute) so the module no longer carries a no-op stub; ensure only used symbols (OrchardNote, PreparedIncomingViewingKey) remain imported.
154-172: Trial-decryption is O(non-driver-accounts × all_notes) per chunk — fine today, but worth noting.For each non-driver account this loop trial-decrypts every fetched note locally. With
CHUNK_SIZE = 2048and a small number of accounts this is negligible, but ifaccount_indicesgrows (multi-account UI, restored wallets) the cost scales linearly with both. If that becomes a hot path, batching the IVKs into a single SDK call (or usingbatch_decrypt_notesif the SDK exposes one) would be worth investigating. No change requested for this PR.🤖 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 154 - 172, The loop over prepared.iter().skip(1) performs trial decryption of every note for each non-driver account (using try_decrypt_note over result.all_notes) which is O(accounts × all_notes); to address future scaling, refactor to batch-decrypt IVKs instead of per-account per-note calls: replace the nested loop that fills decrypted_by_account with a batched decrypt call (or SDK.batch_decrypt_notes if available) that accepts the collection of ivks and notes and returns which notes decrypt to which ivk, then map those results to DiscoveredNote entries (preserving position and cmx extraction logic) to avoid repeated work in the try_decrypt_note path.
🤖 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 1157-1191: Before calling the host loader, check that each "load"
callback is paired with its corresponding "free" callback and return an Err if
one is set without the other; specifically validate on_load_shielded_notes_fn
with on_load_shielded_notes_free_fn (and likewise
on_load_shielded_sync_states_fn with on_load_shielded_sync_states_free_fn)
before invoking the loader, and only construct the NotesGuard (or equivalent
SyncStates guard) after confirming both callbacks are present so host buffers
are freed; apply the same paired validation to both restore paths around the
code that calls load_notes/load_sync_states and constructs the guard.
In `@packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- Around line 215-216: The current code sets checkpoint_id with
new_index.try_into().unwrap_or(u32::MAX), which silently collapses all
subsequent checkpoint IDs to u32::MAX when new_index overflows u32 and
re-introduces the non-monotonic dedup failure; change the behavior to fail
loudly on overflow instead: replace the unwrap_or fallback with an explicit
error/expectation on the try_into result (e.g. propagate an error or use expect
with a clear message) so that when aligned_start + result.total_notes_scanned
(new_index) cannot fit into a u32 the function returns an error or panics rather
than silently using u32::MAX, preserving strict monotonic checkpoint IDs (refer
to new_index, checkpoint_id, aligned_start, result.total_notes_scanned and the
try_into call).
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 2189-2232: The loop writes into buf at the original row index
while skipping malformed rows, leaving uninitialized slots but returning
allocation.entriesInitialized as the count; fix by compacting initialized
entries so the published buffer is contiguous: either pre-scan rows to count
valid entries and allocate buf of that size, or write to
buf[allocation.entriesInitialized] (incrementing entriesInitialized only when
you populate an entry) instead of buf[idx]; ensure you set entriesPtr to the
pointer of the first initialized element and use that for the
shieldedLoadAllocations key and resultCount; apply the same change to
loadShieldedSyncStates() so ShieldedNoteRestoreFFI entries (and analogous
structs) are tightly packed before exposing them to Rust.
- Around line 2167-2179: The current loaders fetch all PersistentShieldedNote
rows (and similarly PersistentShieldedSyncState around lines 2258-2270) and risk
returning notes from other networks; restrict them to this handler's network by
first obtaining the current wallet ids (e.g. via loadWalletList() or the same
wallet-list source used by loadWalletList()) and then replace rows = try
backgroundContext.fetch(descriptor) with a filtered result: fetch then filter by
wallet id (e.g. rows.filter { walletIds.contains($0.walletId) }) before creating
ShieldedLoadAllocation and proceeding; apply the identical wallet-id filtering
to the sync-state loader that marshals PersistentShieldedSyncState rows.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- Around line 221-241: The switchTo(walletId: Data) path currently drops the
requested shielded accounts and falls back to [0]; preserve and pass the
requested account list through to bind so boundAccounts/addressesByAccount
remain correct. Modify switchTo to accept or retrieve the same accounts
parameter used during initial bind (the requested shielded account set, e.g.,
"accounts" or "requestedAccounts") and forward it into the bind(...) call
(bind(walletManager:walletId:network:resolver:accounts:)), or stash the
previously requested accounts on the ShieldedService instance and use that stash
when calling bind; ensure boundAccounts and addressesByAccount are populated
from that accounts list rather than defaulting to [0].
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift`:
- Around line 61-67: shieldedAccountsForThisWallet currently returns
shieldedService.boundAccounts unfiltered, so it can show accounts from a
previously-bound wallet; change it to only return boundAccounts when the
service's bound wallet id matches the view's wallet id (e.g., compare
shieldedService.boundWalletId or shieldedService.currentWalletId to the
view/model's wallet.id) and otherwise return []; locate the computed property
shieldedAccountsForThisWallet and add this wallet-id guard around
shieldedService.boundAccounts (or clear/replace the bound accounts earlier in
navigation if that pattern is preferred).
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageModelListViews.swift`:
- Around line 1708-1750: The overlay currently shows when visible.isEmpty which
hides the segmented picker even for filtered-empty views; change the overlay
condition to only show when the full store is empty by gating the overlay on
scoped.isEmpty (i.e., replace the overlay's if visible.isEmpty check with if
scoped.isEmpty) so the inline filtered-empty Section (handled where
visible.isEmpty && !scoped.isEmpty) still allows the user to switch filters;
update the overlay closure that contains ContentUnavailableView accordingly.
---
Duplicate comments:
In `@packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- Around line 313-315: After a successful state_transition.broadcast(...) the
subsequent call to mark_notes_spent(...) must be best-effort only; change the
three call sites where you call self.mark_notes_spent(id,
&selected_notes).await? (and similar invocations at the other two sites) so that
you do not propagate errors when broadcast returned Ok—capture the Result, log a
warning with context (including the error and the associated id/nullifiers) and
continue without returning Err; only propagate mark_notes_spent failures if the
broadcast itself failed, otherwise treat failures as non-fatal and let the
normal nullifier-sync heal the local state.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- Around line 215-219: Compute new_index once and reuse it for both the
checkpoint id and the per-account watermark updates: hoist let new_index =
aligned_start + result.total_notes_scanned above the if appended > 0 block,
derive checkpoint_id from that single new_index (as you already do for
checkpoint_tree via store.checkpoint_tree(checkpoint_id)), and use the same
new_index when computing watermarks in the per-account watermark loop so both
checkpoint and watermarks are guaranteed to stay in sync.
- Line 17: Remove the unused PaymentAddress import and its dead-code helper:
delete PaymentAddress from the use list in sync.rs (the grovedb_commitment_tree
import) and remove the helper function named _unused_payment_address (and its
#[allow(dead_code)] attribute) so the module no longer carries a no-op stub;
ensure only used symbols (OrchardNote, PreparedIncomingViewingKey) remain
imported.
- Around line 154-172: The loop over prepared.iter().skip(1) performs trial
decryption of every note for each non-driver account (using try_decrypt_note
over result.all_notes) which is O(accounts × all_notes); to address future
scaling, refactor to batch-decrypt IVKs instead of per-account per-note calls:
replace the nested loop that fills decrypted_by_account with a batched decrypt
call (or SDK.batch_decrypt_notes if available) that accepts the collection of
ivks and notes and returns which notes decrypt to which ivk, then map those
results to DiscoveredNote entries (preserving position and cmx extraction logic)
to avoid repeated work in the try_decrypt_note path.
🪄 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: 330c3010-0058-4cbd-813a-0e5a92df7e51
📒 Files selected for processing (13)
packages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/shielded_persistence.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/sync.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentShieldedNote.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentShieldedSyncState.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageExplorerView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageModelListViews.swift
…from Phase 2b
Three fixes to the shared-tree shielded coordinator, the first
verified end-to-end against a live simulator wallet (a shielded
transfer that previously failed with 'Merkle witness unavailable'
now completes and debits the balance).
1. Witness bug — mark every commitment, not just owned ones
The shared commitment tree appended a position as Marked only
if the owning wallet's IVK recognized it in that very sync
pass. With multiple wallets sharing one tree and binding at
different times, a note appended before its owner bound stayed
Ephemeral forever (shardtree has no retroactive marking), so
the note showed as balance but couldn't be witnessed at spend
time. Confirmed on the user's regtest wallet: two wallets
('Reg' owns nothing, 'Wallet2' owns the notes); 'Reg' synced
first and appended every commitment Ephemeral. `sync_notes_across`
now appends every position with `marked = true` — the tree is a
chain-wide structure, so it must be witness-complete regardless
of bind order. Per-wallet ownership is still tracked separately
in the per-SubwalletId note store, so privacy/accounting is
unchanged. Regression test reproduces the exact failing tree
shape (6 commitments, single checkpoint, persist+reload) and
asserts every position witnesses.
2. [BLOCKING] remove_wallet now detaches shielded state
`PlatformWalletManager::remove_wallet` never called
`coordinator.unregister_wallet`, so a deleted wallet's
SubwalletId→AccountViewingKeys + cloned WalletPersister stayed
registered and the next sync kept fetching / re-persisting its
private notes. Now unregisters on remove. `unregister_wallet`
also purges the per-subwallet store state (notes, watermarks,
nullifier checkpoints) via the new `ShieldedStore::purge_wallet`.
3. [BLOCKING] clear() now purges per-subwallet store state
`NetworkShieldedCoordinator::clear()` emptied only the
registries, leaving `FileBackedShieldedStore::subwallets`
(notes + `last_synced_note_index`) intact. A same-session
re-bind after Clear resumed from the stale watermark, reported
caught-up, and never re-emitted notes until process restart.
`clear()` now calls the new `ShieldedStore::purge_all_subwallets`,
delivering the documented 'resync from index 0' contract
in-process.
Also addressed from the review:
- shielded_shield FFI no longer transmutes the host signer borrow
to `&'static`; round-trips the pointer through `usize` and
re-materializes the borrow inside the worker task (avoids a
latent UAF / signing-oracle hazard if block_on_worker ever
stops being synchronous). Param retyped `*mut`→`*const` to
match read-only semantics.
New trait methods `ShieldedStore::{purge_wallet, purge_all_subwallets}`
implemented for both InMemoryShieldedStore and
FileBackedShieldedStore; the shared SQLite commitment tree is
deliberately left intact by both (chain-wide cache, not
per-wallet).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed in 6e66e65 (the two blocking findings were posted in the review body, not as inline threads, so replying here): 🔴 Blocking — remove_wallet() never unregisters from the coordinator — Fixed. 🔴 Blocking — clear() leaves per-subwallet store state intact — Fixed. 🟡/💬 — signer Bonus — root-caused and fixed a separate Deferred to follow-up (acknowledged, not in this commit): shield/shield_from_asset_lock relay-ACK vs proven-execution (Type 15/18 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The 2eaffb5→6e66e65f delta cleanly resolves all four prior blockers/security items: remove_wallet() now calls coordinator.unregister_wallet() (wallet_lifecycle.rs:383-386), coordinator.clear() + unregister_wallet() purge per-SubwalletId store state via new purge_wallet/purge_all_subwallets trait methods (coordinator.rs:244, 369), platform_wallet_manager_shielded_shield replaced the &'static transmute with a usize round-trip that re-materializes the borrow inside the worker (shielded_send.rs:293-304), and signer_address_handle is now *const. The new always-marked sync path plus the all_marked_tree_witnesses_every_position_after_reload regression test fix the multi-wallet shared-tree witness bug. Five carried-forward findings remain STILL VALID on unchanged surfaces (Type 15/18 broadcast-vs-broadcast_and_wait, shielded_add_account half-live, ShieldedSyncManager::stop cancel-only, clearLocalState race, amount=0 boundary). One new lower-priority issue identified by codex on bind_shielded (merge-not-replace on rebind). No new regressions; no blockers.
🟡 5 suggestion(s) | 💬 1 nitpick(s)
6 additional finding(s)
suggestion: Type 15 / Type 18 shields still report success on relay ACK, not on proven execution
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (line 214)
STILL VALID at 6e66e65 — operations.rs untouched in this delta. shield() (line 216) and shield_from_asset_lock() (lines 280-283) call state_transition.broadcast(sdk, None) and return Ok(()) as soon as one peer ACKs submission, while the same module's unshield, transfer, and withdraw use broadcast_and_wait::<StateTransitionProofResult> (lines 348, 422, 502). Through platform_wallet_manager_shielded_shield this lets a hostile or faulty DAPI gateway acknowledge submission, suppress retries, and make the host believe a shield succeeded even when Platform later rejects the transition or never includes it. The Type 15 path's rich addresses_not_enough_funds diagnostic at lines 217-237 is also unreachable under submission-only success semantics — only local broadcast-machinery errors propagate. Type 18 is worse because the asset-lock proof is single-use: a false-positive success strands the user's L1 outpoint with no in-app indication. Align Type 15/18 with the spend-side broadcast_and_wait::<StateTransitionProofResult> so all five shielded transitions share one success contract.
suggestion: shielded_add_account() returns Ok after updating only the wallet keyset — coordinator registry never sees the new account
packages/rs-platform-wallet/src/wallet/platform_wallet.rs (line 417)
STILL VALID at 6e66e65 — platform_wallet.rs:417-437 untouched. shielded_add_account inserts the new OrchardKeySet into self.shielded_keys and the in-code comment at lines 431-435 acknowledges it does not refresh the coordinator's accounts registry. After the Phase-2b refactor NetworkShieldedCoordinator::sync() iterates only coordinator.accounts, so background sync will never trial-decrypt or persist notes for the new account, while local helpers like shielded_account_indices()/shielded_default_address() accept it immediately. Notes addressed to this account silently accumulate undetected until the host runs bind_shielded again with the full account list. With #1/#2 now fixed, this is the last unsoundness in the wallet↔coordinator registry contract. Either drop this from the public surface or extend the signature to take &Arc<NetworkShieldedCoordinator> and register the new account atomically alongside the keyset insert.
suggestion: ShieldedSyncManager::stop() reads as a barrier but only cancels future loop iterations
packages/rs-platform-wallet/src/manager/shielded_sync.rs (line 270)
STILL VALID at 6e66e65. stop() take()s the cancellation token and calls cancel() — it does not join the background thread, await an in-flight sync_now(false) / coordinator.sync() pass, or otherwise guarantee that host-visible mutations have stopped before returning. After the Phase-2b refactor the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out after callers believe shielded sync has been stopped. The clearShielded and remove_wallet flows both rely on this barrier semantic, so the gap surfaces directly in both: an in-flight pass that already snapshotted accounts/subwallets keeps running and can re-create rows even after the new purge_all_subwallets()/purge_wallet() writes have landed. Either give stop() real quiescence semantics (await the running pass) or rename it to reflect cancel-only behavior and add a separate quiesce() for Clear and unregister flows.
suggestion: clearLocalState still races any in-flight Rust shielded sync pass — no FFI quiescence barrier
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift (line 419)
STILL VALID at 6e66e65. The new coordinator.clear() → purge_all_subwallets() chain in this delta closes the prior 'stale watermarks survive Clear' gap, but the cross-language race remains. On the Rust side clearShielded does shielded_sync().stop() followed by coordinator.clear(), but ShieldedSyncManager::stop() is cancel-only. An already-running pass that snapshotted accounts/subwallets before clear() can continue through the persister fan-out after clearLocalState() returns, recreating PersistentShieldedNote / PersistentShieldedSyncState rows during or after the SwiftData delete — and, post-fix, also reinserting entries into the in-memory subwallets BTreeMap the purge just emptied. From an adversary-modeling angle: a user who initiates Clear in response to a perceived compromise cannot rely on the wipe completing before the next sync persists new private state. Closing the window requires an FFI that awaits the in-flight pass before returning (a quiesce/drain primitive), or an unbind FFI that drops the Rust-side ShieldedWallet/FileBackedShieldedStore so the persister callback can't fire post-Clear.
suggestion: bind_shielded() merges onto stale coordinator state on rebind despite documented 'replace' semantics
packages/rs-platform-wallet/src/wallet/platform_wallet.rs (line 327)
New observation at 6e66e65. The doc comment at platform_wallet.rs:319-320 promises 'a second call replaces the previously-bound shielded wallet', but the implementation only swaps self.shielded_keys (line 366-367) and then calls coordinator.register_wallet(...) + restore_for_wallet(...). register_wallet (coordinator.rs:218-223) only does accounts.insert(...) — it never removes prior entries for this wallet_id, and it does not purge the store's per-SubwalletId subwallets state. Consequence on same-process rebind: (a) accounts dropped from the new bind set keep their coordinator accounts entry, their last_synced_note_index watermark and any pending nullifier reservations in the store; (b) the new bind's restore_for_wallet then layers on top, so note selection sees stale pending_nullifiers from abandoned spend attempts and the now-orphaned account's watermark blocks re-sync. Coordinator already has the right primitives — call coordinator.unregister_wallet(self.wallet_id).await before register_wallet so rebind is replace-not-merge, matching the doc contract.
nitpick: shielded_shield_from_account(amount = 0) falls through into the expensive proof build path
packages/rs-platform-wallet/src/wallet/platform_wallet.rs (line 699)
STILL VALID at 6e66e65 — no amount == 0 guard at the Rust/FFI boundary in shielded_shield_from_account. With amount = 0 and any fee-reserve-covering balance, the selection loop exits immediately (accumulated_claim(0) >= amount(0) true on iteration 0), the post-loop check 0 < 0 is false, and the empty inputs map is forwarded into the shield build path to fail deep in Halo 2 circuit synthesis only after the ~30 s proof setup on the worker thread. SwiftExampleApp's UI guards amount > 0, but platform_wallet_manager_shielded_shield does not enforce this at the FFI boundary, so any non-Swift host (Kotlin, headless test, C consumer) pays the full proof cost before hitting the misleading failure — also a small prover-thread DoS surface against hosts that expose the FFI to less-trusted callers. Reject amount == 0 at the top of shielded_shield_from_account with PlatformWalletError::ShieldedBuildError("amount must be > 0").
if amount == 0 {
return Err(PlatformWalletError::ShieldedBuildError(
"amount must be > 0".to_string(),
));
}
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:214-290: Type 15 / Type 18 shields still report success on relay ACK, not on proven execution
STILL VALID at 6e66e65f — operations.rs untouched in this delta. `shield()` (line 216) and `shield_from_asset_lock()` (lines 280-283) call `state_transition.broadcast(sdk, None)` and return `Ok(())` as soon as one peer ACKs submission, while the same module's `unshield`, `transfer`, and `withdraw` use `broadcast_and_wait::<StateTransitionProofResult>` (lines 348, 422, 502). Through `platform_wallet_manager_shielded_shield` this lets a hostile or faulty DAPI gateway acknowledge submission, suppress retries, and make the host believe a shield succeeded even when Platform later rejects the transition or never includes it. The Type 15 path's rich `addresses_not_enough_funds` diagnostic at lines 217-237 is also unreachable under submission-only success semantics — only local broadcast-machinery errors propagate. Type 18 is worse because the asset-lock proof is single-use: a false-positive success strands the user's L1 outpoint with no in-app indication. Align Type 15/18 with the spend-side `broadcast_and_wait::<StateTransitionProofResult>` so all five shielded transitions share one success contract.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:417-437: shielded_add_account() returns Ok after updating only the wallet keyset — coordinator registry never sees the new account
STILL VALID at 6e66e65f — platform_wallet.rs:417-437 untouched. `shielded_add_account` inserts the new `OrchardKeySet` into `self.shielded_keys` and the in-code comment at lines 431-435 acknowledges it does not refresh the coordinator's `accounts` registry. After the Phase-2b refactor `NetworkShieldedCoordinator::sync()` iterates only `coordinator.accounts`, so background sync will never trial-decrypt or persist notes for the new account, while local helpers like `shielded_account_indices()`/`shielded_default_address()` accept it immediately. Notes addressed to this account silently accumulate undetected until the host runs `bind_shielded` again with the full account list. With #1/#2 now fixed, this is the last unsoundness in the wallet↔coordinator registry contract. Either drop this from the public surface or extend the signature to take `&Arc<NetworkShieldedCoordinator>` and register the new account atomically alongside the keyset insert.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/shielded_sync.rs`:270-280: ShieldedSyncManager::stop() reads as a barrier but only cancels future loop iterations
STILL VALID at 6e66e65f. `stop()` `take()`s the cancellation token and calls `cancel()` — it does not join the background thread, await an in-flight `sync_now(false)` / `coordinator.sync()` pass, or otherwise guarantee that host-visible mutations have stopped before returning. After the Phase-2b refactor the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out after callers believe shielded sync has been stopped. The `clearShielded` and `remove_wallet` flows both rely on this barrier semantic, so the gap surfaces directly in both: an in-flight pass that already snapshotted `accounts`/`subwallets` keeps running and can re-create rows even after the new `purge_all_subwallets()`/`purge_wallet()` writes have landed. Either give `stop()` real quiescence semantics (await the running pass) or rename it to reflect cancel-only behavior and add a separate `quiesce()` for Clear and unregister flows.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:419-467: clearLocalState still races any in-flight Rust shielded sync pass — no FFI quiescence barrier
STILL VALID at 6e66e65f. The new `coordinator.clear()` → `purge_all_subwallets()` chain in this delta closes the prior 'stale watermarks survive Clear' gap, but the cross-language race remains. On the Rust side `clearShielded` does `shielded_sync().stop()` followed by `coordinator.clear()`, but `ShieldedSyncManager::stop()` is cancel-only. An already-running pass that snapshotted `accounts`/`subwallets` before `clear()` can continue through the persister fan-out after `clearLocalState()` returns, recreating `PersistentShieldedNote` / `PersistentShieldedSyncState` rows during or after the SwiftData delete — and, post-fix, also reinserting entries into the in-memory `subwallets` BTreeMap the purge just emptied. From an adversary-modeling angle: a user who initiates Clear in response to a perceived compromise cannot rely on the wipe completing before the next sync persists new private state. Closing the window requires an FFI that awaits the in-flight pass before returning (a `quiesce`/drain primitive), or an unbind FFI that drops the Rust-side `ShieldedWallet`/`FileBackedShieldedStore` so the persister callback can't fire post-Clear.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:327-405: bind_shielded() merges onto stale coordinator state on rebind despite documented 'replace' semantics
New observation at 6e66e65f. The doc comment at platform_wallet.rs:319-320 promises 'a second call replaces the previously-bound shielded wallet', but the implementation only swaps `self.shielded_keys` (line 366-367) and then calls `coordinator.register_wallet(...)` + `restore_for_wallet(...)`. `register_wallet` (coordinator.rs:218-223) only does `accounts.insert(...)` — it never removes prior entries for this `wallet_id`, and it does not purge the store's per-`SubwalletId` `subwallets` state. Consequence on same-process rebind: (a) accounts dropped from the new bind set keep their coordinator `accounts` entry, their `last_synced_note_index` watermark and any pending nullifier reservations in the store; (b) the new bind's `restore_for_wallet` then layers on top, so note selection sees stale `pending_nullifiers` from abandoned spend attempts and the now-orphaned account's watermark blocks re-sync. Coordinator already has the right primitives — call `coordinator.unregister_wallet(self.wallet_id).await` before `register_wallet` so rebind is replace-not-merge, matching the doc contract.
- [NITPICK] In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:699-845: shielded_shield_from_account(amount = 0) falls through into the expensive proof build path
STILL VALID at 6e66e65f — no `amount == 0` guard at the Rust/FFI boundary in `shielded_shield_from_account`. With `amount = 0` and any fee-reserve-covering balance, the selection loop exits immediately (`accumulated_claim(0) >= amount(0)` true on iteration 0), the post-loop check `0 < 0` is false, and the empty inputs map is forwarded into the shield build path to fail deep in Halo 2 circuit synthesis only after the ~30 s proof setup on the worker thread. SwiftExampleApp's UI guards `amount > 0`, but `platform_wallet_manager_shielded_shield` does not enforce this at the FFI boundary, so any non-Swift host (Kotlin, headless test, C consumer) pays the full proof cost before hitting the misleading failure — also a small prover-thread DoS surface against hosts that expose the FFI to less-trusted callers. Reject `amount == 0` at the top of `shielded_shield_from_account` with `PlatformWalletError::ShieldedBuildError("amount must be > 0")`.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…n shield Two follow-ups from the 6e66e65 review. bind_shielded rebind — replace, not merge (the doc contract) `register_wallet` replaces the coordinator's `accounts` entries for the wallet, but never purged the store's per-`SubwalletId` state. A same-process rebind therefore left stale `last_synced_note_index` watermarks, orphaned accounts dropped from the new bind set, and abandoned `pending_nullifiers` reservations behind — the last of which can make note selection skip otherwise-spendable notes. `bind_shielded` now calls `coordinator.unregister_wallet(self.wallet_id)` (which purges store state via `purge_wallet`) before `register_wallet`, so a rebind is a clean replace; no-op on first bind. `restore_for_wallet` then re-hydrates from the persisted snapshot as before. shielded_shield_from_account — reject amount == 0 With `amount == 0` the selection loop exits on iteration 0 (claim 0 >= 0) and the post-loop `0 < 0` check doesn't fire, so an empty inputs map flowed into the ~30 s Halo 2 proof build and failed deep + opaquely. Swift's UI guards this, but other FFI hosts didn't — also a minor prover-thread DoS surface. Guard at the boundary with ShieldedBuildError. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Follow-ups to the 6e66e65 review (findings were in the review body, so summarizing here): Fixed in fd934ba:
Deferred to dedicated issues (behavior/API changes that deserve isolated commits):
Also note: a separate live spend blocker surfaced during simulator verification — repeat shielded spend fails with |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at fd934ba. The 23-line delta cleanly resolves prior findings #5 (bind_shielded merge-not-replace) and #6 (amount==0 falling through to ~30s proof setup): coordinator.unregister_wallet(self.wallet_id).await now precedes register_wallet (platform_wallet.rs:379), and shielded_shield_from_account returns ShieldedBuildError("amount must be > 0") at the API boundary (platform_wallet.rs:728-732). No new regressions in the delta. Four prior findings remain STILL VALID against unchanged surfaces and are carried forward: Type 15/18 broadcast-vs-broadcast_and_wait, shielded_add_account half-live, ShieldedSyncManager::stop cancel-only, and the clearLocalState FFI quiescence race. No new findings in the latest delta. No blockers.
🟡 4 suggestion(s)
4 additional finding(s)
suggestion: Type 15 / Type 18 shields still report success on relay ACK, not on proven execution
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (line 214)
STILL VALID at fd934ba — operations.rs unchanged in this delta. shield() (line 216) and shield_from_asset_lock() (lines 280-283) call state_transition.broadcast(sdk, None) and return Ok(()) as soon as one peer ACKs submission, while the same module's unshield, transfer, and withdraw use broadcast_and_wait::<StateTransitionProofResult> (lines 348, 422, 502). Through platform_wallet_manager_shielded_shield this lets a hostile or faulty DAPI gateway acknowledge submission, suppress retries, and make the host believe a shield succeeded even when Platform later rejects the transition or never includes it. The Type 15 path's rich addresses_not_enough_funds diagnostic at lines 217-237 is unreachable under submission-only semantics — only local broadcast-machinery errors propagate. Type 18 is worse because the asset-lock proof is single-use: a false-positive success strands the user's L1 outpoint with no in-app indication. Align Type 15/18 with the spend-side broadcast_and_wait::<StateTransitionProofResult> so all five shielded transitions share one success contract.
suggestion: shielded_add_account() returns Ok after updating only the wallet keyset — coordinator registry never sees the new account
packages/rs-platform-wallet/src/wallet/platform_wallet.rs (line 428)
STILL VALID at fd934ba — platform_wallet.rs:428-448 untouched in this delta (verified by reading the function: it inserts OrchardKeySet into self.shielded_keys at line 441, then returns Ok(()) with the comment at 442-446 acknowledging the coordinator's accounts registry isn't refreshed). After the Phase-2b refactor NetworkShieldedCoordinator::sync() iterates only coordinator.accounts, so background sync will never trial-decrypt or persist notes for the new account, while local helpers like shielded_account_indices()/shielded_default_address() accept it immediately. Notes addressed to this account silently accumulate undetected until the host runs bind_shielded again with the full account list. With the rebind-is-replace fix now landed at platform_wallet.rs:379, hosts have a clean recovery path, but the API still returns Ok after producing a half-live account. Either drop this from the public surface or extend the signature to take &Arc<NetworkShieldedCoordinator> and register the new account atomically alongside the keyset insert.
suggestion: ShieldedSyncManager::stop() reads as a barrier but only cancels future loop iterations
packages/rs-platform-wallet/src/manager/shielded_sync.rs (line 270)
STILL VALID at fd934ba — verified by reading lines 270-280: stop() take()s the cancellation token and calls token.cancel(), with no thread-join, no in-flight sync_now/coordinator.sync() await, and no guarantee that host-visible mutations have stopped before returning. After the Phase-2b refactor the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out after callers believe shielded sync has been stopped. The clearShielded and remove_wallet flows both rely on this barrier semantic, so the gap surfaces directly in both: an in-flight pass that already snapshotted accounts/subwallets keeps running and can re-create rows even after purge_all_subwallets()/purge_wallet() writes have landed. The new coordinator.unregister_wallet call in bind_shielded (line 379) is also exposed to this race on same-process rebind — a concurrent in-flight sync that snapshotted the prior accounts entry can still write to the persister between the unregister and the subsequent restore_for_wallet, producing exactly the stale-state condition the rebind purge was meant to eliminate. Either give stop() real quiescence semantics (await the running pass) or rename it to reflect cancel-only behavior and add a separate quiesce() for Clear, unregister, and rebind flows.
suggestion: clearLocalState still races any in-flight Rust shielded sync pass — no FFI quiescence barrier
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift (line 419)
STILL VALID at fd934ba. The coordinator.clear() → purge_all_subwallets() chain closes the prior 'stale watermarks survive Clear' gap, but the cross-language race remains. On the Rust side clearShielded does shielded_sync().stop() followed by coordinator.clear(), but ShieldedSyncManager::stop() is cancel-only (verified at shielded_sync.rs:270-280). An already-running pass that snapshotted accounts/subwallets before clear() can continue through the persister fan-out after clearLocalState() returns, recreating PersistentShieldedNote / PersistentShieldedSyncState rows during or after the SwiftData delete — and also reinserting entries into the in-memory subwallets BTreeMap the purge just emptied. From an adversary-modeling angle: a user who initiates Clear in response to a perceived compromise cannot rely on the wipe completing before the next sync persists new private state. Closing the window requires an FFI that awaits the in-flight pass before returning (a quiesce/drain primitive), or an unbind FFI that drops the Rust-side ShieldedWallet/FileBackedShieldedStore so the persister callback can't fire post-Clear.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:214-290: Type 15 / Type 18 shields still report success on relay ACK, not on proven execution
STILL VALID at fd934baa — operations.rs unchanged in this delta. `shield()` (line 216) and `shield_from_asset_lock()` (lines 280-283) call `state_transition.broadcast(sdk, None)` and return `Ok(())` as soon as one peer ACKs submission, while the same module's `unshield`, `transfer`, and `withdraw` use `broadcast_and_wait::<StateTransitionProofResult>` (lines 348, 422, 502). Through `platform_wallet_manager_shielded_shield` this lets a hostile or faulty DAPI gateway acknowledge submission, suppress retries, and make the host believe a shield succeeded even when Platform later rejects the transition or never includes it. The Type 15 path's rich `addresses_not_enough_funds` diagnostic at lines 217-237 is unreachable under submission-only semantics — only local broadcast-machinery errors propagate. Type 18 is worse because the asset-lock proof is single-use: a false-positive success strands the user's L1 outpoint with no in-app indication. Align Type 15/18 with the spend-side `broadcast_and_wait::<StateTransitionProofResult>` so all five shielded transitions share one success contract.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:428-448: shielded_add_account() returns Ok after updating only the wallet keyset — coordinator registry never sees the new account
STILL VALID at fd934baa — platform_wallet.rs:428-448 untouched in this delta (verified by reading the function: it inserts `OrchardKeySet` into `self.shielded_keys` at line 441, then returns `Ok(())` with the comment at 442-446 acknowledging the coordinator's `accounts` registry isn't refreshed). After the Phase-2b refactor `NetworkShieldedCoordinator::sync()` iterates only `coordinator.accounts`, so background sync will never trial-decrypt or persist notes for the new account, while local helpers like `shielded_account_indices()`/`shielded_default_address()` accept it immediately. Notes addressed to this account silently accumulate undetected until the host runs `bind_shielded` again with the full account list. With the rebind-is-replace fix now landed at platform_wallet.rs:379, hosts have a clean recovery path, but the API still returns Ok after producing a half-live account. Either drop this from the public surface or extend the signature to take `&Arc<NetworkShieldedCoordinator>` and register the new account atomically alongside the keyset insert.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/shielded_sync.rs`:270-280: ShieldedSyncManager::stop() reads as a barrier but only cancels future loop iterations
STILL VALID at fd934baa — verified by reading lines 270-280: `stop()` `take()`s the cancellation token and calls `token.cancel()`, with no thread-join, no in-flight `sync_now`/`coordinator.sync()` await, and no guarantee that host-visible mutations have stopped before returning. After the Phase-2b refactor the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out after callers believe shielded sync has been stopped. The `clearShielded` and `remove_wallet` flows both rely on this barrier semantic, so the gap surfaces directly in both: an in-flight pass that already snapshotted `accounts`/`subwallets` keeps running and can re-create rows even after `purge_all_subwallets()`/`purge_wallet()` writes have landed. The new `coordinator.unregister_wallet` call in `bind_shielded` (line 379) is also exposed to this race on same-process rebind — a concurrent in-flight sync that snapshotted the prior `accounts` entry can still write to the persister between the unregister and the subsequent `restore_for_wallet`, producing exactly the stale-state condition the rebind purge was meant to eliminate. Either give `stop()` real quiescence semantics (await the running pass) or rename it to reflect cancel-only behavior and add a separate `quiesce()` for Clear, unregister, and rebind flows.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:419-467: clearLocalState still races any in-flight Rust shielded sync pass — no FFI quiescence barrier
STILL VALID at fd934baa. The `coordinator.clear()` → `purge_all_subwallets()` chain closes the prior 'stale watermarks survive Clear' gap, but the cross-language race remains. On the Rust side `clearShielded` does `shielded_sync().stop()` followed by `coordinator.clear()`, but `ShieldedSyncManager::stop()` is cancel-only (verified at shielded_sync.rs:270-280). An already-running pass that snapshotted `accounts`/`subwallets` before `clear()` can continue through the persister fan-out after `clearLocalState()` returns, recreating `PersistentShieldedNote` / `PersistentShieldedSyncState` rows during or after the SwiftData delete — and also reinserting entries into the in-memory `subwallets` BTreeMap the purge just emptied. From an adversary-modeling angle: a user who initiates Clear in response to a perceived compromise cannot rely on the wipe completing before the next sync persists new private state. Closing the window requires an FFI that awaits the in-flight pass before returning (a `quiesce`/drain primitive), or an unbind FFI that drops the Rust-side `ShieldedWallet`/`FileBackedShieldedStore` so the persister callback can't fire post-Clear.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
|
Thanks — confirming the fd934ba review. The two fixes (rebind replace-not-merge, amount==0 guard) are acknowledged as resolved. The four carried-forward suggestions are all tracked and deferred to dedicated issues (each is a behavior/API change or a cross-language quiescence redesign that warrants its own commit, not a same-PR patch):
No new code changes this round (no new findings, no blockers per the review). Separately, the repeat-spend |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at f9810d4. New delta: rs-drive joint count-and-sum AVG no-proof dispatcher and executors land, plus surrounding routing/test additions. The only candidate new finding (codex-general's 'GroupByIn + range no-proof collapses into one entry') is demoted from blocking to suggestion: the joint dispatcher faithfully reproduces sum's pre-existing executor behavior and the prior independent count+sum AVG composition (the PR's own parity test asserts this), so it is not a regression introduced by this PR. The underlying contract drift between the AverageMode docs ('one entry per In value') and the no-prove behavior for the range case is pre-existing and now also diverges visibly from the prove path's carrier_aggregate. All four carried-forward shielded-wallet findings from fd934ba remain STILL VALID against unchanged source.
🟡 5 suggestion(s)
5 additional finding(s)
suggestion: GroupByIn + range no-proof collapses every In branch into a single entry with empty in_key/key, contradicting AverageMode docs and the prove path
packages/rs-drive/src/query/drive_document_count_and_sum_query/drive_dispatcher.rs (line 246)
When AverageMode::GroupByIn is combined with a range clause, this branch unwraps the joint executor's Aggregate { count, sum } (already folded across all In branches inside aggregate_range_count_and_sum) and rewraps it as Entries(vec![AverageEntry { in_key: None, key: Vec::new(), count: Some(count), sum: Some(sum) }]). The In axis is lost: callers cannot tell which In branch contributed what, and multiple In values are silently collapsed into one total.
Two separate contracts collide here:
- The public
AverageMode::GroupByIndoc indrive_document_average_query/mod.rs:48-49promises 'one entry perInvalue, each with its own (count, sum)'. - The prove path at
drive_document_average_query/drive_dispatcher.rs:200-241routesGroupByIn + rangetoexecute_carrier_aggregate_count_and_sum_with_proof, which preserves per-In branches.
The joint executor is faithfully matching sum's pre-existing execute_range_sum_no_proof behavior (execute_range_sum.rs:130-134 folds the compound-summed branch to a single SumEntry) and the PR's parity test (joint_range_group_by_in_executor_matches_independent_count_plus_sum) confirms this is intentional alignment with the prior independent count+sum composition — so it is not a regression introduced by this PR. But the result is that prove and no-prove now visibly disagree for the same query shape on a load-bearing public mode, and the AverageMode docs are wrong for the range case.
Pick one: either preserve per-In (count, sum) pairs on the no-prove path (use the compound aggregate fan-out's per-branch results instead of summing them into a total in aggregate_range_count_and_sum), or amend the AverageMode docstring to explicitly note that GroupByIn + range no-proof folds across In. The current 'silently fold + emit one synthetic entry with empty in_key' shape is the worst of both — it pretends to honor the GroupByIn shape while losing the dimension that gives it meaning.
suggestion: Type 15 / Type 18 shield flows still report success on relay ACK instead of proven execution
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (line 214)
Carried forward from fd934ba, STILL VALID at f9810d4 — operations.rs is unchanged in this delta. shield() (line 216) and shield_from_asset_lock() (lines 280-283) call state_transition.broadcast(sdk, None) and return Ok(()) as soon as one DAPI peer ACKs submission, while the other shielded flows in this same file (unshield, transfer, withdraw) use broadcast_and_wait::<StateTransitionProofResult> for proven execution.
The success contracts are inconsistent across otherwise-parallel FFI operations, and a faulty or malicious DAPI peer can therefore ACK the bytes, suppress retries, and make the host show a successful shield even when Platform later rejects the transition or never includes it. Type 18 is the worst case because the asset-lock proof is single-use, so a false positive at this boundary can strand the user's L1 outpoint with no in-app indication. The rich addresses_not_enough_funds diagnostic at lines 217-237 is also effectively dead under submission-only semantics — only local broadcast errors reach it. Align Type 15/18 with the spend-side flows by waiting on broadcast_and_wait::<StateTransitionProofResult>.
suggestion: shielded_add_account() returns Ok after updating only the wallet keyset; coordinator registry stays stale until rebind
packages/rs-platform-wallet/src/wallet/platform_wallet.rs (line 429)
Carried forward from fd934ba, STILL VALID at f9810d4 — platform_wallet.rs:429-448 is unchanged. The method inserts the new OrchardKeySet into self.shielded_keys and returns Ok(()), with an inline comment at lines 442-446 explicitly acknowledging that the coordinator's accounts registry is not refreshed. After the Phase-2b coordinator refactor, background shielded sync iterates the coordinator registry (not self.shielded_keys), so the newly added account becomes half-live: wallet-local helpers (shielded_account_indices, shielded_default_address) accept it immediately, but the coordinator never trial-decrypts or persists notes for it until the host re-runs bind_shielded with the full account list. Notes addressed to the new account can therefore accumulate silently while the API reports success. Either refresh the coordinator registry from within this method, or make the post-condition explicit in the API name (e.g. shielded_register_account_keys_only) so callers cannot mistake success for 'account is now syncing'.
suggestion: ShieldedSyncManager::stop() reads as a quiescence barrier but only cancels future loop iterations
packages/rs-platform-wallet/src/manager/shielded_sync.rs (line 270)
Carried forward from fd934ba, STILL VALID at f9810d4. stop() only takes the cancellation token and calls token.cancel(). It does not join the background thread, await an in-flight sync_now(false) / coordinator.sync() pass, or otherwise guarantee that host-visible mutations have ceased before returning. Callers like clearShielded and rebind flows treat stop() as if it were a barrier (the FFI host runs SwiftData deletes immediately afterward), but an already-running pass can continue through note persistence and event emission after stop() returns — cancellation is only checked between loop iterations. Add a quiescing primitive that awaits the in-flight pass (or join the worker thread) before returning, then route clearShielded and rebind flows through it. The current method's name promises more than it delivers.
suggestion: clearLocalState races an in-flight Rust shielded sync pass; private notes can be re-persisted after Clear
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift (line 419)
Carried forward from fd934ba, STILL VALID at f9810d4. clearLocalState() calls managerForStop.clearShielded() at line 451 (which on the Rust side stops the sync loop, drops coordinator registrations, and resets the caught-up cooldown), then immediately deletes the SwiftData PersistentShieldedNote / PersistentShieldedSyncState rows at lines 465-467. But ShieldedSyncManager::stop() in shielded_sync.rs:270-280 is cancellation-only, so an already-running pass that snapshotted accounts/subwallets before clearShielded() can continue through the persister callback after this method returns, writing rows into SwiftData on a different ModelContext during or after the wipe. The newer coordinator purge fixes the stale-watermark survival path, but it does not close this cross-language ordering race; without a quiescence barrier in Rust, Clear remains best-effort even when the user invokes it in response to a perceived compromise. The right fix is on the Rust side (add a true drain/quiesce primitive and call it from clearShielded) — per packages/swift-sdk/CLAUDE.md's 'marshalling, not deciding' rule, Swift should not be the layer that owns the ordering decision.
🤖 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.
- [SUGGESTION] In `packages/rs-drive/src/query/drive_document_count_and_sum_query/drive_dispatcher.rs`:246-258: GroupByIn + range no-proof collapses every In branch into a single entry with empty in_key/key, contradicting AverageMode docs and the prove path
When `AverageMode::GroupByIn` is combined with a range clause, this branch unwraps the joint executor's `Aggregate { count, sum }` (already folded across all In branches inside `aggregate_range_count_and_sum`) and rewraps it as `Entries(vec![AverageEntry { in_key: None, key: Vec::new(), count: Some(count), sum: Some(sum) }])`. The In axis is lost: callers cannot tell which In branch contributed what, and multiple In values are silently collapsed into one total.
Two separate contracts collide here:
1. The public `AverageMode::GroupByIn` doc in `drive_document_average_query/mod.rs:48-49` promises 'one entry per `In` value, each with its own (count, sum)'.
2. The prove path at `drive_document_average_query/drive_dispatcher.rs:200-241` routes `GroupByIn + range` to `execute_carrier_aggregate_count_and_sum_with_proof`, which preserves per-In branches.
The joint executor is faithfully matching sum's pre-existing `execute_range_sum_no_proof` behavior (`execute_range_sum.rs:130-134` folds the compound-summed branch to a single SumEntry) and the PR's parity test (`joint_range_group_by_in_executor_matches_independent_count_plus_sum`) confirms this is intentional alignment with the prior independent count+sum composition — so it is not a regression introduced by this PR. But the result is that prove and no-prove now visibly disagree for the same query shape on a load-bearing public mode, and the AverageMode docs are wrong for the range case.
Pick one: either preserve per-In `(count, sum)` pairs on the no-prove path (use the compound aggregate fan-out's per-branch results instead of summing them into a total in `aggregate_range_count_and_sum`), or amend the AverageMode docstring to explicitly note that `GroupByIn + range` no-proof folds across In. The current 'silently fold + emit one synthetic entry with empty in_key' shape is the worst of both — it pretends to honor the GroupByIn shape while losing the dimension that gives it meaning.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:214-283: Type 15 / Type 18 shield flows still report success on relay ACK instead of proven execution
Carried forward from fd934baa, STILL VALID at f9810d49 — operations.rs is unchanged in this delta. `shield()` (line 216) and `shield_from_asset_lock()` (lines 280-283) call `state_transition.broadcast(sdk, None)` and return `Ok(())` as soon as one DAPI peer ACKs submission, while the other shielded flows in this same file (`unshield`, `transfer`, `withdraw`) use `broadcast_and_wait::<StateTransitionProofResult>` for proven execution.
The success contracts are inconsistent across otherwise-parallel FFI operations, and a faulty or malicious DAPI peer can therefore ACK the bytes, suppress retries, and make the host show a successful shield even when Platform later rejects the transition or never includes it. Type 18 is the worst case because the asset-lock proof is single-use, so a false positive at this boundary can strand the user's L1 outpoint with no in-app indication. The rich `addresses_not_enough_funds` diagnostic at lines 217-237 is also effectively dead under submission-only semantics — only local broadcast errors reach it. Align Type 15/18 with the spend-side flows by waiting on `broadcast_and_wait::<StateTransitionProofResult>`.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:429-448: shielded_add_account() returns Ok after updating only the wallet keyset; coordinator registry stays stale until rebind
Carried forward from fd934baa, STILL VALID at f9810d49 — `platform_wallet.rs:429-448` is unchanged. The method inserts the new `OrchardKeySet` into `self.shielded_keys` and returns `Ok(())`, with an inline comment at lines 442-446 explicitly acknowledging that the coordinator's `accounts` registry is not refreshed. After the Phase-2b coordinator refactor, background shielded sync iterates the coordinator registry (not `self.shielded_keys`), so the newly added account becomes half-live: wallet-local helpers (`shielded_account_indices`, `shielded_default_address`) accept it immediately, but the coordinator never trial-decrypts or persists notes for it until the host re-runs `bind_shielded` with the full account list. Notes addressed to the new account can therefore accumulate silently while the API reports success. Either refresh the coordinator registry from within this method, or make the post-condition explicit in the API name (e.g. `shielded_register_account_keys_only`) so callers cannot mistake success for 'account is now syncing'.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/shielded_sync.rs`:270-280: ShieldedSyncManager::stop() reads as a quiescence barrier but only cancels future loop iterations
Carried forward from fd934baa, STILL VALID at f9810d49. `stop()` only takes the cancellation token and calls `token.cancel()`. It does not join the background thread, await an in-flight `sync_now(false)` / `coordinator.sync()` pass, or otherwise guarantee that host-visible mutations have ceased before returning. Callers like `clearShielded` and rebind flows treat `stop()` as if it were a barrier (the FFI host runs SwiftData deletes immediately afterward), but an already-running pass can continue through note persistence and event emission after `stop()` returns — cancellation is only checked between loop iterations. Add a quiescing primitive that awaits the in-flight pass (or join the worker thread) before returning, then route `clearShielded` and rebind flows through it. The current method's name promises more than it delivers.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:419-467: clearLocalState races an in-flight Rust shielded sync pass; private notes can be re-persisted after Clear
Carried forward from fd934baa, STILL VALID at f9810d49. `clearLocalState()` calls `managerForStop.clearShielded()` at line 451 (which on the Rust side stops the sync loop, drops coordinator registrations, and resets the caught-up cooldown), then immediately deletes the SwiftData `PersistentShieldedNote` / `PersistentShieldedSyncState` rows at lines 465-467. But `ShieldedSyncManager::stop()` in `shielded_sync.rs:270-280` is cancellation-only, so an already-running pass that snapshotted accounts/subwallets before `clearShielded()` can continue through the persister callback after this method returns, writing rows into SwiftData on a different `ModelContext` during or after the wipe. The newer coordinator purge fixes the stale-watermark survival path, but it does not close this cross-language ordering race; without a quiescence barrier in Rust, Clear remains best-effort even when the user invokes it in response to a perceived compromise. The right fix is on the Rust side (add a true drain/quiesce primitive and call it from `clearShielded`) — per `packages/swift-sdk/CLAUDE.md`'s 'marshalling, not deciding' rule, Swift should not be the layer that owns the ordering decision.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
|
Thanks @thepastaclaw. All five items here are suggestions (no blocking), and the four carried-forward shielded-wallet findings are already tracked as standalone issues and intentionally deferred out of this PR's scope:
These are behavior changes we're deliberately handling separately rather than expanding this PR. The one new item — GroupByIn + range no-proof collapsing In branches ( |
…termark Repeat shielded spends failed with "Anchor not found in the recorded anchors tree" (#3703). Root cause: sync_notes_across gated the commitment-tree append on the minimum per-subwallet watermark, so a re-fetch from a chunk boundary (or a lagging / late-bound subwallet) re-appended positions the tree already held — duplicating leaves, corrupting shardtree's internal nodes, and producing per-position witnesses that resolved against roots Platform never recorded. The checkpoint id (aligned_start + total_notes_scanned) could also collide across consecutive passes. Fix: add ShieldedStore::tree_size() (via max_leaf_position) and gate the append on the tree's own leaf count — append-once, global. Checkpoint at the true post-append tree size, which only grows so the id is strictly monotonic. Save decrypted notes per-subwallet watermark so a caught-up subwallet doesn't re-derive nullifiers and the new-note count stays honest. Add a tree_size persist/reload regression test. Verified on regtest: two consecutive shielded transfers now succeed and the rebuilt tree has consistent checkpoints (id == leaf count) with no double-append.
… Sync Status Rework the Shielded Sync Status card to show the network-wide picture rather than a single bound wallet: Total Shielded Balance summed across every wallet's unspent notes, and a Notes Synced watermark (max last-synced index across subwallets) so large pools show sync progress. Drop the Orchard address row and the per-account state breakdown; sync status, counters, and actions are unchanged. Display-only via SwiftData @query (network-wide, no wallet scoping) — no FFI.
|
Resolved 15 review threads that are addressed by code now on the branch:
Left open intentionally:
None of the remaining items block the now-working send path. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 407e0da. The latest delta (f9810d4→407e0da9) reworks the shielded commitment-tree append gate: a new ShieldedStore::tree_size() accessor replaces the prior per-subwallet min-watermark gate, making appends idempotent under multi-subwallet rescans and using the post-append tree size as a strictly-monotonic checkpoint id; per-subwallet watermarks now gate only note saving. The Rust correctness fix is structurally sound and pinned by a new persist+reload regression test. Two new findings introduced by the delta: (a) MultiSyncNotesResult.total_scanned was redefined as 'positions appended' but the exported C ABI doc, Swift mirror, and UI label still describe it as 'encrypted notes scanned' — a host-visible cross-language contract drift; (b) the new ShieldedNetworkSummaryRows view in CoreContentView.swift reads @Query with no network predicate, so wallets on multiple networks (regtest + testnet) get a cross-network sum and a max-across-networks watermark, while the Platform Sync Status section above it correctly scopes to walletIdsOnNetwork. All five prior findings remain STILL VALID against unchanged surfaces (Type 15/18 broadcast vs broadcast_and_wait, shielded_add_account half-live, ShieldedSyncManager::stop() cancel-only, clearLocalState quiescence race, rs-drive GroupByIn+range no-prove fold). No new blockers.
🟡 7 suggestion(s)
7 additional finding(s)
suggestion: `total_scanned` redefined as 'positions appended' on the Rust side, but the exported C ABI, Swift mirror, and UI still describe it as 'encrypted notes scanned'
packages/rs-platform-wallet/src/wallet/shielded/sync.rs (line 115)
New in this delta. MultiSyncNotesResult.total_scanned (sync.rs:115-120) was re-defined to mean 'commitments newly appended to the shared tree (positions ≥ tree_size)', and the returned value at sync.rs:417-423 is now appended as u64. That value flows unchanged through rs-platform-wallet-ffi/src/shielded_sync.rs into ShieldedSyncWalletResultFFI.total_scanned, whose exported ABI doc at rs-platform-wallet-ffi/src/shielded_types.rs:45 still reads Total encrypted notes scanned (decrypted + skipped). Swift's ShieldedWalletSyncResult.totalScanned mirror and the SwiftExampleApp UI carry the wire-level scan-volume framing forward unchanged.
Concrete cross-language impact: the value Swift now displays as 'encrypted notes scanned' is no longer scan volume — it's the count of positions actually appended this pass. Under the new tree_size gate the SDK routinely re-fetches positions the tree already holds (chunk-boundary realignment, lagging-subwallet rewind), and those re-fetched positions now subtract from the host-visible number while the field name and doc imply they should be included. Any host telemetry, debug log, or UI inference built on the old semantics (e.g. 'scan throughput per pass') silently becomes wrong without a compile-time signal.
Pick one: restore the old exported meaning for total_scanned on the FFI side (the underlying value is still computable from aligned_start + result.total_notes_scanned - already_have), or version/rename the field and update the C ABI doc, Swift mirror doc, and UI label to the new 'new tree positions appended' semantics so Rust and the host agree on what the number means.
suggestion: `ShieldedNetworkSummaryRows` aggregates `@Query` results across every network on disk — inconsistent with the Platform Sync Status section's `walletIdsOnNetwork` scoping
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift (line 1238)
New in this delta. ShieldedNetworkSummaryRows declares @Query private var allNotes: [PersistentShieldedNote] and @Query private var syncStates: [PersistentShieldedSyncState] with no predicate (lines 1239-1240), then folds across the entire SwiftData store: totalUnspentCredits sums every unspent note's value, and notesSynced is syncStates.map(\.lastSyncedIndex).max(). PersistentShieldedNote is keyed by (walletId, accountIndex) with no networkRaw column — the network is implicit via the parent PersistentWallet.networkRaw. The Platform Sync Status section in the same view does network scoping correctly: it builds walletIdsOnNetwork from allWallets.filter { $0.networkRaw == raw } and filters platformAddresses against it.
Real-user impact: a developer with a regtest wallet (local dev) + a testnet wallet (staging) will see the Sync Status card add their shielded balances together regardless of which network is active, while the Platform Balance row immediately above honors the active network. The notes-synced watermark is worse — Orchard commitment trees are per-chain, so taking max() across networks blends two unrelated tip positions into a single number that is meaningless against any individual network's tip. The doc comment at lines 1232-1237 explicitly assumes 'each subwallet advances toward the same tip', which is true within a network but does not hold across networks.
Fix is the same shape already used above for platformAddresses: thread the active-network walletIds set in (via init parameter so the @Query predicate can be constructor-built, or via environment) and filter both queries against it.
suggestion: Type 15 / Type 18 shield flows still report success on relay ACK instead of proven execution
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (line 214)
Carried forward, STILL VALID at 407e0da — verified by reading the function: operations.rs:216 (shield()) and 280-283 (shield_from_asset_lock()) both call state_transition.broadcast(sdk, None) and return Ok(()) as soon as one DAPI peer ACKs submission, while the same module's unshield, transfer, and withdraw use broadcast_and_wait::<StateTransitionProofResult> for proven execution.
The success contracts are inconsistent across otherwise-parallel FFI operations: a hostile or faulty DAPI gateway can ACK the bytes, suppress retries, and surface a successful shield to the iOS host even when Platform later rejects the transition or never includes it. Type 18 is the worst case because the asset-lock proof is single-use — a false positive at this boundary strands the user's L1 outpoint with no in-app indication. The rich addresses_not_enough_funds diagnostic at lines 217-237 is also effectively dead under submission-only semantics — only local broadcast errors reach it. Align Type 15/18 with the spend-side flows by waiting on broadcast_and_wait::<StateTransitionProofResult> so all five shielded transitions share one success contract.
suggestion: `shielded_add_account()` returns Ok after updating only the wallet keyset — coordinator registry stays stale until rebind
packages/rs-platform-wallet/src/wallet/platform_wallet.rs (line 429)
Carried forward, STILL VALID at 407e0da — verified by reading platform_wallet.rs:429-448. The method inserts the new OrchardKeySet into self.shielded_keys at line 441 and returns Ok(()), with the in-code comment at 442-446 explicitly acknowledging the coordinator's accounts registry isn't refreshed. After the Phase-2b refactor NetworkShieldedCoordinator::sync() iterates only coordinator.accounts, so the newly added account becomes half-live: wallet-local helpers (shielded_account_indices, shielded_default_address) accept it immediately, but the coordinator never trial-decrypts or persists notes for it until the host re-runs bind_shielded with the full account list.
Privacy/UX framing: a host that calls shielded_add_account to begin receiving to a new diversifier sees a successful return and may share the new address externally; payments to it accumulate on chain but the wallet's UI/sync state never reflects them. The PR description acknowledges this gap under deferred follow-up #3705. Either refresh the coordinator registry from within this method, or rename to make the post-condition explicit (e.g. shielded_register_account_keys_only) so callers cannot mistake success for 'account is now syncing'.
suggestion: `ShieldedSyncManager::stop()` reads as a quiescence barrier but only cancels future loop iterations
packages/rs-platform-wallet/src/manager/shielded_sync.rs (line 270)
Carried forward, STILL VALID at 407e0da — verified by reading lines 270-280: stop() only take()s the cancellation token and calls token.cancel(), with no thread-join, no in-flight sync_now/coordinator.sync() await, and no guarantee that host-visible mutations have stopped before returning. After the Phase-2b refactor the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out (which calls back into Swift's PlatformWalletPersistenceHandler on a background ModelContext) after callers believe shielded sync has been stopped.
Both the clearShielded FFI and the remove_wallet FFI rely on this barrier semantic, and the same race surfaces on bind_shielded's same-process rebind path. The new tree_size-gated append in this delta makes a stop-then-purge race slightly less catastrophic on the Rust side (a concurrent post-stop pass re-appending positions the tree already holds is now an idempotent no-op rather than a corruption), but the persister callback can still emit PersistentShieldedNote/PersistentShieldedSyncState rows back to Swift after Clear returns. PR follow-up #3706 acknowledges this gap. Either give stop() real quiescence semantics (await the running pass) or rename to reflect cancel-only behavior and expose a separate quiesce() FFI for Clear, unregister, and rebind flows.
suggestion: `clearLocalState` races an in-flight Rust shielded sync pass — private notes can be re-persisted after Clear
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift (line 419)
Carried forward, STILL VALID at 407e0da — ShieldedService.swift not in this delta. clearLocalState() calls managerForStop.clearShielded() (Rust-side: stops the sync loop via cancel-only stop(), drops coordinator registrations, purges store subwallet state), then immediately deletes PersistentShieldedNote/PersistentShieldedSyncState rows. Because ShieldedSyncManager::stop() is cancellation-only, an already-running pass that snapshotted accounts/subwallets before clearShielded() can continue through the persister callback after this method returns, writing rows into SwiftData on a different ModelContext during or after the wipe and reinserting entries into the in-memory subwallets BTreeMap the purge just emptied.
This delta's tree_size-gated append removes the worst pre-existing failure mode (tree corruption from concurrent appends), but does not close the cross-language ordering race — the persister callback writes notes/sync-state rows downstream of append_commitment, and those writes can still happen post-Clear. Per packages/swift-sdk/CLAUDE.md's 'marshalling, not deciding' rule, the fix belongs on the Rust side: add a true drain/quiesce primitive and call it from clearShielded. The new unscoped ShieldedNetworkSummaryRows @Query makes this race more visible to users — rows the persister writes post-Clear appear in the Sync Status card immediately.
suggestion: GroupByIn + range no-proof collapses every In branch into a single entry with empty in_key/key, contradicting AverageMode docs and the prove path
packages/rs-drive/src/query/drive_document_count_and_sum_query/drive_dispatcher.rs (line 246)
Carried forward from f9810d4, STILL VALID at 407e0da — rs-drive untouched by this delta. When AverageMode::GroupByIn is combined with a range clause, the no-proof branch unwraps the joint executor's Aggregate { count, sum } (already folded across all In branches inside aggregate_range_count_and_sum) and rewraps it as Entries(vec![AverageEntry { in_key: None, key: Vec::new(), count: Some(count), sum: Some(sum) }]). The In axis is lost — callers cannot tell which In branch contributed what, and multiple In values are silently collapsed into one total.
Two contracts collide: the public AverageMode::GroupByIn doc in drive_document_average_query/mod.rs:48-49 promises 'one entry per In value', and the prove path at drive_document_average_query/drive_dispatcher.rs:200-241 routes GroupByIn + range through execute_carrier_aggregate_count_and_sum_with_proof, which preserves per-In branches. So prove and no-prove visibly disagree for the same query shape on a load-bearing public mode. The joint executor faithfully matches the pre-existing execute_range_sum_no_proof fold, so this is not a regression introduced by this PR — but the AverageMode docs are wrong for the range case. Either preserve per-In (count, sum) pairs on the no-prove path, or amend the AverageMode docstring to explicitly note that GroupByIn + range no-proof folds across In.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:115-120: `total_scanned` redefined as 'positions appended' on the Rust side, but the exported C ABI, Swift mirror, and UI still describe it as 'encrypted notes scanned'
New in this delta. `MultiSyncNotesResult.total_scanned` (sync.rs:115-120) was re-defined to mean 'commitments newly appended to the shared tree (positions ≥ tree_size)', and the returned value at sync.rs:417-423 is now `appended as u64`. That value flows unchanged through `rs-platform-wallet-ffi/src/shielded_sync.rs` into `ShieldedSyncWalletResultFFI.total_scanned`, whose exported ABI doc at `rs-platform-wallet-ffi/src/shielded_types.rs:45` still reads `Total encrypted notes scanned (decrypted + skipped)`. Swift's `ShieldedWalletSyncResult.totalScanned` mirror and the SwiftExampleApp UI carry the wire-level scan-volume framing forward unchanged.
Concrete cross-language impact: the value Swift now displays as 'encrypted notes scanned' is no longer scan volume — it's the count of positions actually appended this pass. Under the new tree_size gate the SDK routinely re-fetches positions the tree already holds (chunk-boundary realignment, lagging-subwallet rewind), and those re-fetched positions now subtract from the host-visible number while the field name and doc imply they should be included. Any host telemetry, debug log, or UI inference built on the old semantics (e.g. 'scan throughput per pass') silently becomes wrong without a compile-time signal.
Pick one: restore the old exported meaning for `total_scanned` on the FFI side (the underlying value is still computable from `aligned_start + result.total_notes_scanned - already_have`), or version/rename the field and update the C ABI doc, Swift mirror doc, and UI label to the new 'new tree positions appended' semantics so Rust and the host agree on what the number means.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift`:1238-1298: `ShieldedNetworkSummaryRows` aggregates `@Query` results across every network on disk — inconsistent with the Platform Sync Status section's `walletIdsOnNetwork` scoping
New in this delta. `ShieldedNetworkSummaryRows` declares `@Query private var allNotes: [PersistentShieldedNote]` and `@Query private var syncStates: [PersistentShieldedSyncState]` with no predicate (lines 1239-1240), then folds across the entire SwiftData store: `totalUnspentCredits` sums every unspent note's value, and `notesSynced` is `syncStates.map(\.lastSyncedIndex).max()`. `PersistentShieldedNote` is keyed by `(walletId, accountIndex)` with no `networkRaw` column — the network is implicit via the parent `PersistentWallet.networkRaw`. The Platform Sync Status section in the same view does network scoping correctly: it builds `walletIdsOnNetwork` from `allWallets.filter { $0.networkRaw == raw }` and filters `platformAddresses` against it.
Real-user impact: a developer with a regtest wallet (local dev) + a testnet wallet (staging) will see the Sync Status card add their shielded balances together regardless of which network is active, while the Platform Balance row immediately above honors the active network. The notes-synced watermark is worse — Orchard commitment trees are per-chain, so taking `max()` across networks blends two unrelated tip positions into a single number that is meaningless against any individual network's tip. The doc comment at lines 1232-1237 explicitly assumes 'each subwallet advances toward the same tip', which is true within a network but does not hold across networks.
Fix is the same shape already used above for `platformAddresses`: thread the active-network `walletIds` set in (via init parameter so the `@Query` predicate can be constructor-built, or via environment) and filter both queries against it.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:214-283: Type 15 / Type 18 shield flows still report success on relay ACK instead of proven execution
Carried forward, STILL VALID at 407e0da9 — verified by reading the function: operations.rs:216 (`shield()`) and 280-283 (`shield_from_asset_lock()`) both call `state_transition.broadcast(sdk, None)` and return `Ok(())` as soon as one DAPI peer ACKs submission, while the same module's `unshield`, `transfer`, and `withdraw` use `broadcast_and_wait::<StateTransitionProofResult>` for proven execution.
The success contracts are inconsistent across otherwise-parallel FFI operations: a hostile or faulty DAPI gateway can ACK the bytes, suppress retries, and surface a successful shield to the iOS host even when Platform later rejects the transition or never includes it. Type 18 is the worst case because the asset-lock proof is single-use — a false positive at this boundary strands the user's L1 outpoint with no in-app indication. The rich `addresses_not_enough_funds` diagnostic at lines 217-237 is also effectively dead under submission-only semantics — only local broadcast errors reach it. Align Type 15/18 with the spend-side flows by waiting on `broadcast_and_wait::<StateTransitionProofResult>` so all five shielded transitions share one success contract.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:429-448: `shielded_add_account()` returns Ok after updating only the wallet keyset — coordinator registry stays stale until rebind
Carried forward, STILL VALID at 407e0da9 — verified by reading platform_wallet.rs:429-448. The method inserts the new `OrchardKeySet` into `self.shielded_keys` at line 441 and returns `Ok(())`, with the in-code comment at 442-446 explicitly acknowledging the coordinator's `accounts` registry isn't refreshed. After the Phase-2b refactor `NetworkShieldedCoordinator::sync()` iterates only `coordinator.accounts`, so the newly added account becomes half-live: wallet-local helpers (`shielded_account_indices`, `shielded_default_address`) accept it immediately, but the coordinator never trial-decrypts or persists notes for it until the host re-runs `bind_shielded` with the full account list.
Privacy/UX framing: a host that calls `shielded_add_account` to begin receiving to a new diversifier sees a successful return and may share the new address externally; payments to it accumulate on chain but the wallet's UI/sync state never reflects them. The PR description acknowledges this gap under deferred follow-up #3705. Either refresh the coordinator registry from within this method, or rename to make the post-condition explicit (e.g. `shielded_register_account_keys_only`) so callers cannot mistake success for 'account is now syncing'.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/shielded_sync.rs`:270-280: `ShieldedSyncManager::stop()` reads as a quiescence barrier but only cancels future loop iterations
Carried forward, STILL VALID at 407e0da9 — verified by reading lines 270-280: `stop()` only `take()`s the cancellation token and calls `token.cancel()`, with no thread-join, no in-flight `sync_now`/`coordinator.sync()` await, and no guarantee that host-visible mutations have stopped before returning. After the Phase-2b refactor the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out (which calls back into Swift's `PlatformWalletPersistenceHandler` on a background `ModelContext`) after callers believe shielded sync has been stopped.
Both the `clearShielded` FFI and the `remove_wallet` FFI rely on this barrier semantic, and the same race surfaces on `bind_shielded`'s same-process rebind path. The new `tree_size`-gated append in this delta makes a stop-then-purge race slightly less catastrophic on the Rust side (a concurrent post-stop pass re-appending positions the tree already holds is now an idempotent no-op rather than a corruption), but the persister callback can still emit `PersistentShieldedNote`/`PersistentShieldedSyncState` rows back to Swift after Clear returns. PR follow-up #3706 acknowledges this gap. Either give `stop()` real quiescence semantics (await the running pass) or rename to reflect cancel-only behavior and expose a separate `quiesce()` FFI for Clear, unregister, and rebind flows.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:419-467: `clearLocalState` races an in-flight Rust shielded sync pass — private notes can be re-persisted after Clear
Carried forward, STILL VALID at 407e0da9 — ShieldedService.swift not in this delta. `clearLocalState()` calls `managerForStop.clearShielded()` (Rust-side: stops the sync loop via cancel-only `stop()`, drops coordinator registrations, purges store subwallet state), then immediately deletes `PersistentShieldedNote`/`PersistentShieldedSyncState` rows. Because `ShieldedSyncManager::stop()` is cancellation-only, an already-running pass that snapshotted accounts/subwallets before `clearShielded()` can continue through the persister callback after this method returns, writing rows into SwiftData on a different `ModelContext` during or after the wipe and reinserting entries into the in-memory `subwallets` BTreeMap the purge just emptied.
This delta's `tree_size`-gated append removes the worst pre-existing failure mode (tree corruption from concurrent appends), but does not close the cross-language ordering race — the persister callback writes notes/sync-state rows downstream of `append_commitment`, and those writes can still happen post-Clear. Per `packages/swift-sdk/CLAUDE.md`'s 'marshalling, not deciding' rule, the fix belongs on the Rust side: add a true drain/quiesce primitive and call it from `clearShielded`. The new unscoped `ShieldedNetworkSummaryRows` `@Query` makes this race more visible to users — rows the persister writes post-Clear appear in the Sync Status card immediately.
- [SUGGESTION] In `packages/rs-drive/src/query/drive_document_count_and_sum_query/drive_dispatcher.rs`:246-258: GroupByIn + range no-proof collapses every In branch into a single entry with empty in_key/key, contradicting AverageMode docs and the prove path
Carried forward from f9810d49, STILL VALID at 407e0da9 — rs-drive untouched by this delta. When `AverageMode::GroupByIn` is combined with a range clause, the no-proof branch unwraps the joint executor's `Aggregate { count, sum }` (already folded across all In branches inside `aggregate_range_count_and_sum`) and rewraps it as `Entries(vec![AverageEntry { in_key: None, key: Vec::new(), count: Some(count), sum: Some(sum) }])`. The In axis is lost — callers cannot tell which In branch contributed what, and multiple In values are silently collapsed into one total.
Two contracts collide: the public `AverageMode::GroupByIn` doc in `drive_document_average_query/mod.rs:48-49` promises 'one entry per `In` value', and the prove path at `drive_document_average_query/drive_dispatcher.rs:200-241` routes `GroupByIn + range` through `execute_carrier_aggregate_count_and_sum_with_proof`, which preserves per-In branches. So prove and no-prove visibly disagree for the same query shape on a load-bearing public mode. The joint executor faithfully matches the pre-existing `execute_range_sum_no_proof` fold, so this is not a regression introduced by this PR — but the AverageMode docs are wrong for the range case. Either preserve per-In `(count, sum)` pairs on the no-prove path, or amend the AverageMode docstring to explicitly note that `GroupByIn + range` no-proof folds across In.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
|
Thanks, agreed on scope: none of those deferred/follow-up items should block this PR. I opened the pre-existing |
- Spend flows: post-broadcast `mark_notes_spent` is now best-effort. A local spent-state write failure after a successful `broadcast_and_wait` no longer surfaces as a send failure (it warns; the next nullifier sync reconciles), so a successful spend can't be reported as failed and invite duplicate retries. - Sync: the tree checkpoint id hard-errors instead of saturating at `u32::MAX`, preserving the strict-monotonic-id guarantee the tree_size append gate depends on. - Sync: `MultiSyncNotesResult.total_scanned` keeps its documented wire-level scan-volume meaning (matching the exported FFI/Swift/UI "Scanned" contract) rather than the "positions appended" value the #3703 change had introduced. - FFI: validate the shielded load/free callback pairs before restore — a loader wired without its matching free callback leaked the host-allocated buffer on every load. - Extract `select_shield_inputs` (pure) + unit tests: dust-skip, exact-reserve, amount==total-reserve, multi-input accrual, and insufficient-balance.
- Send: validate the scaled integer for the active ledger (duffs for Core, credits for shielded/platform) is > 0 before enabling send, so sub-unit amounts can't slip through as 0. - Send: the Platform->Shielded shield always self-shields, so block the send when a non-empty entered recipient differs from the wallet's own Orchard address instead of silently reporting success. - bindShielded: enforce the documented <= 64 account cap before crossing the FFI boundary. - Persistence: scope shielded note / sync-state restore to the handler's network so a per-network manager can't rehydrate foreign-network rows. - Sync Status: `ShieldedNetworkSummaryRows` is scoped to the active network's wallet ids — no cross-network balance sum or max-across- networks watermark. - AccountListView: `shieldedAccountsForThisWallet` gates on the bound wallet id so it can't briefly show a previous wallet's accounts.
… single-flight - Reuse rs-sdk's canonical `fetch_inputs_with_nonce` (now `pub` + re-exported from `platform::transition`) in the shield path instead of re-implementing the fetch-and-validate dance. This swaps the old warn-and-proceed balance check for the SDK's hard `AddressNotEnoughFundsError`, so an underfunded shield fails before the ~30 s Halo 2 proof rather than getting rejected by drive-abci after it. The local `checked_add(1)` nonce increment is kept (the canonical `nonce_inc` uses an unchecked `+ 1` that would wrap a u32::MAX address into a replay nonce). - Add a per-wallet `shield_guard` (`Arc<Mutex<()>>`, shared across cloned wallet handles) held across fetch → build → broadcast in `shielded_shield_from_account`, serializing shield-class operations so two concurrent calls (double-tap / retry-while-proving) can't fetch and build with the same address nonce and have the second rejected as a replay. Addresses review threads on the shield-flow nonce handling (reuse rs-sdk helpers; concurrent-shield TOCTOU).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 7917a9f. Prior findings #1 (total_scanned cross-language drift) and #2 (ShieldedNetworkSummaryRows unscoped @query) are FIXED. Prior findings #3 (Type 15/18 ACK-only success), #4 (shielded_add_account half-live, deferred #3705), #5 (ShieldedSyncManager::stop cancel-only, deferred #3706), #6 (clearLocalState race), and #7 (rs-drive GroupByIn+range no-proof fold) remain STILL VALID against unchanged source and are carried forward. New in this delta: Codex caught a real SendViewModel regression — the new self-shield guard explicitly tells the user they may leave the recipient blank, but detectAddressType clears detectedFlow when the field is empty so canSend disables the send path; the blank-recipient affordance is unreachable. The delta also lands several net-defensive improvements (pure unit-tested select_shield_inputs, u32 checkpoint-id hard-fail, FFI persistence callback-pair validation, canSend integer-amount gating, AccountListView wallet-id scoping, best-effort finalize_pending post-broadcast). No new blockers.
🟡 6 suggestion(s) | 💬 1 nitpick(s)
7 additional finding(s)
suggestion: New self-shield guard tells the user the recipient may be left blank, but blank input still disables the Platform→Shielded send path
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift (line 160)
NEW in this delta. The new guard at executeSend's .platformToShielded branch (around lines 412-422) reads: "Shield always sends to your own shielded address; enter your own address or leave it blank." The Rust shieldedShield FFI has no recipient parameter and always shields to the wallet's own default Orchard address, so the blank-recipient affordance is the intended UX.
The affordance is unreachable, though. detectAddressType() at lines 160-167 explicitly sets detectedAddressType = .unknown, detectedFlow = nil, and estimatedFee = nil when the trimmed recipient is empty. updateFlow() at lines 174-192 only maps .platformToShielded from (.orchard, .platform), so the blank input never produces a non-nil flow. canSend at line 120 requires detectedFlow != nil, so an empty recipient field disables the send button entirely. Users must paste their own Orchard address manually for shield to work — directly contradicting the in-app instruction.
Fix shape: when selectedSource == .platform and the recipient field is blank, treat the flow as .platformToShielded (or compute it from selectedSource alone for that one source). Either thread the empty-recipient case through updateFlow(), or short-circuit in canSend for the self-shield case.
suggestion: Type 15 / Type 18 shield flows still report success on relay ACK instead of proven execution
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (line 214)
CARRIED FORWARD, STILL VALID at 7917a9f. The latest delta only touches post-broadcast finalize_pending in unshield/transfer/withdraw; the shield-side broadcast contract is unchanged. shield() (line 216) and shield_from_asset_lock() (lines 280-283) call state_transition.broadcast(sdk, None) and return Ok(()) as soon as one DAPI peer ACKs submission, while unshield, transfer, withdraw (lines 348, 422, 502) use broadcast_and_wait::<StateTransitionProofResult> for proven execution.
A hostile or faulty DAPI gateway can ACK the bytes, suppress retries, and surface a successful shield to the iOS host even when Platform later rejects the transition or never includes it. Type 18 is the worst case because the asset-lock proof is single-use — a false positive at this boundary strands the user's L1 outpoint with no in-app indication. The rich addresses_not_enough_funds diagnostic at lines 217-237 is also effectively dead under submission-only semantics. Align Type 15/18 with the spend-side flows by waiting on broadcast_and_wait::<StateTransitionProofResult>. Tracked under PR follow-up #3704.
suggestion: shielded_add_account() returns Ok after updating only the wallet keyset — coordinator registry stays stale until rebind
packages/rs-platform-wallet/src/wallet/platform_wallet.rs (line 428)
CARRIED FORWARD, STILL VALID at 7917a9f. The delta extracts select_shield_inputs and adds tests but leaves shielded_add_account unchanged. The method inserts the new OrchardKeySet into self.shielded_keys and returns Ok(()) with an in-code comment acknowledging the coordinator's accounts registry isn't refreshed. After Phase-2b, NetworkShieldedCoordinator::sync() iterates only coordinator.accounts, so the newly added account becomes half-live: wallet-local helpers (shielded_account_indices, shielded_default_address) accept it immediately, but the coordinator never trial-decrypts or persists notes for it until the host re-runs bind_shielded.
Privacy/UX framing: a host that calls shielded_add_account to begin receiving to a new diversifier sees a successful return and may share the new address externally; on-chain payments accumulate but the UI never reflects them. The new SendViewModel self-shield enforcement at SendViewModel.swift:412-422 restricts shield destinations to addressesByAccount[0], reinforcing this gap (only account 0 is live). Tracked under PR follow-up #3705. Either refresh the coordinator registry from within this method, or rename to make the post-condition explicit (e.g. shielded_register_account_keys_only).
suggestion: ShieldedSyncManager::stop() reads as a quiescence barrier but only cancels future loop iterations
packages/rs-platform-wallet/src/manager/shielded_sync.rs (line 270)
CARRIED FORWARD, STILL VALID at 7917a9f — shielded_sync.rs unchanged in this delta. stop() only take()s the cancellation token and calls token.cancel(), with no thread-join, no in-flight sync_now/coordinator.sync() await, and no guarantee that host-visible mutations have stopped before returning. After Phase-2b, the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out (calling back into Swift's PlatformWalletPersistenceHandler on a background ModelContext) after callers believe shielded sync has been stopped.
Security-relevant consumers that rely on the barrier semantic: clearShielded (used when the user invokes Clear in response to a perceived compromise), remove_wallet, and same-process bind_shielded. The prior delta's tree_size-gated append keeps a post-stop concurrent pass from corrupting the tree, but the persister callback can still emit PersistentShieldedNote/PersistentShieldedSyncState rows after Clear returns. Boundary concern: platform_wallet_manager_shielded_sync_stop exposes a synchronous FFI call hosts reasonably read as 'the loop is stopped now', which is false. Tracked under PR follow-up #3706. Either give stop() real quiescence semantics or expose a separate quiesce() FFI for Clear/unregister/rebind.
suggestion: clearLocalState races an in-flight Rust shielded sync pass — private notes can be re-persisted after Clear
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift (line 419)
CARRIED FORWARD, STILL VALID at 7917a9f — ShieldedService.swift not in this delta. clearLocalState() calls managerForStop.clearShielded() (Rust-side: stops the sync loop via cancel-only stop(), drops coordinator registrations, purges store subwallet state), then immediately deletes PersistentShieldedNote/PersistentShieldedSyncState rows. Because ShieldedSyncManager::stop() is cancellation-only, an already-running pass that snapshotted accounts/subwallets before clearShielded() can continue through the persister callback after this method returns, writing rows into SwiftData on a different ModelContext during or after the wipe.
Adversary framing: a user who triggers Clear because they believe their device is compromised cannot rely on the wipe completing before the next sync pass persists new private state — encrypted note metadata for the same Orchard accounts will reappear in SwiftData under the original wallet id. The new walletIds-scoped ShieldedNetworkSummaryRows makes any post-Clear leak immediately visible in the Sync Status card. Per packages/swift-sdk/CLAUDE.md's 'marshalling, not deciding' rule, the fix belongs on the Rust side: add a true drain/quiesce primitive and call it from clearShielded.
nitpick: Best-effort `finalize_pending` post-broadcast load-bears on durable pre-broadcast pending_nullifier locks — invariants worth pinning
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (line 354)
NEW in this delta. unshield (line 358), transfer (line 444), and withdraw (line 533) downgrade finalize_pending failure from ? propagation to warn!()-and-continue. The in-code rationale is defensible: 'broadcast already succeeded; surfacing a local write failure as a send failure would invite duplicate retries — the next nullifier sync reconciles any drift.'
The new contract makes the safety of the post-broadcast window depend on invariants not checked at this layer: (1) pending_nullifiers was taken pre-broadcast and survives finalize_pending failure; (2) pending_nullifiers persistence is durable across process restarts (not in-memory only in FileBackedShieldedStore::subwallets); (3) nullifier sync runs before next spend attempt. None are new in this delta, but the explicit best-effort path makes the window structurally common rather than rare.
FFI angle: the warn! log never crosses the Rust/Swift boundary today (no tracing→FFI bridge). Before this change, ShieldedStoreError flowed through WalletShieldedOutcome::Err → ShieldedSyncWalletResultFFI.error_message. After, the host sees Ok(()) and the same notes remain marked unspent locally; next selection picks them again, the user pays the ~30s Halo 2 proof cost, and broadcast fails with a nullifier-already-used error — wasted CPU but not fund loss. Worth either a comment block citing which durable state property guarantees no re-spend, or a focused test asserting pending_nullifiers survives finalize_pending failure + reload.
suggestion: GroupByIn + range no-proof collapses every In branch into a single entry with empty in_key/key, contradicting AverageMode docs and the prove path
packages/rs-drive/src/query/drive_document_count_and_sum_query/drive_dispatcher.rs (line 246)
CARRIED FORWARD from f9810d4, STILL VALID at 7917a9f — rs-drive untouched by this delta. When AverageMode::GroupByIn is combined with a range clause, the no-proof branch unwraps the joint executor's Aggregate { count, sum } (already folded across all In branches inside aggregate_range_count_and_sum) and rewraps it as Entries(vec![AverageEntry { in_key: None, key: Vec::new(), count: Some(count), sum: Some(sum) }]). The In axis is lost.
Two contracts collide: the public AverageMode::GroupByIn doc at drive_document_average_query/mod.rs:48-49 promises 'one entry per In value', and the prove path at drive_document_average_query/drive_dispatcher.rs:200-241 routes GroupByIn + range through execute_carrier_aggregate_count_and_sum_with_proof, which preserves per-In branches. Prove and no-prove visibly disagree for the same query shape on a load-bearing public mode. SDK clients switching between prove and no-prove paths silently see different result shapes — a class of cross-mode aliasing that can mask query-result tampering by a malicious server. Not a regression introduced by this PR. Either preserve per-In (count, sum) pairs on the no-prove path, or amend the AverageMode docstring to explicitly note that GroupByIn + range no-proof folds across In.
🤖 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.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift`:160-192: New self-shield guard tells the user the recipient may be left blank, but blank input still disables the Platform→Shielded send path
NEW in this delta. The new guard at executeSend's `.platformToShielded` branch (around lines 412-422) reads: "Shield always sends to your own shielded address; enter your own address or leave it blank." The Rust `shieldedShield` FFI has no recipient parameter and always shields to the wallet's own default Orchard address, so the blank-recipient affordance is the intended UX.
The affordance is unreachable, though. `detectAddressType()` at lines 160-167 explicitly sets `detectedAddressType = .unknown`, `detectedFlow = nil`, and `estimatedFee = nil` when the trimmed recipient is empty. `updateFlow()` at lines 174-192 only maps `.platformToShielded` from `(.orchard, .platform)`, so the blank input never produces a non-nil flow. `canSend` at line 120 requires `detectedFlow != nil`, so an empty recipient field disables the send button entirely. Users must paste their own Orchard address manually for shield to work — directly contradicting the in-app instruction.
Fix shape: when `selectedSource == .platform` and the recipient field is blank, treat the flow as `.platformToShielded` (or compute it from `selectedSource` alone for that one source). Either thread the empty-recipient case through `updateFlow()`, or short-circuit in `canSend` for the self-shield case.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:214-290: Type 15 / Type 18 shield flows still report success on relay ACK instead of proven execution
CARRIED FORWARD, STILL VALID at 7917a9f79d. The latest delta only touches post-broadcast `finalize_pending` in unshield/transfer/withdraw; the shield-side broadcast contract is unchanged. `shield()` (line 216) and `shield_from_asset_lock()` (lines 280-283) call `state_transition.broadcast(sdk, None)` and return `Ok(())` as soon as one DAPI peer ACKs submission, while `unshield`, `transfer`, `withdraw` (lines 348, 422, 502) use `broadcast_and_wait::<StateTransitionProofResult>` for proven execution.
A hostile or faulty DAPI gateway can ACK the bytes, suppress retries, and surface a successful shield to the iOS host even when Platform later rejects the transition or never includes it. Type 18 is the worst case because the asset-lock proof is single-use — a false positive at this boundary strands the user's L1 outpoint with no in-app indication. The rich `addresses_not_enough_funds` diagnostic at lines 217-237 is also effectively dead under submission-only semantics. Align Type 15/18 with the spend-side flows by waiting on `broadcast_and_wait::<StateTransitionProofResult>`. Tracked under PR follow-up #3704.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:428-448: shielded_add_account() returns Ok after updating only the wallet keyset — coordinator registry stays stale until rebind
CARRIED FORWARD, STILL VALID at 7917a9f79d. The delta extracts `select_shield_inputs` and adds tests but leaves `shielded_add_account` unchanged. The method inserts the new `OrchardKeySet` into `self.shielded_keys` and returns `Ok(())` with an in-code comment acknowledging the coordinator's `accounts` registry isn't refreshed. After Phase-2b, `NetworkShieldedCoordinator::sync()` iterates only `coordinator.accounts`, so the newly added account becomes half-live: wallet-local helpers (`shielded_account_indices`, `shielded_default_address`) accept it immediately, but the coordinator never trial-decrypts or persists notes for it until the host re-runs `bind_shielded`.
Privacy/UX framing: a host that calls `shielded_add_account` to begin receiving to a new diversifier sees a successful return and may share the new address externally; on-chain payments accumulate but the UI never reflects them. The new SendViewModel self-shield enforcement at SendViewModel.swift:412-422 restricts shield destinations to `addressesByAccount[0]`, reinforcing this gap (only account 0 is live). Tracked under PR follow-up #3705. Either refresh the coordinator registry from within this method, or rename to make the post-condition explicit (e.g. `shielded_register_account_keys_only`).
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/shielded_sync.rs`:270-280: ShieldedSyncManager::stop() reads as a quiescence barrier but only cancels future loop iterations
CARRIED FORWARD, STILL VALID at 7917a9f79d — shielded_sync.rs unchanged in this delta. `stop()` only `take()`s the cancellation token and calls `token.cancel()`, with no thread-join, no in-flight `sync_now`/`coordinator.sync()` await, and no guarantee that host-visible mutations have stopped before returning. After Phase-2b, the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out (calling back into Swift's `PlatformWalletPersistenceHandler` on a background `ModelContext`) after callers believe shielded sync has been stopped.
Security-relevant consumers that rely on the barrier semantic: `clearShielded` (used when the user invokes Clear in response to a perceived compromise), `remove_wallet`, and same-process `bind_shielded`. The prior delta's tree_size-gated append keeps a post-stop concurrent pass from corrupting the tree, but the persister callback can still emit `PersistentShieldedNote`/`PersistentShieldedSyncState` rows after Clear returns. Boundary concern: `platform_wallet_manager_shielded_sync_stop` exposes a synchronous FFI call hosts reasonably read as 'the loop is stopped now', which is false. Tracked under PR follow-up #3706. Either give `stop()` real quiescence semantics or expose a separate `quiesce()` FFI for Clear/unregister/rebind.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:419-467: clearLocalState races an in-flight Rust shielded sync pass — private notes can be re-persisted after Clear
CARRIED FORWARD, STILL VALID at 7917a9f79d — ShieldedService.swift not in this delta. `clearLocalState()` calls `managerForStop.clearShielded()` (Rust-side: stops the sync loop via cancel-only `stop()`, drops coordinator registrations, purges store subwallet state), then immediately deletes `PersistentShieldedNote`/`PersistentShieldedSyncState` rows. Because `ShieldedSyncManager::stop()` is cancellation-only, an already-running pass that snapshotted accounts/subwallets before `clearShielded()` can continue through the persister callback after this method returns, writing rows into SwiftData on a different `ModelContext` during or after the wipe.
Adversary framing: a user who triggers Clear because they believe their device is compromised cannot rely on the wipe completing before the next sync pass persists new private state — encrypted note metadata for the same Orchard accounts will reappear in SwiftData under the original wallet id. The new `walletIds`-scoped `ShieldedNetworkSummaryRows` makes any post-Clear leak immediately visible in the Sync Status card. Per `packages/swift-sdk/CLAUDE.md`'s 'marshalling, not deciding' rule, the fix belongs on the Rust side: add a true drain/quiesce primitive and call it from `clearShielded`.
- [NITPICK] In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:354-540: Best-effort `finalize_pending` post-broadcast load-bears on durable pre-broadcast pending_nullifier locks — invariants worth pinning
NEW in this delta. `unshield` (line 358), `transfer` (line 444), and `withdraw` (line 533) downgrade `finalize_pending` failure from `?` propagation to `warn!()`-and-continue. The in-code rationale is defensible: 'broadcast already succeeded; surfacing a local write failure as a send failure would invite duplicate retries — the next nullifier sync reconciles any drift.'
The new contract makes the safety of the post-broadcast window depend on invariants not checked at this layer: (1) `pending_nullifiers` was taken pre-broadcast and survives `finalize_pending` failure; (2) `pending_nullifiers` persistence is durable across process restarts (not in-memory only in `FileBackedShieldedStore::subwallets`); (3) nullifier sync runs before next spend attempt. None are new in this delta, but the explicit best-effort path makes the window structurally common rather than rare.
FFI angle: the `warn!` log never crosses the Rust/Swift boundary today (no tracing→FFI bridge). Before this change, `ShieldedStoreError` flowed through `WalletShieldedOutcome::Err → ShieldedSyncWalletResultFFI.error_message`. After, the host sees `Ok(())` and the same notes remain marked unspent locally; next selection picks them again, the user pays the ~30s Halo 2 proof cost, and broadcast fails with a nullifier-already-used error — wasted CPU but not fund loss. Worth either a comment block citing which durable state property guarantees no re-spend, or a focused test asserting `pending_nullifiers` survives `finalize_pending` failure + reload.
- [SUGGESTION] In `packages/rs-drive/src/query/drive_document_count_and_sum_query/drive_dispatcher.rs`:246-258: GroupByIn + range no-proof collapses every In branch into a single entry with empty in_key/key, contradicting AverageMode docs and the prove path
CARRIED FORWARD from f9810d49, STILL VALID at 7917a9f79d — rs-drive untouched by this delta. When `AverageMode::GroupByIn` is combined with a range clause, the no-proof branch unwraps the joint executor's `Aggregate { count, sum }` (already folded across all In branches inside `aggregate_range_count_and_sum`) and rewraps it as `Entries(vec![AverageEntry { in_key: None, key: Vec::new(), count: Some(count), sum: Some(sum) }])`. The In axis is lost.
Two contracts collide: the public `AverageMode::GroupByIn` doc at `drive_document_average_query/mod.rs:48-49` promises 'one entry per `In` value', and the prove path at `drive_document_average_query/drive_dispatcher.rs:200-241` routes `GroupByIn + range` through `execute_carrier_aggregate_count_and_sum_with_proof`, which preserves per-In branches. Prove and no-prove visibly disagree for the same query shape on a load-bearing public mode. SDK clients switching between prove and no-prove paths silently see different result shapes — a class of cross-mode aliasing that can mask query-result tampering by a malicious server. Not a regression introduced by this PR. Either preserve per-In `(count, sum)` pairs on the no-prove path, or amend the AverageMode docstring to explicitly note that `GroupByIn + range` no-proof folds across In.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…effort finalize invariant - The Platform→Shielded self-shield guard previously told the user they could "leave it blank", but a blank recipient clears `detectedFlow` upstream (detectAddressType → updateFlow) so `canSend` disables the button — the branch is only reachable with a non-empty address. Reword the message to match the working flow (enter your own shielded address). (Making blank-input self-shield reachable would need a send-flow redesign — source selection with an empty recipient — and is left as a follow-up.) - Document the no-double-spend invariant behind the best-effort post-broadcast `finalize_pending`: the authoritative no-reuse guarantee is the on-chain nullifier set, not the local mark, so a failed finalize worst-cases to a re-selected note rejected at broadcast (wasted proof, never fund loss); `pending_nullifiers` is in-memory only.
|
Thanks @thepastaclaw. The two new findings in this review are fixed in 64096d9:
Also note 82cb6e5 (pushed just before this review's sha) already addressed the shield-flow nonce items from the prior round — reuse of rs-sdk's The remaining open items are all deferred/tracked: Type 15/18 relay-ACK → #3704, |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 82cb6e5. The 46-line delta is a clean defensive hardening of the Type 15 shield flow: per-wallet shield_guard: Arc<tokio::sync::Mutex<()>> serializes selection→build→broadcast on a single wallet, and the inline AddressInfo::fetch_many + warn-and-proceed dance is replaced with rs-sdk's canonical fetch_inputs_with_nonce (now pub, re-exported from transition), which hard-fails via AddressNotEnoughFundsError before paying the ~30s Halo 2 proof. The checked_add(1) on the returned nonce closes a u32::MAX wrap. All seven prior findings remain STILL VALID against unchanged source and are carried forward (Type 15/18 ACK-vs-proven, shielded_add_account half-live, stop() cancel-only, clearLocalState race, blank-recipient self-shield, finalize_pending warn-and-continue, rs-drive GroupByIn+range no-proof shape). New findings in the latest delta: (a) nonce_inc in rs-sdk still uses unchecked +1 for every other caller (put_identity, transfer_address_funds, top_up_identity_from_addresses, address_credit_withdrawal, rs-sdk's own shield.rs), so the wrap defense applies only to the platform-wallet shield path; (b) the cross-crate error funnel in operations::shield collapses the structured AddressNotEnoughFundsError to a flat string before it reaches Swift, making the now-common pre-broadcast failure path less informative than the rare post-broadcast one. No blockers; deferred shield items are tracked under PR follow-ups #3704/#3705/#3706.
🟡 8 suggestion(s) | 💬 1 nitpick(s)
9 additional finding(s)
suggestion: Type 15 / Type 18 shield flows still report success on relay ACK instead of proven execution
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (line 187)
CARRIED FORWARD, STILL VALID at 82cb6e5. Verified by reading the file: shield() at line 187 calls state_transition.broadcast(sdk, None).await and returns Ok(()) once submission is acknowledged, and shield_from_asset_lock() at lines 251-254 does the same, while unshield/transfer/withdraw use broadcast_and_wait::<StateTransitionProofResult> for proven execution. The latest delta tightens the pre-broadcast validation (hard balance check + checked nonce + single-flight guard) but does not change the post-broadcast success contract. A hostile or faulty DAPI gateway can ACK the bytes, suppress retries, and surface a successful shield to the iOS host even when Platform later rejects the transition or never includes it. Type 18 is the worst case because the asset-lock proof is single-use — a false-positive at this boundary strands the user's L1 outpoint with no in-app indication. Under the new pre-flight, the rich addresses_not_enough_funds diagnostic at lines 188-208 is also even harder to reach: the under-balance class is rejected before broadcast, so this handler now only fires for races between the pre-flight read and the proven-block read. Align Type 15/18 with the spend-side broadcast_and_wait::<StateTransitionProofResult> so all five shielded transitions share one success contract. Tracked under PR follow-up #3704.
suggestion: shielded_add_account() returns Ok after updating only the wallet keyset — coordinator registry stays stale until rebind
packages/rs-platform-wallet/src/wallet/platform_wallet.rs (line 429)
CARRIED FORWARD, STILL VALID at 82cb6e5. Verified at platform_wallet.rs:440-459: the method inserts the new OrchardKeySet into self.shielded_keys at line 452 and returns Ok(()), with an in-code comment at 453-457 explicitly acknowledging the coordinator's accounts registry isn't refreshed. After the Phase-2b refactor NetworkShieldedCoordinator::sync() iterates only coordinator.accounts, so the newly added account is half-live: wallet-local helpers (shielded_account_indices, shielded_default_address) accept it immediately, but background sync never trial-decrypts or persists notes for it until the host re-runs bind_shielded with the full account list. Privacy framing: a host that calls shielded_add_account to begin receiving to a new diversifier sees a successful return and may share the new address externally; on-chain payments accumulate but the wallet's UI/sync state never reflects them. The SendViewModel self-shield enforcement structurally hides the gap on the send side (only account 0 is offered) but not on the receive side. The Result<(), _> post-condition is too broad for what the function actually guarantees. Tracked under PR follow-up #3705. Either refresh the coordinator registry from within this method, or rename to make the post-condition explicit (e.g. shielded_register_account_keys_only).
suggestion: ShieldedSyncManager::stop() reads as a quiescence barrier but only cancels future loop iterations
packages/rs-platform-wallet/src/manager/shielded_sync.rs (line 270)
CARRIED FORWARD, STILL VALID at 82cb6e5 — shielded_sync.rs unchanged in this delta. stop() only take()s the cancellation token and calls token.cancel(), with no thread-join, no in-flight sync_now/coordinator.sync() await, and no guarantee that host-visible mutations have stopped before returning. After Phase-2b the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out (calling back into Swift's PlatformWalletPersistenceHandler on a background ModelContext) after callers believe shielded sync has been stopped. Security-relevant consumers that rely on the barrier semantic: clearShielded (used in response to perceived device compromise), remove_wallet, and same-process bind_shielded. The new per-wallet shield_guard in this delta is orthogonal — it serializes shield-class FFI calls on a single wallet but does not interact with the manager-wide sync loop. platform_wallet_manager_shielded_sync_stop exposes a synchronous FFI call hosts reasonably read as 'the loop is stopped now', which is false. Tracked under PR follow-up #3706. Either give stop() real quiescence semantics or expose a separate quiesce() FFI for Clear/unregister/rebind.
suggestion: clearLocalState races an in-flight Rust shielded sync pass — private notes can be re-persisted after Clear
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift (line 419)
CARRIED FORWARD, STILL VALID at 82cb6e5 — ShieldedService.swift not in this delta. clearLocalState() calls managerForStop.clearShielded() (Rust-side: stops the sync loop via cancel-only stop(), drops coordinator registrations, purges store subwallet state), then immediately deletes PersistentShieldedNote/PersistentShieldedSyncState rows on the view's ModelContext. Because ShieldedSyncManager::stop() is cancellation-only at the Rust boundary, an already-running pass that snapshotted accounts/subwallets before clearShielded() can continue through the persister callback after this method returns, writing rows into SwiftData on a different ModelContext during or after the wipe. The recent tree_size-gated append makes the tree side idempotent but does not close this cross-language ordering race. Adversary framing: a user who triggers Clear because they believe their device is compromised cannot rely on the wipe completing before the next sync pass persists new private state. Per swift-sdk/CLAUDE.md's 'marshalling, not deciding' rule, the fix belongs on the Rust side — add a true drain/quiesce primitive and call it from clearShielded.
suggestion: Self-shield guard tells the user the recipient may be left blank, but blank input still disables the Platform→Shielded send path
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift (line 160)
CARRIED FORWARD, STILL VALID at 82cb6e5 — SendViewModel.swift not in this delta. The .platformToShielded branch of executeSend (lines 419-420) reads 'Shield always sends to your own shielded address; enter your own address or leave it blank', but detectAddressType() at 160-167 sets detectedAddressType = .unknown / detectedFlow = nil / estimatedFee = nil when the trimmed recipient is empty, updateFlow() at 174-192 only maps .platformToShielded from (.orchard, .platform), and canSend at line 120 requires detectedFlow != nil. So a blank recipient disables the send button entirely, directly contradicting the in-app instruction. The Rust shieldedShield FFI has no recipient parameter and always shields to the wallet's own default Orchard address, so the blank-recipient affordance is the intended UX but is unreachable. Defensive-coding angle: the path that does work is 'paste your own address' — which expands the surface for typo / copy-paste-wrong-wallet errors that the blank path was meant to avoid. Cross-language scope: this is a Swift-side gate stopping a path the FFI would accept; non-Swift hosts using platform_wallet_manager_shielded_shield directly are unaffected. Fix: thread the empty-recipient case through updateFlow() when selectedSource == .platform, or short-circuit canSend for the self-shield case.
suggestion: nonce_inc still uses unchecked `+ 1` for every caller except the new platform-wallet shield path
packages/rs-sdk/src/platform/transition/address_inputs.rs (line 41)
NEW at 82cb6e5. The latest delta migrates rs-platform-wallet::operations::shield to fetch_inputs_with_nonce + an explicit nonce.checked_add(1), and the inline comment (operations.rs:144-147) explicitly cites why: 'the canonical nonce_inc uses an unchecked + 1, which would wrap an address at u32::MAX into a replay nonce — drive-abci treats wrap-to-0 as a replay and rejects it after the wallet has spent ~30 s on a Halo 2 proof'. But nonce_inc itself is unchanged. Every other caller — put_identity.rs, transfer_address_funds.rs, top_up_identity_from_addresses.rs, address_credit_withdrawal.rs, and (notably) rs-sdk/src/platform/transition/shield.rs — still goes through the silent-wrap path, so the new guard defends only one of two shield code paths in the same monorepo. In release mode u32::MAX + 1 wraps with no panic and no Result, and nonce_inc's total-looking signature lies on the boundary. Blast radius is not theoretical-only: nonces are per-address and monotonic; a long-lived high-throughput address can reach the limit, after which every caller above silently emits a replay-shaped transition and the host wastes proof CPU before drive-abci rejects. Either replace the body with checked_add(1) returning a Result so every caller is protected, or rename to nonce_inc_unchecked and have every caller explicitly opt into the wrap.
suggestion: Cross-crate error funnel collapses AddressNotEnoughFundsError into a flat string before it reaches Swift
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (line 150)
NEW at 82cb6e5. shield() now calls fetch_inputs_with_nonce, which intentionally errors with AddressesNotEnoughFundsError when any input is short — that hard pre-flight balance check is this PR's value-add over the prior warn-and-proceed path. The call site at operations.rs:150-152 wraps every variant of dash_sdk::Error into one opaque string: .map_err(|e| PlatformWalletError::ShieldedBuildError(format!("fetch input nonces: {e}"))). That string is what crosses the Rust→Swift FFI boundary via ShieldedSyncWalletResultFFI.error_message. The structured AddressesNotEnoughFundsError accessors (required_balance(), addresses_with_info()) are no longer reachable from the host — exactly the same information the broadcast-side handler at lines 188-208 hand-builds into a rich 'required X credits; claimed [addr=(nonce N, M credits)…]; platform sees […]' diagnostic. UX consequence: the now-common pre-broadcast failure becomes the less informative one on the host side, while the rarer post-broadcast failure still gets the curated string. Detect the discriminant on the fetch path and route it through the same addresses_not_enough_funds → required_balance / addresses_with_info / format_addresses_with_info formatter the broadcast handler uses, keeping the FFI shape (string body) while carrying the structured info across the boundary.
suggestion: GroupByIn + range no-proof collapses every In branch into a single entry, contradicting AverageMode docs and the prove path
packages/rs-drive/src/query/drive_document_count_and_sum_query/drive_dispatcher.rs (line 246)
CARRIED FORWARD from f9810d4, STILL VALID at 82cb6e5 — rs-drive untouched by this delta. When AverageMode::GroupByIn is combined with a range clause, the no-proof branch unwraps the joint executor's Aggregate { count, sum } (already folded across all In branches inside aggregate_range_count_and_sum) and rewraps it as Entries(vec![AverageEntry { in_key: None, key: Vec::new(), count: Some(count), sum: Some(sum) }]). The In axis is lost. Two contracts collide: the public AverageMode::GroupByIn doc at drive_document_average_query/mod.rs:48-49 promises 'one entry per In value', and the prove path at drive_document_average_query/drive_dispatcher.rs:200-241 routes GroupByIn + range through execute_carrier_aggregate_count_and_sum_with_proof, which preserves per-In branches. Prove and no-prove visibly disagree for the same query shape on a load-bearing public mode — a class of cross-mode aliasing that can mask query-result tampering by a malicious server (a client comparing prove vs no-prove for a sanity check would diverge). Not a regression introduced by this PR. Either preserve per-In (count, sum) pairs on the no-prove path, or amend the AverageMode docstring to note that GroupByIn + range no-proof folds across In.
nitpick: Best-effort finalize_pending post-broadcast load-bears on durable pre-broadcast pending_nullifier locks — invariants worth pinning
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (line 326)
CARRIED FORWARD, STILL VALID at 82cb6e5 — operations.rs Type 16/17/19 paths unchanged. unshield, transfer, and withdraw downgrade finalize_pending failure from ? propagation to warn!()-and-continue. The rationale is defensible (broadcast already succeeded; surfacing a local write failure invites duplicate retries; next nullifier sync reconciles drift), but the safety of the post-broadcast window depends on invariants not checked at this layer: (1) pending_nullifiers was taken pre-broadcast and survives finalize_pending failure; (2) pending_nullifiers persistence is durable across process restarts (the file documents mark_pending as in-memory only on FileBackedShieldedStore::subwallets); (3) nullifier sync runs before the next spend attempt. FFI angle: the warn! log doesn't cross the Rust/Swift boundary today, so the host sees Ok(()) while local notes stay marked unspent — next selection picks them again, the user pays ~30s of Halo 2 proof, and broadcast fails with nullifier-already-used. Wasted CPU but not fund loss. Pin this with either an in-code comment block citing which durable-state property guarantees no re-spend, or a focused test asserting pending_nullifiers survives finalize_pending failure + process restart.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:187-254: Type 15 / Type 18 shield flows still report success on relay ACK instead of proven execution
CARRIED FORWARD, STILL VALID at 82cb6e52. Verified by reading the file: `shield()` at line 187 calls `state_transition.broadcast(sdk, None).await` and returns `Ok(())` once submission is acknowledged, and `shield_from_asset_lock()` at lines 251-254 does the same, while `unshield`/`transfer`/`withdraw` use `broadcast_and_wait::<StateTransitionProofResult>` for proven execution. The latest delta tightens the pre-broadcast validation (hard balance check + checked nonce + single-flight guard) but does not change the post-broadcast success contract. A hostile or faulty DAPI gateway can ACK the bytes, suppress retries, and surface a successful shield to the iOS host even when Platform later rejects the transition or never includes it. Type 18 is the worst case because the asset-lock proof is single-use — a false-positive at this boundary strands the user's L1 outpoint with no in-app indication. Under the new pre-flight, the rich `addresses_not_enough_funds` diagnostic at lines 188-208 is also even harder to reach: the under-balance class is rejected before broadcast, so this handler now only fires for races between the pre-flight read and the proven-block read. Align Type 15/18 with the spend-side `broadcast_and_wait::<StateTransitionProofResult>` so all five shielded transitions share one success contract. Tracked under PR follow-up #3704.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:429-459: shielded_add_account() returns Ok after updating only the wallet keyset — coordinator registry stays stale until rebind
CARRIED FORWARD, STILL VALID at 82cb6e52. Verified at platform_wallet.rs:440-459: the method inserts the new `OrchardKeySet` into `self.shielded_keys` at line 452 and returns `Ok(())`, with an in-code comment at 453-457 explicitly acknowledging the coordinator's `accounts` registry isn't refreshed. After the Phase-2b refactor `NetworkShieldedCoordinator::sync()` iterates only `coordinator.accounts`, so the newly added account is half-live: wallet-local helpers (`shielded_account_indices`, `shielded_default_address`) accept it immediately, but background sync never trial-decrypts or persists notes for it until the host re-runs `bind_shielded` with the full account list. Privacy framing: a host that calls `shielded_add_account` to begin receiving to a new diversifier sees a successful return and may share the new address externally; on-chain payments accumulate but the wallet's UI/sync state never reflects them. The SendViewModel self-shield enforcement structurally hides the gap on the send side (only account 0 is offered) but not on the receive side. The `Result<(), _>` post-condition is too broad for what the function actually guarantees. Tracked under PR follow-up #3705. Either refresh the coordinator registry from within this method, or rename to make the post-condition explicit (e.g. `shielded_register_account_keys_only`).
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/shielded_sync.rs`:270-280: ShieldedSyncManager::stop() reads as a quiescence barrier but only cancels future loop iterations
CARRIED FORWARD, STILL VALID at 82cb6e52 — shielded_sync.rs unchanged in this delta. `stop()` only `take()`s the cancellation token and calls `token.cancel()`, with no thread-join, no in-flight `sync_now`/`coordinator.sync()` await, and no guarantee that host-visible mutations have stopped before returning. After Phase-2b the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out (calling back into Swift's `PlatformWalletPersistenceHandler` on a background `ModelContext`) after callers believe shielded sync has been stopped. Security-relevant consumers that rely on the barrier semantic: `clearShielded` (used in response to perceived device compromise), `remove_wallet`, and same-process `bind_shielded`. The new per-wallet `shield_guard` in this delta is orthogonal — it serializes shield-class FFI calls on a single wallet but does not interact with the manager-wide sync loop. `platform_wallet_manager_shielded_sync_stop` exposes a synchronous FFI call hosts reasonably read as 'the loop is stopped now', which is false. Tracked under PR follow-up #3706. Either give `stop()` real quiescence semantics or expose a separate `quiesce()` FFI for Clear/unregister/rebind.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:419-467: clearLocalState races an in-flight Rust shielded sync pass — private notes can be re-persisted after Clear
CARRIED FORWARD, STILL VALID at 82cb6e52 — ShieldedService.swift not in this delta. `clearLocalState()` calls `managerForStop.clearShielded()` (Rust-side: stops the sync loop via cancel-only `stop()`, drops coordinator registrations, purges store subwallet state), then immediately deletes `PersistentShieldedNote`/`PersistentShieldedSyncState` rows on the view's `ModelContext`. Because `ShieldedSyncManager::stop()` is cancellation-only at the Rust boundary, an already-running pass that snapshotted accounts/subwallets before `clearShielded()` can continue through the persister callback after this method returns, writing rows into SwiftData on a different `ModelContext` during or after the wipe. The recent tree_size-gated append makes the tree side idempotent but does not close this cross-language ordering race. Adversary framing: a user who triggers Clear because they believe their device is compromised cannot rely on the wipe completing before the next sync pass persists new private state. Per swift-sdk/CLAUDE.md's 'marshalling, not deciding' rule, the fix belongs on the Rust side — add a true drain/quiesce primitive and call it from `clearShielded`.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift`:160-192: Self-shield guard tells the user the recipient may be left blank, but blank input still disables the Platform→Shielded send path
CARRIED FORWARD, STILL VALID at 82cb6e52 — SendViewModel.swift not in this delta. The `.platformToShielded` branch of `executeSend` (lines 419-420) reads 'Shield always sends to your own shielded address; enter your own address or leave it blank', but `detectAddressType()` at 160-167 sets `detectedAddressType = .unknown` / `detectedFlow = nil` / `estimatedFee = nil` when the trimmed recipient is empty, `updateFlow()` at 174-192 only maps `.platformToShielded` from `(.orchard, .platform)`, and `canSend` at line 120 requires `detectedFlow != nil`. So a blank recipient disables the send button entirely, directly contradicting the in-app instruction. The Rust `shieldedShield` FFI has no recipient parameter and always shields to the wallet's own default Orchard address, so the blank-recipient affordance is the intended UX but is unreachable. Defensive-coding angle: the path that *does* work is 'paste your own address' — which expands the surface for typo / copy-paste-wrong-wallet errors that the blank path was meant to avoid. Cross-language scope: this is a Swift-side gate stopping a path the FFI would accept; non-Swift hosts using `platform_wallet_manager_shielded_shield` directly are unaffected. Fix: thread the empty-recipient case through `updateFlow()` when `selectedSource == .platform`, or short-circuit `canSend` for the self-shield case.
- [SUGGESTION] In `packages/rs-sdk/src/platform/transition/address_inputs.rs`:41-47: nonce_inc still uses unchecked `+ 1` for every caller except the new platform-wallet shield path
NEW at 82cb6e52. The latest delta migrates `rs-platform-wallet::operations::shield` to `fetch_inputs_with_nonce` + an explicit `nonce.checked_add(1)`, and the inline comment (operations.rs:144-147) explicitly cites why: 'the canonical `nonce_inc` uses an unchecked `+ 1`, which would wrap an address at u32::MAX into a replay nonce — drive-abci treats wrap-to-0 as a replay and rejects it after the wallet has spent ~30 s on a Halo 2 proof'. But `nonce_inc` itself is unchanged. Every other caller — `put_identity.rs`, `transfer_address_funds.rs`, `top_up_identity_from_addresses.rs`, `address_credit_withdrawal.rs`, and (notably) `rs-sdk/src/platform/transition/shield.rs` — still goes through the silent-wrap path, so the new guard defends only one of two shield code paths in the same monorepo. In release mode `u32::MAX + 1` wraps with no panic and no `Result`, and `nonce_inc`'s total-looking signature lies on the boundary. Blast radius is not theoretical-only: nonces are per-address and monotonic; a long-lived high-throughput address can reach the limit, after which every caller above silently emits a replay-shaped transition and the host wastes proof CPU before drive-abci rejects. Either replace the body with `checked_add(1)` returning a `Result` so every caller is protected, or rename to `nonce_inc_unchecked` and have every caller explicitly opt into the wrap.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:150-162: Cross-crate error funnel collapses AddressNotEnoughFundsError into a flat string before it reaches Swift
NEW at 82cb6e52. `shield()` now calls `fetch_inputs_with_nonce`, which intentionally errors with `AddressesNotEnoughFundsError` when any input is short — that hard pre-flight balance check is this PR's value-add over the prior warn-and-proceed path. The call site at operations.rs:150-152 wraps every variant of `dash_sdk::Error` into one opaque string: `.map_err(|e| PlatformWalletError::ShieldedBuildError(format!("fetch input nonces: {e}")))`. That string is what crosses the Rust→Swift FFI boundary via `ShieldedSyncWalletResultFFI.error_message`. The structured `AddressesNotEnoughFundsError` accessors (`required_balance()`, `addresses_with_info()`) are no longer reachable from the host — exactly the same information the broadcast-side handler at lines 188-208 hand-builds into a rich 'required X credits; claimed [addr=(nonce N, M credits)…]; platform sees […]' diagnostic. UX consequence: the now-common pre-broadcast failure becomes the *less* informative one on the host side, while the rarer post-broadcast failure still gets the curated string. Detect the discriminant on the fetch path and route it through the same `addresses_not_enough_funds → required_balance / addresses_with_info / format_addresses_with_info` formatter the broadcast handler uses, keeping the FFI shape (string body) while carrying the structured info across the boundary.
- [SUGGESTION] In `packages/rs-drive/src/query/drive_document_count_and_sum_query/drive_dispatcher.rs`:246-258: GroupByIn + range no-proof collapses every In branch into a single entry, contradicting AverageMode docs and the prove path
CARRIED FORWARD from f9810d49, STILL VALID at 82cb6e52 — rs-drive untouched by this delta. When `AverageMode::GroupByIn` is combined with a range clause, the no-proof branch unwraps the joint executor's `Aggregate { count, sum }` (already folded across all In branches inside `aggregate_range_count_and_sum`) and rewraps it as `Entries(vec![AverageEntry { in_key: None, key: Vec::new(), count: Some(count), sum: Some(sum) }])`. The In axis is lost. Two contracts collide: the public `AverageMode::GroupByIn` doc at `drive_document_average_query/mod.rs:48-49` promises 'one entry per In value', and the prove path at `drive_document_average_query/drive_dispatcher.rs:200-241` routes `GroupByIn + range` through `execute_carrier_aggregate_count_and_sum_with_proof`, which preserves per-In branches. Prove and no-prove visibly disagree for the same query shape on a load-bearing public mode — a class of cross-mode aliasing that can mask query-result tampering by a malicious server (a client comparing prove vs no-prove for a sanity check would diverge). Not a regression introduced by this PR. Either preserve per-In `(count, sum)` pairs on the no-prove path, or amend the AverageMode docstring to note that `GroupByIn + range` no-proof folds across In.
- [NITPICK] In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:326-540: Best-effort finalize_pending post-broadcast load-bears on durable pre-broadcast pending_nullifier locks — invariants worth pinning
CARRIED FORWARD, STILL VALID at 82cb6e52 — operations.rs Type 16/17/19 paths unchanged. `unshield`, `transfer`, and `withdraw` downgrade `finalize_pending` failure from `?` propagation to `warn!()`-and-continue. The rationale is defensible (broadcast already succeeded; surfacing a local write failure invites duplicate retries; next nullifier sync reconciles drift), but the safety of the post-broadcast window depends on invariants not checked at this layer: (1) `pending_nullifiers` was taken pre-broadcast and survives `finalize_pending` failure; (2) `pending_nullifiers` persistence is durable across process restarts (the file documents `mark_pending` as in-memory only on `FileBackedShieldedStore::subwallets`); (3) nullifier sync runs before the next spend attempt. FFI angle: the `warn!` log doesn't cross the Rust/Swift boundary today, so the host sees `Ok(())` while local notes stay marked unspent — next selection picks them again, the user pays ~30s of Halo 2 proof, and broadcast fails with nullifier-already-used. Wasted CPU but not fund loss. Pin this with either an in-code comment block citing which durable-state property guarantees no re-spend, or a focused test asserting `pending_nullifiers` survives `finalize_pending` failure + process restart.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review at 64096d9. The 17-line delta is documentation-only: (a) operations.rs::unshield gains an invariant block pinning that the on-chain nullifier set — not the in-memory pending_nullifiers map — is the authoritative no-reuse guarantee, and that worst-case for a missed local mark is a wasted ~30s proof, never fund loss; transfer/withdraw reference it via 'see unshield'. (b) SendViewModel.swift removes the unreachable 'leave it blank' affordance from the self-shield error string and pins the canSend gating chain in an inline comment. Two prior findings (#5 blank-recipient self-shield UX, #9 finalize_pending invariants) are RESOLVED. Seven prior findings remain STILL VALID at this SHA against unchanged source and are carried forward. No new findings, no blockers.
🟡 7 suggestion(s)
7 additional finding(s)
suggestion: Type 15 / Type 18 shield flows still report success on relay ACK instead of proven execution
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (line 187)
Verified at 64096d9: shield() (line 187) calls state_transition.broadcast(sdk, None).await and returns Ok(()) once one DAPI peer ACKs submission, and shield_from_asset_lock() (lines 251-254) does the same, while sibling operations unshield (line 319), transfer, and withdraw use broadcast_and_wait::<StateTransitionProofResult> for proven execution. The post-broadcast success contract is asymmetric across the five shielded transitions in the same module.
A hostile or faulty DAPI gateway can ACK the bytes, suppress retries, and surface a successful shield to the iOS host even when Platform later rejects the transition or never includes it. Type 18 (shield_from_asset_lock) is the worst case because the asset-lock proof is single-use — a false positive at this boundary strands the user's L1 outpoint with no in-app indication. The new pre-flight (fetch_inputs_with_nonce + checked nonce + per-wallet shield_guard) shrinks the under-balance class of false positives but does not change the post-broadcast contract: a DAPI-level ACK can still mask a Platform-level rejection. The rich addresses_not_enough_funds diagnostic at lines 188-208 now only fires for races between the pre-flight read and the proven-block read. Align Type 15/18 with broadcast_and_wait::<StateTransitionProofResult> so all five shielded transitions share one success contract. Tracked under PR follow-up #3704.
suggestion: shielded_add_account() returns Ok after updating only the wallet keyset — coordinator registry stays stale until rebind
packages/rs-platform-wallet/src/wallet/platform_wallet.rs (line 440)
Verified at platform_wallet.rs:440-459: the method inserts a new OrchardKeySet into self.shielded_keys and returns Ok(()), with the inline NOTE at lines 453-457 explicitly acknowledging the coordinator's accounts registry is not refreshed. After the Phase-2b refactor, NetworkShieldedCoordinator::sync() iterates only coordinator.accounts, so the new account is half-live: wallet-local helpers (shielded_account_indices, shielded_default_address) accept it immediately, but background sync never trial-decrypts or persists notes for it until the host re-runs bind_shielded with the full account list.
The Result<(), _> post-condition is materially broader than what the function guarantees — both for in-Rust callers and across the FFI boundary, a successful return suggests 'account is now syncing' when it actually means 'keys cached locally'. Privacy framing: a host that calls shielded_add_account to begin receiving to a new diversifier sees a successful return and may share the new address externally; on-chain payments accumulate but the wallet's UI/sync state never reflects them. SendViewModel's self-shield enforcement structurally hides the gap on the send side (only account 0 offered) but not on the receive side. Either refresh the coordinator registry from within this method, or rename to make the post-condition explicit (e.g. shielded_register_account_keys_only). Tracked under PR follow-up #3705.
suggestion: ShieldedSyncManager::stop() reads as a quiescence barrier but only cancels future loop iterations
packages/rs-platform-wallet/src/manager/shielded_sync.rs (line 270)
Verified at shielded_sync.rs:270-280: stop() only take()s the cancellation token and calls token.cancel() — no JoinHandle::join, no in-flight sync_now/coordinator.sync() await, and no guarantee that host-visible mutations have stopped before returning. After Phase-2b, the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out (calling back into Swift's PlatformWalletPersistenceHandler on a background ModelContext) after callers believe shielded sync has been stopped.
A synchronous pub fn stop(&self) -> () invites the reading 'after this returns, nothing else happens', but the implementation only sets a cancellation request. platform_wallet_manager_shielded_sync_stop exposes that same shape across the FFI to Swift, where it reasonably reads as 'the loop is stopped now' — which is false. Security-relevant consumers that rely on the barrier semantic: clearShielded (used in response to perceived device compromise), remove_wallet, and same-process bind_shielded. The per-wallet shield_guard added in a prior delta is orthogonal — it serializes shield-class FFI calls on a single wallet but does not interact with the manager-wide sync loop. Either give stop() real quiescence semantics (await the in-flight pass via a JoinHandle or oneshot) or expose a separate quiesce() FFI for Clear/unregister/rebind and rename this to cancel() to match the actual contract. Tracked under PR follow-up #3706.
suggestion: clearLocalState races an in-flight Rust shielded sync pass — private notes can be re-persisted after Clear
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift (line 419)
Verified at ShieldedService.swift:419-472: clearLocalState() calls managerForStop.clearShielded() (Rust-side: stops the sync loop via cancel-only stop(), drops coordinator registrations, purges store subwallet state) at line 451, then immediately deletes PersistentShieldedNote/PersistentShieldedSyncState rows on the view's ModelContext at lines 465-467. Because ShieldedSyncManager::stop() is cancellation-only at the Rust boundary, an already-running pass that snapshotted accounts/subwallets before clearShielded() can continue through the persister callback after this method returns, writing rows into SwiftData on a different ModelContext during or after the wipe.
This is a cross-language ordering race: the persister callback re-enters Swift (PlatformWalletPersistenceHandler) on a Rust-thread-pool thread that is not synchronized with the SwiftData wipe happening on the view's context. The tree_size-gated commitment append from an earlier delta makes the tree side idempotent but does not close this race for the persister fan-out. Adversary framing: a user who triggers Clear because they believe their device is compromised cannot rely on the wipe completing before the next sync pass persists new private state. Per swift-sdk/CLAUDE.md's 'marshalling, not deciding' rule, the fix belongs on the Rust side — add a true drain/quiesce primitive and call it from clearShielded, not in Swift.
suggestion: nonce_inc still uses unchecked `+ 1` for every caller except the platform-wallet shield path
packages/rs-sdk/src/platform/transition/address_inputs.rs (line 41)
Verified at address_inputs.rs:41-47: nonce_inc returns (nonce + 1, credits) with no checked arithmetic. The PR's rs-platform-wallet::operations::shield migrated to fetch_inputs_with_nonce + an explicit nonce.checked_add(1) (operations.rs:150-162), with an inline comment citing the wrap risk: 'the canonical nonce_inc uses an unchecked + 1, which would wrap an address at u32::MAX into a replay nonce — drive-abci treats wrap-to-0 as a replay and rejects it after the wallet has spent ~30 s on a Halo 2 proof'. But nonce_inc itself is unchanged. Every other caller — put_identity.rs, transfer_address_funds.rs, top_up_identity_from_addresses.rs, address_credit_withdrawal.rs, and (notably) rs-sdk/src/platform/transition/shield.rs — still goes through the silent-wrap path. The new guard defends only one of two shield code paths in the same monorepo.
In release mode u32::MAX + 1 wraps with no panic and no Result, and nonce_inc's total-looking fn(...) -> BTreeMap<...> signature lies on the boundary. Nonces are per-address and monotonic on drive-abci; a long-lived high-throughput address (or one driven up by an attacker who can keep sending funds to it) can reach the limit, after which every caller above silently emits a replay-shaped transition and the host wastes proof CPU before drive-abci rejects with no actionable signal. On shielded paths that means wasting Halo 2 proving cost per attempt — practical self-DoS. Either replace the body with checked_add(1) returning Result<_, _> so every caller is protected by the type system, or rename to nonce_inc_unchecked and force every caller to explicitly opt into wrap semantics.
suggestion: Cross-crate error funnel collapses AddressNotEnoughFundsError into a flat string before it reaches Swift
packages/rs-platform-wallet/src/wallet/shielded/operations.rs (line 150)
Verified at operations.rs:150-152: shield() calls fetch_inputs_with_nonce, which intentionally errors with AddressesNotEnoughFundsError when any input is short — the hard pre-flight balance check is this PR's value-add over the prior warn-and-proceed path. The call site wraps every variant of dash_sdk::Error into one opaque string: .map_err(|e| PlatformWalletError::ShieldedBuildError(format!("fetch input nonces: {e}"))). That string is what crosses the Rust→Swift FFI boundary via ShieldedSyncWalletResultFFI.error_message.
The structured AddressesNotEnoughFundsError accessors (required_balance(), addresses_with_info()) are no longer reachable from the host — exactly the same information the broadcast-side handler at lines 188-208 hand-builds into a rich 'required X credits; claimed [addr=(nonce N, M credits)…]; platform sees […]' diagnostic. The type system already encodes the discriminant; collapsing to format!("...{e}") discards that information at the very layer best positioned to compose a structured message. UX consequence: the now-common pre-broadcast failure is the less informative one on the host side, while the rarer post-broadcast failure still gets the curated string — exactly the opposite of where the structure should live. Users may retry blind on the more common path, expanding the timing window for races between the pre-flight read and the proven-block read. Detect the discriminant on the fetch path and route it through the same addresses_not_enough_funds → required_balance / addresses_with_info / format_addresses_with_info formatter the broadcast handler uses, keeping the FFI shape (string body) while carrying the structured info across the boundary.
suggestion: GroupByIn + range no-proof collapses every In branch into a single entry, contradicting AverageMode docs and the prove path
packages/rs-drive/src/query/drive_document_count_and_sum_query/drive_dispatcher.rs (line 246)
Carried forward from f9810d4, still valid at 64096d9 — rs-drive untouched by this PR's deltas. When AverageMode::GroupByIn is combined with a range clause, the no-proof branch unwraps the joint executor's Aggregate { count, sum } (already folded across all In branches inside aggregate_range_count_and_sum) and rewraps it as Entries(vec![AverageEntry { in_key: None, key: Vec::new(), count: Some(count), sum: Some(sum) }]). The In axis is lost.
Two contracts collide: the public AverageMode::GroupByIn doc at drive_document_average_query/mod.rs:48-49 promises 'one entry per In value', and the prove path at drive_document_average_query/drive_dispatcher.rs:200-241 routes GroupByIn + range through execute_carrier_aggregate_count_and_sum_with_proof, which preserves per-In branches. Prove and no-prove visibly disagree for the same query shape on a load-bearing public mode — a class of cross-mode aliasing that affects every cross-language client (rs-sdk, js-evo-sdk, wasm-sdk) that falls back from prove to no-prove, and that defeats a client comparing prove vs no-prove as a sanity check against a malicious server. Both variants remain type-correct, making it easy for downstream code to mis-handle. Not a regression introduced by this PR. Either preserve per-In (count, sum) pairs on the no-prove path, or amend the AverageMode docstring to note that GroupByIn + range no-proof folds across In.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:187-254: Type 15 / Type 18 shield flows still report success on relay ACK instead of proven execution
Verified at 64096d99: `shield()` (line 187) calls `state_transition.broadcast(sdk, None).await` and returns `Ok(())` once one DAPI peer ACKs submission, and `shield_from_asset_lock()` (lines 251-254) does the same, while sibling operations `unshield` (line 319), `transfer`, and `withdraw` use `broadcast_and_wait::<StateTransitionProofResult>` for proven execution. The post-broadcast success contract is asymmetric across the five shielded transitions in the same module.
A hostile or faulty DAPI gateway can ACK the bytes, suppress retries, and surface a successful shield to the iOS host even when Platform later rejects the transition or never includes it. Type 18 (`shield_from_asset_lock`) is the worst case because the asset-lock proof is single-use — a false positive at this boundary strands the user's L1 outpoint with no in-app indication. The new pre-flight (`fetch_inputs_with_nonce` + checked nonce + per-wallet `shield_guard`) shrinks the under-balance class of false positives but does not change the post-broadcast contract: a DAPI-level ACK can still mask a Platform-level rejection. The rich `addresses_not_enough_funds` diagnostic at lines 188-208 now only fires for races between the pre-flight read and the proven-block read. Align Type 15/18 with `broadcast_and_wait::<StateTransitionProofResult>` so all five shielded transitions share one success contract. Tracked under PR follow-up #3704.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:440-459: shielded_add_account() returns Ok after updating only the wallet keyset — coordinator registry stays stale until rebind
Verified at platform_wallet.rs:440-459: the method inserts a new `OrchardKeySet` into `self.shielded_keys` and returns `Ok(())`, with the inline NOTE at lines 453-457 explicitly acknowledging the coordinator's `accounts` registry is not refreshed. After the Phase-2b refactor, `NetworkShieldedCoordinator::sync()` iterates only `coordinator.accounts`, so the new account is half-live: wallet-local helpers (`shielded_account_indices`, `shielded_default_address`) accept it immediately, but background sync never trial-decrypts or persists notes for it until the host re-runs `bind_shielded` with the full account list.
The `Result<(), _>` post-condition is materially broader than what the function guarantees — both for in-Rust callers and across the FFI boundary, a successful return suggests 'account is now syncing' when it actually means 'keys cached locally'. Privacy framing: a host that calls `shielded_add_account` to begin receiving to a new diversifier sees a successful return and may share the new address externally; on-chain payments accumulate but the wallet's UI/sync state never reflects them. SendViewModel's self-shield enforcement structurally hides the gap on the send side (only account 0 offered) but not on the receive side. Either refresh the coordinator registry from within this method, or rename to make the post-condition explicit (e.g. `shielded_register_account_keys_only`). Tracked under PR follow-up #3705.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/shielded_sync.rs`:270-280: ShieldedSyncManager::stop() reads as a quiescence barrier but only cancels future loop iterations
Verified at shielded_sync.rs:270-280: `stop()` only `take()`s the cancellation token and calls `token.cancel()` — no `JoinHandle::join`, no in-flight `sync_now`/`coordinator.sync()` await, and no guarantee that host-visible mutations have stopped before returning. After Phase-2b, the loop is manager-wide and can still queue per-wallet changesets through the persister fan-out (calling back into Swift's `PlatformWalletPersistenceHandler` on a background `ModelContext`) after callers believe shielded sync has been stopped.
A synchronous `pub fn stop(&self) -> ()` invites the reading 'after this returns, nothing else happens', but the implementation only sets a cancellation request. `platform_wallet_manager_shielded_sync_stop` exposes that same shape across the FFI to Swift, where it reasonably reads as 'the loop is stopped now' — which is false. Security-relevant consumers that rely on the barrier semantic: `clearShielded` (used in response to perceived device compromise), `remove_wallet`, and same-process `bind_shielded`. The per-wallet `shield_guard` added in a prior delta is orthogonal — it serializes shield-class FFI calls on a single wallet but does not interact with the manager-wide sync loop. Either give `stop()` real quiescence semantics (await the in-flight pass via a `JoinHandle` or oneshot) or expose a separate `quiesce()` FFI for Clear/unregister/rebind and rename this to `cancel()` to match the actual contract. Tracked under PR follow-up #3706.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:419-472: clearLocalState races an in-flight Rust shielded sync pass — private notes can be re-persisted after Clear
Verified at ShieldedService.swift:419-472: `clearLocalState()` calls `managerForStop.clearShielded()` (Rust-side: stops the sync loop via cancel-only `stop()`, drops coordinator registrations, purges store subwallet state) at line 451, then immediately deletes `PersistentShieldedNote`/`PersistentShieldedSyncState` rows on the view's `ModelContext` at lines 465-467. Because `ShieldedSyncManager::stop()` is cancellation-only at the Rust boundary, an already-running pass that snapshotted accounts/subwallets before `clearShielded()` can continue through the persister callback after this method returns, writing rows into SwiftData on a different `ModelContext` during or after the wipe.
This is a cross-language ordering race: the persister callback re-enters Swift (`PlatformWalletPersistenceHandler`) on a Rust-thread-pool thread that is not synchronized with the SwiftData wipe happening on the view's context. The tree_size-gated commitment append from an earlier delta makes the tree side idempotent but does not close this race for the persister fan-out. Adversary framing: a user who triggers Clear because they believe their device is compromised cannot rely on the wipe completing before the next sync pass persists new private state. Per swift-sdk/CLAUDE.md's 'marshalling, not deciding' rule, the fix belongs on the Rust side — add a true drain/quiesce primitive and call it from `clearShielded`, not in Swift.
- [SUGGESTION] In `packages/rs-sdk/src/platform/transition/address_inputs.rs`:41-47: nonce_inc still uses unchecked `+ 1` for every caller except the platform-wallet shield path
Verified at address_inputs.rs:41-47: `nonce_inc` returns `(nonce + 1, credits)` with no checked arithmetic. The PR's `rs-platform-wallet::operations::shield` migrated to `fetch_inputs_with_nonce` + an explicit `nonce.checked_add(1)` (operations.rs:150-162), with an inline comment citing the wrap risk: 'the canonical `nonce_inc` uses an unchecked `+ 1`, which would wrap an address at u32::MAX into a replay nonce — drive-abci treats wrap-to-0 as a replay and rejects it after the wallet has spent ~30 s on a Halo 2 proof'. But `nonce_inc` itself is unchanged. Every other caller — `put_identity.rs`, `transfer_address_funds.rs`, `top_up_identity_from_addresses.rs`, `address_credit_withdrawal.rs`, and (notably) `rs-sdk/src/platform/transition/shield.rs` — still goes through the silent-wrap path. The new guard defends only one of two shield code paths in the same monorepo.
In release mode `u32::MAX + 1` wraps with no panic and no `Result`, and `nonce_inc`'s total-looking `fn(...) -> BTreeMap<...>` signature lies on the boundary. Nonces are per-address and monotonic on drive-abci; a long-lived high-throughput address (or one driven up by an attacker who can keep sending funds to it) can reach the limit, after which every caller above silently emits a replay-shaped transition and the host wastes proof CPU before drive-abci rejects with no actionable signal. On shielded paths that means wasting Halo 2 proving cost per attempt — practical self-DoS. Either replace the body with `checked_add(1)` returning `Result<_, _>` so every caller is protected by the type system, or rename to `nonce_inc_unchecked` and force every caller to explicitly opt into wrap semantics.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:150-162: Cross-crate error funnel collapses AddressNotEnoughFundsError into a flat string before it reaches Swift
Verified at operations.rs:150-152: `shield()` calls `fetch_inputs_with_nonce`, which intentionally errors with `AddressesNotEnoughFundsError` when any input is short — the hard pre-flight balance check is this PR's value-add over the prior warn-and-proceed path. The call site wraps every variant of `dash_sdk::Error` into one opaque string: `.map_err(|e| PlatformWalletError::ShieldedBuildError(format!("fetch input nonces: {e}")))`. That string is what crosses the Rust→Swift FFI boundary via `ShieldedSyncWalletResultFFI.error_message`.
The structured `AddressesNotEnoughFundsError` accessors (`required_balance()`, `addresses_with_info()`) are no longer reachable from the host — exactly the same information the broadcast-side handler at lines 188-208 hand-builds into a rich 'required X credits; claimed [addr=(nonce N, M credits)…]; platform sees […]' diagnostic. The type system already encodes the discriminant; collapsing to `format!("...{e}")` discards that information at the very layer best positioned to compose a structured message. UX consequence: the now-common pre-broadcast failure is the *less* informative one on the host side, while the rarer post-broadcast failure still gets the curated string — exactly the opposite of where the structure should live. Users may retry blind on the more common path, expanding the timing window for races between the pre-flight read and the proven-block read. Detect the discriminant on the fetch path and route it through the same `addresses_not_enough_funds → required_balance / addresses_with_info / format_addresses_with_info` formatter the broadcast handler uses, keeping the FFI shape (string body) while carrying the structured info across the boundary.
- [SUGGESTION] In `packages/rs-drive/src/query/drive_document_count_and_sum_query/drive_dispatcher.rs`:246-258: GroupByIn + range no-proof collapses every In branch into a single entry, contradicting AverageMode docs and the prove path
Carried forward from f9810d49, still valid at 64096d99 — rs-drive untouched by this PR's deltas. When `AverageMode::GroupByIn` is combined with a range clause, the no-proof branch unwraps the joint executor's `Aggregate { count, sum }` (already folded across all In branches inside `aggregate_range_count_and_sum`) and rewraps it as `Entries(vec![AverageEntry { in_key: None, key: Vec::new(), count: Some(count), sum: Some(sum) }])`. The In axis is lost.
Two contracts collide: the public `AverageMode::GroupByIn` doc at `drive_document_average_query/mod.rs:48-49` promises 'one entry per In value', and the prove path at `drive_document_average_query/drive_dispatcher.rs:200-241` routes `GroupByIn + range` through `execute_carrier_aggregate_count_and_sum_with_proof`, which preserves per-In branches. Prove and no-prove visibly disagree for the same query shape on a load-bearing public mode — a class of cross-mode aliasing that affects every cross-language client (rs-sdk, js-evo-sdk, wasm-sdk) that falls back from prove to no-prove, and that defeats a client comparing prove vs no-prove as a sanity check against a malicious server. Both variants remain type-correct, making it easy for downstream code to mis-handle. Not a regression introduced by this PR. Either preserve per-In `(count, sum)` pairs on the no-prove path, or amend the AverageMode docstring to note that `GroupByIn + range` no-proof folds across In.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
2 participants
Status: ✅ Working end-to-end
The full shielded send matrix now works on regtest — client plumbing,
anchor selection, witness construction, proof, and broadcast. The two
blockers that previously held this PR are both resolved:
fixed and merged independently in fix(drive,drive-abci): retire SHIELDED_MOST_RECENT_ANCHOR_KEY; derive most-recent from [8] and never empty it #3605 (retire
SHIELDED_MOST_RECENT_ANCHOR_KEY; derive most-recent from theanchors-by-height tree and never empty it), with follow-ups fix(drive,drive-abci): post-merge follow-ups for shielded anchor refactor #3606
and fix(drive): rebalance shielded credit pool subtree keys by access frequency #3607.
recorded anchors tree", Shielded: repeat spend fails with "Anchor not found in the recorded anchors tree" #3703) — fixed in this PR (see below).
Verified live: two consecutive shielded transfers succeed and the
rebuilt commitment tree has consistent checkpoints.
Issue being fixed or feature implemented
The Send Dash sheet's four shielded flows all fell through to a
placeholder error even though the spend operations already existed on
the Rust side. This PR threads all four end-to-end so the full Send Dash
matrix works, and fixes the shielded sync/witness regressions that
surfaced once real spends ran against a multi-wallet, shared commitment
tree.
Fixes #3703.
What was done?
platform-wallet — shielded send wiring
old
ShieldedWalletwrapper was removed): Type 16 transfer, Type 17unshield, Type 19 withdraw, and Type 15 shield-from-account. The
shield helper auto-selects Platform Payment inputs in ascending
derivation order covering
amount + fee, fetches per-input noncesfrom Platform (replacing the old
nonce = 0stub), and signs via ahost
Signer<PlatformAddress>.against
getShieldedAnchors/getMostRecentShieldedAnchorso thespend bundle's anchor matches a Platform-recorded anchor by
construction, with a
ShieldedTreeDivergeddiagnostic when nothingmatches.
platform-wallet — sync / witness correctness
every appended position. The tree is chain-wide and wallets bind at
different times; deciding retention from "is this position owned right
now" left a note appended before its owner bound permanently
unwitnessable (balance showed, spend failed with "Merkle witness
unavailable"). Per-wallet ownership is tracked separately in the
per-
SubwalletIdnotes store.sync_notes_acrosspreviously gated thecommitment-tree append on the minimum per-subwallet watermark, so a
re-fetch from a chunk boundary (or a lagging / late-bound subwallet)
re-appended positions the tree already held — duplicating leaves,
corrupting shardtree's internal nodes, and producing per-position
witnesses that resolved against roots Platform never recorded. New
ShieldedStore::tree_size()(viamax_leaf_position) gates the appendon the tree's own leaf count (append-once, global); the checkpoint id
is the true post-append tree size (strictly monotonic, collision-free);
note saving is gated per-subwallet watermark so a caught-up subwallet
doesn't re-derive. Includes a
tree_sizepersist/reload regressiontest.
register);
shieldrejectsamount == 0;remove_walletandcoordinator
clear()now purge per-subwallet store state(
purge_wallet/purge_all_subwallets) so a later re-bind resyncsfrom index 0 instead of resuming behind a stale watermark.
rs-platform-wallet-ffi
shielded_sendmodule (feature-gatedshielded): prover warm-up +readiness getters, and manager-handle FFIs for transfer / unshield /
withdraw / shield.
shieldtakes a*const SignerHandle(resolved viaa
usizeround-trip rather than a&'statictransmute) for the hostkeychain signer.
swift-sdk
PlatformWalletManagerasync methods:shieldedTransfer,shieldedUnshield,shieldedWithdraw,shieldedShield, all run offthe main actor so the ~30 s first-call Halo 2 proof build doesn't
block UI; plus
warmUpShieldedProver()/isShieldedProverReady.the network-wide picture — Total Shielded Balance summed across
every wallet's unspent notes, and a Notes Synced watermark so large
pools show sync progress — instead of a single bound wallet. The
Orchard address row and per-account state breakdown are removed; sync
status, counters, and actions are unchanged. Display-only via
SwiftData
@Query.swift-example-app
SendViewModel.executeSendreplaces the four shielded placeholderbranches with real FFI calls; the prover is warmed up at app start.
Send matrix after this PR
Type 18 (
shield_from_asset_lock, direct L1 → Shielded) is wired butits broadcast still reports on relay-ACK rather than proven execution —
tracked in #3704.
How Has This Been Tested?
cargo fmt --allclean;cargo check -p rs-platform-wallet --features shieldedgreen against the mergedv3.1-devbase.tree_size_tracks_leaf_count_across_reloadandall_marked_tree_witnesses_every_position_after_reloadregressiontests.
build_ios.sh --target simgreen.shielded transfers from a 0.808 DASH wallet both succeed (the second
is the case that previously failed with "Anchor not found"). On-disk
verification: the rebuilt commitment tree has consistent checkpoints
(
id == leaf countat every checkpoint: 8→7, 10→9, 12→11) with nodouble-append, vs. the pre-fix corrupted tree (
id=8 → position 13).Balance shows the cross-wallet aggregate, Notes Synced reflects the
watermark, and the Orchard address / per-account rows are gone.
Breaking Changes
None at the consensus level.
ShieldedStoregainstree_size(),purge_wallet(), andpurge_all_subwallets();witnesstakes(position, checkpoint_depth). All impls are in-tree and updated; no out-of-treeconsumers.
SendViewModel.executeSendgains a requiredwalletManagerparameter; the only call site is in-tree and updated.
Deferred follow-ups (tracked separately)
shielded_add_account()updates keys but not the coordinator registry (half-live account).ShieldedSyncManager::stop()is cancel-only, not a quiescence barrier (Clear-flow race).Checklist:
🤖 Generated with Claude Code