feat(swift-sdk): add full shielded pool (ZK) support for iOS#3348
Merged
Conversation
Add Swift SDK wrappers and Rust FFI crypto functions enabling iOS apps to perform shielded transactions on Dash Platform. **Rust FFI crypto module** (rs-sdk-ffi/src/shielded/crypto/): - Proving key warmup with OnceLock cache (~30s first call) - Orchard address derivation from spending key - Bundle building: shield, transfer, unshield, withdrawal - Encrypted note trial decryption with IVK **Swift transport layer** (8 files, 2169 lines): - @_silgen_name declarations for 22 existing shielded FFI functions - High-level async API: queries, transitions, builders, broadcast - Nullifier BLAST sync with privacy-preserving trunk/branch queries - Memory-safe recursive pointer pinning (withFFIBundle) **Swift crypto layer** (3 files, 861 lines): - @_silgen_name declarations for 7 new crypto FFI functions - DecryptedNote, SpendableNoteInfo types with JSON serialization - High-level async API: warmupProvingKey, deriveShieldedAddress, buildShieldBundle, buildTransferBundle, decryptNotes, etc. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a shielded crypto FFI layer and a Rust ShieldedPool client plus comprehensive Swift bindings: Rust exposes address derivation, bundle builders, note decryption, proving-key warmup/caching, pool-client sync/build APIs and nullifier sync; Swift adds FFI declarations, FFI-aligned types, high-level SDK services, and a ShieldedPoolClient wrapper. Changes
Sequence Diagram(s)sequenceDiagram
participant Swift as Swift SDK
participant FFI as Rust FFI
participant Prover as CachedProver
participant PoolDB as Commitment Tree / DB
participant SDK as Dash SDK runtime
Swift->>FFI: dash_sdk_shielded_build_transfer_bundle(spending_key, anchor, notes_json, recipient, amount, memo)
FFI->>PoolDB: read witnesses / select notes / get anchor
FFI->>FFI: derive keys (spending -> fvk/sak)
FFI->>Prover: request proof generation (CachedProver)
Prover-->>FFI: return proof bytes
FFI->>FFI: sign bundle, serialize to JSON/hex
FFI-->>Swift: return DashSDKResult(serialized_bundle)
Swift->>SDK: dash_sdk_shielded_broadcast(transition_bytes) (optional)
SDK-->>Swift: broadcast result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip Rust Clippy can be used to improve the quality of Rust code reviews.Clippy is the official Rust linter. It provides lints to catch common mistakes and improve your Rust code. To configure Clippy, add a See Clippy Documentation for more details. |
- Fix derive_keys to return (FVK, ASK) instead of unused SpendingKey - Add tracing::debug! for silently skipped malformed notes in decrypt - OrchardProver import is required for proving_key() method dispatch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
Rust Quality Review FindingsFixed (in a16eede)
Remaining Items (non-blocking)Medium: Low: Low: Low: No unit tests for internal helpers Positive Notes
|
Member
Author
Security Audit: Rust Crypto FFI ModuleCRITICAL: Missing change output in transfer bundle (fund loss)File: The FFI transfer builder sends Example: User has a 1,000,000 credit note, wants to transfer 100 credits → 999,900 credits burned as fee. Fix needed: Add change output logic + fee computation matching DPP reference. Derive change address from HIGH: Missing fee calculation in all spend bundlesFiles: None of the spend-type FFI functions compute or enforce a minimum fee. The DPP reference code calls Fix needed: Add fee computation (requires HIGH: Integer overflow in
|
Member
Author
Security Audit: Swift Shielded WrappersMEDIUM: Struct layout not guaranteed C-compatibleFiles: ShieldedTypes.swift, NullifierSyncTypes.swift The manually-defined Swift structs (FFISerializedAction, FFIOrchardBundleParams, FFINullifierSyncConfig, etc.) are NOT guaranteed to match Rust `#[repr(C)]` layout. Swift does not guarantee C-compatible layout for Swift-defined structs — this works today but could break with a compiler update. Fix: Add these types to the C header (regenerate via cbindgen), OR add runtime size assertions: MEDIUM: Silent padding in dataToBytes32/36/64File: ShieldedTypes.swift lines 120-171 These helpers silently zero-pad undersized input. A 31-byte nullifier becomes a different 32-byte value. Combined with missing size validation in `parseBundleJSON`, malformed data can produce wrong cryptographic values. Fix: Add `precondition(data.count == 32)` or validate sizes after hex decoding in parseBundleJSON. MEDIUM: No field size validation in parseBundleJSONFile: ShieldedCryptoTypes.swift lines 167-240 After hex decoding, no check that nullifier=32B, rk=32B, cmx=32B, spendAuthSig=64B, etc. Wrong-sized fields silently flow through. MEDIUM: Spending key not zeroed after useFile: ShieldedCryptoService.swift `skCopy` and `skTuple` persist in memory after FFI call. Use `Data.resetBytes(in:)` and zero the tuple. LOW: Dead code — BundleFFIStorageFile: ShieldedPoolService.swift lines 1141-1167 `BundleFFIStorage` and `buildFFIBundleParams` create params with nil pointers. Never called — dead code that would be unsound if used. Remove. LOW: compactMap silently drops malformed entriesFile: ShieldedPoolService.swift — getShieldedAnchors, checkNullifiers, getEncryptedNotes Malformed entries in query responses are silently dropped. For checkNullifiers, a dropped entry means the caller doesn't know a nullifier's status. Use `map` with explicit errors instead. LOW: Recursive withFFIBundle can stack overflowFile: ShieldedPoolService.swift lines 1042-1087 Each action adds a recursion level. A bundle with thousands of actions overflows the stack. Add `precondition(bundle.actions.count <= 2048)`. LOW: Duplicate SendableSdkPtr wrappersShieldedPoolService.swift and NullifierSyncService.swift each define their own `SendableSdkPtr`/`NullifierSendableSdkPtr`. Consolidate to one shared definition. Positive Notes
|
CRITICAL: Transfer bundle was missing change output — all unspent note value burned as fee. Now computes fee via compute_minimum_shielded_fee, adds change output to sender's address, and properly deducts fee. HIGH: Unshield and withdrawal builders had no fee deduction, producing bundles that would be rejected on-chain. Now compute and deduct minimum fee. HIGH: total_spent sum used unchecked .sum() that could overflow in release builds. Now uses checked_add via try_fold. MEDIUM: Added i64::MAX validation for unshield/withdrawal amounts (matching DPP reference). Added amount > 0 check for shield. Swift fixes: - Removed dead BundleFFIStorage code (nil-pointer params would be UB) - Added precondition(actions.count <= 2048) in withFFIBundle - Added field size validation in parseBundleJSON after hex decoding Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
Security Fixes Applied (16cfce5)All critical and high severity audit findings have been fixed: CRITICAL — Fixed: Missing change output in transfer bundle
HIGH — Fixed: Missing fee calculation in unshield/withdrawal
HIGH — Fixed: Integer overflow in `total_spent` sum
MEDIUM — Fixed: Missing `i64::MAX` validation
Swift fixes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3348 +/- ##
============================================
+ Coverage 75.37% 75.66% +0.28%
============================================
Files 3000 3000
Lines 274421 275571 +1150
============================================
+ Hits 206843 208505 +1662
+ Misses 67578 67066 -512
🚀 New features to boost your workflow:
|
…struct pointers Bundle build functions now return heap-allocated DashSDKOrchardBundleParams* instead of JSON strings. The Swift side wraps these in ShieldedBundleHandle (auto-freed on dealloc) that can be passed directly to transition functions. This eliminates: - JSON serialization/deserialization overhead on the critical crypto path - NSNumber precision issues for large u64 values - parseBundleJSON and all associated field validation code New pattern: let bundle = try await sdk.buildShieldBundle(spendingKey: key, amount: 100_000) try await sdk.shieldFunds(inputs: [...], bundle: bundle, ...) Added transition overloads (shieldFunds, shieldedTransfer, unshieldFunds) that accept ShieldedBundleHandle directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Spending key raw bytes are now zeroized immediately after deriving SpendingKey/FullViewingKey/SpendAuthorizingKey in all crypto FFI functions (address derivation, bundle building, note decryption). Uses the `zeroize` crate (moved from dev-dependencies to dependencies) to overwrite the local [u8; 32] copy with zeros before it goes out of scope. This limits the window during which raw key material sits in memory, reducing exposure to memory dump attacks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swift (1)
119-171: Silent zero-padding can mask cryptographic input errors.The
dataToBytes32,dataToBytes36, anddataToBytes64helpers silently zero-pad undersized inputs. For cryptographic material (keys, signatures, anchors), this could mask errors where truncated data is passed, potentially leading to invalid cryptographic operations.While callers in
ShieldedCryptoService.swiftdo validate input sizes, a defensive approach would add assertions or preconditions here.Proposed defensive fix
/// Copy Data into a 32-byte tuple, zero-padding if shorter. func dataToBytes32(_ data: Data) -> Bytes32Tuple { + precondition(data.count <= 32, "dataToBytes32: input exceeds 32 bytes (\(data.count))") var tuple: Bytes32Tuple = ( 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 )Or for stricter validation on cryptographic inputs, consider a separate
dataToBytes32Exactvariant that requires exactly 32 bytes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swift` around lines 119 - 171, The helpers dataToBytes32, dataToBytes36, and dataToBytes64 currently silently zero-pad short Data which can hide cryptographic input errors; update each function to validate input length (use precondition or assert) and fail fast when src.count is less than the expected size, or alternatively add strict variants dataToBytes32Exact / dataToBytes36Exact / dataToBytes64Exact that require src.count == 32/36/64 and raise a clear error (or call preconditionFailure) before performing the unsafe copy; ensure you reference these functions (dataToBytes32/dataToBytes36/dataToBytes64) so callers can switch to the strict versions or handle the precondition failure.packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift (5)
1106-1108: Force unwrap ofbuf.baseAddress!assumes non-empty inputs.The force unwrap on Line 1107 will crash if
inputsis empty (since an empty array's buffer has no base address). While callers currently validate that inputs is non-empty, adding a guard withinwithFFIShieldInputswould make it more defensive.Proposed defensive fix
nonisolated func withFFIShieldInputs<R>( _ inputs: [ShieldFundsInput], body: (UnsafePointer<FFIShieldInput>) -> R ) -> R { + precondition(!inputs.isEmpty, "withFFIShieldInputs requires non-empty inputs") + // We need stable pointers to each address and private key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift` around lines 1106 - 1108, withFFIShieldInputs currently force-unwraps buf.baseAddress which will crash for an empty inputs array; update the implementation in withFFIShieldInputs to safely handle an empty buffer by checking if buf.baseAddress is nil and, in that case, invoke body with a nil/empty pointer (or return an appropriate empty result) instead of force-unwrapping, otherwise pass buf.baseAddress to body — reference the withFFIShieldInputs function to locate and change this behavior.
167-173: SamecompactMapconcern for nullifier statuses.Similar to
getShieldedAnchors, malformed entries are silently dropped. The count of returned statuses may not match the input nullifiers count, which could confuse callers expecting a 1:1 mapping.
1084-1126: Missing precondition on inputs count inwithFFIShieldInputs.For consistency with
withFFIBundle, consider adding a precondition to guard against stack overflow from recursive nesting with a very large number of shield inputs.Proposed fix
nonisolated func withFFIShieldInputs<R>( _ inputs: [ShieldFundsInput], body: (UnsafePointer<FFIShieldInput>) -> R ) -> R { + // Guard against stack overflow from recursive withUnsafeBytes nesting + precondition(inputs.count <= 256, "Too many shield inputs (\(inputs.count) > 256)") + // We need stable pointers to each address and private key. func recurse(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift` around lines 1084 - 1126, Add the same precondition used in withFFIBundle to withFFIShieldInputs to guard against excessive recursion depth: at the top of withFFIShieldInputs (before defining recurse) add a precondition that inputs.count is <= the maximum allowed (use the same constant or check expression used by withFFIBundle) so the function fails fast instead of risking stack overflow from deep recursive nesting in recurse.
959-962: Consider consolidatingSendableSdkPtrwrapper.The PR comments noted that
SendableSdkPtris duplicated across files. Consider extracting to a shared internal utility to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift` around lines 959 - 962, The SendableSdkPtr wrapper (the private final class SendableSdkPtr with ptr: UnsafeMutablePointer<SDKHandle>) is duplicated; extract it into a single internal utility type and replace the per-file copies. Create a single internal/internal-only Swift file (e.g., an internal utilities module within the package) that declares the SendableSdkPtr type (keeping `@unchecked` Sendable and the UnsafeMutablePointer<SDKHandle> property and initializer), update files that currently declare their own copies to import/use that shared type, and remove the duplicate declarations so all references use the consolidated SendableSdkPtr symbol.
55-83: Silent drop of invalid anchors viacompactMap.Line 76 uses
compactMapwhich silently drops anchors that fail hex decoding. While this prevents a single malformed entry from failing the entire query, it could mask data integrity issues. Consider logging dropped entries or returning a result that indicates partial success.Alternative with logging
- let anchors: [Data] = hexArray.compactMap { hexToData($0) } + var anchors: [Data] = [] + for (i, hex) in hexArray.enumerated() { + if let data = hexToData(hex) { + anchors.append(data) + } else { + // Log or track malformed entries + assertionFailure("Malformed anchor hex at index \(i)") + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift` around lines 55 - 83, getShieldedAnchors currently uses compactMap to silently drop hex strings that fail decoding (via hexToData), which masks malformed entries; change the logic after parsing hexArray so you attempt to decode every string (e.g., map over hexArray to produce optional Data), collect any failed entries, and if any failures exist resume the continuation with a specific error (e.g., SDKError.serializationError or a new SDKError.dataIntegrity) listing the invalid hex strings; if there are no failures resume returning the decoded [Data]; reference getShieldedAnchors, shieldedExtractString, hexToData, and SDKError.serializationError when making this change.packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift (2)
126-131: Silent failure on JSON serialization may hide bugs.Returning
"[]"when JSON serialization fails silently hides errors. Consider throwing an error or at least logging to aid debugging.Proposed improvement
- guard let data = try? JSONSerialization.data(withJSONObject: jsonArray, options: []), - let string = String(data: data, encoding: .utf8) - else { - return "[]" - } - return string + do { + let data = try JSONSerialization.data(withJSONObject: jsonArray, options: []) + guard let string = String(data: data, encoding: .utf8) else { + throw SDKError.serializationError("Failed to encode JSON as UTF-8") + } + return string + } catch { + // This would require changing the function signature to throws + assertionFailure("JSON serialization failed: \(error)") + return "[]" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift` around lines 126 - 131, The current conversion of jsonArray to a JSON string uses a silent try? on JSONSerialization.data(withJSONObject: jsonArray, options: []) and returns "[]" on failure, which hides errors; replace the try? with a do/catch around JSONSerialization.data(withJSONObject:jsonArray, options:) and either throw a descriptive error from the enclosing function or log the caught error (using the module's logger) before returning, so failures are surfaced instead of silently returning "[]".
141-150: Same silent failure pattern.Same concern as
encryptedNotesToJSON- silent failure on JSON serialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift` around lines 141 - 150, The function spendableNotesToJSON currently swallows JSONSerialization errors; change its signature to throw (func spendableNotesToJSON(_ notes: [SpendableNoteInfo]) throws -> String) and replace the try? with a do/try/catch or direct try so serialization failures propagate to the caller; in the catch rethrow the error (or let it bubble) instead of returning "[]", ensuring callers of spendableNotesToJSON can handle/report the real serialization error. Ensure you update call sites accordingly and keep the mapping via SpendableNoteInfo.toJSON unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs`:
- Around line 696-705: The current guard checks only unshield_amount (and
similar withdrawal_amount) against i64::MAX, but must validate the sum of amount
+ fee (the required total) to avoid overflow when Orchard expects an i64; change
the checks in the bundle builder to compute required =
amount.checked_add(min_fee) (or equivalent) and reject/return
DashSDKResult::error(DashSDKError::new(...)) if required > i64::MAX as u64 or if
the addition overflows. Update the checks that reference unshield_amount and
withdrawal_amount (and the other similar guards noted at the ranges) to use
required (amount + min_fee) and handle overflow detection consistently before
building the bundle.
- Around line 549-588: The code always uses num_actions = spends.len().max(2),
which forces a two-action fee even when a single-action transfer exactly matches
available note value; instead compute the one-action fee first (use num_actions
= spends.len().max(1) or explicitly 1 for recipient-only), check if
transfer_amount + one_action_fee <= total_spent: if it equals total_spent accept
with no change (pass None for ChangeOutput to build_spend_bundle_local); if it
is less than total_spent then a change output will exist so recompute min_fee
with num_actions = spends.len().max(2), recompute required and change_amount and
validate against total_spent before calling build_spend_bundle_local with
Some(ChangeOutput { address: change_address, amount: change_amount }); ensure
you reference and update variables num_actions, min_fee, required,
change_amount, and the call to build_spend_bundle_local accordingly and handle
overflow checks as before.
- Around line 533-534: Update the three FFI functions
dash_sdk_shielded_build_transfer_bundle,
dash_sdk_shielded_build_unshield_bundle, and
dash_sdk_shielded_build_withdrawal_bundle to accept a platform_version parameter
and use that when computing the minimum fee; specifically, remove the call to
PlatformVersion::latest() inside each builder and pass the incoming
platform_version into compute_minimum_shielded_fee (or any internal helper that
previously called PlatformVersion::latest()), and update all call
sites/signatures accordingly so the caller supplies the negotiated platform
version rather than relying on the SDK-compiled latest.
In `@packages/rs-sdk-ffi/src/shielded/crypto/decrypt.rs`:
- Around line 122-170: The loop currently decodes hex for
cmx/nullifier/encrypted_note and silently continues on malformed fields;
instead, after decoding cmx_bytes, nf_bytes, and enc_note_bytes validate their
lengths (expect cmx_bytes.len() == 32, nf_bytes.len() == 32,
encrypted_note.len() == 216) and return a caller-visible error when any length
is incorrect; update the block that builds ShieldedEncryptedNote (in decrypt.rs
where cmx_bytes, nf_bytes, enc_note_bytes are created and ShieldedEncryptedNote
is constructed) to perform these checks and propagate an Err (with a clear
message mentioning which field and index failed) rather than calling
tracing::debug and continue, leaving the subsequent try_decrypt_note and
decrypted push logic unchanged.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncTypes.swift`:
- Around line 111-118: The toFFI() function currently coerces any non-32-byte
poolIdentifier into a zeroed default and sets hasPoolId = false; instead,
validate poolIdentifier eagerly in toFFI(): if poolIdentifier is non-nil but
length != 32, fail fast (throw an error or return a Result/failable initializer)
rather than mutating the caller's request; only call dataToBytes32(pid) and set
hasPoolId = true when pid.count == 32, otherwise propagate the validation error
to the caller so FFINullifierSyncConfig is never silently created with a zeroed
poolIdTuple.
- Around line 13-74: The Swift-native definitions for FFINullifierSyncConfig,
FFINullifierSyncMetrics, and FFINullifierSyncResult are not guaranteed to have C
layout and will break FFI (Bool and tuple fields are brittle); fix by importing
the generated C FFI header (e.g., `#import` <DashSDKFFI/nullifier_sync_types.h>)
and using the C-defined structs instead of the Swift structs (or if you must
keep Swift types, replace fragile fields like Bool/tuple with fixed-size
C-compatible types and add static MemoryLayout.size/stride/alignment assertions
for FFINullifierSyncConfig, FFINullifierSyncMetrics, and FFINullifierSyncResult
to ensure ABI parity with the C/Rust definitions, and ensure the pool_identifier
type (Bytes32Tuple) matches the C typedef).
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift`:
- Around line 297-314: After hex decoding nullifierHex, cmxHex, addressHex,
rhoHex, and rseedHex (the variables used to construct DecryptedNote), validate
each Data's byte count matches the Rust DecryptedNoteJson spec: nullifier == 32,
cmx == 32, address == 43, rho == 32, rseed == 32; if any size mismatches, throw
SDKError.serializationError with a message identifying the field and index i
(e.g., "DecryptedNote[\(i)] invalid <field> size: expected X, got Y") so
malformed data is not silently accepted before creating the DecryptedNote.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swift`:
- Around line 15-52: FFISerializedAction may not have a guaranteed C-compatible
layout in Swift; add a runtime size assertion or freeze the type to ensure FFI
compatibility: verify MemoryLayout<FFISerializedAction>.size equals 208 at
startup or in tests (using an assert) and/or annotate the struct with `@frozen` to
prevent reordering, referencing the FFISerializedAction type and the expected
208 byte size so future changes will fail fast if the layout diverges.
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift`:
- Around line 126-131: The current conversion of jsonArray to a JSON string uses
a silent try? on JSONSerialization.data(withJSONObject: jsonArray, options: [])
and returns "[]" on failure, which hides errors; replace the try? with a
do/catch around JSONSerialization.data(withJSONObject:jsonArray, options:) and
either throw a descriptive error from the enclosing function or log the caught
error (using the module's logger) before returning, so failures are surfaced
instead of silently returning "[]".
- Around line 141-150: The function spendableNotesToJSON currently swallows
JSONSerialization errors; change its signature to throw (func
spendableNotesToJSON(_ notes: [SpendableNoteInfo]) throws -> String) and replace
the try? with a do/try/catch or direct try so serialization failures propagate
to the caller; in the catch rethrow the error (or let it bubble) instead of
returning "[]", ensuring callers of spendableNotesToJSON can handle/report the
real serialization error. Ensure you update call sites accordingly and keep the
mapping via SpendableNoteInfo.toJSON unchanged.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift`:
- Around line 1106-1108: withFFIShieldInputs currently force-unwraps
buf.baseAddress which will crash for an empty inputs array; update the
implementation in withFFIShieldInputs to safely handle an empty buffer by
checking if buf.baseAddress is nil and, in that case, invoke body with a
nil/empty pointer (or return an appropriate empty result) instead of
force-unwrapping, otherwise pass buf.baseAddress to body — reference the
withFFIShieldInputs function to locate and change this behavior.
- Around line 1084-1126: Add the same precondition used in withFFIBundle to
withFFIShieldInputs to guard against excessive recursion depth: at the top of
withFFIShieldInputs (before defining recurse) add a precondition that
inputs.count is <= the maximum allowed (use the same constant or check
expression used by withFFIBundle) so the function fails fast instead of risking
stack overflow from deep recursive nesting in recurse.
- Around line 959-962: The SendableSdkPtr wrapper (the private final class
SendableSdkPtr with ptr: UnsafeMutablePointer<SDKHandle>) is duplicated; extract
it into a single internal utility type and replace the per-file copies. Create a
single internal/internal-only Swift file (e.g., an internal utilities module
within the package) that declares the SendableSdkPtr type (keeping `@unchecked`
Sendable and the UnsafeMutablePointer<SDKHandle> property and initializer),
update files that currently declare their own copies to import/use that shared
type, and remove the duplicate declarations so all references use the
consolidated SendableSdkPtr symbol.
- Around line 55-83: getShieldedAnchors currently uses compactMap to silently
drop hex strings that fail decoding (via hexToData), which masks malformed
entries; change the logic after parsing hexArray so you attempt to decode every
string (e.g., map over hexArray to produce optional Data), collect any failed
entries, and if any failures exist resume the continuation with a specific error
(e.g., SDKError.serializationError or a new SDKError.dataIntegrity) listing the
invalid hex strings; if there are no failures resume returning the decoded
[Data]; reference getShieldedAnchors, shieldedExtractString, hexToData, and
SDKError.serializationError when making this change.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swift`:
- Around line 119-171: The helpers dataToBytes32, dataToBytes36, and
dataToBytes64 currently silently zero-pad short Data which can hide
cryptographic input errors; update each function to validate input length (use
precondition or assert) and fail fast when src.count is less than the expected
size, or alternatively add strict variants dataToBytes32Exact /
dataToBytes36Exact / dataToBytes64Exact that require src.count == 32/36/64 and
raise a clear error (or call preconditionFailure) before performing the unsafe
copy; ensure you reference these functions
(dataToBytes32/dataToBytes36/dataToBytes64) so callers can switch to the strict
versions or handle the precondition failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c74bf892-bcf4-4046-adc8-27c581560457
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
packages/rs-sdk-ffi/Cargo.tomlpackages/rs-sdk-ffi/src/shielded/crypto/address.rspackages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rspackages/rs-sdk-ffi/src/shielded/crypto/decrypt.rspackages/rs-sdk-ffi/src/shielded/crypto/mod.rspackages/rs-sdk-ffi/src/shielded/mod.rspackages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swift
CI fix: Replace from_raw_parts_mut implicit casts with Vec::from_raw_parts in bundle_params_free (newer Rust rejects the implicit coercion). CodeRabbit fixes: - Transfer fee: try 1-action fee first, only add change output fee when surplus exists (avoids rejecting valid exact-value transfers) - Validate required (amount+fee) against i64::MAX for unshield/withdrawal, not just the amount alone - Validate decoded field sizes in decrypt_notes (cmx=32, nullifier=32, encrypted_note>=84 bytes) instead of silently skipping - Swift: poolIdentifier validation throws instead of silently falling back to default pool - Swift: add field size validation in parseDecryptedNotesJSON (nullifier=32, cmx=32, address=43, rho=32, rseed=32) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs (1)
584-585:⚠️ Potential issue | 🟠 MajorUse negotiated platform version for fee calculation, not
PlatformVersion::latest().This still binds fee floor behavior to compile-time SDK version rather than runtime negotiated protocol version.
#!/bin/bash # Confirm remaining hardcoded latest-version fee paths. rg -n "PlatformVersion::latest\\(\\)" packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs -C2 # Compare with existing runtime-version usage patterns elsewhere in rs-sdk-ffi. rg -n "wrapper\\.sdk\\.version\\(" packages/rs-sdk-ffi/src --glob "*.rs" -C1Also applies to: 796-797, 969-970
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs` around lines 584 - 585, Replace the hardcoded PlatformVersion::latest() used for fee calculation with the negotiated/runtime platform version: obtain the version from the SDK runtime (e.g. call wrapper.sdk.version(...) or use the PlatformVersion passed into the surrounding function) and assign that to the platform_version variable used in fee computation in bundle_build.rs (affecting the sites where PlatformVersion::latest() is used such as the instances around the platform_version binding and the other occurrences mentioned). Ensure the code uses the same runtime-negotiated PlatformVersion that the rest of rs-sdk-ffi uses for fees rather than PlatformVersion::latest().packages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncTypes.swift (1)
13-74:⚠️ Potential issue | 🟠 MajorDon't pass Swift-native nullifier-sync structs across the FFI boundary.
These are ordinary Swift structs, but
NullifierSyncService.swiftpasses them todash_sdk_sync_nullifiers*by pointer and readsFFINullifierSyncResultback the same way. Swift does not guarantee C layout for native structs here, soBoolandBytes32Tuplecan drift from the Rust#[repr(C)]layout and corrupt config/result fields. Use generated C definitions fromDashSDKFFI, or add explicit size/stride/alignment assertions as a stopgap.Does Swift guarantee C-compatible memory layout for native Swift structs containing `Bool` and tuple fields when passing them to `@_silgen_name` FFI functions, or should imported C structs / generated headers be used instead?As per coding guidelines, "Swift code must properly wrap FFI functions with correct memory management across Swift/Rust boundaries".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncTypes.swift` around lines 13 - 74, The FFI structs FFINullifierSyncConfig and FFINullifierSyncResult (and their use in NullifierSyncService.swift when calling dash_sdk_sync_nullifiers*) are plain Swift structs and may not have C-compatible layout (Bool and tuple Bytes32Tuple can differ), so stop passing these Swift-native types across the boundary; instead import and use the generated C-compatible definitions from DashSDKFFI (or the C header equivalents) for config/result and change the call sites in NullifierSyncService.swift to allocate/populate those C structs and pass their pointers to dash_sdk_sync_nullifiers*, and when reading results use the provided C struct layout and dash_sdk_nullifier_sync_result_free for cleanup; if a temporary stopgap is required, add explicit static assertions about size/stride/alignment for FFINullifierSyncConfig/FFINullifierSyncResult to match the C types before using them.
🧹 Nitpick comments (2)
packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs (2)
58-60: Consider splitting helper logic from FFI entrypoints.This module now mixes parsing, bundle construction, serialization, and FFI glue in one very large file. Splitting by concern would improve maintainability and testability.
Also applies to: 422-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs` around lines 58 - 60, The file mixes parsing, bundle construction, serialization, and FFI glue; extract pure logic into separate modules (e.g., move parsing code into a parse_bundle module/function, bundle construction into build_bundle/construct_bundle, and serialization into serialize_bundle) and leave only thin FFI entrypoints (the extern "C"/#[no_mangle] functions currently in bundle_build.rs) to call those helpers; update visibility (pub(crate)/pub) and imports so the FFI functions only delegate to the extracted helpers and add tests for the helper functions separately.
289-297: Return ownedStringfromparse_c_strto eliminate lifetime hazard.The current unconstrained
'alifetime is easy to misuse—callers could specify a shorter lifetime than the pointer's actual validity. ReturningStringremoves this footgun while remaining compatible with all call sites (String auto-derefs to&strwhen passed toparse_spendable_notes).Refactor sketch
-unsafe fn parse_c_str<'a>(ptr: *const std::os::raw::c_char, name: &str) -> Result<&'a str, String> { +unsafe fn parse_c_str(ptr: *const std::os::raw::c_char, name: &str) -> Result<String, String> { if ptr.is_null() { return Err(format!("{} is null", name)); } std::ffi::CStr::from_ptr(ptr) .to_str() - .map_err(|e| format!("{} is not valid UTF-8: {}", name, e)) + .map(|s| s.to_owned()) + .map_err(|e| format!("{} is not valid UTF-8: {}", name, e)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs` around lines 289 - 297, The helper parse_c_str currently returns a borrowed &str with an unconstrained lifetime 'a which is unsafe to expose; change its signature to return Result<String, String> (remove the lifetime param), keep the null check, and convert the CStr to an owned String by calling to_str() and then .to_owned() (or .map(|s| s.to_owned()) in the map chain) while preserving the existing error mapping for invalid UTF-8; update all call sites that expect &str (they can accept &owned_string) and adjust the function name parse_c_str and its error messages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk-ffi/src/shielded/crypto/address.rs`:
- Around line 18-20: Update the doc comment for the Orchard address function
that returns a DashSDKResult (the comment currently instructs callers to use
dash_sdk_string_free); change the free function name to dash_sdk_result_free so
it correctly documents how to release the DashSDKResult/String payload. Locate
the doc block near the function that produces the hex-encoded 43-byte Orchard
raw address (the one returning DashSDKResult) and replace "dash_sdk_string_free"
with "dash_sdk_result_free".
In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs`:
- Around line 1-10: The crate-level/docs for the Orchard bundle FFI functions
still say they return a JSON string; update all such doc comments in
bundle_build.rs (including the header and the other occurrences noted) to state
that the functions now return DashSDKResultDataType::BinaryData containing a
heap-allocated pointer to a DashSDKOrchardBundleParams struct (i.e., an FFI
pointer to the bundle params) rather than JSON, and briefly note that the iOS
side must parse the FFI struct pointer accordingly; keep mention of the
underlying use of serialize_authorized_bundle/compute_platform_sighash/GroveDb
builder as-is.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncService.swift`:
- Around line 58-68: The background closure in NullifierSyncService's async
calls (see methods syncNullifiers and syncNullifiersRaw using
DispatchQueue.global().async) currently only captures the raw
NullifierSendableSdkPtr and can race with SDK deinit; update those closures to
capture self (e.g. change DispatchQueue.global().async { ... } to
DispatchQueue.global().async { [self] in ... }) so the owning SDK instance is
retained for the duration of the FFI call (dash_sdk_sync_nullifiers /
dash_sdk_sync_nullifiers_with_result) and avoid dangling handle use; apply the
same capture fix to the other occurrences mentioned in the comment.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncTypes.swift`:
- Around line 94-137: The toFFI() method currently forwards any UInt32 poolType
to Rust and allows poolType == 2 without a poolIdentifier; update toFFI() to
validate poolType and poolIdentifier up front: accept only 0, 1, or 2 for
poolType and if poolType == 2 require poolIdentifier to be non-nil and exactly
32 bytes (throw SDKError.invalidParameter with a clear message otherwise);
ensure these checks occur before constructing the FFINullifierSyncConfig so
invalid combinations are rejected in Swift rather than in the native layer.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoService.swift`:
- Around line 467-479: The closure passed to DispatchQueue.global().async
captures bundle.ptr but not the ShieldedBundleHandle itself, allowing
ShieldedBundleHandle.deinit to free that pointer while the FFI call runs; update
shieldedTransfer(bundle:valueBalance:),
unshieldFunds(outputAddress:amount:bundle:), and the other noted overloads to
retain the handle for the duration of the async work (e.g., capture a strong
reference like let retainedBundle = bundle before entering the closure and
reference retainedBundle inside the closure) so the underlying pointer owned by
ShieldedBundleHandle is not deallocated until after the FFI call returns; ensure
the same pattern is applied where bundle.ptr is used with dash_sdk_shielded_*
FFI calls and when using withFFIShieldInputs so the handle stays alive across
the GCD/FFI boundary.
- Around line 59-64: The temporary 32-byte spending-key tuple (created via
dataToBytes32(spendingKey) and named skTuple) is left in Swift memory after each
FFI call (e.g., dash_sdk_shielded_derive_address) — wrap the creation and FFI
invocation in a small helper that zeroes the skTuple in a defer block and uses
withUnsafePointer/withUnsafeMutablePointer for the call; replace direct uses in
the current method and the other locations (lines noted: 107-112, 181-183,
265-267, 353-355, 421-424) to call that helper so every code path zeroizes the
buffer after the native call returns. Ensure the helper accepts the
spendingKey/Data, performs dataToBytes32, calls the provided closure that
invokes the FFI (e.g., dash_sdk_shielded_derive_address) using
withUnsafePointer(to: &skTuple), and then overwrites skTuple bytes with zeros in
defer.
- Around line 67-73: The code currently only verifies the Rust result is hex
before converting to Data; add a length check after hexToData to ensure
addressData.count == 43 (the promised Orchard address length) and if not, call
continuation.resume(throwing: SDKError.serializationError(...)) so
malformed/truncated/oversized FFI output is rejected; locate the block around
cryptoExtractString(result) / hexToData(...) / continuation and insert the
validation using the same SDKError.serializationError and continuation.resume
error path.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift`:
- Around line 68-82: The SpendableNoteInfo initializer and its serialization
path must validate fixed-size fields before crossing to Rust: ensure address
length == 43, rho and rseed length == 32, and merklePath.count == 32 with each
node length == 32; add these checks in the public init (and the corresponding
model/serializer used by toJSON) and return/throw a clear error (or fail the
initializer) when sizes are incorrect so malformed data is rejected in Swift
rather than sent over the FFI boundary; refer to SpendableNoteInfo (init) and
the toJSON/serialization function to locate where to add these guards and error
handling.
---
Duplicate comments:
In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs`:
- Around line 584-585: Replace the hardcoded PlatformVersion::latest() used for
fee calculation with the negotiated/runtime platform version: obtain the version
from the SDK runtime (e.g. call wrapper.sdk.version(...) or use the
PlatformVersion passed into the surrounding function) and assign that to the
platform_version variable used in fee computation in bundle_build.rs (affecting
the sites where PlatformVersion::latest() is used such as the instances around
the platform_version binding and the other occurrences mentioned). Ensure the
code uses the same runtime-negotiated PlatformVersion that the rest of
rs-sdk-ffi uses for fees rather than PlatformVersion::latest().
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncTypes.swift`:
- Around line 13-74: The FFI structs FFINullifierSyncConfig and
FFINullifierSyncResult (and their use in NullifierSyncService.swift when calling
dash_sdk_sync_nullifiers*) are plain Swift structs and may not have C-compatible
layout (Bool and tuple Bytes32Tuple can differ), so stop passing these
Swift-native types across the boundary; instead import and use the generated
C-compatible definitions from DashSDKFFI (or the C header equivalents) for
config/result and change the call sites in NullifierSyncService.swift to
allocate/populate those C structs and pass their pointers to
dash_sdk_sync_nullifiers*, and when reading results use the provided C struct
layout and dash_sdk_nullifier_sync_result_free for cleanup; if a temporary
stopgap is required, add explicit static assertions about size/stride/alignment
for FFINullifierSyncConfig/FFINullifierSyncResult to match the C types before
using them.
---
Nitpick comments:
In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs`:
- Around line 58-60: The file mixes parsing, bundle construction, serialization,
and FFI glue; extract pure logic into separate modules (e.g., move parsing code
into a parse_bundle module/function, bundle construction into
build_bundle/construct_bundle, and serialization into serialize_bundle) and
leave only thin FFI entrypoints (the extern "C"/#[no_mangle] functions currently
in bundle_build.rs) to call those helpers; update visibility (pub(crate)/pub)
and imports so the FFI functions only delegate to the extracted helpers and add
tests for the helper functions separately.
- Around line 289-297: The helper parse_c_str currently returns a borrowed &str
with an unconstrained lifetime 'a which is unsafe to expose; change its
signature to return Result<String, String> (remove the lifetime param), keep the
null check, and convert the CStr to an owned String by calling to_str() and then
.to_owned() (or .map(|s| s.to_owned()) in the map chain) while preserving the
existing error mapping for invalid UTF-8; update all call sites that expect &str
(they can accept &owned_string) and adjust the function name parse_c_str and its
error messages accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9358a0b9-d240-4b0a-aa04-645fa07e6181
📒 Files selected for processing (9)
packages/rs-sdk-ffi/Cargo.tomlpackages/rs-sdk-ffi/src/shielded/crypto/address.rspackages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rspackages/rs-sdk-ffi/src/shielded/crypto/decrypt.rspackages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rs-sdk-ffi/src/shielded/crypto/decrypt.rs
- packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoFFI.swift
- packages/rs-sdk-ffi/Cargo.toml
Add a high-level shielded pool client that manages the commitment tree,
note tracking, and bundle building entirely in Rust behind an opaque FFI
handle. iOS never touches merkle paths or tree state.
Rust pool_client module (1184 lines):
- ShieldedPoolClient struct: OrchardKeySet + notes + SQLite-backed
ClientPersistentCommitmentTree + sync checkpoints
- create/destroy lifecycle with automatic tree resumption
- sync_notes: fetch encrypted notes, trial decrypt, append to tree,
track owned notes with nullifiers
- sync_nullifiers: BLAST privacy-preserving nullifier sync with
incremental catch-up via height/timestamp checkpoints
- build_shield/transfer/unshield/withdrawal_bundle: automatic note
selection, tree.witness() for merkle paths, tree.anchor() for root,
uses DPP builders directly for correct fee/change handling
Swift ShieldedPoolClient class (509 lines):
- init(dbPath:spendingKey:) with automatic deinit cleanup
- address/balance properties
- async syncNotes/syncNullifiers (dispatched to background queue)
- async buildShieldBundle/buildTransferBundle/buildUnshieldBundle/
buildWithdrawalBundle returning ShieldedBundleHandle
End-to-end iOS flow now works for ALL shielded operations:
let client = try ShieldedPoolClient(dbPath: path, spendingKey: key)
try await client.syncNotes(sdk: sdk)
let bundle = try await client.buildTransferBundle(
recipientAddress: addr, amount: 1000)
try await sdk.shieldedTransfer(bundle: bundle, valueBalance: ...)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CRITICAL: Retain ShieldedBundleHandle in transition closure capture lists ([self, retainedBundle]) to prevent use-after-free when ARC releases the handle before the GCD closure completes. CRITICAL: Add [self] capture in NullifierSyncService closures to retain the SDK instance through the FFI call duration. MAJOR: Zero skTuple (spending key bytes) via defer after every FFI call in ShieldedCryptoService (7 call sites). MAJOR: Validate derived address length is exactly 43 bytes. MAJOR: Validate poolType (0/1/2) and require poolIdentifier when poolType == 2 (individual token) in NullifierSyncConfig.toFFI(). MAJOR: Add SpendableNoteInfo.validate() checking address=43B, rho/rseed=32B, merklePath=32 entries of 32B each. Called before JSON serialization. MINOR: Fix stale doc comments in address.rs and bundle_build.rs (JSON -> struct pointer return type). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lation OrchardAction, OrchardBundle, and WithdrawalPooling are captured in DispatchQueue.global().async closures crossing @mainactor isolation. Swift 6 strict concurrency requires these to conform to Sendable. All three are simple value types with only Sendable fields (Data, UInt8). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift (3)
138-153: Silent failure returns empty array instead of propagating error.When
JSONSerialization.datafails, the function returns"[]"instead of throwing or logging. This could mask serialization bugs where notes appear to be empty when they actually failed to serialize.♻️ Proposed fix to throw on failure
-func encryptedNotesToJSON(_ notes: [EncryptedNote]) -> String { +func encryptedNotesToJSON(_ notes: [EncryptedNote]) throws -> String { let jsonArray: [[String: Any]] = notes.map { note in [ "cmx": note.cmx.toHexString(), "nullifier": note.nullifier.toHexString(), "encryptedNote": note.encryptedNote.toHexString() ] } - guard let data = try? JSONSerialization.data(withJSONObject: jsonArray, options: []), - let string = String(data: data, encoding: .utf8) - else { - return "[]" - } - return string + let data = try JSONSerialization.data(withJSONObject: jsonArray, options: []) + guard let string = String(data: data, encoding: .utf8) else { + throw SDKError.serializationError("Failed to encode JSON as UTF-8") + } + return string }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift` around lines 138 - 153, The function encryptedNotesToJSON currently swallows JSONSerialization failures and returns "[]"; change its signature to throw (e.g., func encryptedNotesToJSON(_ notes: [EncryptedNote]) throws -> String) and replace the optional try? block with a throwing try so serialization errors are propagated to callers; also handle the String(data:encoding:) conversion by throwing an error if encoding fails instead of returning "[]". Update any callers of encryptedNotesToJSON to handle the thrown error accordingly.
190-200:@unchecked Sendableon class with raw pointer.
ShieldedBundleHandleis marked@unchecked Sendablebut holds a raw pointer that isn't thread-safe. If the handle is shared across actors/threads and accessed concurrently, this could cause data races. Consider documenting that the handle must not be accessed concurrently, or removeSendableconformance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift` around lines 190 - 200, ShieldedBundleHandle is marked `@unchecked` Sendable while storing a raw UnsafeMutablePointer<FFIOrchardBundleParams> (ptr), which is not thread-safe; remove the `@unchecked` Sendable annotation from the ShieldedBundleHandle declaration and add a doc comment on ShieldedBundleHandle stating that instances are not safe for concurrent access (or alternatively route all access through an actor/lock or provide a thread-safe wrapper around ptr and use that instead); ensure deinit still calls dash_sdk_shielded_bundle_params_free(ptr) unchanged.
162-178: Inconsistent error handling after validation passes.After successfully validating all notes, if
JSONSerialization.datafails, the function returns"[]". Since validation already passed, this silently discards valid data due to an unexpected serialization failure.♻️ Proposed fix
let jsonArray: [[String: Any]] = notes.map { $0.toJSON() } - guard let data = try? JSONSerialization.data(withJSONObject: jsonArray, options: []), - let string = String(data: data, encoding: .utf8) - else { - return "[]" - } - return string + let data = try JSONSerialization.data(withJSONObject: jsonArray, options: []) + guard let string = String(data: data, encoding: .utf8) else { + throw SDKError.serializationError("Failed to encode spendable notes JSON as UTF-8") + } + return string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift` around lines 162 - 178, spendableNotesToJSON currently validates each SpendableNoteInfo then silently returns "[]" if JSONSerialization.data fails; change this to surface the serialization failure: perform the JSONSerialization.data call with try/catch (or use try?), capture the thrown error, and throw an appropriate SDKError (e.g., SDKError.serializationError or SDKError.internalError) with a clear message like "Failed to serialize spendable notes" including the underlying error; update the guard that currently returns "[]" to instead throw that SDKError so valid data isn't discarded silently.packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swift (2)
119-134: Silent truncation of oversized data indataToBytes32.The
min(src.count, 32)pattern silently truncates data larger than 32 bytes. For cryptographic fields like nullifiers and keys, passing oversized data likely indicates a bug that should fail loudly rather than silently truncate.Consider adding a precondition or logging when
data.count > 32.♻️ Proposed defensive check
func dataToBytes32(_ data: Data) -> Bytes32Tuple { + assert(data.count <= 32, "dataToBytes32 received \(data.count) bytes, expected <= 32") var tuple: Bytes32Tuple = (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swift` around lines 119 - 134, The function dataToBytes32 silently truncates input longer than 32 bytes; add a defensive check at the start of dataToBytes32 to fail loudly when data.count > 32 (e.g., use precondition or throw/assert with a clear message) so oversized cryptographic fields (nullifiers/keys) are not silently truncated; keep the existing copy logic for valid sizes and reference the Bytes32Tuple and dataToBytes32 symbols when making the change.
300-323: Clarify pointer assignment pattern inbuildFFIActions.The function creates
FFISerializedActionwithencrypted_note: niland a comment indicating the pointer should be set viawithUnsafeBytesduring the FFI call. This requires careful caller coordination.Consider documenting the expected usage pattern more explicitly, or restructure to use a closure-based API that guarantees pointer lifetime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swift` around lines 300 - 323, The current buildFFIActions creates FFISerializedAction instances with encrypted_note: nil and relies on the caller to set the pointer with withUnsafeBytes, which is error-prone; change buildFFIActions to either (1) document the exact caller pattern for withUnsafeBytes and pointer lifetime for encrypted_note, or preferably (2) convert buildFFIActions into a closure-based API that accepts a closure (e.g. buildFFIActions(from:actions, useFFIActions: ([_]FFISerializedAction, [_]Data) -> R) -> R) so you allocate storage, call withUnsafeBytes on each encryptedNote inside the closure to set encrypted_note on each FFISerializedAction and invoke the FFI within that closure ensuring pointers remain valid, updating references to FFISerializedAction, encrypted_note, and withUnsafeBytes accordingly.packages/rs-sdk-ffi/src/shielded/pool_client/mod.rs (1)
59-67: Potential integer overflow inrecalculate_balance().The
.sum()operation on note values can overflow if the total exceedsu64::MAX. While unlikely in practice, the PR objectives noted thatchecked_addwas a HIGH security finding for sum operations in bundle building. Consider using checked arithmetic here as well for consistency.♻️ Proposed fix using checked arithmetic
pub(crate) fn recalculate_balance(&self) -> u64 { - self.notes + self.notes .iter() .filter(|n| !n.is_spent) .map(|n| n.value) - .sum() + .try_fold(0u64, |acc, v| acc.checked_add(v)) + .unwrap_or(u64::MAX) // saturate on overflow }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/pool_client/mod.rs` around lines 59 - 67, The current ShieldedPoolClient::recalculate_balance uses .sum() which can overflow; replace the summation with a checked accumulation (e.g., iter().filter(...).fold(0u64, |acc, n| acc.checked_add(n.value).unwrap_or(u64::MAX))) so you use checked_add on each note.value and saturate to u64::MAX on overflow (or otherwise handle overflow consistently), referencing ShieldedPoolClient::recalculate_balance, the notes iterator, the is_spent predicate, and note.value to locate and update the logic.packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs (1)
638-644: Silent dust burning may surprise users.When the surplus is less than the additional fee required for a change output, the code silently burns the difference as extra fee (line 640). This could lose user funds unexpectedly. Consider either:
- Returning an error explaining the dust situation
- Documenting this behavior clearly in the function docs
- Adding a minimum dust threshold check
💡 Option: Return error when dust would be burned
} else { // Surplus exists — recompute with change output (adds an action) let with_change_actions = spends.len().max(2); let with_change_fee = compute_minimum_shielded_fee(with_change_actions, platform_version); let with_change_required = match transfer_amount.checked_add(with_change_fee) { // ... }; if with_change_required > total_spent { - // Can't afford change output fee — use no-change fee and burn the dust - (no_change_fee, 0u64) + // Surplus exists but isn't enough to cover change output fee + let dust = total_spent - no_change_required; + return DashSDKResult::error(DashSDKError::new( + DashSDKErrorCode::InvalidParameter, + format!( + "Insufficient funds for change output: {} dust would be lost as fee", + dust + ), + )); } else { (with_change_fee, total_spent - with_change_required) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs` around lines 638 - 644, The current branch that silently burns dust when with_change_required > total_spent should instead return an explicit error (e.g., DustWouldBeBurned) or enforce a configurable minimum dust threshold; update the logic around with_change_required, total_spent, no_change_fee and with_change_fee to return Result<(fee, change), Error> rather than silently returning (no_change_fee, 0), add a new error variant (DustWouldBeBurned or InsufficientForChange) and a configurable MIN_DUST_THRESHOLD check, and update all callers of this function to handle the new error (or accept documented behavior) so users are not unexpectedly losing funds.packages/rs-sdk-ffi/src/shielded/pool_client/bundle.rs (1)
82-82: Consider handling poisoned mutex gracefully.Using
.unwrap()on a mutex lock will panic if the mutex is poisoned (e.g., a previous thread panicked while holding it). In an FFI context, panics can crash the host application. Consider returning an error instead.♻️ Suggested change
- let tree = client.commitment_tree.lock().unwrap(); + let tree = client.commitment_tree.lock().map_err(|e| { + format!("Failed to lock commitment tree: {}", e) + })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/shielded/pool_client/bundle.rs` at line 82, The direct call let tree = client.commitment_tree.lock().unwrap(); can panic on a poisoned mutex; replace it with explicit handling of lock(). For example, use match client.commitment_tree.lock() { Ok(guard) => guard, Err(poisoned) => return Err(your_error_type::MutexPoisoned("commitment_tree")) } (or, if continuing is acceptable, use poisoned.into_inner() to recover) and propagate that error from the surrounding function so the FFI boundary never panics; update references to commitment_tree and the surrounding function's return type to return an appropriate Result/Error instead of unwrapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs`:
- Around line 436-438: Update the stale "# Returns" doc comments that still say
"JSON string" to correctly describe that these functions return a heap-allocated
pointer to DashSDKOrchardBundleParams (not a JSON string); explicitly state the
returned type DashSDKOrchardBundleParams*, that ownership is transferred to the
caller, that the pointer may be null on error, and that the caller is
responsible for freeing the allocated object; apply this change to all four
functions in bundle_build.rs whose return sections reference "JSON string" (the
ones returning DashSDKOrchardBundleParams pointers).
In `@packages/rs-sdk-ffi/src/shielded/pool_client/bundle.rs`:
- Around line 269-274: Add the same zero-amount guard that exists for the shield
bundle to the transfer, unshield and withdrawal bundle builders: in the
TransferBundleBuilder, UnshieldBundleBuilder and WithdrawalBundleBuilder build
methods (the blocks around the ranges flagged), check if amount == 0 and return
a DashSDKResult::error(DashSDKError::new(DashSDKErrorCode::InvalidParameter,
"transfer amount must be greater than zero".to_string())) (or the analogous
message for unshield/withdrawal) before proceeding, mirroring the validation in
crypto/bundle_build.rs so degenerate zero-amount bundles are rejected.
- Around line 38-44: The summation of unspent note values using `.sum()` can
overflow silently; update the calculation of `total_available` (currently `let
total_available: u64 = unspent.iter().map(|n| n.value).sum();`) to use a checked
accumulation like `try_fold` with `checked_add` to detect overflow and propagate
an `Err` (e.g., return an error indicating overflow) before comparing to
`amount`; ensure you reference the same pattern used in `crypto/bundle_build.rs`
so `unspent`, `total_available`, and `amount` are handled safely and
consistently.
- Around line 496-500: The match on the incoming pooling byte incorrectly
accepts any value >=2 as Pooling::Standard; change the logic in bundle.rs around
pooling_enum to explicitly accept only 0 => Pooling::Never, 1 =>
Pooling::IfAvailable, 2 => Pooling::Standard and treat any other byte as an
error: return a Result::Err (using the function's existing error type or create
a descriptive error like InvalidPoolingValue) instead of silently coercing;
update the surrounding function signature/propagation (where pooling_enum is
used) so the error can be returned to the caller.
In `@packages/rs-sdk-ffi/src/shielded/pool_client/sync.rs`:
- Around line 100-111: The mutex lock usage on client.commitment_tree currently
uses .lock().unwrap(), which can panic if the mutex is poisoned; replace both
occurrences (the append call and the checkpoint call) to handle poisoned mutexes
safely by using .lock().unwrap_or_else(|e| e.into_inner()) or by mapping the
PoisonError into a DashSDKResult::error and returning it from the FFI function;
update the code paths that call commitment_tree.lock().unwrap() to either
recover with into_inner() or return a proper DashSDKError (preserving existing
error formatting like "Failed to append note to tree" / checkpoint errors) so no
panic can cross the FFI boundary.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncTypes.swift`:
- Around line 13-40: Change the FFINullifierSyncConfig field has_pool_identifier
from Bool to UInt8 to ensure C-compatible FFI layout; update the Swift-side
conversion in the toFFI() helper to pass hasPoolId ? 1 : 0 as a UInt8 when
constructing FFINullifierSyncConfig so the memory layout matches
DashSDKNullifierSyncConfig on the Rust side and preserves semantics.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolClient.swift`:
- Around line 50-56: The spending key tuple skTuple created by dataToBytes32 and
passed to dash_sdk_shielded_pool_client_create is not zeroized after the FFI
call; add a defer that overwrites/zeroes skTuple immediately after the
withCString/withUnsafePointer block to remove sensitive key material from Swift
memory, following the same zeroization pattern used in
ShieldedCryptoService.swift (i.e., create skTuple as a var, perform the FFI
call, then in a defer wipe all 32 bytes of skTuple before leaving the scope).
---
Nitpick comments:
In `@packages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rs`:
- Around line 638-644: The current branch that silently burns dust when
with_change_required > total_spent should instead return an explicit error
(e.g., DustWouldBeBurned) or enforce a configurable minimum dust threshold;
update the logic around with_change_required, total_spent, no_change_fee and
with_change_fee to return Result<(fee, change), Error> rather than silently
returning (no_change_fee, 0), add a new error variant (DustWouldBeBurned or
InsufficientForChange) and a configurable MIN_DUST_THRESHOLD check, and update
all callers of this function to handle the new error (or accept documented
behavior) so users are not unexpectedly losing funds.
In `@packages/rs-sdk-ffi/src/shielded/pool_client/bundle.rs`:
- Line 82: The direct call let tree = client.commitment_tree.lock().unwrap();
can panic on a poisoned mutex; replace it with explicit handling of lock(). For
example, use match client.commitment_tree.lock() { Ok(guard) => guard,
Err(poisoned) => return Err(your_error_type::MutexPoisoned("commitment_tree")) }
(or, if continuing is acceptable, use poisoned.into_inner() to recover) and
propagate that error from the surrounding function so the FFI boundary never
panics; update references to commitment_tree and the surrounding function's
return type to return an appropriate Result/Error instead of unwrapping.
In `@packages/rs-sdk-ffi/src/shielded/pool_client/mod.rs`:
- Around line 59-67: The current ShieldedPoolClient::recalculate_balance uses
.sum() which can overflow; replace the summation with a checked accumulation
(e.g., iter().filter(...).fold(0u64, |acc, n|
acc.checked_add(n.value).unwrap_or(u64::MAX))) so you use checked_add on each
note.value and saturate to u64::MAX on overflow (or otherwise handle overflow
consistently), referencing ShieldedPoolClient::recalculate_balance, the notes
iterator, the is_spent predicate, and note.value to locate and update the logic.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swift`:
- Around line 138-153: The function encryptedNotesToJSON currently swallows
JSONSerialization failures and returns "[]"; change its signature to throw
(e.g., func encryptedNotesToJSON(_ notes: [EncryptedNote]) throws -> String) and
replace the optional try? block with a throwing try so serialization errors are
propagated to callers; also handle the String(data:encoding:) conversion by
throwing an error if encoding fails instead of returning "[]". Update any
callers of encryptedNotesToJSON to handle the thrown error accordingly.
- Around line 190-200: ShieldedBundleHandle is marked `@unchecked` Sendable while
storing a raw UnsafeMutablePointer<FFIOrchardBundleParams> (ptr), which is not
thread-safe; remove the `@unchecked` Sendable annotation from the
ShieldedBundleHandle declaration and add a doc comment on ShieldedBundleHandle
stating that instances are not safe for concurrent access (or alternatively
route all access through an actor/lock or provide a thread-safe wrapper around
ptr and use that instead); ensure deinit still calls
dash_sdk_shielded_bundle_params_free(ptr) unchanged.
- Around line 162-178: spendableNotesToJSON currently validates each
SpendableNoteInfo then silently returns "[]" if JSONSerialization.data fails;
change this to surface the serialization failure: perform the
JSONSerialization.data call with try/catch (or use try?), capture the thrown
error, and throw an appropriate SDKError (e.g., SDKError.serializationError or
SDKError.internalError) with a clear message like "Failed to serialize spendable
notes" including the underlying error; update the guard that currently returns
"[]" to instead throw that SDKError so valid data isn't discarded silently.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swift`:
- Around line 119-134: The function dataToBytes32 silently truncates input
longer than 32 bytes; add a defensive check at the start of dataToBytes32 to
fail loudly when data.count > 32 (e.g., use precondition or throw/assert with a
clear message) so oversized cryptographic fields (nullifiers/keys) are not
silently truncated; keep the existing copy logic for valid sizes and reference
the Bytes32Tuple and dataToBytes32 symbols when making the change.
- Around line 300-323: The current buildFFIActions creates FFISerializedAction
instances with encrypted_note: nil and relies on the caller to set the pointer
with withUnsafeBytes, which is error-prone; change buildFFIActions to either (1)
document the exact caller pattern for withUnsafeBytes and pointer lifetime for
encrypted_note, or preferably (2) convert buildFFIActions into a closure-based
API that accepts a closure (e.g. buildFFIActions(from:actions, useFFIActions:
([_]FFISerializedAction, [_]Data) -> R) -> R) so you allocate storage, call
withUnsafeBytes on each encryptedNote inside the closure to set encrypted_note
on each FFISerializedAction and invoke the FFI within that closure ensuring
pointers remain valid, updating references to FFISerializedAction,
encrypted_note, and withUnsafeBytes accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffe6db9c-4d7a-49ed-898b-0257b64b504e
📒 Files selected for processing (13)
packages/rs-sdk-ffi/src/shielded/crypto/address.rspackages/rs-sdk-ffi/src/shielded/crypto/bundle_build.rspackages/rs-sdk-ffi/src/shielded/mod.rspackages/rs-sdk-ffi/src/shielded/pool_client/bundle.rspackages/rs-sdk-ffi/src/shielded/pool_client/mod.rspackages/rs-sdk-ffi/src/shielded/pool_client/sync.rspackages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedCryptoTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolClientFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedTypes.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncService.swift
SendableSdkPtr is used by both ShieldedPoolService.swift and ShieldedCryptoService.swift within the same module. Swift's private access restricts to the declaring file, causing compilation failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift`:
- Around line 1084-1125: withFFIShieldInputs currently takes raw privateKey Data
slices and passes pointers into FFIShieldInput (which has no length), risking
OOB reads if keys != 32 bytes; add validation to ensure each
ShieldFundsInput.privateKey.count == 32 (either by making withFFIShieldInputs
throwing or adding a private func validateShieldInputs(_:) that throws
SDKError.invalidParameter with the index on mismatch) and call that validation
from callers such as shieldFunds and buildShield (or before constructing
addrPtrs/pkPtrs) so only validated 32-byte keys are forwarded to FFIShieldInput.
- Around line 427-434: The guard branches inside instantLock.withUnsafeBytes and
transaction.withUnsafeBytes currently return an empty DashSDKResult() which can
be interpreted as success; instead check for empty Data before calling
withUnsafeBytes (e.g., verify instantLock.isEmpty or transaction.isEmpty and
return a DashSDKResult populated with an appropriate error code/message), and
remove the DashSDKResult() fallbacks inside the withUnsafeBytes guards so you
never report success when baseAddress is nil; update the shieldFromInstantLock
path (and similar code around shieldedExtractVoid and the other occurrence at
the second location) to use this explicit error result so the FFI call is only
attempted when buffers are non-empty and valid.
- Around line 69-77: The JSON parsing currently uses compactMap (e.g., in the
block converting hexArray to anchors via hexToData), which silently drops
malformed entries; change these conversions to strict decoding: map each hex
string and if any hexToData conversion fails, resume the continuation throwing
an SDKError.serializationError with a descriptive message instead of compacting;
apply the same strict approach to the nullifier and note array decodings
referenced around lines handling nullifiers and notes, and in the
checkNullifiers flow validate that statuses.count == nullifiers.count and throw
an error if they differ to preserve 1:1 alignment.
- Around line 1009-1054: The helper currently assumes field sizes and uses
precondition, which can abort or produce corrupted FFI payloads; instead, add
explicit throwing validation in the public entry points that construct
OrchardBundle (validate OrchardBundle.anchor length, bindingSignature length,
each action.nullifier/rk/cmx/cvNet/spendAuthSig byte widths, and encryptedNote
lengths) and remove reliance on precondition in the internal helper that builds
FFISerializedAction/FFIOrchardBundleParams; use the existing helpers
dataToBytes32/dataToBytes64 to check exact lengths and throw a descriptive error
type (not precondition) on mismatch, and ensure the code that calls ffi (the
block using bundle.proof.withUnsafeBytes and
FFISerializedAction/FFIOrchardBundleParams) only runs after validation so memory
ownership across Swift/Rust boundaries remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 585088bf-41b4-4a5c-88db-8f42a7eefcba
📒 Files selected for processing (1)
packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift
…rnal These helpers are used by both ShieldedPoolService.swift and ShieldedCryptoService.swift within the same module. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift (4)
270-275:⚠️ Potential issue | 🔴 CriticalValidate every
ShieldFundsInput.privateKeylength before pointer handoff.
FFIShieldInputhas no private-key length field, so passing non-32-byte keys throughwithFFIShieldInputsrisks out-of-bounds reads on the Rust side.Suggested fix
+ private func validateShieldInputs(_ inputs: [ShieldFundsInput]) throws { + for (index, input) in inputs.enumerated() { + guard input.privateKey.count == 32 else { + throw SDKError.invalidParameter("Input \(index) private key must be exactly 32 bytes") + } + } + } public func shieldFunds(...) async throws { + try validateShieldInputs(inputs) ... } public func buildShield(...) async throws -> Data { + try validateShieldInputs(inputs) ... }As per coding guidelines, Swift code must properly wrap FFI functions with correct memory management across Swift/Rust boundaries.
Also applies to: 680-685, 1084-1125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift` around lines 270 - 275, Before handing memory to Rust, validate that every ShieldFundsInput.privateKey is exactly 32 bytes long and fail early if not: in shieldFunds and any other call sites that use withFFIShieldInputs/FFIShieldInput (e.g., the conversion helpers around withFFIShieldInputs), check each input.privateKey.count == 32 and throw/return a descriptive Swift error if any length is wrong; do this validation outside the unsafe pointer block so no malformed keys are passed into withUnsafeBytes/withFFIShieldInputs, and add unit tests exercising short/long keys to ensure the guard is effective.
427-434:⚠️ Potential issue | 🟠 MajorPrevent empty instant-lock/transaction buffers from becoming false success.
When
instantLockortransactionis empty, the nilbaseAddressfallback returnsDashSDKResult()and can mask a failed call path as success.Suggested fix
+ guard !instantLock.isEmpty else { + throw SDKError.invalidParameter("Instant lock must not be empty") + } + guard !transaction.isEmpty else { + throw SDKError.invalidParameter("Transaction must not be empty") + } - guard let ilBase = ilPtr.baseAddress?.assumingMemoryBound(to: UInt8.self) else { - return DashSDKResult() - } + let ilBase = ilPtr.baseAddress!.assumingMemoryBound(to: UInt8.self) - guard let txBase = txPtr.baseAddress?.assumingMemoryBound(to: UInt8.self) else { - return DashSDKResult() - } + let txBase = txPtr.baseAddress!.assumingMemoryBound(to: UInt8.self)As per coding guidelines, Swift code must properly wrap FFI functions with correct memory management across Swift/Rust boundaries.
Also applies to: 753-760
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift` around lines 427 - 434, The current instantLock.withUnsafeBytes and transaction.withUnsafeBytes blocks return a default DashSDKResult() when baseAddress is nil, which treats empty buffers as a false success; instead, explicitly detect empty instantLock or transaction (e.g., check instantLock.isEmpty / transaction.isEmpty or nil baseAddress) and return a proper failure DashSDKResult populated to indicate invalid/empty input before attempting the FFI call; update the instantLock.withUnsafeBytes / transaction.withUnsafeBytes handling (and the analogous block at lines 753-760) to produce and return an error result rather than DashSDKResult() so the caller and Rust boundary see a real failure.
69-77:⚠️ Potential issue | 🟠 MajorDo not silently drop malformed shielded records.
compactMaphere hides malformed FFI JSON entries and can return truncated results; incheckNullifiersit can also break 1:1 mapping with requested nullifiers.Suggested fix
- let anchors: [Data] = hexArray.compactMap { hexToData($0) } + let anchors = try hexArray.enumerated().map { index, hex in + guard let data = hexToData(hex), data.count == 32 else { + throw SDKError.serializationError("Invalid anchor at index \(index)") + } + return data + } - let statuses: [NullifierStatus] = jsonArray.compactMap { obj in - guard let hexStr = obj["nullifier"] as? String, - let isSpent = obj["isSpent"] as? Bool, - let data = hexToData(hexStr) - else { return nil } - return NullifierStatus(nullifier: data, isSpent: isSpent) - } + let statuses: [NullifierStatus] = try jsonArray.enumerated().map { index, obj in + guard let hexStr = obj["nullifier"] as? String, + let isSpent = obj["isSpent"] as? Bool, + let data = hexToData(hexStr), + data.count == 32 else { + throw SDKError.serializationError("Invalid nullifier status at index \(index)") + } + return NullifierStatus(nullifier: data, isSpent: isSpent) + } + guard statuses.count == nullifiers.count else { + throw SDKError.serializationError("Nullifier status count mismatch") + } - let notes: [EncryptedNote] = jsonArray.compactMap { obj in - guard let cmxHex = obj["cmx"] as? String, - let nfHex = obj["nullifier"] as? String, - let encHex = obj["encryptedNote"] as? String, - let cmx = hexToData(cmxHex), - let nf = hexToData(nfHex), - let enc = hexToData(encHex) - else { return nil } - return EncryptedNote(cmx: cmx, nullifier: nf, encryptedNote: enc) - } + let notes: [EncryptedNote] = try jsonArray.enumerated().map { index, obj in + guard let cmxHex = obj["cmx"] as? String, + let nfHex = obj["nullifier"] as? String, + let encHex = obj["encryptedNote"] as? String, + let cmx = hexToData(cmxHex), cmx.count == 32, + let nf = hexToData(nfHex), nf.count == 32, + let enc = hexToData(encHex) else { + throw SDKError.serializationError("Invalid encrypted note at index \(index)") + } + return EncryptedNote(cmx: cmx, nullifier: nf, encryptedNote: enc) + }As per coding guidelines, Swift code must properly wrap FFI functions with correct memory management across Swift/Rust boundaries.
Also applies to: 167-174, 212-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift` around lines 69 - 77, The code silently drops malformed hex entries by using compactMap when building anchors — replace the compactMap logic in ShieldedPoolService (where hexArray is parsed and anchors is created) with a strict mapping that validates each hex string via hexToData and, on the first failure, calls continuation.resume(throwing: SDKError.serializationError(...)) with a descriptive message including the offending hex and/or its index so you preserve 1:1 mapping (this change must also be applied to the other similar sites such as the checkNullifiers flow and the other ranges noted); ensure you never return a shortened anchors array and always resume with an error if any entry fails to decode.
1005-1054:⚠️ Potential issue | 🟠 MajorAvoid
precondition-based bundle validation on public input paths.
OrchardBundleis public; relying onpreconditionand fixed-width converters here can crash the process instead of returning a recoverable SDK error before FFI marshalling.As per coding guidelines, Swift code must properly wrap FFI functions with correct memory management across Swift/Rust boundaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift` around lines 1005 - 1054, The code currently uses precondition(...) inside nonisolated func withFFIBundle(_:body:) which can crash the process for public inputs; instead, perform safe, recoverable validation at the start of withFFIBundle (check bundle.actions.count <= 2048 and any fixed-width conversion constraints) and return a throwable/Result-based failure before any FFI marshalling. Change withFFIBundle to either throw an SDK error (e.g., throw ShieldedPoolError.bundleTooLarge) or return Result<R, SDKError>, validate sizes and buffer lengths up-front, and only proceed to build FFISerializedAction/FFIOrchardBundleParams when validation passes; update callers to handle the thrown error/Result accordingly and ensure dataToBytes32/dataToBytes64 are called only after validation to avoid panics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift`:
- Around line 270-275: Before handing memory to Rust, validate that every
ShieldFundsInput.privateKey is exactly 32 bytes long and fail early if not: in
shieldFunds and any other call sites that use withFFIShieldInputs/FFIShieldInput
(e.g., the conversion helpers around withFFIShieldInputs), check each
input.privateKey.count == 32 and throw/return a descriptive Swift error if any
length is wrong; do this validation outside the unsafe pointer block so no
malformed keys are passed into withUnsafeBytes/withFFIShieldInputs, and add unit
tests exercising short/long keys to ensure the guard is effective.
- Around line 427-434: The current instantLock.withUnsafeBytes and
transaction.withUnsafeBytes blocks return a default DashSDKResult() when
baseAddress is nil, which treats empty buffers as a false success; instead,
explicitly detect empty instantLock or transaction (e.g., check
instantLock.isEmpty / transaction.isEmpty or nil baseAddress) and return a
proper failure DashSDKResult populated to indicate invalid/empty input before
attempting the FFI call; update the instantLock.withUnsafeBytes /
transaction.withUnsafeBytes handling (and the analogous block at lines 753-760)
to produce and return an error result rather than DashSDKResult() so the caller
and Rust boundary see a real failure.
- Around line 69-77: The code silently drops malformed hex entries by using
compactMap when building anchors — replace the compactMap logic in
ShieldedPoolService (where hexArray is parsed and anchors is created) with a
strict mapping that validates each hex string via hexToData and, on the first
failure, calls continuation.resume(throwing: SDKError.serializationError(...))
with a descriptive message including the offending hex and/or its index so you
preserve 1:1 mapping (this change must also be applied to the other similar
sites such as the checkNullifiers flow and the other ranges noted); ensure you
never return a shortened anchors array and always resume with an error if any
entry fails to decode.
- Around line 1005-1054: The code currently uses precondition(...) inside
nonisolated func withFFIBundle(_:body:) which can crash the process for public
inputs; instead, perform safe, recoverable validation at the start of
withFFIBundle (check bundle.actions.count <= 2048 and any fixed-width conversion
constraints) and return a throwable/Result-based failure before any FFI
marshalling. Change withFFIBundle to either throw an SDK error (e.g., throw
ShieldedPoolError.bundleTooLarge) or return Result<R, SDKError>, validate sizes
and buffer lengths up-front, and only proceed to build
FFISerializedAction/FFIOrchardBundleParams when validation passes; update
callers to handle the thrown error/Result accordingly and ensure
dataToBytes32/dataToBytes64 are called only after validation to avoid panics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88050937-f590-4c91-ac65-ae872d27830f
📒 Files selected for processing (1)
packages/swift-sdk/Sources/SwiftDashSDK/Shielded/ShieldedPoolService.swift
Swift 6 strict concurrency requires Sendable for types captured across isolation boundaries. All shielded types are pure value types with only Sendable fields (Data, UInt64, Bool, etc.) so conformance is trivial. Added to: ShieldFundsInput, NullifierStatus, EncryptedNote, DecryptedNote, SpendableNoteInfo, NullifierSyncConfig, NullifierSyncMetrics, NullifierSyncResult. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The cbindgen-generated header now exports all shielded FFI types (DashSDKOrchardBundleParams, DashSDKSerializedAction, etc.) and functions. Remove: - ShieldedFFI.swift (22 @_silgen_name declarations) - ShieldedCryptoFFI.swift (7 @_silgen_name declarations) - ShieldedPoolClientFFI.swift (10 @_silgen_name declarations) - Manual Swift struct definitions for FFI types Replace with typealiases (FFIOrchardBundleParams = DashSDKOrchardBundleParams) so existing code using FFI* names continues to compile. This fixes the Swift 6 "ambiguous use" error where @_silgen_name declarations conflicted with the same symbols imported from the DashSDKFFI C header module. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
C header maps Rust usize to uintptr_t (Swift UInt), but Data.count returns Int. Added UInt() wrapping at all 21 FFI call sites across 4 files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #3348 adds ~5800 lines of shielded pool (Orchard/ZK) support for iOS across Rust FFI and Swift layers. Five blocking issues were confirmed: the Rust struct exposes mutable fields without synchronization while Swift marks the wrapper as @unchecked Sendable; wallet restart loses all previously discovered notes; note selection ignores fees causing spurious build failures; a debug_assert fires incorrectly in the dust-burning path; and there are zero tests for the entire module. Five additional suggestions and nitpicks address silent u64→u32 truncation, hardcoded platform versioning, missing zero-amount validation in standalone FFI functions, silent dust burning, and dead code.
Reviewed commit: 7dc7969
💬 10 nitpick(s)
1 additional finding(s) omitted (not in diff).
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "0e12fea589fe339c58dc8dfe88e4018742c0637ba355fea53db0371cb2dd47e6"
)Xcode manual integration:
|
- Remove unused [self] capture in NullifierSyncService closures - Make ffiConfig and concatenated immutable before @sendable closure capture - Add _ = to defer withUnsafeMutableBytes (unused result warning) - Change var ffiAction to let (never mutated) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. Wrap all mutable state in Mutex<ShieldedPoolState> — notes, last_synced_index, commitment_tree all behind a single lock. Prevents data races from concurrent Swift FFI calls. 2. Reset last_synced_index to 0 on create — forces full resync on app restart since notes aren't persisted separately. The commitment tree IS persisted (SQLite) so tree state is preserved. 3. Fee-aware note selection — select_notes_for_amount now selects for amount + estimated_fee. Uses iterative approach: select for amount first, compute fee from selected count, re-select if the total is insufficient. Prevents spurious build failures when exact-value notes exist but can't cover the fee. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small incremental push that fixes Swift 6 concurrency warnings and Int→UInt type casts for FFI usize parameters. The changes are correct but the concurrency fix is incomplete in one function. None of the prior blocking findings (data race, note loss, fee-blind selection, broken assert) were addressed in this push — though the newest commit (e99d33edb) addresses Rust-side pool_client findings.
Reviewed commit: e99d33e
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/Shielded/NullifierSyncService.swift`:
- [SUGGESTION] lines 170-184: Incomplete concurrency fix in syncNullifiersRaw — mutable vars still captured by @Sendable closure
The sibling function `syncNullifiers` (lines 62–68) was properly fixed: `var ffiConfig` → `let ffiConfig` using an if-expression, and `let nullifierData = concatenated` creates an immutable copy for capture. But `syncNullifiersRaw` still uses `var concatenated` (line 170) and `var ffiConfig` (line 178) captured by the `@Sendable` `DispatchQueue.global().async` closure (line 184). Under Swift 6 strict concurrency, capturing mutable local variables in a `@Sendable` closure is an error. Apply the same pattern used in `syncNullifiers`.
Apply same concurrency fix as syncNullifiers: use let ffiConfig with if-expression and let nullifierData = concatenated for immutable capture in @sendable closure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix integer overflow in select_notes_for_amount: use checked_add via try_fold instead of .sum() - Zero spending key tuple in ShieldedPoolClient.init via defer - Validate privateKey is 32 bytes in shieldFunds before passing to FFI Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
Validated |
QuantumExplorer
added a commit
that referenced
this pull request
Mar 17, 2026
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Details
Rust FFI Crypto Module (
rs-sdk-ffi/src/shielded/crypto/)dash_sdk_shielded_warmup_proving_key— Halo2 ProvingKey cache with OnceLockdash_sdk_shielded_derive_address— SpendingKey → Orchard payment addressdash_sdk_shielded_build_shield_bundle— Output-only bundle (transparent → shielded)dash_sdk_shielded_build_transfer_bundle— Spend+output bundle (shielded → shielded)dash_sdk_shielded_build_unshield_bundle— Spend bundle (shielded → platform address)dash_sdk_shielded_build_withdrawal_bundle— Spend bundle (shielded → L1)dash_sdk_shielded_decrypt_notes— Trial decrypt notes with IVKSwift Transport Layer (8 files)
#[repr(C)]types@_silgen_namedeclarations for 22 FFI functions@MainActor extension SDKwithFFIBundleSwift Crypto Layer (3 files)
@_silgen_namedeclarations for 7 new crypto FFI functionsDecryptedNote,SpendableNoteInfotypes with JSON round-tripwarmupProvingKey,deriveShieldedAddress,buildShieldBundle, etc.End-to-end iOS flow
Test plan
cargo check -p rs-sdk-ffipassescargo fmtis clean./build_ios.shand verify shielded symbols exported🤖 Generated with Claude Code
Summary by CodeRabbit