feat(swift-sdk,platform-wallet): wire shielded send end-to-end (all 4 transitions)#3603
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: "8cc6db83af83cb990ea9d270effcc0715fc47c7493d76abfced197adc51cfe86"
)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
…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
- 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.
…e-flight
The shield pre-flight `fetch_inputs_with_nonce` hard balance check raises
a structured `AddressNotEnoughFundsError`, but the call site collapsed
every error variant to `format!("fetch input nonces: {e}")` before it
crossed the Rust→Swift FFI, so the host lost the address/balance/required
detail — making the now-common pre-broadcast failure less informative
than the rarer post-broadcast one.
Add an `address_not_enough_funds` extractor (mirroring the plural
broadcast-side `addresses_not_enough_funds`) and, on the fetch path,
format a structured "shield input <addr> has insufficient balance:
requires X credits, has Y" message. FFI shape is unchanged (string body);
other error variants still fall back to the flat string.
|
On (a) The wrap only triggers at The shielded shield path keeps its local If we ever want the broader hardening it can be its own focused rs-sdk change, but it's not a blocker here. |
) `shield()` (Type 15) and `shield_from_asset_lock()` (Type 18) reported success on relay-ACK via `broadcast()`, while the spend-side flows (unshield/transfer/withdraw) wait for proven execution via `broadcast_and_wait::<StateTransitionProofResult>`. A faulty or hostile DAPI gateway could therefore ACK the bytes and surface a successful shield even when Platform later rejects or never includes the transition. Type 18 is the worst case: the asset-lock proof is single-use, so a false-positive strands the user's L1 outpoint with no in-app signal. Switch both shield flows to `broadcast_and_wait::<StateTransitionProofResult>` so all five shielded transitions share one proven-execution success contract. The proven result is discarded (callers only need the confirmation); the rich `addresses_not_enough_funds` diagnostic is retained for the now-rare post-pre-flight rejection. Closes #3704.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review at 0098206. The 41-line delta adds a new singular address_not_enough_funds extractor (operations.rs:78-94) mirroring the existing plural addresses_not_enough_funds, routing pre-flight fetch_inputs_with_nonce errors through it so the structured (address, balance, required) info reaches Swift as a formatted diagnostic instead of an opaque string. This resolves prior finding #6. No new findings introduced. Six prior findings remain valid against unchanged source and are carried forward, all of severity suggestion (no blockers).
🟡 6 suggestion(s)
6 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 225)
Verified at operations.rs:225 — shield() calls state_transition.broadcast(sdk, None).await and returns Ok(()) once one DAPI peer ACKs submission, and shield_from_asset_lock() at lines 289-292 does the same. Sibling operations unshield, transfer, and withdraw use broadcast_and_wait::<StateTransitionProofResult> for proven execution, so the post-broadcast success contract is asymmetric across the five shielded transitions in this module. The latest delta enriches the pre-broadcast error path but does not change this post-broadcast 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 strands the user's L1 outpoint with no in-app indication. The new structured pre-flight diagnostic for AddressNotEnoughFundsError (lines 174-190) shrinks the under-balance class of false positives but does not address the post-broadcast gap: a DAPI-level ACK can still mask a Platform-level rejection. Align Type 15/18 with broadcast_and_wait::<StateTransitionProofResult> so all five shielded transitions share one success contract. Tracked under 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 at line 452 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. Across the Rust→Swift FFI boundary, a successful return suggests 'account is now syncing' when it actually means 'keys cached locally only'. 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. 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 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 the 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. 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 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)
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 (verified above), 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 on the view's context. 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:194-198) and the inline comment at lines 168-171 cites the wrap risk explicitly. 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 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: 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 — 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 that falls back from prove to no-prove, and defeats a client comparing prove vs no-prove as a sanity check against 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 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`:225-292: Type 15 / Type 18 shield flows still report success on relay ACK instead of proven execution
Verified at operations.rs:225 — `shield()` calls `state_transition.broadcast(sdk, None).await` and returns `Ok(())` once one DAPI peer ACKs submission, and `shield_from_asset_lock()` at lines 289-292 does the same. Sibling operations `unshield`, `transfer`, and `withdraw` use `broadcast_and_wait::<StateTransitionProofResult>` for proven execution, so the post-broadcast success contract is asymmetric across the five shielded transitions in this module. The latest delta enriches the pre-broadcast error path but does not change this post-broadcast 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 strands the user's L1 outpoint with no in-app indication. The new structured pre-flight diagnostic for `AddressNotEnoughFundsError` (lines 174-190) shrinks the under-balance class of false positives but does not address the post-broadcast gap: a DAPI-level ACK can still mask a Platform-level rejection. Align Type 15/18 with `broadcast_and_wait::<StateTransitionProofResult>` so all five shielded transitions share one success contract. Tracked under 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` at line 452 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. Across the Rust→Swift FFI boundary, a successful return suggests 'account is now syncing' when it actually means 'keys cached locally only'. 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. 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 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 the 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`. 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 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
`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 (verified above), 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 on the view's context. 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:194-198) and the inline comment at lines 168-171 cites the wrap risk explicitly. 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 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-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 — 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 that falls back from prove to no-prove, and defeats a client comparing prove vs no-prove as a sanity check against 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 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.
…, not last scanned The `last_synced_index` doc comments described it as the "highest/last global note index scanned" (inclusive phrasing), but the code uses it as an exclusive count / next-index-to-scan: it's advanced as `aligned_start + total_notes_scanned` and gated as `position < watermark`, so `0` unambiguously means "nothing scanned" and scanning through position N-1 sets it to N. Correct the wording across the trait method, SubwalletState, the changeset start-state, the FFI type, and the SwiftData model so readers don't infer a fresh-vs-scanned-index-0 ambiguity that the exclusive-count semantics don't actually have. No behavior change.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
At 6f70314d5f5c6e076c4f2fb910501e0d768d2b95, the previous shield-send regression is fixed: Type 15 and Type 18 now use broadcast_and_wait::<StateTransitionProofResult> before reporting success. The remaining verified issues are lifecycle/API contract problems around shielded sync stop/clear, plus one unchecked nonce helper in rs-sdk. The GroupByIn + range no-proof report is not valid on this SHA because the collapsed shape is explicitly documented and regression-tested.
🟡 3 suggestion(s)
3 additional finding(s)
suggestion: `platform_wallet_manager_shielded_sync_stop` returns before shielded work has actually stopped
packages/rs-platform-wallet-ffi/src/shielded_sync.rs (line 70)
This FFI entry point reports success immediately after manager.shielded_sync().stop(), but ShieldedSyncManager::stop() only removes the cancellation token and calls token.cancel(); it does not join the worker thread or wait for an in-flight sync_now() / coordinator.sync() pass to finish. That leaves the public Swift-facing API with stronger semantics than the implementation provides: callers can observe stopShieldedSync() returning while Rust still has a sync pass running and can still emit completion events or persistence callbacks. The API either needs a real quiescence barrier or documentation and naming that make the best-effort behavior explicit.
suggestion: `platform_wallet_manager_shielded_clear` can return before an in-flight sync pass has drained
packages/rs-platform-wallet-ffi/src/shielded_sync.rs (line 407)
platform_wallet_manager_shielded_clear calls manager.shielded_sync().stop() and then clears the coordinator registries, but stop() is still cancel-only. A sync pass that started before this call can continue running after clear returns, because the cancellation token is not checked inside the already-started pass. That makes the host-side contract unsafe: Swift is told the clear completed and immediately wipes PersistentShieldedNote / PersistentShieldedSyncState, but a pass that already progressed far enough can still finish and route stale results back through the persistence/event path. For a user-facing Clear flow, returning before background work has actually quiesced defeats the intended local-data-erasure boundary.
suggestion: `nonce_inc` still wraps `u32::MAX` back to zero
packages/rs-sdk/src/platform/transition/address_inputs.rs (line 41)
nonce_inc() returns (nonce + 1, credits) with unchecked arithmetic. AddressNonce is a u32, so in release builds u32::MAX + 1 silently wraps to 0. This helper is still used by multiple transition builders (transfer_address_funds, top_up_identity_from_addresses, shield, address_credit_withdrawal, and put_identity), so the PR's checked increment in one shielded path does not fix the shared primitive the other callers rely on. The overflow check needs to live in this helper or its type contract so callers cannot accidentally build replay-shaped transitions after nonce exhaustion.
🤖 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-ffi/src/shielded_sync.rs`:70-77: `platform_wallet_manager_shielded_sync_stop` returns before shielded work has actually stopped
This FFI entry point reports success immediately after `manager.shielded_sync().stop()`, but `ShieldedSyncManager::stop()` only removes the cancellation token and calls `token.cancel()`; it does not join the worker thread or wait for an in-flight `sync_now()` / `coordinator.sync()` pass to finish. That leaves the public Swift-facing API with stronger semantics than the implementation provides: callers can observe `stopShieldedSync()` returning while Rust still has a sync pass running and can still emit completion events or persistence callbacks. The API either needs a real quiescence barrier or documentation and naming that make the best-effort behavior explicit.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/shielded_sync.rs`:407-421: `platform_wallet_manager_shielded_clear` can return before an in-flight sync pass has drained
`platform_wallet_manager_shielded_clear` calls `manager.shielded_sync().stop()` and then clears the coordinator registries, but `stop()` is still cancel-only. A sync pass that started before this call can continue running after `clear` returns, because the cancellation token is not checked inside the already-started pass. That makes the host-side contract unsafe: Swift is told the clear completed and immediately wipes `PersistentShieldedNote` / `PersistentShieldedSyncState`, but a pass that already progressed far enough can still finish and route stale results back through the persistence/event path. For a user-facing Clear flow, returning before background work has actually quiesced defeats the intended local-data-erasure boundary.
- [SUGGESTION] In `packages/rs-sdk/src/platform/transition/address_inputs.rs`:41-46: `nonce_inc` still wraps `u32::MAX` back to zero
`nonce_inc()` returns `(nonce + 1, credits)` with unchecked arithmetic. `AddressNonce` is a `u32`, so in release builds `u32::MAX + 1` silently wraps to `0`. This helper is still used by multiple transition builders (`transfer_address_funds`, `top_up_identity_from_addresses`, `shield`, `address_credit_withdrawal`, and `put_identity`), so the PR's checked increment in one shielded path does not fix the shared primitive the other callers rely on. The overflow check needs to live in this helper or its type contract so callers cannot accidentally build replay-shaped transitions after nonce exhaustion.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
#3706) `ShieldedSyncManager::stop()` was cancel-only: it cancelled the loop token but a pass already inside `sync_now`/`coordinator.sync()` ran to completion, including its persister-callback fan-out. So `clearShielded` could wipe the registries + SwiftData rows while an in-flight pass kept going and re-persisted `PersistentShieldedNote`/`PersistentShieldedSyncState` rows afterward — defeating the local-erasure boundary a user expects from Clear (especially when invoked in response to a perceived compromise). Add `ShieldedSyncManager::quiesce()`: raise a `quiescing` gate (so `sync_now`/`sync_wallet` bail after taking the `is_syncing` slot), cancel the loop, then wait for `is_syncing` to clear. `is_syncing` is held for the whole pass including the persister fan-out, so its falling edge with the gate up is a sound "fully drained" signal; the gate is reopened before returning. Route the Clear and stop FFIs through `quiesce()` so both block until the in-flight pass has actually drained. `stop()` stays as the cancel-only primitive for fast full-manager shutdown. Closes #3706.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the checked-out head at 8c80874 and revalidated all three mandatory prior findings against the current source. All three prior findings are STILL VALID and should be carried forward; the latest delta itself is documentation-only around shielded sync watermark semantics, and I did not confirm any new findings in those touched files.
🟡 3 suggestion(s)
3 additional finding(s)
suggestion: `platform_wallet_manager_shielded_sync_stop` returns success before shielded work has actually quiesced
packages/rs-platform-wallet-ffi/src/shielded_sync.rs (line 70)
STILL VALID. The FFI entry point returns PlatformWalletFFIResult::ok() immediately after manager.shielded_sync().stop(), but ShieldedSyncManager::stop() in packages/rs-platform-wallet/src/manager/shielded_sync.rs:270 only takes the cancellation token and calls token.cancel(). It does not join the background thread, wait for an in-flight sync_now() / coordinator.sync() pass to finish, or guarantee that persistence callbacks back into Swift have stopped before the function returns. The exported synchronous API therefore advertises a stronger postcondition than the implementation provides: callers can observe stopShieldedSync() succeeding while Rust still owns active sync work and can still mutate host-visible state.
suggestion: `platform_wallet_manager_shielded_clear` can return before an already-started sync pass has drained
packages/rs-platform-wallet-ffi/src/shielded_sync.rs (line 407)
STILL VALID. platform_wallet_manager_shielded_clear() still calls manager.shielded_sync().stop() and then clears the coordinator, but stop() is cancel-only and is not a quiescence barrier. A pass that entered coordinator.sync(force) before this call can continue running after clear returns, because cancellation is only observed between loop iterations, not inside the already-running pass. That breaks the host-side clear contract: Swift is told the clear completed and immediately deletes PersistentShieldedNote / PersistentShieldedSyncState, while the in-flight Rust persister path can still recreate those rows afterward. For a local-data-erasure flow, returning before background shielded work has actually drained is a real correctness and privacy gap.
suggestion: `nonce_inc` still silently wraps `u32::MAX` back to zero
packages/rs-sdk/src/platform/transition/address_inputs.rs (line 41)
STILL VALID. nonce_inc() still maps each entry to (nonce + 1, credits) with unchecked arithmetic even though AddressNonce is u32. In release builds, u32::MAX + 1 wraps to 0, so callers silently build replay-shaped transitions after nonce exhaustion instead of getting an explicit error. This helper is still used by multiple transition builders in the current tree (put_identity, transfer_address_funds, shield, top_up_identity_from_addresses, and address_credit_withdrawal), so the checked increment added in one wallet-local shield path does not fix the shared primitive. The overflow guard needs to live in this helper or its type contract so callers cannot accidentally opt into wraparound semantics.
🤖 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-ffi/src/shielded_sync.rs`:70-77: `platform_wallet_manager_shielded_sync_stop` returns success before shielded work has actually quiesced
STILL VALID. The FFI entry point returns `PlatformWalletFFIResult::ok()` immediately after `manager.shielded_sync().stop()`, but `ShieldedSyncManager::stop()` in `packages/rs-platform-wallet/src/manager/shielded_sync.rs:270` only takes the cancellation token and calls `token.cancel()`. It does not join the background thread, wait for an in-flight `sync_now()` / `coordinator.sync()` pass to finish, or guarantee that persistence callbacks back into Swift have stopped before the function returns. The exported synchronous API therefore advertises a stronger postcondition than the implementation provides: callers can observe `stopShieldedSync()` succeeding while Rust still owns active sync work and can still mutate host-visible state.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/shielded_sync.rs`:407-421: `platform_wallet_manager_shielded_clear` can return before an already-started sync pass has drained
STILL VALID. `platform_wallet_manager_shielded_clear()` still calls `manager.shielded_sync().stop()` and then clears the coordinator, but `stop()` is cancel-only and is not a quiescence barrier. A pass that entered `coordinator.sync(force)` before this call can continue running after `clear` returns, because cancellation is only observed between loop iterations, not inside the already-running pass. That breaks the host-side clear contract: Swift is told the clear completed and immediately deletes `PersistentShieldedNote` / `PersistentShieldedSyncState`, while the in-flight Rust persister path can still recreate those rows afterward. For a local-data-erasure flow, returning before background shielded work has actually drained is a real correctness and privacy gap.
- [SUGGESTION] In `packages/rs-sdk/src/platform/transition/address_inputs.rs`:41-46: `nonce_inc` still silently wraps `u32::MAX` back to zero
STILL VALID. `nonce_inc()` still maps each entry to `(nonce + 1, credits)` with unchecked arithmetic even though `AddressNonce` is `u32`. In release builds, `u32::MAX + 1` wraps to `0`, so callers silently build replay-shaped transitions after nonce exhaustion instead of getting an explicit error. This helper is still used by multiple transition builders in the current tree (`put_identity`, `transfer_address_funds`, `shield`, `top_up_identity_from_addresses`, and `address_credit_withdrawal`), so the checked increment added in one wallet-local shield path does not fix the shared primitive. The overflow guard needs to live in this helper or its type contract so callers cannot accidentally opt into wraparound semantics.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
|
Folded two more of the deferred items into this PR:
Both verified (host check + ffi check + iOS build green). PR description updated accordingly. Still deferred/tracked: #3705 ( |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
At e768cdc6dedf2d368b1271e7f57276575f64759d, the new shielded-sync quiesce path does fix the old "cancel-only" drain problem, and the reported background_cancel restart race does not reproduce in the checked-in code. Two issues remain: quiesce() is still not a full completion-event barrier for the exported stop/clear APIs, and the shared SDK nonce helper still wraps exhausted u32 nonces back to zero.
🟡 2 suggestion(s)
2 additional finding(s)
suggestion: `quiesce()` returns before the final shielded completion event has been delivered
packages/rs-platform-wallet/src/manager/shielded_sync.rs (line 314)
quiesce() only waits for is_syncing to become false, but sync_now() clears that flag before it dispatches on_shielded_sync_completed at lines 383-385. Both exported callers rely on quiesce() as their barrier: platform_wallet_manager_shielded_sync_stop() documents that no further completion events will be emitted after it returns, and platform_wallet_manager_shielded_clear() calls quiesce() before the host starts its Clear flow. That contract is not true at this SHA. The Rust event path still invokes the FFI callback after quiesce() can unblock, and the Swift trampoline then schedules handleShieldedSyncCompleted onto the main actor, so the host can still observe a late shielded-sync completion after stop/clear has already returned. The persistence drain itself is complete by then, but the API still exposes a stale post-stop/post-clear event to the host.
suggestion: `nonce_inc()` silently wraps exhausted address nonces back to zero
packages/rs-sdk/src/platform/transition/address_inputs.rs (line 41)
AddressNonce is a u32, but nonce_inc() increments each fetched nonce with unchecked nonce + 1. In release builds, an on-chain nonce of u32::MAX wraps to 0, so every caller that uses automatic nonce fetching can build a transition with a replay-shaped nonce instead of surfacing an explicit exhaustion error. The helper is still used by multiple transition builders at this head, including transfer_address_funds, shield_funds, top_up_from_addresses, withdraw_address_funds, and put_with_address_funding_fetching_nonces, so the unsafe behavior is shared across the SDK rather than confined to one wallet path.
🤖 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/manager/shielded_sync.rs`:314-320: `quiesce()` returns before the final shielded completion event has been delivered
`quiesce()` only waits for `is_syncing` to become false, but `sync_now()` clears that flag before it dispatches `on_shielded_sync_completed` at lines 383-385. Both exported callers rely on `quiesce()` as their barrier: `platform_wallet_manager_shielded_sync_stop()` documents that no further completion events will be emitted after it returns, and `platform_wallet_manager_shielded_clear()` calls `quiesce()` before the host starts its Clear flow. That contract is not true at this SHA. The Rust event path still invokes the FFI callback after `quiesce()` can unblock, and the Swift trampoline then schedules `handleShieldedSyncCompleted` onto the main actor, so the host can still observe a late shielded-sync completion after stop/clear has already returned. The persistence drain itself is complete by then, but the API still exposes a stale post-stop/post-clear event to the host.
- [SUGGESTION] In `packages/rs-sdk/src/platform/transition/address_inputs.rs`:41-46: `nonce_inc()` silently wraps exhausted address nonces back to zero
`AddressNonce` is a `u32`, but `nonce_inc()` increments each fetched nonce with unchecked `nonce + 1`. In release builds, an on-chain nonce of `u32::MAX` wraps to `0`, so every caller that uses automatic nonce fetching can build a transition with a replay-shaped nonce instead of surfacing an explicit exhaustion error. The helper is still used by multiple transition builders at this head, including `transfer_address_funds`, `shield_funds`, `top_up_from_addresses`, `withdraw_address_funds`, and `put_with_address_funding_fetching_nonces`, so the unsafe behavior is shared across the SDK rather than confined to one wallet path.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…ing is_syncing `quiesce()` drains on the falling edge of `is_syncing`, but `sync_now` cleared that flag *before* dispatching `on_shielded_sync_completed`, so a stop/clear caller could unblock while the completion event (FFI callback → Swift `handleShieldedSyncCompleted`) was still pending — surfacing a stale post-stop/post-clear event even though the persistence drain was already done. Move the event dispatch ahead of the `is_syncing` clear so the quiesce barrier covers the event too. (sync_wallet emits no post-flag event, so it's unaffected.)
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The current head fixes the earlier Rust-side quiesce() ordering bug inside ShieldedSyncManager::sync_now(), but one host-visible stop/clear race remains at the Swift callback boundary. Separately, the SDK still auto-increments address nonces with unchecked u32 + 1, so exhausted nonces wrap to zero instead of failing locally.
🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
…ar_shielded() The Clear FFI was stitching two library calls inline (quiesce the sync manager, then clear the coordinator), which bends the FFI contract (resolve handle → call one library function → marshal result). Add `PlatformWalletManager::clear_shielded()` that owns the quiesce+clear sequencing in the library, and reduce `platform_wallet_manager_shielded_clear` to a single `block_on(manager.clear_shielded())` call. No behavior change.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the reviewable findings against 47c3f527e0054545e5916aa5ef082401be5cb978. The Rust side now correctly holds is_syncing until after the completion callback and documents stop/clear as a full drain barrier, but the Swift trampoline still defers the observable state update until after stopShieldedSync() or clearShielded() returns. I did not confirm any additional defects in the touched Rust refactor.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
Code ReviewI verified the reviewable findings against Prior-finding reconciliation
New latest-delta findings: none confirmed. 🟡 1 suggestion(s) Carried-forward prior finding🟡 suggestion: `stopShieldedSync()` and `clearShielded()` can still surface a completion event after they return
This callback turns the Rust completion notification into As written, callers can clear host persistence or inspect manager state after 🤖 Prompt for all review comments with AI agents |
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Going to just merge this one in, then fix any issues after the fact. Otherwise we will diverge too much.
…t stop doc The Rust quiesce barrier guarantees no persistence after stop/clear returns, but the completion event is hopped onto the host's executor (Swift `@MainActor`), so a final already-dispatched event can land just after `stopShieldedSync()`/`clearShielded()` return and briefly repopulate the in-memory mirror Clear just zeroed (no SwiftData rows are rewritten — cosmetic only). - Correct the `platform_wallet_manager_shielded_sync_stop` doc to state the real guarantee (no further persistence, no new pass, Rust-side event dispatched) and not over-promise about when a host's run loop applies a trailing event. - Guard the example app's `handleShieldedSyncEvent` with `isBound` so an event arriving after Clear (which sets `isBound = false`) is dropped. `bind()` sets `isBound = true` before its sync events flow, so no legitimate post-bind event is dropped.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the checkout is exactly 36608425f75aa1238216729f85c2bf0d2449b206. The reported Swift/Rust boundary issue is real: stopShieldedSync() and clearShielded() can return before lastShieldedSyncEvent is updated because the completion callback is re-dispatched onto @MainActor. The Rust FFI comments now document that weaker guarantee, and the example app guards its own clear flow, but the reusable Swift SDK still exposes the trailing event to any other consumer.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
…p/clear (SDK-level) The example-app guard only protected that one consumer; the reusable SDK still published a trailing `lastShieldedSyncEvent` after `stopShieldedSync()` / `clearShielded()` returned, because the Rust completion callback is re-dispatched onto the manager's `@MainActor`. Gate it in the SDK: `PlatformWalletManager` gains an internal `suppressShieldedCompletionEvents` flag that `handleShieldedSyncCompleted` checks before publishing. stop/clear set it true after the Rust drain returns; startShieldedSync / syncShieldedNow clear it so a new run's events flow again. The manager is `@MainActor` and stop/clear are synchronous (they hold the main actor through the FFI while the trailing event only enqueues), so the flag is set before that event task runs — dropping it deterministically, with no risk of dropping a legitimate post-restart event. Now every SDK consumer gets the documented contract, not just the example app.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the reported issues against the checked-out head b97ff6dd4340e3d028054c6fd07c714bce8de2e5. Two findings are real in this SHA: the Swift shielded-sync suppression fix is still bypassable on an immediate restart, and the Rust SDK still wraps u32::MAX address nonces back to zero on its auto-fetch path. Neither issue is blocking under the repository’s consensus rules, but both are valid review comments.
🟡 2 suggestion(s)
2 additional finding(s)
suggestion: Shielded completion suppression is not tied to a sync generation, so an old completion can leak into the next run
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift (line 61)
This handler only checks a single boolean gate before publishing lastShieldedSyncEvent, while the FFI callback always re-enqueues completions onto Task { @MainActor ... } at lines 589-590. stopShieldedSync() and clearShielded() correctly set suppressShieldedCompletionEvents = true, but both startShieldedSync() at lines 184-185 and syncShieldedNow() at lines 276-278 clear that flag before the main actor yields. Because PlatformWalletManager is @MainActor, a caller can stop or clear and then restart in the same actor turn, leaving the old completion task queued. When that queued task finally runs, the flag is already false again and the stale completion from the previous Rust pass is published as if it belonged to the new run. That means the current fix only suppresses trailing completions until the next start, not until the drained callback from the previous generation is definitively consumed.
suggestion: `nonce_inc()` wraps exhausted address nonces back to zero on auto-fetched inputs
packages/rs-sdk/src/platform/transition/address_inputs.rs (line 41)
AddressNonce is a u32, but this helper increments each fetched nonce with unchecked nonce + 1. In release builds, an on-chain nonce of u32::MAX therefore wraps to 0 instead of failing locally. That affects every convenience path that auto-fetches nonces through this helper, including shielding, address transfers, address withdrawals, and identity create/top-up with address funding. Drive explicitly treats state_nonce == u32::MAX as terminal in packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/address_balances_and_nonces.rs, and the integration test at packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs:8818 documents that such an address is effectively locked. The SDK should reject this state before signing rather than silently preparing a transition with wrapped nonce 0.
🤖 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/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift`:61-66: Shielded completion suppression is not tied to a sync generation, so an old completion can leak into the next run
This handler only checks a single boolean gate before publishing `lastShieldedSyncEvent`, while the FFI callback always re-enqueues completions onto `Task { @MainActor ... }` at lines 589-590. `stopShieldedSync()` and `clearShielded()` correctly set `suppressShieldedCompletionEvents = true`, but both `startShieldedSync()` at lines 184-185 and `syncShieldedNow()` at lines 276-278 clear that flag before the main actor yields. Because `PlatformWalletManager` is `@MainActor`, a caller can stop or clear and then restart in the same actor turn, leaving the old completion task queued. When that queued task finally runs, the flag is already false again and the stale completion from the previous Rust pass is published as if it belonged to the new run. That means the current fix only suppresses trailing completions until the next start, not until the drained callback from the previous generation is definitively consumed.
- [SUGGESTION] In `packages/rs-sdk/src/platform/transition/address_inputs.rs`:41-46: `nonce_inc()` wraps exhausted address nonces back to zero on auto-fetched inputs
`AddressNonce` is a `u32`, but this helper increments each fetched nonce with unchecked `nonce + 1`. In release builds, an on-chain nonce of `u32::MAX` therefore wraps to `0` instead of failing locally. That affects every convenience path that auto-fetches nonces through this helper, including shielding, address transfers, address withdrawals, and identity create/top-up with address funding. Drive explicitly treats `state_nonce == u32::MAX` as terminal in `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/address_balances_and_nonces.rs`, and the integration test at `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs:8818` documents that such an address is effectively locked. The SDK should reject this state before signing rather than silently preparing a transition with wrapped nonce `0`.
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
All five shielded transitions (shield, shield-from-asset-lock, transfer,
unshield, withdraw) wait for proven execution via
broadcast_and_wait::<StateTransitionProofResult>, so the host only seessuccess once Platform has actually included the transition (#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.
Review hardening folded into this PR
Beyond the original scope, this PR resolves these review findings:
(
broadcast_and_wait), matching the spend-side success contract.ShieldedSyncManager::quiesce()(cancel +drain the in-flight pass) and routed the Clear and stop FFIs through
it, so a sync pass can no longer re-persist notes after Clear returns.
pre-flight balance check reusing rs-sdk's
fetch_inputs_with_noncewith structured error surfacing;
u32checkpoint-id hard-fail; FFIload/free callback-pair validation; network-scoped shielded restore +
Sync Status summary; per-wallet shield single-flight guard;
amount/recipient send validation; and a pure unit-tested
select_shield_inputs.Deferred follow-ups (tracked separately)
shielded_add_account()updates keys but not thecoordinator registry (half-live account; receive-side only).
sync/scale testing.
GroupByIn + rangeno-proof fold(out of scope; documented + regression-tested).
nonce_incunchecked+1in rs-sdk — assessed won't-fix: reachingu32::MAXon a single address is practically unreachable.Checklist:
🤖 Generated with Claude Code