fix: use header file to define platform-wallet-ffi public ABI and force swift-sdk to use it#3553
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (49)
📝 WalkthroughWalkthroughThis PR transitions the Rust platform wallet FFI from manually maintained C headers to automatically generated ones via cbindgen. It adds build-system infrastructure, removes hand-written headers, simplifies configuration, and updates all Swift FFI consumers to use result code constants and unsigned integer types consistently. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ZocoLini
force-pushed
the
fix/ffi-config
branch
5 times, most recently
from
103b4ea to
908e4c1
Compare
Collaborator
|
🕓 Ready for review (commit 6b28e6b) |
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: "e2f647b212866f4ba9c58c7c1223b6de0cfda1c67065c732087ddd1928f48fd6"
)Xcode manual integration:
|
![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: 2
🧹 Nitpick comments (6)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swift (1)
61-75: Consider scoping/renaminghashDataandhashHex.These are declared at module scope without an access modifier, so they are
internaland visible across the entireSwiftDashSDKmodule under very generic names.hashDatain particular is the kind of name that invites accidental shadowing or collisions when more hashing helpers get added later. Consider scoping them to a namespace (e.g., aFFITuplecaselessenum) or extendingData/Stringso the call site readsData(ffiTuple32:)/String(hexFFITuple32:).♻️ Example: namespace under a caseless enum
-/// Convert a 32-byte FFI tuple into `Data` for SwiftData persistence. -@inline(__always) -func hashData(_ hash: FFIByteTuple32) -> Data { - Swift.withUnsafeBytes(of: hash) { Data($0) } -} - -/// Hex-encode a 32-byte FFI tuple. -@inline(__always) -func hashHex(_ hash: FFIByteTuple32) -> String { - Swift.withUnsafeBytes(of: hash) { buf in - buf.map { String(format: "%02x", $0) }.joined() - } -} +enum FFITuple { + /// Convert a 32-byte FFI tuple into `Data` for SwiftData persistence. + `@inline`(__always) + static func data(_ hash: FFIByteTuple32) -> Data { + Swift.withUnsafeBytes(of: hash) { Data($0) } + } + + /// Hex-encode a 32-byte FFI tuple. + `@inline`(__always) + static func hex(_ hash: FFIByteTuple32) -> String { + Swift.withUnsafeBytes(of: hash) { buf in + buf.map { String(format: "%02x", $0) }.joined() + } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swift` around lines 61 - 75, The helpers hashData(_:) and hashHex(_:) are module-scoped with generic names that risk collisions; refactor by moving them into a narrow namespace such as a caseless enum (e.g., FFITuple) or by adding typed initializers/extensions (e.g., Data(ffiTuple32:) and String(hexFFITuple32:)) so callers use FFITuple.hashData(_:), FFITuple.hashHex(_:), or the new initializers; update uses of hashData and hashHex to the chosen scoped names and keep the implementation using Swift.withUnsafeBytes(of: hash) with FFIByteTuple32 unchanged.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift (1)
158-164: Pointer rebinding is correct — minor note on the precondition cast.
UnsafeRawPointer(array.items!) → assumingMemoryBound(to: UInt8.self)is the right way to flatten the imported(UInt8 × 32)tuple-pointer for row indexing.One small note:
Int(array.count)will trap ifarray.count(ausize/UInt) ever exceedsInt.max. In practice identity arrays are tiny so this is academic, but if you want to be paranoid, compare in the unsigned domain instead:♻️ Optional unsigned-domain bounds check
- precondition(index >= 0 && index < Int(array.count), "index out of range") - let raw = UnsafeRawPointer(array.items!) + precondition(index >= 0 && UInt(index) < array.count, "index out of range") + let raw = UnsafeRawPointer(array.items!)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift` around lines 158 - 164, In identifierFromFFIArray, avoid casting array.count to Int for the bounds check; instead keep the index non-negative and compare in the unsigned domain by converting the non-negative index to UInt and comparing that against array.count (i.e., replace the precondition that uses Int(array.count) with one that checks index >= 0 and UInt(index) < array.count), leaving the UnsafeRawPointer/assumingMemoryBound logic unchanged.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift (1)
1693-1693: Minor inconsistency: explicit initializer vs. default initializer pattern used elsewhere.The PR migrated FFI struct initializations to the default initializer form (e.g., line 1512 now uses
ContactRequestHandleArray()). This call site still uses the explicitContactRequestHandleArray(handles: nil, count: 0)form. Functionally equivalent, but worth aligning for consistency.♻️ Proposed alignment
- var array = ContactRequestHandleArray(handles: nil, count: 0) + var array = ContactRequestHandleArray()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift` at line 1693, Replace the explicit FFI struct initialization var array = ContactRequestHandleArray(handles: nil, count: 0) with the default initializer form used elsewhere (ContactRequestHandleArray()) in ManagedPlatformWallet.swift; update the declaration of array to use ContactRequestHandleArray() so it matches the other initializations (e.g., the one near line 1512) for consistency.packages/rs-platform-wallet-ffi/Cargo.toml (1)
50-52: Consider bumpingcbindgento0.29for recent improvements.
cbindgen = "0.27"works without issues, but the latest version is0.29.2with no breaking changes between versions. Since this is a build-dependency only, upgrading to0.29would provide bug fixes and feature additions from0.28and0.29with no compatibility risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/Cargo.toml` around lines 50 - 52, Update the build-dependency entry for cbindgen in Cargo.toml from "0.27" to a 0.29.x release (e.g., "0.29" or "0.29.2") to pick up recent fixes and features; locate the [build-dependencies] section and change the cbindgen version string accordingly (the unique symbol to edit is the cbindgen dependency entry).packages/rs-platform-wallet-ffi/build.rs (2)
26-31: Builder error message could mislead.
.expect("Unable to generate bindings")swallows the underlyingcbindgen::Errordetail. When generation fails (e.g. a parse error in a transitively-included crate), the build log only prints "Unable to generate bindings", which makes diagnosis harder. Consider letting the error propagate via?from aResult-returning helper or formatting the error explicitly.♻️ Surface cbindgen errors
- cbindgen::Builder::new() - .with_crate(&crate_dir) - .with_config(config) - .generate() - .expect("Unable to generate bindings") - .write_to_file(&output_path); + let bindings = cbindgen::Builder::new() + .with_crate(&crate_dir) + .with_config(config) + .generate() + .unwrap_or_else(|e| panic!("cbindgen failed for {}: {e}", crate_name)); + bindings.write_to_file(&output_path);As per coding guidelines, "Follow rustfmt defaults and keep code clippy-clean for Rust modules" — clippy may also suggest
unwrap_or_elseoverexpectwith non-static context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/build.rs` around lines 26 - 31, The cbindgen generation call currently uses .generate().expect("Unable to generate bindings") which hides the underlying cbindgen::Error; change the error handling in the Builder chain (the cbindgen::Builder::new()... .generate() call) to propagate or surface the original error (e.g. return a Result and use ? on generate(), or replace expect with unwrap_or_else / map_err that formats and includes the cbindgen::Error) so the build log prints the actual error details from generate() instead of the static "Unable to generate bindings" message.
12-15: Fragile reliance on Cargo's internalOUT_DIRlayout.
Path::ancestors().nth(3)assumes the exacttarget/<profile>/build/<crate>-<hash>/outshape. Cargo doesn't formally guarantee this layout, and it differs subtly under--target <triple>builds (where it becomestarget/<triple>/<profile>/...— still 3 levels up, so this happens to work today). Consider adding a defensive check thattarget_dirends with the expected profile component before writing into it, or surface theOUT_DIRvalue in the panic message so build failures are diagnosable.♻️ Better diagnostics on failure
- let target_dir = Path::new(&out_dir) - .ancestors() - .nth(3) // This line moves up to the target/<PROFILE> directory - .expect("Failed to find target dir"); + let target_dir = Path::new(&out_dir) + .ancestors() + .nth(3) // target/<PROFILE> (or target/<triple>/<PROFILE> for cross-target) + .unwrap_or_else(|| panic!("Could not derive target dir from OUT_DIR={}", out_dir));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/build.rs` around lines 12 - 15, The current computation of target_dir using Path::ancestors().nth(3) is fragile; update the logic around out_dir and target_dir (the Path::ancestors().nth(3) call and the expect message) to verify the discovered directory actually ends with the expected profile component (e.g. "debug" or "release") before proceeding, and improve the panic/expect message to include the original OUT_DIR value and the computed path for easier diagnostics; specifically, in the build.rs code that defines target_dir and uses out_dir, add a defensive check on target_dir.file_name() (or equivalent) against the PROFILE env var and change the expect message to print both OUT_DIR and the computed target_dir when the check fails.
🤖 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-platform-wallet-ffi/src/persistence.rs`:
- Around line 163-179: The docstrings for the FFI callback fields
(on_persist_account_addresses_fn, on_persist_account_fn,
on_persist_wallet_metadata_fn) currently claim that non-zero return codes are
merely advisory and Rust will "log and continue", but the implementation (see
store_account_addresses and similar code paths) actually treats a non-zero rc as
an Err and aborts the persistence round; update the docstrings to reflect the
real behavior: state that callers must return 0 on success, any non-zero return
will be propagated as a PersistenceError and will abort the operation (contrast
to on_changeset_begin_fn / on_persist_address_balances_fn which are
log-and-continue), and mention that the C/Swift header generated by cbindgen
will expose this contract so clients must handle errors accordingly.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swift`:
- Around line 130-132: The current combined guard in ManagedAssetLockManager
(the one checking result, txBytesPtr and txLen) can produce a misleading
PlatformWalletError when result == PLATFORM_WALLET_FFI_RESULT_SUCCESS but the
returned buffer is nil/empty; split the check: first validate the FFI result
(throw PlatformWalletError(result:error:) only when the result indicates an
actual FFI error), then separately validate txBytesPtr and txLen and throw a
distinct, descriptive PlatformWalletError (e.g., "empty or nil transaction
buffer returned from FFI") when the buffer is missing or zero-length; apply the
same split-guard pattern to the createFundedProof and resume call sites so
SUCCESS-with-empty-buffer surfaces a clear error instead of falling through to
the generic result-based error.
---
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/build.rs`:
- Around line 26-31: The cbindgen generation call currently uses
.generate().expect("Unable to generate bindings") which hides the underlying
cbindgen::Error; change the error handling in the Builder chain (the
cbindgen::Builder::new()... .generate() call) to propagate or surface the
original error (e.g. return a Result and use ? on generate(), or replace expect
with unwrap_or_else / map_err that formats and includes the cbindgen::Error) so
the build log prints the actual error details from generate() instead of the
static "Unable to generate bindings" message.
- Around line 12-15: The current computation of target_dir using
Path::ancestors().nth(3) is fragile; update the logic around out_dir and
target_dir (the Path::ancestors().nth(3) call and the expect message) to verify
the discovered directory actually ends with the expected profile component (e.g.
"debug" or "release") before proceeding, and improve the panic/expect message to
include the original OUT_DIR value and the computed path for easier diagnostics;
specifically, in the build.rs code that defines target_dir and uses out_dir, add
a defensive check on target_dir.file_name() (or equivalent) against the PROFILE
env var and change the expect message to print both OUT_DIR and the computed
target_dir when the check fails.
In `@packages/rs-platform-wallet-ffi/Cargo.toml`:
- Around line 50-52: Update the build-dependency entry for cbindgen in
Cargo.toml from "0.27" to a 0.29.x release (e.g., "0.29" or "0.29.2") to pick up
recent fixes and features; locate the [build-dependencies] section and change
the cbindgen version string accordingly (the unique symbol to edit is the
cbindgen dependency entry).
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Line 1693: Replace the explicit FFI struct initialization var array =
ContactRequestHandleArray(handles: nil, count: 0) with the default initializer
form used elsewhere (ContactRequestHandleArray()) in
ManagedPlatformWallet.swift; update the declaration of array to use
ContactRequestHandleArray() so it matches the other initializations (e.g., the
one near line 1512) for consistency.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swift`:
- Around line 61-75: The helpers hashData(_:) and hashHex(_:) are module-scoped
with generic names that risk collisions; refactor by moving them into a narrow
namespace such as a caseless enum (e.g., FFITuple) or by adding typed
initializers/extensions (e.g., Data(ffiTuple32:) and String(hexFFITuple32:)) so
callers use FFITuple.hashData(_:), FFITuple.hashHex(_:), or the new
initializers; update uses of hashData and hashHex to the chosen scoped names and
keep the implementation using Swift.withUnsafeBytes(of: hash) with
FFIByteTuple32 unchanged.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift`:
- Around line 158-164: In identifierFromFFIArray, avoid casting array.count to
Int for the bounds check; instead keep the index non-negative and compare in the
unsigned domain by converting the non-negative index to UInt and comparing that
against array.count (i.e., replace the precondition that uses Int(array.count)
with one that checks index >= 0 and UInt(index) < array.count), leaving the
UnsafeRawPointer/assumingMemoryBound logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25ec70e4-05ff-4645-be87-0092a21988f1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
packages/rs-platform-wallet-ffi/Cargo.tomlpackages/rs-platform-wallet-ffi/build.rspackages/rs-platform-wallet-ffi/cbindgen.tomlpackages/rs-platform-wallet-ffi/platform_wallet_ffi.hpackages/rs-platform-wallet-ffi/src/core_address_types.rspackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/tokens/burn.rspackages/rs-platform-wallet-ffi/src/tokens/claim.rspackages/rs-platform-wallet-ffi/src/tokens/destroy_frozen_funds.rspackages/rs-platform-wallet-ffi/src/tokens/freeze.rspackages/rs-platform-wallet-ffi/src/tokens/group_queries.rspackages/rs-platform-wallet-ffi/src/tokens/mint.rspackages/rs-platform-wallet-ffi/src/tokens/pause.rspackages/rs-platform-wallet-ffi/src/tokens/resume.rspackages/rs-platform-wallet-ffi/src/tokens/set_price.rspackages/rs-platform-wallet-ffi/src/tokens/transfer.rspackages/rs-platform-wallet-ffi/src/tokens/unfreeze.rspackages/rs-platform-wallet-ffi/src/tokens/update_config.rspackages/rs-platform-wallet-ffi/src/wallet_restore_types.rspackages/rs-platform-wallet/src/wallet/tokens/group_queries.rspackages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swiftpackages/swift-sdk/Sources/SwiftDashSDK/FFI/MnemonicResolverAndPersister.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/AssetLockFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ContactRequest.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ContestVoteState.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreWalletFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWalletChangeSetFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/EstablishedContact.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/IdentityManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/IdentityRegistrationFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/Tokens/TokenActions.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/Tokens/TokenGroupActionQueries.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (7)
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWalletChangeSetFFI.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/IdentityRegistrationFFI.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerFFI.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/AssetLockFFI.swift
- packages/rs-platform-wallet-ffi/platform_wallet_ffi.h
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreWalletFFI.swift
- packages/rs-platform-wallet-ffi/cbindgen.toml
…ift-sdk to use it
ZocoLini
force-pushed
the
fix/ffi-config
branch
from
908e4c1 to
6b28e6b
Compare
QuantumExplorer
approved these changes
Apr 28, 2026
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
3 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.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Refactor