chore(swift-sdk): swift and unified sdk generation rewrite#3401
Merged
QuantumExplorer merged 1 commit intov3.1-devfrom Mar 31, 2026
Merged
chore(swift-sdk): swift and unified sdk generation rewrite#3401QuantumExplorer merged 1 commit intov3.1-devfrom
QuantumExplorer merged 1 commit intov3.1-devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new unified Rust FFI crate ( Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI
participant BuildScript as packages/swift-sdk/build_ios.sh
participant Cargo as cargo (rs-unified-sdk-ffi)
participant Xcode as xcodebuild
participant XCFramework as DashSDKFFI.xcframework
CI->>BuildScript: run --target all --profile release
BuildScript->>Cargo: cargo build -p rs-unified-sdk-ffi (per-target)
Cargo-->>BuildScript: per-target static libs + include/<crate>/.../*.h
BuildScript->>Xcode: xcodebuild -create-xcframework (combine libs)
Xcode-->>BuildScript: DashSDKFFI.xcframework
BuildScript->>XCFramework: inject module.modulemap & umbrella header
BuildScript-->>CI: produce DashSDKFFI.xcframework artifact
sequenceDiagram
participant Consumer as Downstream
participant Unified as rs-unified-sdk-ffi
participant DashSPV as dash-spv-ffi
participant KeyWallet as key-wallet-ffi
participant RSFFI as rs-sdk-ffi
Consumer->>Unified: link / call exposed API
Unified->>DashSPV: re-exported symbols
Unified->>KeyWallet: re-exported symbols
Unified->>RSFFI: re-exported symbols
DashSPV-->>Consumer: FFI surface (via Unified)
KeyWallet-->>Consumer: FFI surface (via Unified)
RSFFI-->>Consumer: FFI surface (via Unified)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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/unified-sdk-size
branch
from
822bd7d to
873657e
Compare
ZocoLini
changed the title
ZocoLini
changed the title
ZocoLini
changed the title
![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: 3
🤖 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/build_ios.sh`:
- Around line 239-243: The script currently hardcodes EXCLUDED_ARCHS="x86_64"
which can remove the only simulator arch on Intel hosts; update the logic around
detect_sim_target() so EXCLUDED_ARCHS is set conditionally: call
detect_sim_target() (the function that determines the simulator arch) and only
set or append EXCLUDED_ARCHS when its value is different from the detected
simulator arch, otherwise leave EXCLUDED_ARCHS empty (or unset) so
SWIFT_DESTINATION ("generic/platform=iOS Simulator") and SWIFT_SCHEME/
SWIFT_PROJECT continue to work for Intel macs; ensure the conditional uses the
same symbol names detect_sim_target, EXCLUDED_ARCHS, SWIFT_DESTINATION in the
change.
- Around line 227-232: The xcodebuild invocation uses ${VAR:+...} expansions
that preserve inner quotes so xcodebuild gets quoted paths; fix by building a
shell array of arguments and conditionally appending pairs when variables are
set (check IOS_LIB, MAC_LIB, INTEL_MAC_LIB, SIM_LIB and append -library "$VAR"
-headers "$VAR_HEADERS" accordingly) and then call xcodebuild
-create-xcframework with the argument array and -output "$XCFRAMEWORK" (use
"${args[@]}" to expand).
- Around line 14-18: The script computes SCRIPT_DIR and ROOT_DIR but subsequent
cargo invocations run from the current working directory, causing workspace
resolution to fail when the script is invoked outside the repo; update the build
commands to run cargo from the workspace root by changing execution to use
ROOT_DIR (e.g., run cargo with cwd="$ROOT_DIR" or prefix commands with (cd
"$ROOT_DIR" && cargo build -p ...)) so all cargo build -p invocations resolve
relative to ROOT_DIR; locate uses of cargo in this file and ensure they execute
in the ROOT_DIR context (refer to SCRIPT_DIR and ROOT_DIR variables).
🪄 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: 87520075-efc6-4b6d-b726-7ecc721f611d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.tomlpackages/rs-sdk-ffi/Cargo.tomlpackages/rs-sdk-ffi/build.rspackages/rs-sdk-ffi/build_ios.shpackages/rs-sdk-ffi/cbindgen-core.tomlpackages/rs-sdk-ffi/cbindgen-ios.tomlpackages/rs-sdk-ffi/cbindgen.tomlpackages/rs-sdk-ffi/cbindgen_minimal.tomlpackages/rs-sdk-ffi/combine_headers.shpackages/rs-sdk-ffi/src/callback_bridge.rspackages/rs-sdk-ffi/src/context_provider_stubs.rspackages/rs-sdk-ffi/src/lib.rspackages/rs-unified-sdk-ffi/Cargo.tomlpackages/rs-unified-sdk-ffi/src/lib.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyDerivation.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (8)
- packages/rs-sdk-ffi/cbindgen-ios.toml
- packages/rs-sdk-ffi/cbindgen_minimal.toml
- packages/rs-sdk-ffi/cbindgen-core.toml
- packages/rs-sdk-ffi/src/callback_bridge.rs
- packages/rs-sdk-ffi/build_ios.sh
- packages/rs-sdk-ffi/src/lib.rs
- packages/rs-sdk-ffi/src/context_provider_stubs.rs
- packages/rs-sdk-ffi/combine_headers.sh
ZocoLini
force-pushed
the
fix/unified-sdk-size
branch
from
873657e to
5faec9f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3401 +/- ##
============================================
- Coverage 79.93% 79.92% -0.01%
============================================
Files 2861 2861
Lines 280236 280236
============================================
- Hits 223995 223991 -4
- Misses 56241 56245 +4
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/swift-sdk/build_ios.sh (3)
227-232:⚠️ Potential issue | 🔴 CriticalBuild
xcodebuildarguments via an array, not${VAR:+...}.This expansion pattern can pass quoted fragments incorrectly and break
-library/-headersparsing when slices are present.Suggested fix
-xcodebuild -create-xcframework \ - ${IOS_LIB:+-library "$IOS_LIB" -headers "$IOS_HEADERS"} \ - ${MAC_LIB:+-library "$MAC_LIB" -headers "$MAC_HEADERS"} \ - ${INTEL_MAC_LIB:+-library "$INTEL_MAC_LIB" -headers "$INTEL_MAC_HEADERS"} \ - ${SIM_LIB:+-library "$SIM_LIB" -headers "$SIM_HEADERS"} \ - -output "$XCFRAMEWORK" +xcframework_args=() +[[ -n "${IOS_LIB:-}" ]] && xcframework_args+=( -library "$IOS_LIB" -headers "$IOS_HEADERS" ) +[[ -n "${MAC_LIB:-}" ]] && xcframework_args+=( -library "$MAC_LIB" -headers "$MAC_HEADERS" ) +[[ -n "${INTEL_MAC_LIB:-}" ]] && xcframework_args+=( -library "$INTEL_MAC_LIB" -headers "$INTEL_MAC_HEADERS" ) +[[ -n "${SIM_LIB:-}" ]] && xcframework_args+=( -library "$SIM_LIB" -headers "$SIM_HEADERS" ) + +xcodebuild -create-xcframework \ + "${xcframework_args[@]}" \ + -output "$XCFRAMEWORK"#!/bin/bash set -euo pipefail FILE="packages/swift-sdk/build_ios.sh" rg -n -A6 -B2 'xcodebuild -create-xcframework|\$\{IOS_LIB:\+|\$\{MAC_LIB:\+|\$\{SIM_LIB:\+' "$FILE"🤖 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 227 - 232, Replace the inline `${VAR:+...}` expansions with a shell array of arguments that you push into before calling xcodebuild: check the block that invokes xcodebuild -create-xcframework and the variables IOS_LIB, IOS_HEADERS, MAC_LIB, MAC_HEADERS, INTEL_MAC_LIB, INTEL_MAC_HEADERS, SIM_LIB, SIM_HEADERS; for each pair (e.g. IOS_LIB/IOS_HEADERS) test for non-empty and append two separate elements "-library" and the "$IOS_LIB" and "-headers" and "$IOS_HEADERS" to an args array, then call xcodebuild "${args[@]}" -output "$XCFRAMEWORK" so quoted fragments are preserved and -library/-headers parsing is correct.
239-243:⚠️ Potential issue | 🟠 MajorDo not hardcode
EXCLUDED_ARCHS="x86_64"for simulator verification.Line 242 excludes Intel simulator arch unconditionally. On Intel hosts, that removes the only simulator arch and breaks verification builds.
Suggested fix
SWIFT_PROJECT="$SCRIPT_DIR/SwiftExampleApp/SwiftExampleApp.xcodeproj" SWIFT_SCHEME="SwiftExampleApp" SWIFT_DESTINATION="generic/platform=iOS Simulator" -EXCLUDED_ARCHS="x86_64" +EXCLUDED_ARCHS="" +if [[ "$(uname -m)" == "arm64" ]]; then + EXCLUDED_ARCHS="x86_64" +fi#!/bin/bash set -euo pipefail FILE="packages/swift-sdk/build_ios.sh" rg -n -A4 -B4 'detect_sim_target|EXCLUDED_ARCHS|SWIFT_DESTINATION' "$FILE"🤖 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 239 - 243, The script currently hardcodes EXCLUDED_ARCHS="x86_64", which breaks simulator verification on Intel hosts; update the logic around EXCLUDED_ARCHS (and related SWIFT_DESTINATION handling) to compute it dynamically: detect the host architecture (e.g. uname -m) or reuse an existing detect_sim_target routine, and only set EXCLUDED_ARCHS="x86_64" when running on Apple Silicon or when building a simulator target that should exclude x86_64; otherwise leave EXCLUDED_ARCHS empty or set to an appropriate value for Intel hosts. Modify the section that defines EXCLUDED_ARCHS and any code that references it (EXCLUDED_ARCHS, SWIFT_DESTINATION, detect_sim_target) so the assignment is conditional based on the detected host/target architecture rather than hardcoded.
14-16:⚠️ Potential issue | 🟠 MajorRun Cargo builds from the workspace root.
ROOT_DIRis computed, but on Line 185/195/205/215cargo build -p ...still resolves from the caller’s current directory. Running this script outside repo root can fail workspace resolution.Suggested fix
-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -ROOT_DIR="$SCRIPT_DIR/../../" +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +ROOT_DIR="$(cd "$SCRIPT_DIR/../.." && pwd)" TARGET_DIR="$ROOT_DIR/target" +cd "$ROOT_DIR"#!/bin/bash set -euo pipefail FILE="packages/swift-sdk/build_ios.sh" echo "== Path setup and cwd usage ==" rg -n 'SCRIPT_DIR=|ROOT_DIR=|^\s*cd\s+"\$ROOT_DIR"' "$FILE" echo "== Cargo invocations ==" rg -n 'cargo build -p' "$FILE"Also applies to: 185-216
🤖 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 14 - 16, The cargo builds are being executed from the caller’s CWD rather than the computed workspace root (ROOT_DIR), which breaks workspace resolution when the script is run outside the repo; update each cargo invocation (the commands around where cargo build -p ... appear) to run from ROOT_DIR by either prefixing them with an explicit cd into ROOT_DIR (e.g., (cd "$ROOT_DIR" && cargo build -p ...)) or using cargo's --manifest-path pointing at "$ROOT_DIR/Cargo.toml"; ensure this change is applied for every cargo build invocation referenced in the script so SCRIPT_DIR/ROOT_DIR/TARGET_DIR are consistently respected.
🤖 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/build_ios.sh`:
- Around line 76-95: The --target and --profile argument handlers in
build_ios.sh access $2 without verifying it exists, causing unbound variable
failures under set -euo pipefail; add an argument-count guard before each case
so you validate a value is present (e.g., test that $# -ge 2 or that "${2-}" is
non-empty) and if missing call log_error with a descriptive message and
show_help, then return/exit appropriately; update the blocks around the --target
case (the case $2 in ... esac) and the --profile handler (where PROFILE="$2" is
set and validated) to perform this check before using $2.
---
Duplicate comments:
In `@packages/swift-sdk/build_ios.sh`:
- Around line 227-232: Replace the inline `${VAR:+...}` expansions with a shell
array of arguments that you push into before calling xcodebuild: check the block
that invokes xcodebuild -create-xcframework and the variables IOS_LIB,
IOS_HEADERS, MAC_LIB, MAC_HEADERS, INTEL_MAC_LIB, INTEL_MAC_HEADERS, SIM_LIB,
SIM_HEADERS; for each pair (e.g. IOS_LIB/IOS_HEADERS) test for non-empty and
append two separate elements "-library" and the "$IOS_LIB" and "-headers" and
"$IOS_HEADERS" to an args array, then call xcodebuild "${args[@]}" -output
"$XCFRAMEWORK" so quoted fragments are preserved and -library/-headers parsing
is correct.
- Around line 239-243: The script currently hardcodes EXCLUDED_ARCHS="x86_64",
which breaks simulator verification on Intel hosts; update the logic around
EXCLUDED_ARCHS (and related SWIFT_DESTINATION handling) to compute it
dynamically: detect the host architecture (e.g. uname -m) or reuse an existing
detect_sim_target routine, and only set EXCLUDED_ARCHS="x86_64" when running on
Apple Silicon or when building a simulator target that should exclude x86_64;
otherwise leave EXCLUDED_ARCHS empty or set to an appropriate value for Intel
hosts. Modify the section that defines EXCLUDED_ARCHS and any code that
references it (EXCLUDED_ARCHS, SWIFT_DESTINATION, detect_sim_target) so the
assignment is conditional based on the detected host/target architecture rather
than hardcoded.
- Around line 14-16: The cargo builds are being executed from the caller’s CWD
rather than the computed workspace root (ROOT_DIR), which breaks workspace
resolution when the script is run outside the repo; update each cargo invocation
(the commands around where cargo build -p ... appear) to run from ROOT_DIR by
either prefixing them with an explicit cd into ROOT_DIR (e.g., (cd "$ROOT_DIR"
&& cargo build -p ...)) or using cargo's --manifest-path pointing at
"$ROOT_DIR/Cargo.toml"; ensure this change is applied for every cargo build
invocation referenced in the script so SCRIPT_DIR/ROOT_DIR/TARGET_DIR are
consistently respected.
🪄 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: a0be97b7-e2b0-41de-ba3d-18541f121dcb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.github/workflows/swift-sdk-build.yml.github/workflows/swift-sdk-release.ymlCargo.tomlpackages/rs-sdk-ffi/Cargo.tomlpackages/rs-sdk-ffi/build.rspackages/rs-sdk-ffi/build_ios.shpackages/rs-sdk-ffi/cbindgen-core.tomlpackages/rs-sdk-ffi/cbindgen-ios.tomlpackages/rs-sdk-ffi/cbindgen.tomlpackages/rs-sdk-ffi/cbindgen_minimal.tomlpackages/rs-sdk-ffi/combine_headers.shpackages/rs-sdk-ffi/src/callback_bridge.rspackages/rs-sdk-ffi/src/context_provider_stubs.rspackages/rs-sdk-ffi/src/lib.rspackages/rs-unified-sdk-ffi/Cargo.tomlpackages/rs-unified-sdk-ffi/src/lib.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyDerivation.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (8)
- packages/rs-sdk-ffi/cbindgen-ios.toml
- packages/rs-sdk-ffi/cbindgen_minimal.toml
- packages/rs-sdk-ffi/src/callback_bridge.rs
- packages/rs-sdk-ffi/build_ios.sh
- packages/rs-sdk-ffi/src/lib.rs
- packages/rs-sdk-ffi/cbindgen-core.toml
- packages/rs-sdk-ffi/src/context_provider_stubs.rs
- packages/rs-sdk-ffi/combine_headers.sh
✅ Files skipped from review due to trivial changes (5)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
- packages/rs-unified-sdk-ffi/Cargo.toml
- packages/rs-unified-sdk-ffi/src/lib.rs
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rs-sdk-ffi/cbindgen.toml
- packages/rs-sdk-ffi/Cargo.toml
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swift
- packages/rs-sdk-ffi/build.rs
ZocoLini
force-pushed
the
fix/unified-sdk-size
branch
from
5faec9f to
3b762c7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/swift-sdk-build.yml (1)
57-59:⚠️ Potential issue | 🟡 MinorMissing macOS Rust target for
--target allbuild.The workflow adds
aarch64-apple-iosandaarch64-apple-ios-sim, but--target all(Line 113) also builds foraarch64-apple-darwin(macOS). While the ARM64 self-hosted runner may have this as the default host target, explicitly adding it ensures reliability across different runner configurations.Suggested fix
- name: Add iOS Rust targets run: | - rustup target add aarch64-apple-ios aarch64-apple-ios-sim + rustup target add aarch64-apple-ios aarch64-apple-ios-sim aarch64-apple-darwin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/swift-sdk-build.yml around lines 57 - 59, The CI step that adds iOS Rust targets (step name "Add iOS Rust targets") omits the macOS ARM target required by the --target all build; update that step to also run rustup target add aarch64-apple-darwin so the workflow explicitly installs the macOS target alongside aarch64-apple-ios and aarch64-apple-ios-sim, ensuring the --target all build (referenced on Line 113) can build for macOS reliably.
🧹 Nitpick comments (3)
Cargo.toml (1)
46-46: Consider placing each workspace member on its own line for consistency.The rest of the
memberslist has one package per line, but line 46 combines two entries. This is a minor formatting inconsistency.Suggested fix
- "packages/wasm-sdk", "packages/rs-unified-sdk-ffi", + "packages/wasm-sdk", + "packages/rs-unified-sdk-ffi",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` at line 46, The workspace members list currently has "packages/wasm-sdk", "packages/rs-unified-sdk-ffi" on the same line; update the Cargo.toml members array so each package path is on its own line (split the combined entry into separate lines) to match the existing one-per-line formatting for other entries and keep "packages/wasm-sdk" and "packages/rs-unified-sdk-ffi" as distinct lines within the members array.packages/swift-sdk/build_ios.sh (1)
164-167: macOS-specificsed -i ''syntax.Line 166 uses BSD
sed's-i ''which won't work on GNU/Linux. This is acceptable given the script targets macOS for iOS builds, but worth noting if cross-platform CI is ever considered. The TODO comment appropriately documents this as a temporary workaround pending upstream fixes.🤖 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 164 - 167, The sed invocation 'sed -i '' 's/FFIAssetLockFundingType/uint32_t/g' "$h"' is macOS/BSD-specific and will fail on GNU/Linux; wrap the HEADERS_DIR loop or replace that sed call with a portable alternative: either detect platform via uname (use sed -i '' on Darwin and sed -i on Linux) or perform the same substitution with perl -i to match the earlier perl usage. Update the code path that contains the HEADERS_DIR loop and the sed command so the substitution always runs cross-platform while preserving the existing TODO comment about this being a temporary workaround.packages/rs-sdk-ffi/build.rs (1)
9-10: Directory-levelrerun-if-changedmay miss new files.Cargo's
rerun-if-changedon a directory only triggers when the directory's mtime changes, not necessarily when files inside are added. Consider using a build-time glob or explicitly listing key source files for more reliable rebuild triggering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/build.rs` around lines 9 - 10, The current build.rs uses directory-level println!("cargo:rerun-if-changed=src/") which may not detect new files; update build.rs to enumerate source files (e.g., use glob or walkdir) and emit a separate println!("cargo:rerun-if-changed={path}") for each matched file under src (and keep the existing cbindgen.toml emit). Locate and replace the println!("cargo:rerun-if-changed=src/") usage in build.rs with a small file-walk that lists all relevant extensions (e.g., .rs, headers, generated files) and prints a per-file rerun-if-changed line so adding files triggers rebuilds.
🤖 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.rs`:
- Around line 26-31: The call chain using cbindgen::Builder (with_crate,
with_config, generate) currently ignores the return value of
cbindgen::Bindings::write_to_file(&output_path); update this to handle the
result: if write_to_file returns bool, check the boolean and call process::exit
or panic with a clear message including output_path when it is false; if your
cbindgen version returns a Result, call .expect("Failed to write bindings to
file") or propagate the error instead. Locate the call to write_to_file in the
builder chain and replace the ignored call with an explicit result check or
expect to surface I/O failures.
- Around line 12-15: The call chain that generates C headers via cbindgen
currently ignores the boolean result of write_to_file(), so write failures won't
fail the build; update the cbindgen usage in build.rs (the
cbindgen::Builder::new(), .with_crate(&crate_dir), .with_config(config),
.generate() / .write_to_file(&output_path) sequence) to check the result and
fail the build on error—e.g., call .write_to_file(&output_path) and assert or
propagate failure (use expect or a Result check) so disk/full/permission errors
cause the build to error.
---
Outside diff comments:
In @.github/workflows/swift-sdk-build.yml:
- Around line 57-59: The CI step that adds iOS Rust targets (step name "Add iOS
Rust targets") omits the macOS ARM target required by the --target all build;
update that step to also run rustup target add aarch64-apple-darwin so the
workflow explicitly installs the macOS target alongside aarch64-apple-ios and
aarch64-apple-ios-sim, ensuring the --target all build (referenced on Line 113)
can build for macOS reliably.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 46: The workspace members list currently has "packages/wasm-sdk",
"packages/rs-unified-sdk-ffi" on the same line; update the Cargo.toml members
array so each package path is on its own line (split the combined entry into
separate lines) to match the existing one-per-line formatting for other entries
and keep "packages/wasm-sdk" and "packages/rs-unified-sdk-ffi" as distinct lines
within the members array.
In `@packages/rs-sdk-ffi/build.rs`:
- Around line 9-10: The current build.rs uses directory-level
println!("cargo:rerun-if-changed=src/") which may not detect new files; update
build.rs to enumerate source files (e.g., use glob or walkdir) and emit a
separate println!("cargo:rerun-if-changed={path}") for each matched file under
src (and keep the existing cbindgen.toml emit). Locate and replace the
println!("cargo:rerun-if-changed=src/") usage in build.rs with a small file-walk
that lists all relevant extensions (e.g., .rs, headers, generated files) and
prints a per-file rerun-if-changed line so adding files triggers rebuilds.
In `@packages/swift-sdk/build_ios.sh`:
- Around line 164-167: The sed invocation 'sed -i ''
's/FFIAssetLockFundingType/uint32_t/g' "$h"' is macOS/BSD-specific and will fail
on GNU/Linux; wrap the HEADERS_DIR loop or replace that sed call with a portable
alternative: either detect platform via uname (use sed -i '' on Darwin and sed
-i on Linux) or perform the same substitution with perl -i to match the earlier
perl usage. Update the code path that contains the HEADERS_DIR loop and the sed
command so the substitution always runs cross-platform while preserving the
existing TODO comment about this being a temporary workaround.
🪄 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: dc3f75d1-e3bf-4f73-941a-13561d2f7f1b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.github/workflows/swift-sdk-build.yml.github/workflows/swift-sdk-release.ymlCargo.tomlpackages/rs-sdk-ffi/Cargo.tomlpackages/rs-sdk-ffi/build.rspackages/rs-sdk-ffi/build_ios.shpackages/rs-sdk-ffi/cbindgen-core.tomlpackages/rs-sdk-ffi/cbindgen-ios.tomlpackages/rs-sdk-ffi/cbindgen.tomlpackages/rs-sdk-ffi/cbindgen_minimal.tomlpackages/rs-sdk-ffi/combine_headers.shpackages/rs-sdk-ffi/src/callback_bridge.rspackages/rs-sdk-ffi/src/context_provider_stubs.rspackages/rs-sdk-ffi/src/lib.rspackages/rs-unified-sdk-ffi/Cargo.tomlpackages/rs-unified-sdk-ffi/src/lib.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyDerivation.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (8)
- packages/rs-sdk-ffi/cbindgen_minimal.toml
- packages/rs-sdk-ffi/combine_headers.sh
- packages/rs-sdk-ffi/cbindgen-ios.toml
- packages/rs-sdk-ffi/cbindgen-core.toml
- packages/rs-sdk-ffi/src/lib.rs
- packages/rs-sdk-ffi/build_ios.sh
- packages/rs-sdk-ffi/src/callback_bridge.rs
- packages/rs-sdk-ffi/src/context_provider_stubs.rs
✅ Files skipped from review due to trivial changes (4)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
- packages/rs-unified-sdk-ffi/src/lib.rs
- packages/rs-unified-sdk-ffi/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/swift-sdk-release.yml
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swift
- packages/rs-sdk-ffi/cbindgen.toml
- packages/rs-sdk-ffi/Cargo.toml
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyDerivation.swift
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: "44063c6e3958087fb07bd8f8b809d431bbe18559b5969458628b5aa8a03a5663"
)Xcode manual integration:
|
ZocoLini
force-pushed
the
fix/unified-sdk-size
branch
from
3b762c7 to
5e1b6e9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/swift-sdk/build_ios.sh (3)
189-191: Consider validating that headers directory exists before callinginject_modulemap.If the Rust build doesn't generate headers in the expected location (
$TARGET_DIR/$TARGET/$OUTPUT_DIR/include),inject_modulemapwill fail with unclear errors when trying to iterate over"$HEADERS_DIR"/*/*.h. Adding a quick existence check would improve debuggability.Suggested validation
IOS_LIB="$TARGET_DIR/$IOS_TARGET/$OUTPUT_DIR/librs_unified_sdk_ffi.a" IOS_HEADERS="$TARGET_DIR/$IOS_TARGET/$OUTPUT_DIR/include" + if [[ ! -d "$IOS_HEADERS" ]]; then + log_error "Headers directory not found: $IOS_HEADERS" + exit 1 + fi inject_modulemap "$IOS_HEADERS"🤖 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 189 - 191, Before calling inject_modulemap, validate that the computed headers path (IOS_HEADERS derived from TARGET_DIR, IOS_TARGET, OUTPUT_DIR) exists and is a directory; if not, log a clear error and exit or skip injection. Update the build_ios.sh flow around the IOS_HEADERS and inject_modulemap invocation to check [ -d "$IOS_HEADERS" ] (or equivalent) and handle the missing-directory case with a descriptive message referencing IOS_HEADERS so inject_modulemap is not called on a non-existent HEADERS_DIR.
22-22: Clarify the comment about "dev" vs "debug".The comment is slightly misleading. Cargo's built-in profile is named
devby convention (notdebug), and it outputs to thedebug/directory. This isn't a restriction but rather Cargo's standard naming.-PROFILE="dev" # Rust doesn't allow us to use "debug" for some reason, the profile name internally is dev +PROFILE="dev" # Cargo's built-in profile is named "dev"; output goes to "debug/" directory🤖 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 22, The comment next to PROFILE="dev" is misleading; update it to state that Cargo's built-in profile is named "dev" by convention (not "debug") and that its artifacts are placed in the debug/ output directory, i.e., change the comment around PROFILE="dev" to accurately reflect Cargo naming/convention rather than implying a Rust restriction.
164-167:sed -i ''is macOS-specific; document or guard for portability.The
sed -i ''syntax on line 166 is BSD/macOS-specific. GNU sed on Linux requiressed -iwithout the empty string argument. Since this script targets Apple platforms and will typically run on macOS, this may be acceptable, but consider adding a comment or detecting the platform if cross-platform execution is needed.🤖 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 164 - 167, The sed -i '' invocation is macOS/BSD-specific and should be guarded for portability: replace the direct use of sed -i '' in the for-loop (where HEADERS_DIR is iterated and the line sed -i '' 's/FFIAssetLockFundingType/uint32_t/g' "$h" appears) with a small platform check that sets a SED_INPLACE variable (e.g., use "-i ''" on Darwin and "-i" on Linux) or switch to a portable alternative (use perl -i or sed -i.bak and remove the backup); update the loop to use that variable and add a brief comment explaining the portability handling.
🤖 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/build_ios.sh`:
- Around line 230-235: The xcodebuild invocation uses unquoted parameter
expansions like ${IOS_LIB:+-library "$IOS_LIB" -headers "$IOS_HEADERS"} which
can lose quoting and break on paths with spaces; update the xcodebuild argument
construction to preserve quoting either by wrapping the entire expansion in
quotes (e.g. use "${IOS_LIB:+-library \"$IOS_LIB\" -headers \"$IOS_HEADERS\"}"
style) or, preferably, refactor to build an argument array (e.g., args+=(
-library "$IOS_LIB" -headers "$IOS_HEADERS" ) when IOS_LIB is set) and then call
xcodebuild "${args[@]}" so variables IOS_LIB, IOS_HEADERS, MAC_LIB, MAC_HEADERS,
INTEL_MAC_LIB, INTEL_MAC_HEADERS, SIM_LIB, and SIM_HEADERS are each passed as
properly quoted separate arguments.
---
Nitpick comments:
In `@packages/swift-sdk/build_ios.sh`:
- Around line 189-191: Before calling inject_modulemap, validate that the
computed headers path (IOS_HEADERS derived from TARGET_DIR, IOS_TARGET,
OUTPUT_DIR) exists and is a directory; if not, log a clear error and exit or
skip injection. Update the build_ios.sh flow around the IOS_HEADERS and
inject_modulemap invocation to check [ -d "$IOS_HEADERS" ] (or equivalent) and
handle the missing-directory case with a descriptive message referencing
IOS_HEADERS so inject_modulemap is not called on a non-existent HEADERS_DIR.
- Line 22: The comment next to PROFILE="dev" is misleading; update it to state
that Cargo's built-in profile is named "dev" by convention (not "debug") and
that its artifacts are placed in the debug/ output directory, i.e., change the
comment around PROFILE="dev" to accurately reflect Cargo naming/convention
rather than implying a Rust restriction.
- Around line 164-167: The sed -i '' invocation is macOS/BSD-specific and should
be guarded for portability: replace the direct use of sed -i '' in the for-loop
(where HEADERS_DIR is iterated and the line sed -i ''
's/FFIAssetLockFundingType/uint32_t/g' "$h" appears) with a small platform check
that sets a SED_INPLACE variable (e.g., use "-i ''" on Darwin and "-i" on Linux)
or switch to a portable alternative (use perl -i or sed -i.bak and remove the
backup); update the loop to use that variable and add a brief comment explaining
the portability handling.
🪄 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: 6bd1eb96-e013-4a09-94d7-40c093bdedd0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/swift-sdk-build.yml.github/workflows/swift-sdk-release.ymlCargo.tomlpackages/rs-sdk-ffi/Cargo.tomlpackages/rs-sdk-ffi/build.rspackages/rs-sdk-ffi/build_ios.shpackages/rs-sdk-ffi/cbindgen-core.tomlpackages/rs-sdk-ffi/cbindgen-ios.tomlpackages/rs-sdk-ffi/cbindgen.tomlpackages/rs-sdk-ffi/cbindgen_minimal.tomlpackages/rs-sdk-ffi/combine_headers.shpackages/rs-sdk-ffi/include/dash_sdk_ffi.hpackages/rs-sdk-ffi/src/callback_bridge.rspackages/rs-sdk-ffi/src/context_provider_stubs.rspackages/rs-sdk-ffi/src/lib.rspackages/rs-unified-sdk-ffi/Cargo.tomlpackages/rs-unified-sdk-ffi/src/lib.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyDerivation.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (9)
- packages/rs-sdk-ffi/cbindgen_minimal.toml
- packages/rs-sdk-ffi/combine_headers.sh
- packages/rs-sdk-ffi/cbindgen-core.toml
- packages/rs-sdk-ffi/include/dash_sdk_ffi.h
- packages/rs-sdk-ffi/src/lib.rs
- packages/rs-sdk-ffi/src/callback_bridge.rs
- packages/rs-sdk-ffi/cbindgen-ios.toml
- packages/rs-sdk-ffi/src/context_provider_stubs.rs
- packages/rs-sdk-ffi/build_ios.sh
✅ Files skipped from review due to trivial changes (7)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
- packages/rs-unified-sdk-ffi/src/lib.rs
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyDerivation.swift
- packages/rs-sdk-ffi/build.rs
- packages/rs-unified-sdk-ffi/Cargo.toml
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/swift-sdk-release.yml
- .github/workflows/swift-sdk-build.yml
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedPlatformAccount.swift
- packages/rs-sdk-ffi/cbindgen.toml
- packages/rs-sdk-ffi/Cargo.toml
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.
Note: Looks like a ecent commit in rust-dashcore added enum variants with the same name and exported, cbindgen generated C compatible enums, but, because this is not valid C, swift is not able to compile. The build_ios.sh script contains a quick fix to fix it but I am currently working on rust-dashcore CI to avoid this things from happening by validating the headers generated are C compatible
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests