fix(swift-sdk): minimally unblock iOS CI build plumbing#3143
fix(swift-sdk): minimally unblock iOS CI build plumbing#3143thepastaclaw wants to merge 4 commits intodashpay:v3.1-devfrom
Conversation
thepastaclaw
requested review from
QuantumExplorer and
shumkov
as code owners
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughUpdates adjust iOS build scripts to centralize SPV target paths and robustly handle simulator/device merges and symbol extraction, and make the rs-sdk-ffi address provider vtable fields explicitly nullable via new public alias types. Changes
Sequence Diagram(s)(omitted — changes are configuration, scripting, and small API additions without a new multi-component sequential flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/rs-sdk-ffi/build_ios.sh (1)
479-479: Pre-existing:evalon a space-delimited command string is fragile.Not introduced by this PR, but
eval $XCFRAMEWORK_CMDwill break if any path component contains spaces or shell metacharacters. An array-based approach (XCFRAMEWORK_CMD=(xcodebuild …); "${XCFRAMEWORK_CMD[@]}") would be more robust. Fine to defer since CI paths are controlled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/build_ios.sh` at line 479, The script uses eval $XCFRAMEWORK_CMD which is brittle with spaces or metacharacters; change XCFRAMEWORK_CMD to be an array (e.g., XCFRAMEWORK_CMD=(xcodebuild ...)) and invoke it with "${XCFRAMEWORK_CMD[@]}" instead of using eval so arguments are preserved; update any places that build the command string (where XCFRAMEWORK_CMD is assigned or appended) to push elements into the array rather than concatenating into a single string.packages/swift-sdk/build_ios.sh (3)
52-54:trapregistered after bothmktempcalls — minor temp-file leak on secondmktempfailureIf
mktempon line 53 fails (underset -euo pipefail), the script exits before line 54 runs, leavingNM_MAIN_OUTPUTuntracked. Negligible in practice, but easy to harden:♻️ Proposed fix: initialise variables and set trap before allocating
+NM_MAIN_OUTPUT="" +NM_SPV_OUTPUT="" +trap 'rm -f "$NM_MAIN_OUTPUT" "$NM_SPV_OUTPUT"' EXIT NM_MAIN_OUTPUT="$(mktemp)" NM_SPV_OUTPUT="$(mktemp)" -trap 'rm -f "$NM_MAIN_OUTPUT" "$NM_SPV_OUTPUT"' EXIT
rm -f ""is a no-op, so the trap fires safely even before the variables are populated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/build_ios.sh` around lines 52 - 54, Initialize NM_MAIN_OUTPUT and NM_SPV_OUTPUT to empty strings and register the trap before calling mktemp to avoid an untracked temp file if the second mktemp fails; specifically, set NM_MAIN_OUTPUT="" and NM_SPV_OUTPUT="" then add the existing trap 'rm -f "$NM_MAIN_OUTPUT" "$NM_SPV_OUTPUT"' and only afterward call NM_MAIN_OUTPUT="$(mktemp)" and NM_SPV_OUTPUT="$(mktemp)" in build_ios.sh so the trap always safely cleans up even if a mktemp call exits early.
62-62: Consider guarding onNM_SPV_OUTPUTcontent rather thanLIB_SIM_SPVexistence
[[ -f "$LIB_SIM_SPV" ]]here re-tests library existence rather than whether nm actually produced output. Using[[ -s "$NM_SPV_OUTPUT" ]]is semantically tighter — it directly asks "did nm capture anything?" and handles the case where nm ran but produced no output (the|| truepath).♻️ Proposed change (lines 62 and 71)
-elif [[ -f "$LIB_SIM_SPV" ]] && "${SEARCH_CMD[@]}" "_dash_spv_ffi_config_add_peer" "$NM_SPV_OUTPUT" >/dev/null; then +elif [[ -s "$NM_SPV_OUTPUT" ]] && "${SEARCH_CMD[@]}" "_dash_spv_ffi_config_add_peer" "$NM_SPV_OUTPUT" >/dev/null; then-elif [[ -f "$LIB_SIM_SPV" ]] && "${SEARCH_CMD[@]}" "_dash_spv_ffi_config_set_restrict_to_configured_peers" "$NM_SPV_OUTPUT" >/dev/null; then +elif [[ -s "$NM_SPV_OUTPUT" ]] && "${SEARCH_CMD[@]}" "_dash_spv_ffi_config_set_restrict_to_configured_peers" "$NM_SPV_OUTPUT" >/dev/null; thenAlso applies to: 71-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/build_ios.sh` at line 62, Replace the file-existence guard with a check that nm actually produced output: in the conditional that currently uses [[ -f "$LIB_SIM_SPV" ]] alongside SEARCH_CMD searching for "_dash_spv_ffi_config_add_peer" in $NM_SPV_OUTPUT, change the test to [[ -s "$NM_SPV_OUTPUT" ]] so you assert that nm captured non-empty output before running SEARCH_CMD; apply the same change to the analogous check later (the other occurrence around the SEARCH_CMD usage) so both conditions guard on NM_SPV_OUTPUT content rather than LIB_SIM_SPV presence.
60-76: Duplicate symbol-check blocks could be extracted into a helper functionLines 60–67 and 69–76 are structurally identical. A small helper keeps future symbol additions DRY.
♻️ Proposed refactor
+check_symbol() { + local sym="$1" + if "${SEARCH_CMD[@]}" "$sym" "$NM_MAIN_OUTPUT" >/dev/null; then + return 0 + elif [[ -s "$NM_SPV_OUTPUT" ]] && "${SEARCH_CMD[@]}" "$sym" "$NM_SPV_OUTPUT" >/dev/null; then + return 0 + else + echo "❌ Missing symbol: ${sym#_} (in both main and spv libs)" + CHECK_OK=0 + fi +} + +check_symbol "_dash_spv_ffi_config_add_peer" +check_symbol "_dash_spv_ffi_config_set_restrict_to_configured_peers" -if "${SEARCH_CMD[@]}" "_dash_spv_ffi_config_add_peer" "$NM_MAIN_OUTPUT" >/dev/null; then - : -elif [[ -f "$LIB_SIM_SPV" ]] && "${SEARCH_CMD[@]}" "_dash_spv_ffi_config_add_peer" "$NM_SPV_OUTPUT" >/dev/null; then - : -else - echo "❌ Missing symbol: dash_spv_ffi_config_add_peer (in both main and spv libs)" - CHECK_OK=0 -fi - -if "${SEARCH_CMD[@]}" "_dash_spv_ffi_config_set_restrict_to_configured_peers" "$NM_MAIN_OUTPUT" >/dev/null; then - : -elif [[ -f "$LIB_SIM_SPV" ]] && "${SEARCH_CMD[@]}" "_dash_spv_ffi_config_set_restrict_to_configured_peers" "$NM_SPV_OUTPUT" >/dev/null; then - : -else - echo "❌ Missing symbol: dash_spv_ffi_config_set_restrict_to_configured_peers (in both main and spv libs)" - CHECK_OK=0 -fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/build_ios.sh` around lines 60 - 76, Extract the duplicated symbol-check logic into a helper function (e.g., check_symbol) that takes a symbol name, uses SEARCH_CMD with NM_MAIN_OUTPUT and, if LIB_SIM_SPV exists, NM_SPV_OUTPUT, and on failure echoes the specific missing-symbol message and sets CHECK_OK=0; then replace both blocks that check "_dash_spv_ffi_config_add_peer" and "_dash_spv_ffi_config_set_restrict_to_configured_peers" with calls to this helper. Ensure the helper references SEARCH_CMD, NM_MAIN_OUTPUT, LIB_SIM_SPV, NM_SPV_OUTPUT and updates CHECK_OK exactly as the duplicated blocks do.
🤖 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/build_ios.sh`:
- Around line 452-466: Inside the BUILD_ARCH="universal" branch where
ARM_SIM_SPV_LIB and X86_SIM_SPV_LIB are checked, add a diagnostic log when
exactly one of those files exists (and thus SIM_SPV_LIB stays unset) so the
merge is not silently skipped; detect presence of ARM_SIM_SPV_LIB and
X86_SIM_SPV_LIB, and if only one is found emit a warning message (including
which path is missing and the existing path) to stderr (e.g., via echo ... >&2)
so maintainers know the SPV sim slice wasn't created even though BUILD_ARCH is
universal.
---
Nitpick comments:
In `@packages/rs-sdk-ffi/build_ios.sh`:
- Line 479: The script uses eval $XCFRAMEWORK_CMD which is brittle with spaces
or metacharacters; change XCFRAMEWORK_CMD to be an array (e.g.,
XCFRAMEWORK_CMD=(xcodebuild ...)) and invoke it with "${XCFRAMEWORK_CMD[@]}"
instead of using eval so arguments are preserved; update any places that build
the command string (where XCFRAMEWORK_CMD is assigned or appended) to push
elements into the array rather than concatenating into a single string.
In `@packages/swift-sdk/build_ios.sh`:
- Around line 52-54: Initialize NM_MAIN_OUTPUT and NM_SPV_OUTPUT to empty
strings and register the trap before calling mktemp to avoid an untracked temp
file if the second mktemp fails; specifically, set NM_MAIN_OUTPUT="" and
NM_SPV_OUTPUT="" then add the existing trap 'rm -f "$NM_MAIN_OUTPUT"
"$NM_SPV_OUTPUT"' and only afterward call NM_MAIN_OUTPUT="$(mktemp)" and
NM_SPV_OUTPUT="$(mktemp)" in build_ios.sh so the trap always safely cleans up
even if a mktemp call exits early.
- Line 62: Replace the file-existence guard with a check that nm actually
produced output: in the conditional that currently uses [[ -f "$LIB_SIM_SPV" ]]
alongside SEARCH_CMD searching for "_dash_spv_ffi_config_add_peer" in
$NM_SPV_OUTPUT, change the test to [[ -s "$NM_SPV_OUTPUT" ]] so you assert that
nm captured non-empty output before running SEARCH_CMD; apply the same change to
the analogous check later (the other occurrence around the SEARCH_CMD usage) so
both conditions guard on NM_SPV_OUTPUT content rather than LIB_SIM_SPV presence.
- Around line 60-76: Extract the duplicated symbol-check logic into a helper
function (e.g., check_symbol) that takes a symbol name, uses SEARCH_CMD with
NM_MAIN_OUTPUT and, if LIB_SIM_SPV exists, NM_SPV_OUTPUT, and on failure echoes
the specific missing-symbol message and sets CHECK_OK=0; then replace both
blocks that check "_dash_spv_ffi_config_add_peer" and
"_dash_spv_ffi_config_set_restrict_to_configured_peers" with calls to this
helper. Ensure the helper references SEARCH_CMD, NM_MAIN_OUTPUT, LIB_SIM_SPV,
NM_SPV_OUTPUT and updates CHECK_OK exactly as the duplicated blocks do.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/rs-sdk-ffi/src/address_sync/provider.rs (1)
157-166:has_pendingfallback allocates a fullVecjust to test emptiness.The
Nonefallback callsself.pending_addresses(), which copies every address key into heap-allocatedVec<u8>before calling.is_empty(). Checking only thecountfield of the rawDashSDKPendingAddressListavoids all allocation.♻️ Allocation-free fallback
fn has_pending(&self) -> bool { unsafe { let vtable = &*self.ffi.vtable; if let Some(has_pending) = vtable.has_pending { has_pending(self.ffi.context) } else { - // Default implementation - !self.pending_addresses().is_empty() + // Default: ask the C side without materialising keys + let list = (vtable.pending_addresses)(self.ffi.context); + !list.addresses.is_null() && list.count > 0 } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/address_sync/provider.rs` around lines 157 - 166, The fallback in has_pending currently calls pending_addresses() which allocates a Vec, so change the fallback to inspect the raw DashSDKPendingAddressList count instead of building a Vec: add/use a small unsafe helper that invokes the existing FFI function that returns a DashSDKPendingAddressList (or otherwise obtains the raw pointer/struct), read its count field and return count > 0, and ensure any FFI-owned list is properly freed if required; update has_pending (and reference vtable.has_pending and DashSDKPendingAddressList) to use this allocation-free count check instead of pending_addresses().
🤖 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/address_sync/provider.rs`:
- Around line 76-78: AddressProviderFFI never invokes its optional destroy
callback on drop, leaving FFI-side resources uncleaned; add a Drop impl for
AddressProviderFFI that, in drop, unsafely dereferences self.vtable, checks
vtable.destroy (the NullableDestroyProviderFn) for Some, and if present calls it
with self.context so the FFI-provided destructor runs during Rust cleanup.
- Around line 20-21: CallbackAddressProvider currently never invokes the destroy
callback and leaks resources; implement Drop for CallbackAddressProvider to
fetch the vtable from self.ffi, check vtable.destroy and call it with
self.ffi.context (unsafe block) to ensure C-side resources are freed when the
struct is dropped. Also change the has_pending fallback to avoid allocating a
full Vec by checking pending_addresses() for emptiness without cloning all
address payloads (use pending_addresses().is_empty() if available) or add an
is_empty() method to the AddressProvider trait and call that from has_pending to
avoid the Vec allocation; update references to has_pending, pending_addresses,
AddressProvider, CallbackAddressProvider, and the ffi.vtable.destroy field
accordingly.
---
Nitpick comments:
In `@packages/rs-sdk-ffi/src/address_sync/provider.rs`:
- Around line 157-166: The fallback in has_pending currently calls
pending_addresses() which allocates a Vec, so change the fallback to inspect the
raw DashSDKPendingAddressList count instead of building a Vec: add/use a small
unsafe helper that invokes the existing FFI function that returns a
DashSDKPendingAddressList (or otherwise obtains the raw pointer/struct), read
its count field and return count > 0, and ensure any FFI-owned list is properly
freed if required; update has_pending (and reference vtable.has_pending and
DashSDKPendingAddressList) to use this allocation-free count check instead of
pending_addresses().
| /// Nullable function pointer for checking if there are pending addresses | ||
| pub type NullableHasPendingFn = Option<HasPendingFn>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/rs-sdk-ffi/src/address_sync/provider.rs | head -100Repository: dashpay/platform
Length of output: 4580
🏁 Script executed:
cat -n packages/rs-sdk-ffi/src/address_sync/provider.rs | sed -n '150,170p'Repository: dashpay/platform
Length of output: 872
🏁 Script executed:
rg -A 5 "impl Drop for" packages/rs-sdk-ffi/src/address_sync/Repository: dashpay/platform
Length of output: 42
🌐 Web query:
cbindgen type alias Option function pointer nullable typedef generation
💡 Result:
-
In Rust, function pointer types (
extern "C" fn(...) -> ...) are non-nullable. To accept/hold a nullable C callback, you must useOption<extern "C" fn(...) -> ...>, which uses the nullable-pointer optimization and is ABI-compatible with a single pointer (i.e., CNULLmaps toNone). [1], [2] -
In cbindgen,
typealiases andfn()function-pointer types are supported, but the docs only explicitly call outOption<&T>/Option<&mut T>(notOption<fn(...)>), so atype NullableCb = Option<extern "C" fn(...)>alias may not generate the typedef you want depending on your setup/version. [3]
Practical pattern that works well with cbindgen:
pub type Callback = unsafe extern "C" fn(arg1: i32) -> i32;
// Use Option<Callback> at the FFI boundary where null is allowed:
pub type NullableCallback = Option<Callback>; // (may or may not be emitted as a typedef)
#[repr(C)]
pub struct Api {
pub cb: NullableCallback, // nullable in C via NULL
}- Even if cbindgen doesn’t emit a separate typedef for
NullableCallback, it can still emitCallbackas a normal C function-pointer typedef (e.g.typedef int32_t (*Callback)(int32_t);), and the C side can passNULL; Rust receives that asNoneviaOption<Callback>. This matches Rust’s documented FFI model for nullable function pointers. [1], [2], [3]
Sources:
[1] Rust core docs: nullable function pointers via Option<fn()> (nullable-pointer optimization)
[2] Rust FFI docs: nullable pointer optimization, Option<extern "C" fn(...)> for nullable callbacks
[3] cbindgen docs: supported items include type and fn(); Option support is explicitly listed for references, not function pointers
Add Drop impl to call destroy callback and avoid resource leak.
The destroy field at line 77 is never invoked. If the C callback allocates resources, they will leak when CallbackAddressProvider is dropped. Implement Drop for CallbackAddressProvider to call the destroy callback if present:
impl<'a> Drop for CallbackAddressProvider<'a> {
fn drop(&mut self) {
unsafe {
let vtable = &*self.ffi.vtable;
if let Some(destroy) = vtable.destroy {
destroy(self.ffi.context);
}
}
}
}Optimize has_pending fallback to avoid allocating a full Vec.
Lines 163-164 allocate a complete Vec<(u32, Vec<u8>)> just to call is_empty() when the has_pending callback is absent. Instead, check if pending_addresses() is empty without cloning all address data. If the underlying provider trait requires consuming the list, consider adding a non-allocating is_empty() check to the AddressProvider trait.
cbindgen nullable function pointer aliases:
The NullableXFn = Option<XFn> pattern at lines 20-21, 27-28, and 50-51 is idiomatic for FFI. Rust's Option<extern "C" fn(...)> is guaranteed null-pointer optimized and ABI-compatible. However, cbindgen's typedef generation for such Option<fn> aliases is not explicitly documented; verify your cbindgen configuration emits the expected typedef declarations if distinct C identifiers are required in your API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-sdk-ffi/src/address_sync/provider.rs` around lines 20 - 21,
CallbackAddressProvider currently never invokes the destroy callback and leaks
resources; implement Drop for CallbackAddressProvider to fetch the vtable from
self.ffi, check vtable.destroy and call it with self.ffi.context (unsafe block)
to ensure C-side resources are freed when the struct is dropped. Also change the
has_pending fallback to avoid allocating a full Vec by checking
pending_addresses() for emptiness without cloning all address payloads (use
pending_addresses().is_empty() if available) or add an is_empty() method to the
AddressProvider trait and call that from has_pending to avoid the Vec
allocation; update references to has_pending, pending_addresses,
AddressProvider, CallbackAddressProvider, and the ffi.vtable.destroy field
accordingly.
| /// Optional destructor for cleanup | ||
| pub destroy: Option<DestroyProviderFn>, | ||
| pub destroy: NullableDestroyProviderFn, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Drop implementations for AddressProviderFFI and CallbackAddressProvider
rg -n "impl Drop for AddressProviderFFI|impl Drop for CallbackAddressProvider" --type rust -C 3Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Check the file under review to understand the structure
cat -n packages/rs-sdk-ffi/src/address_sync/provider.rsRepository: dashpay/platform
Length of output: 10790
🏁 Script executed:
# Search more broadly for Drop implementations in the FFI codebase
rg -n "impl Drop" packages/rs-sdk-ffi/ --type rustRepository: dashpay/platform
Length of output: 42
Implement Drop for AddressProviderFFI to ensure the destroy callback is invoked during cleanup.
Currently, AddressProviderFFI has no Drop implementation, so the optional destroy callback is never called. This leaves resource cleanup to the FFI consumer, which is error-prone. Add a Drop impl that safely calls destroy if provided:
impl Drop for AddressProviderFFI {
fn drop(&mut self) {
unsafe {
let vtable = &*self.vtable;
if let Some(destroy) = vtable.destroy {
destroy(self.context);
}
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-sdk-ffi/src/address_sync/provider.rs` around lines 76 - 78,
AddressProviderFFI never invokes its optional destroy callback on drop, leaving
FFI-side resources uncleaned; add a Drop impl for AddressProviderFFI that, in
drop, unsafely dereferences self.vtable, checks vtable.destroy (the
NullableDestroyProviderFn) for Some, and if present calls it with self.context
so the FFI-provided destructor runs during Rust cleanup.
|
CI triage note for Build Swift SDK and Example (no warnings) failure:\n\n- Failing run: https://github.com/dashpay/platform/actions/runs/22265453606/job/64410693152\n- The current error ( |
|
Addressed CodeRabbit feedback:
If you still want this fallback optimized, we should do it as a separate API-shaping PR (new non-allocating callback/trait method), rather than inside this iOS CI plumbing PR. |
|
Triaged the Swift CI failure for this branch. This is not flaky and appears pre-existing/unrelated to this PR: the Swift package fails to compile because |
2 participants
Summary
Minimal CI-unblock PR for systemic iOS build failures.
This intentionally includes only build plumbing changes (no Swift API/behavior changes).
What changed
packages/rs-sdk-ffi/build_ios.shSPV_TARGET_DIR(workspace root target dir:../rust-dashcore/target)BUILD_ARCH=x86to build/usex86_64-apple-iosSPV sim libBUILD_ARCH=universalto lipo SPV sim arm64 + x86_64 before simulator mergelibrs_sdk_ffi.a(temp +mv), avoiding combined-name driftpackages/swift-sdk/build_ios.shnm | greppipes with temp-file symbol checks to avoid SIGPIPE/pipefail false negativesExplicitly out of scope
packages/swift-sdk/Sources/**Validation
bash -n packages/rs-sdk-ffi/build_ios.shbash -n packages/swift-sdk/build_ios.shWhy this shape
This is a narrow CI-rescue patch so reviewers can approve quickly; follow-up refactors can be reviewed separately.
Summary by CodeRabbit