Skip to content

Comments

fix(swift-sdk): fix iOS XCFramework build and update Swift SDK for rust-dashcore changes#3107

Open
lklimek wants to merge 20 commits intov3.1-devfrom
fix/swift-sdk-missing-spv-symbols
Open

fix(swift-sdk): fix iOS XCFramework build and update Swift SDK for rust-dashcore changes#3107
lklimek wants to merge 20 commits intov3.1-devfrom
fix/swift-sdk-missing-spv-symbols

Conversation

@lklimek
Copy link
Contributor

@lklimek lklimek commented Feb 19, 2026

Issue Being Fixed or Feature Description

The Swift SDK CI build (Build Swift SDK and Example) has been failing across all PRs targeting v3.1-dev. This PR fixes all build failures to make CI green again.

What was done?

Seven root causes were identified and fixed across the build pipeline and Swift SDK source:

1. Wrong target directory for SPV build artifacts

rust-dashcore is a Cargo workspace, so cargo build inside the dash-spv-ffi member crate outputs to the workspace root's target/ directory — not dash-spv-ffi/target/. The build script looked for libdash_spv_ffi.a at the wrong path, causing the library merge to be silently skipped.

Fix: Introduced SPV_TARGET_DIR pointing to the workspace root target directory.

2. Inconsistent library naming broke XCFramework structure

The merge step created libDashSDKFFI_combined.a, but the downstream verification expected librs_sdk_ffi.a inside the XCFramework.

Fix: Merge now overwrites librs_sdk_ffi.a in-place (via a temp file), keeping a consistent name.

3. SIGPIPE in symbol verification under set -euo pipefail

The nm | rg pipeline silently failed because rg could exit after finding a match, causing nm to receive SIGPIPE (exit 141).

Fix: Dump nm output to a temp file first, then grep it.

4. unknown type name 'Option' in cbindgen-generated C header

cbindgen 0.27+ doesn't resolve Option<TypeAlias> where the alias is a function pointer type, generating bare Option in the C header which is invalid C.

Fix: Created new type aliases that embed Option directly in the typedef (e.g., pub type OptionalHasPendingFn = Option<unsafe extern "C" fn(...)>), which cbindgen correctly translates to nullable C function pointers.

5. libtool duplicate member warnings

When merging librs_sdk_ffi.a and libdash_spv_ffi.a, libtool -static emits duplicate member warnings for shared dependency objects. Under strict error checking these could be confusing.

Fix: Capture libtool stderr, filter out "duplicate member name" warnings, but still fail on real errors.

6. FFINetworks bitmap type removed from rust-dashcore

The updated rust-dashcore replaced the FFINetworks bitmap type with a single FFINetwork enum. Network parameters were removed from ~30 FFI functions (network is now stored at the wallet/manager level).

Fix: Updated all 6 Swift wrapper files (Wallet, WalletManager, Transaction, Account, ManagedWallet, KeyWalletTypes) to match the new C function signatures.

7. SPV FFI API changes

dash_spv_ffi_init_logging changed from 1 parameter to 4 (level, enable_console, log_dir, max_files). dash_spv_ffi_client_clear_sync_state was removed entirely.

Fix: Updated SPVClient.swift to use the new logging signature and delegated clearSyncState() to clearStorage().

How Has This Been Tested?

  • CI Build Swift SDK and Example (no warnings) workflow passes (builds xcframework, Swift package, and SwiftExampleApp with warnings-as-errors)
  • CI Test rs-sdk-ffi build workflow passes
  • CI Rust packages (rs-sdk-ffi) / Tests passes
  • All code-related CI checks are green
  • Only remaining failures are pre-existing npm/crate security audit issues on v3.1-dev

Breaking Changes

  • WalletManager.init() now takes a network parameter (defaults to .mainnet)
  • NetworkSet.ffiNetworks renamed to NetworkSet.ffiNetwork (returns FFINetwork instead of removed FFINetworks)
  • SPVClient.clearSyncState() now delegates to clearStorage() (granular API was removed from FFI)

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added new wallet initializers and a non-owning wallet handle initialization in the Swift SDK.
    • WalletManager can be instantiated with a per-instance network.
  • Improvements

    • Streamlined wallet/account APIs to reduce repeated network parameters and favor manager-scoped network usage.
    • Enhanced SPV logging and unified iOS build/merge behavior for device and simulator outputs.
    • Clarified nullable callback types for address provider integrations.
  • Chores

    • Bumped build tooling and regenerated headers for compatibility.

…artifacts

rust-dashcore is a Cargo workspace, so `cargo build` inside the
dash-spv-ffi member crate outputs to the workspace root's target/
directory, not dash-spv-ffi/target/. The build script was looking
for libdash_spv_ffi.a in the wrong location, causing the SPV library
merge to be silently skipped and SPV symbols to be missing from the
final XCFramework.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions github-actions bot added this to the v3.1.0 milestone Feb 19, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 19, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Centralizes SPV target paths and unified iOS static library outputs in rs-sdk-ffi build script; makes C callback pointer nullability explicit; consolidates symbol checks in swift-sdk build script; removes per-call multi-network FFI parameters across Swift KeyWallet/WalletManager; updates SPV client init/clear behavior; bumps cbindgen and regenerates header.

Changes

Cohort / File(s) Summary
rs-sdk-ffi iOS build script
packages/rs-sdk-ffi/build_ios.sh
Adds SPV_TARGET_DIR, refactors device/simulator build flows and libtool merging to produce librs_sdk_ffi_merged.alibrs_sdk_ffi.a, handles x86/arm64 simulator cases, uses LIBTOOL_LOG with duplicate-member warning filtering, and standardizes XCFramework inputs to librs_sdk_ffi.a.
Address sync provider types
packages/rs-sdk-ffi/src/address_sync/provider.rs
Introduces public nullable type aliases OptionalHasPendingFn, OptionalGetHighestFoundIndexFn, OptionalDestroyProviderFn and updates AddressProviderVTable fields to use them (explicit nullable C callback pointer semantics).
rs-sdk-ffi build metadata
packages/rs-sdk-ffi/Cargo.toml
Bumps build-dependency cbindgen from 0.27 → 0.29.2.
Autogenerated C header
packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDK.h
Regenerated header comment/tool version updated to reflect newer cbindgen (no API surface changes).
swift-sdk iOS build script
packages/swift-sdk/build_ios.sh
Replaces piped nm/rg symbol checks with a single temporary NM_SYMBOLS file aggregating nm -gU output (main and SPV), performs fixed-string greps against it, removes SEARCH_CMD logic, and ensures cleanup of NM_SYMBOLS.
KeyWallet wrappers (Swift)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/...
Removes multi-network/FFINetworks bitmap usage in favor of single-network semantics: adds ffiNetwork mapping, drops many per-call network FFI args, updates Wallet and WalletManager constructors/APIs to be manager/wallet-scoped network (e.g., WalletManager.init(network:)), adjusts FFI call sites and public initializers/serializers accordingly.
ManagedWallet / Transaction / Account (Swift)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift, .../Transaction.swift, .../Account.swift
Removes stored network where applicable, drops network arguments from FFI calls, introduces separate error variables (deriveError, wifError) in Account flow; preserves public API signatures while changing internal FFI parameter lists.
Wallet (Swift)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift
Consolidates network handling at creation: replaces NetworkSet(...).ffiNetworks with network.ffiValue, removes network args from many account-related FFIs, adds/adjusts public initializers and getAccountCollection() signature.
WalletManager (Swift)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
Introduces per-instance network binding (init(network: KeyWalletNetwork = .mainnet)), removes or deprecates many per-call network parameters, adds/updates add/import/serialize APIs returning serialized data where applicable, and shifts many wrappers to use manager-scoped network.
SPV client (Swift) & storage flow
packages/swift-sdk/Sources/SwiftDashSDK/SPV/SPVClient.swift, packages/swift-sdk/SwiftExampleApp/.../WalletService.swift
Updates dash_spv_ffi_init_logging calls to include extra args (cstr, true, nil, 0); changes clearSyncState to delegate to clearStorage(); WalletService.clearSpvStorage now always calls clearStorage() and handles reset behavior afterward.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through builds and stitched the maze,
Callbacks now nullable, paths set ablaze,
Symbols in one basket, libraries merged tight,
Wallets carry one network, tidy and light,
A nibble, a thump — the repo feels just right.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main objective of the changeset: fixing iOS XCFramework build issues and updating Swift SDK to align with rust-dashcore API changes.
Docstring Coverage ✅ Passed Docstring coverage is 96.49% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/swift-sdk-missing-spv-symbols

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…ramework naming

The previous approach created libDashSDKFFI_combined.a which changed the
library name inside the XCFramework, breaking the symbol verification
that expects librs_sdk_ffi.a. Now the merge overwrites librs_sdk_ffi.a
via a temp file, keeping a consistent name regardless of whether SPV
symbols were merged.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/rs-sdk-ffi/build_ios.sh (1)

446-463: ⚠️ Potential issue | 🟠 Major

Universal and x86 simulator builds have wrong or missing SPV symbols — two interrelated bugs.

Issue 1 — BUILD_ARCH=x86 is now a regression.

Before this PR, both SPV checks at lines 449/451 used incorrect paths ($SPV_CRATE_PATH/target/...), so SIM_SPV_LIB stayed empty and no merge happened — accidentally producing a correct x86_64-only simulator library. After the path fix, line 449 now correctly resolves $SPV_TARGET_DIR/aarch64-apple-ios-sim/release/libdash_spv_ffi.a (which exists because the SPV else build-branch at line 347 always compiles aarch64-apple-ios-sim, even in x86 mode). libtool -static then merges a thin x86_64 librs_sdk_ffi.a with a thin arm64 libdash_spv_ffi.a, producing a broken mixed-architecture combined library.

Issue 2 — BUILD_ARCH=universal gets only arm64 SPV symbols.

The universal simulator librs_sdk_ffi.a is a lipo fat binary (arm64-sim + x86_64). The SPV build correctly compiles both aarch64-apple-ios-sim and x86_64-apple-ios targets (lines 338–346), but the elif chain picks only the arm64-sim SPV lib (line 449 matches first) and never uses the x86_64 one. The resulting combined library carries x86_64 rs_sdk_ffi symbols without any x86_64 SPV symbols.

Root cause: the elif chain is architecture-agnostic, while the choice of SPV lib must depend on $BUILD_ARCH. For universal, both SPV sim libs must be lipo'd together before the libtool merge.

🐛 Proposed fix for the simulator SPV selection block (and coupled SPV build section)

1. Fix the SPV build section so x86 mode compiles the correct SPV target instead of arm64-sim (change to the existing else branch at lines 347–353 — outside the PR diff but tightly coupled):

   if [ "$BUILD_ARCH" = "universal" ]; then
     ...
+  elif [ "$BUILD_ARCH" = "x86" ]; then
+    if [ -n "${RUST_DASHCORE_TOOLCHAIN:-}" ]; then
+      cargo +"${RUST_DASHCORE_TOOLCHAIN}" build --lib --target x86_64-apple-ios --release > /tmp/cargo_build_spv_sim_x86.log 2>&1 || { echo -e "${RED}✗ dash-spv-ffi sim (x86_64) build failed${NC}"; cat /tmp/cargo_build_spv_sim_x86.log; exit 1; }
+    else
+      cargo build --lib --target x86_64-apple-ios --release > /tmp/cargo_build_spv_sim_x86.log 2>&1 || { echo -e "${RED}✗ dash-spv-ffi sim (x86_64) build failed${NC}"; cat /tmp/cargo_build_spv_sim_x86.log; exit 1; }
+    fi
   else
     # arm64-sim only (arm default)
     ...
   fi

2. Fix the simulator SPV lib selection block to handle all three modes correctly:

 if [ -f "$OUTPUT_DIR/simulator/librs_sdk_ffi.a" ]; then
     # Try to merge with SPV sim lib if it exists
     SIM_SPV_LIB=""
-    if [ -f "$SPV_TARGET_DIR/aarch64-apple-ios-sim/release/libdash_spv_ffi.a" ]; then
-      SIM_SPV_LIB="$SPV_TARGET_DIR/aarch64-apple-ios-sim/release/libdash_spv_ffi.a"
-    elif [ -f "$SPV_TARGET_DIR/x86_64-apple-ios/release/libdash_spv_ffi.a" ]; then
-      SIM_SPV_LIB="$SPV_TARGET_DIR/x86_64-apple-ios/release/libdash_spv_ffi.a"
+    if [ "$BUILD_ARCH" = "universal" ] && \
+       [ -f "$SPV_TARGET_DIR/aarch64-apple-ios-sim/release/libdash_spv_ffi.a" ] && \
+       [ -f "$SPV_TARGET_DIR/x86_64-apple-ios/release/libdash_spv_ffi.a" ]; then
+      # Universal: lipo both SPV sim slices into a fat library before merging
+      lipo -create \
+        "$SPV_TARGET_DIR/aarch64-apple-ios-sim/release/libdash_spv_ffi.a" \
+        "$SPV_TARGET_DIR/x86_64-apple-ios/release/libdash_spv_ffi.a" \
+        -output /tmp/libdash_spv_ffi_sim_universal.a
+      SIM_SPV_LIB="/tmp/libdash_spv_ffi_sim_universal.a"
+    elif [ "$BUILD_ARCH" = "x86" ] && [ -f "$SPV_TARGET_DIR/x86_64-apple-ios/release/libdash_spv_ffi.a" ]; then
+      SIM_SPV_LIB="$SPV_TARGET_DIR/x86_64-apple-ios/release/libdash_spv_ffi.a"
+    elif [ -f "$SPV_TARGET_DIR/aarch64-apple-ios-sim/release/libdash_spv_ffi.a" ]; then
+      # arm (default)
+      SIM_SPV_LIB="$SPV_TARGET_DIR/aarch64-apple-ios-sim/release/libdash_spv_ffi.a"
     fi
🤖 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` around lines 446 - 463, The simulator-SPV
selection is architecture-agnostic and merges mismatched thin libs; update the
SPV build logic to compile the correct SPV target for BUILD_ARCH=x86 (i.e.,
build the x86_64-apple-ios SPV target in the SPV build branch used for x86) and
then change the simulator selection that sets SIM_SPV_LIB so it branches on
BUILD_ARCH: for BUILD_ARCH=aarch64 pick
SPV_TARGET_DIR/aarch64-apple-ios-sim/release/libdash_spv_ffi.a, for
BUILD_ARCH=x86 pick SPV_TARGET_DIR/x86_64-apple-ios/release/libdash_spv_ffi.a,
and for BUILD_ARCH=universal first combine both SPV thin libs
(aarch64-apple-ios-sim + x86_64-apple-ios) into a fat lib via lipo and set
SIM_SPV_LIB to that fat lib before calling libtool to merge with
OUTPUT_DIR/simulator/librs_sdk_ffi.a so architectures always match.
🧹 Nitpick comments (1)
packages/rs-sdk-ffi/build_ios.sh (1)

325-326: SPV_TARGET_DIR correctly targets the workspace root — LGTM.

Cargo workspaces use a single top-level target directory; even when cargo build is invoked from inside a member crate directory, compiled artifacts still end up in the workspace root's target/. Pointing SPV_TARGET_DIR at $RUST_DASHCORE_PATH/target is the right fix.

One small portability note: CARGO_TARGET_DIR (env var), build.target-dir (config value), or the --target-dir CLI flag can all redirect artifacts to a custom location. If a CI environment sets CARGO_TARGET_DIR, this hardcoded path will silently diverge from where cargo actually wrote the artifacts. Consider using SPV_TARGET_DIR="${CARGO_TARGET_DIR:-$RUST_DASHCORE_PATH/target}" to honor any override.

🛡️ Proposed defensive fix
-SPV_TARGET_DIR="$RUST_DASHCORE_PATH/target"  # workspace root target dir
+# Honour any external CARGO_TARGET_DIR override; otherwise fall back to workspace root
+SPV_TARGET_DIR="${CARGO_TARGET_DIR:-$RUST_DASHCORE_PATH/target}"
🤖 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` around lines 325 - 326, SPV_TARGET_DIR
currently hardcodes the workspace target path; update its assignment to honor an
existing CARGO_TARGET_DIR override by setting SPV_TARGET_DIR to use
CARGO_TARGET_DIR if defined, falling back to "$RUST_DASHCORE_PATH/target"
otherwise (i.e. implement the shell parameter expansion using CARGO_TARGET_DIR,
referencing SPV_TARGET_DIR, CARGO_TARGET_DIR and RUST_DASHCORE_PATH).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@packages/rs-sdk-ffi/build_ios.sh`:
- Around line 446-463: The simulator-SPV selection is architecture-agnostic and
merges mismatched thin libs; update the SPV build logic to compile the correct
SPV target for BUILD_ARCH=x86 (i.e., build the x86_64-apple-ios SPV target in
the SPV build branch used for x86) and then change the simulator selection that
sets SIM_SPV_LIB so it branches on BUILD_ARCH: for BUILD_ARCH=aarch64 pick
SPV_TARGET_DIR/aarch64-apple-ios-sim/release/libdash_spv_ffi.a, for
BUILD_ARCH=x86 pick SPV_TARGET_DIR/x86_64-apple-ios/release/libdash_spv_ffi.a,
and for BUILD_ARCH=universal first combine both SPV thin libs
(aarch64-apple-ios-sim + x86_64-apple-ios) into a fat lib via lipo and set
SIM_SPV_LIB to that fat lib before calling libtool to merge with
OUTPUT_DIR/simulator/librs_sdk_ffi.a so architectures always match.

---

Nitpick comments:
In `@packages/rs-sdk-ffi/build_ios.sh`:
- Around line 325-326: SPV_TARGET_DIR currently hardcodes the workspace target
path; update its assignment to honor an existing CARGO_TARGET_DIR override by
setting SPV_TARGET_DIR to use CARGO_TARGET_DIR if defined, falling back to
"$RUST_DASHCORE_PATH/target" otherwise (i.e. implement the shell parameter
expansion using CARGO_TARGET_DIR, referencing SPV_TARGET_DIR, CARGO_TARGET_DIR
and RUST_DASHCORE_PATH).

lklimek and others added 2 commits February 19, 2026 08:58
Temporary debug logging to diagnose why SPV symbols are missing from the
merged library even though the merge step succeeds.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The nm | rg/grep pipeline was silently failing under `set -euo pipefail`
because rg/grep could exit early after finding a match, causing nm to
receive SIGPIPE (exit 141). With pipefail, this made the pipeline return
non-zero even when the symbol was found.

Fix by dumping nm output to a temp file first, then grepping it. This
avoids the pipe entirely and eliminates the SIGPIPE race condition.

Also removes temporary debug logging from the previous commit.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lklimek lklimek changed the title fix(swift-sdk): use workspace root target dir for dash-spv-ffi build artifacts fix(swift-sdk): fix SPV symbol linking in iOS XCFramework build Feb 19, 2026
…icate warnings

cbindgen 0.27 does not resolve Option<TypeAlias> when the alias is a
function pointer type, emitting bare `Option` in the C header which is
not a valid C type. Fix by defining Optional* type aliases that include
Option in the typedef itself (e.g. `type OptionalHasPendingFn =
Option<unsafe extern "C" fn(...)>`), which cbindgen correctly translates
to nullable C function pointers.

Also suppress expected "duplicate member name" warnings from libtool
when merging rs-sdk-ffi and dash-spv-ffi static libraries that share
common dependency objects.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/swift-sdk/build_ios.sh (1)

47-63: LGTM — consider adding a trap for defensive temp file cleanup.

The NM_SYMBOLS approach correctly resolves the SIGPIPE issue under set -euo pipefail. Temp file cleanup at line 63 lands before the conditional exit 1 at line 65, so the normal flow is clean.

However, the file leaks if the script is interrupted by a signal (SIGTERM/SIGINT) in the window between line 47 and 63. A single trap right after mktemp eliminates the risk with no behavior change:

♻️ Proposed fix
 NM_SYMBOLS=$(mktemp)
+trap 'rm -f "$NM_SYMBOLS"' EXIT
 nm -gU "$LIB_SIM_MAIN" > "$NM_SYMBOLS" 2>/dev/null || true

The EXIT trap fires on any exit (normal, error, or signal), making the explicit rm -f "$NM_SYMBOLS" on line 63 redundant (but harmless to keep for clarity).

🤖 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 47 - 63, Add a defensive EXIT
trap immediately after creating the temporary file so NM_SYMBOLS is always
removed on script termination; specifically, after the mktemp call that assigns
NM_SYMBOLS (and before any early exits), register a trap for EXIT that runs rm
-f "$NM_SYMBOLS" to ensure cleanup on normal exit, error, or signals (you can
keep the existing explicit rm -f "$NM_SYMBOLS" later for clarity).
🤖 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 451-475: The simulator merge currently picks a single thin SPV lib
into SIM_SPV_LIB and merges it with the fat
OUTPUT_DIR/simulator/librs_sdk_ffi.a, which leaves the other architecture slice
missing dash_spv_ffi symbols when BUILD_ARCH=universal; change the logic in the
block that sets SIM_SPV_LIB and runs libtool so that when BUILD_ARCH=universal
(and both SPV thin libs exist at
SPV_TARGET_DIR/aarch64-apple-ios-sim/release/libdash_spv_ffi.a and
SPV_TARGET_DIR/x86_64-apple-ios/release/libdash_spv_ffi.a) you first lipo
-create those two thin SPV libs into a temporary fat SPV archive and set
SIM_SPV_LIB to that fat archive, then run libtool -static to merge
OUTPUT_DIR/simulator/librs_sdk_ffi.a with the fat SIM_SPV_LIB, and finally clean
up the temporary fat file; ensure existing error logging and mv of the merged
archive to OUTPUT_DIR/simulator/librs_sdk_ffi.a remain intact.

---

Nitpick comments:
In `@packages/swift-sdk/build_ios.sh`:
- Around line 47-63: Add a defensive EXIT trap immediately after creating the
temporary file so NM_SYMBOLS is always removed on script termination;
specifically, after the mktemp call that assigns NM_SYMBOLS (and before any
early exits), register a trap for EXIT that runs rm -f "$NM_SYMBOLS" to ensure
cleanup on normal exit, error, or signals (you can keep the existing explicit rm
-f "$NM_SYMBOLS" later for clarity).

lklimek and others added 2 commits February 19, 2026 09:45
…core

The rust-dashcore update removed the FFINetworks bitmap type and replaced
it with single FFINetwork enum. Network parameter was removed from ~30 FFI
functions (now stored at wallet/manager level). Updated all Swift wrappers
to match the new C function signatures.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…gnature

- dash_spv_ffi_init_logging now takes 4 params (level, enable_console,
  log_dir, max_files) instead of 1
- dash_spv_ffi_client_clear_sync_state was removed; clearSyncState()
  now delegates to clearStorage()

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link

github-actions bot commented Feb 19, 2026

✅ 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: "8c40300d3cdd8818c6ca28c06aa8983716d5dd9c2b4aa6968d38da249fbf0d3b"
)

Xcode manual integration:

  • Download 'DashSDKFFI.xcframework' artifact from the run link above.
  • Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
  • If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (2)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift (1)

7-7: 🛠️ Refactor suggestion | 🟠 Major

Remove unused network property.

The network field is assigned at init but never referenced anywhere in the file. Remove it to eliminate dead code.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift` at
line 7, Remove the unused network property from ManagedWallet: delete the
private let network: KeyWalletNetwork declaration and remove its assignment and
parameter from ManagedWallet's initializer (and any related initializer
parameter or stored-property setup) so no dead field remains; ensure you also
update any initializer calls or usages inside this file to reflect the removed
parameter/assignment and run build to confirm no remaining references to network
within ManagedWallet.
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift (1)

528-532: ⚠️ Potential issue | 🟡 Minor

Restrict nonOwningHandle initializer visibility to prevent unsafe external usage.

Since Wallet is a public class, external code can access this public initializer and create non-owning wrapper instances. The only internal usage is in WalletManager.getWallet(...) — external callers have no legitimate reason to instantiate non-owning wrappers and risk dangling-pointer dereferences if they misunderstand the ownership contract.

Change visibility to internal or package:

Suggested fix
-    public init(nonOwningHandle handle: UnsafeRawPointer, network: KeyWalletNetwork) {
+    internal init(nonOwningHandle handle: UnsafeRawPointer, network: KeyWalletNetwork) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift` around lines
528 - 532, Make the non-owning initializer for Wallet private to the module by
changing the visibility of public init(nonOwningHandle:handle:network:) from
public to internal (or package if you prefer), so external callers cannot
construct non-owning wrappers; update any call sites (e.g.,
WalletManager.getWallet(...)) to remain valid since they are internal and will
still be able to use the internal initializer. Ensure the declaration of
init(nonOwningHandle:network:) is the only visibility change and do not alter
ownership semantics or other initializers on the Wallet class.
🧹 Nitpick comments (3)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (1)

57-65: Vestigial network parameter accepted but silently ignored — deprecate or remove.

Multiple public methods (addWallet, getReceiveAddress, getChangeAddress, processTransaction, getManagedAccount, getManagedTopUpAccount, getManagedAccountCollection, addWalletAndSerialize) accept a network: parameter that is documented as "ignored; manager network is used" but still present in the signature. This is error-prone: a caller passing .testnet would silently get the manager's network behavior instead.

Consider deprecating these parameters:

💡 Example deprecation pattern for addWallet
     `@discardableResult`
     public func addWallet(mnemonic: String, passphrase: String? = nil,
-                          network: KeyWalletNetwork = .mainnet,
                           accountOptions: AccountCreationOption = .default) throws -> Data {

If backward compatibility is needed, keep the old signatures with @available(*, deprecated, message: "network parameter is ignored; the manager's network is used").

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift` around
lines 57 - 65, Several public methods (addWallet, getReceiveAddress,
getChangeAddress, processTransaction, getManagedAccount, getManagedTopUpAccount,
getManagedAccountCollection, addWalletAndSerialize) accept a network: parameter
that is ignored; update these signatures to avoid silent misuse by either
removing the network parameter or marking it deprecated for backward
compatibility; if preserving the parameter for compatibility, annotate each with
`@available`(*, deprecated, message: "network parameter is ignored; the manager's
network is used"), update the doc comments to reflect deprecation, and ensure
any callers/tests are updated to stop relying on the ignored network parameter
so behavior remains explicit.
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swift (1)

24-27: Set.first has nondeterministic ordering — consider documenting or asserting single-element expectation.

Set in Swift has no guaranteed iteration order, so networks.first is unstable when the set contains multiple networks. Since the FFI now only supports a single network, consider adding a precondition or debug assertion:

💡 Suggested defensive assertion
     public var ffiNetwork: FFINetwork {
+        assert(networks.count <= 1, "FFI only supports a single network; NetworkSet contains \(networks.count)")
         let network = networks.first ?? .mainnet
         return network.ffiValue
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swift`
around lines 24 - 27, The current ffiNetwork computed property uses
networks.first which is nondeterministic for Sets; add a defensive check to
assert or precondition that networks contains at most one element (e.g.,
networks.count <= 1) and document that the FFI supports a single network, then
safely unwrap the single element (or fall back to .mainnet) in ffiNetwork;
reference the networks Set and the ffiNetwork property in your change and
include the assertion/debug message so failures surface during development.
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Account.swift (1)

36-56: Pre-existing double-free risk: shared error variable with two defer blocks.

Both defer blocks (lines 36-43 and 52-56) check and free error.message, but they share the same FFIError variable. Since defer blocks run in LIFO order, if the second FFI call (account_derive_private_key_as_wif_at) sets error.message, the inner defer frees it, then the outer defer reads the now-dangling pointer again.

This is pre-existing and not introduced by this PR, but it's on a code path you're touching. Consider scoping a separate FFIError for the second call or nil-ing out error.message after freeing.

🤖 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/KeyWallet/WalletManager.swift`:
- Around line 105-108: The addWallet method currently returns
getWalletIds().last which is TOCTOU-prone and may return an empty Data; instead
call the FFI helper
wallet_manager_add_wallet_from_mnemonic_return_serialized_bytes (the same path
used by addWalletAndSerialize) to obtain the new wallet ID via its output
parameter, check the returned status, convert the output buffer into a 32-byte
Data (or throw if size/status is invalid), free any FFI-allocated memory as done
in addWalletAndSerialize, and return that Data; remove the fallback to Data()
and the getWalletIds() query so addWallet directly returns the ID produced by
the FFI call.

In `@packages/swift-sdk/Sources/SwiftDashSDK/SPV/SPVClient.swift`:
- Around line 490-493: clearSyncState()'s docstring claims it only clears
sync-state but the implementation calls clearStorage() which wipes everything;
update the API to avoid confusion by marking clearSyncState() as deprecated and
redirecting callers to clearStorage(): add an `@available`(*, deprecated, renamed:
"clearStorage") annotation on clearSyncState(), update or remove the misleading
docstring to state it's deprecated/renamed, and update the WalletService call
sites (which currently call clearSyncState() when fullReset=false) to call
clearStorage() explicitly or handle the new behavior so there is no ABI/behavior
mismatch between clearSyncState(), clearStorage(), and WalletService.

---

Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift`:
- Line 7: Remove the unused network property from ManagedWallet: delete the
private let network: KeyWalletNetwork declaration and remove its assignment and
parameter from ManagedWallet's initializer (and any related initializer
parameter or stored-property setup) so no dead field remains; ensure you also
update any initializer calls or usages inside this file to reflect the removed
parameter/assignment and run build to confirm no remaining references to network
within ManagedWallet.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift`:
- Around line 528-532: Make the non-owning initializer for Wallet private to the
module by changing the visibility of public
init(nonOwningHandle:handle:network:) from public to internal (or package if you
prefer), so external callers cannot construct non-owning wrappers; update any
call sites (e.g., WalletManager.getWallet(...)) to remain valid since they are
internal and will still be able to use the internal initializer. Ensure the
declaration of init(nonOwningHandle:network:) is the only visibility change and
do not alter ownership semantics or other initializers on the Wallet class.

---

Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swift`:
- Around line 24-27: The current ffiNetwork computed property uses
networks.first which is nondeterministic for Sets; add a defensive check to
assert or precondition that networks contains at most one element (e.g.,
networks.count <= 1) and document that the FFI supports a single network, then
safely unwrap the single element (or fall back to .mainnet) in ffiNetwork;
reference the networks Set and the ffiNetwork property in your change and
include the assertion/debug message so failures surface during development.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- Around line 57-65: Several public methods (addWallet, getReceiveAddress,
getChangeAddress, processTransaction, getManagedAccount, getManagedTopUpAccount,
getManagedAccountCollection, addWalletAndSerialize) accept a network: parameter
that is ignored; update these signatures to avoid silent misuse by either
removing the network parameter or marking it deprecated for backward
compatibility; if preserving the parameter for compatibility, annotate each with
`@available`(*, deprecated, message: "network parameter is ignored; the manager's
network is used"), update the doc comments to reflect deprecation, and ensure
any callers/tests are updated to stop relying on the ignored network parameter
so behavior remains explicit.

@lklimek lklimek changed the title fix(swift-sdk): fix SPV symbol linking in iOS XCFramework build fix(swift-sdk): fix iOS XCFramework build and update Swift SDK for rust-dashcore changes Feb 19, 2026
lklimek and others added 6 commits February 19, 2026 10:50
Align rs-sdk-ffi with the same cbindgen version already used by
rust-dashcore dependencies, removing the duplicate 0.27.0 entry
from Cargo.lock. Regenerated C headers with cbindgen 0.29.2.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix BUILD_ARCH-aware SPV lib selection in build_ios.sh: x86 mode now
  builds the correct x86_64-apple-ios SPV target, and universal mode
  lipo-creates both SPV thin libs into a fat archive before libtool merge
- Fix addWallet TOCTOU: use wallet_manager_add_wallet_from_mnemonic_return_serialized_bytes
  to get wallet ID directly instead of querying getWalletIds().last
- Deprecate clearSyncState() with @available renamed to clearStorage()
  and update docstring to reflect actual behavior
- Remove unused network property from ManagedWallet
- Change Wallet.init(nonOwningHandle:) from public to internal
- Deprecate vestigial network: parameters across 8 WalletManager methods
- Add assertion for NetworkSet.ffiNetwork single-network invariant
- Fix pre-existing double-free risk in Account.derivePrivateKeyWIF by
  splitting shared FFIError into separate deriveError/wifError variables
- Add trap for temp file cleanup in swift-sdk build_ios.sh

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…orage()

The SwiftExampleApp called clearSyncState() which is now deprecated and
triggers a build error with warnings-as-errors. Since clearSyncState()
already delegates to clearStorage(), use clearStorage() directly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove network: parameter from getManagedAccountCollection() and
getManagedAccount() calls, using the non-deprecated overloads that
rely on the WalletManager's instance-scoped network instead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift (1)

147-182: ⚠️ Potential issue | 🟠 Major

Remove manual wallet_free in error path or disable ownership flag before freeing.

The double-free risk is real. When addAccount throws at line 176, the catch block calls wallet_free(handle) at line 179. However, self.handle and self.ownsHandle = true are already assigned (lines 174–175), so the exception propagates and deinit executes, calling wallet_free(handle) again.

The deinit implementation correctly checks if ownsHandle, but ownsHandle is true when the manual free occurs, leading to a double-free.

Either remove the manual free and rely on deinit:

         do {
             _ = try addAccount(type: .standardBIP44, index: 0, xpub: xpub)
         } catch {
             throw error
         }

Or set ownsHandle = false before the manual free to prevent deinit from freeing again.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift` around lines
147 - 182, In Wallet.init(xpub:network:) there is a double-free risk: after
assigning self.handle and self.ownsHandle = true, the catch block of
addAccount(...) calls wallet_free(handle) which will be freed again in deinit;
to fix, either remove the manual wallet_free(handle) call and let deinit handle
cleanup, or if you must free immediately, set self.ownsHandle = false before
calling wallet_free(handle) (and ensure self.handle remains set or cleared
appropriately); update the catch in init(xpub:network:) accordingly so deinit
won't double-free the handle.
🧹 Nitpick comments (5)
packages/rs-sdk-ffi/build_ios.sh (1)

466-490: libdash_spv_ffi_fat.a is not cleaned up on libtool failure.

The FAT_SPV_SIM archive created at line 467 is only removed at line 490 (success path). The failure branch at lines 481–483 exits without deleting it, leaving a stale file in $OUTPUT_DIR/simulator/. This has no correctness impact since lipo -create overwrites it on retry, but it's a minor cleanliness issue.

🧹 Proposed fix to clean up `FAT_SPV_SIM` on failure
-      if ! libtool -static -o "$OUTPUT_DIR/simulator/librs_sdk_ffi_merged.a" \
+      if ! libtool -static -o "$OUTPUT_DIR/simulator/librs_sdk_ffi_merged.a" \
         "$OUTPUT_DIR/simulator/librs_sdk_ffi.a" \
         "$SIM_SPV_LIB" 2>"$LIBTOOL_LOG"; then
         cat "$LIBTOOL_LOG" >&2
         rm -f "$LIBTOOL_LOG"
+        rm -f "${FAT_SPV_SIM:-}"
         exit 1
       fi
🤖 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` around lines 466 - 490, When libtool fails
while merging simulator libs in the block that sets SIM_SPV_LIB (using
FAT_SPV_SIM and LIBTOOL_LOG), ensure the temporary fat archive (FAT_SPV_SIM /
"$OUTPUT_DIR/simulator/libdash_spv_ffi_fat.a") is removed before exiting: after
printing LIBTOOL_LOG and before exit 1 in the libtool failure branch, rm -f the
FAT_SPV_SIM path (and still remove LIBTOOL_LOG), so the temporary fat SPV
archive is cleaned up on error.
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swift (1)

24-28: assert is debug-only; multi-network NetworkSet will silently pick the first element in release builds.

This is fine for a migration period, but callers that still construct NetworkSet with multiple networks will get no diagnostic in production. If this should be a hard constraint long-term, consider precondition or removing the multi-element init overload.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swift`
around lines 24 - 28, The current ffiNetwork computed property uses assert (in
KeyWalletTypes.swift) so in release builds a NetworkSet with multiple networks
will silently use the first element; change the check to a precondition (or
otherwise enforce/fail) so it triggers in release: in the ffiNetwork getter
replace the assert(networks.count <= 1, ...) with a precondition or throw/return
error handling that enforces the single-network constraint for NetworkSet,
ensuring the check on networks.count and the selection of networks.first (or
.mainnet) are guarded by that enforced condition.
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (2)

249-305: Reused error variable across three sequential FFI calls in getReceiveAddress.

The same FFIError is passed to wallet_manager_get_managed_wallet_info, wallet_manager_get_wallet, and managed_wallet_get_next_bip44_receive_address. This works because each failing call triggers an immediate throw, and successful FFI calls leave error.message as nil. However, it's fragile — if an FFI ever sets a non-null error message on a successful return code, the prior message would leak.

The same pattern applies to getChangeAddress (lines 324-380).

Consider reinitializing the error struct before each FFI call (e.g., error = FFIError()) to make the contract explicit and avoid potential leaks if FFI behavior changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift` around
lines 249 - 305, Reinitialize the FFIError before each FFI call to avoid leaking
a previous error message: in getReceiveAddress (and similarly in
getChangeAddress) create/reset the error struct (FFIError()) immediately before
calling wallet_manager_get_managed_wallet_info, then again before
wallet_manager_get_wallet, and again before
managed_wallet_get_next_bip44_receive_address so each call uses a fresh error
instance; keep the existing defer/frees around the specific error.message checks
and thrown KeyWalletError(ffiError: error) behavior intact.

134-147: Multi-network addWallet and addWalletAndSerialize overloads silently ignore all networks past the first.

addWallet(mnemonic:passphrase:networks:accountOptions:) (line 142) and addWalletAndSerialize(...networks:...) (line 743) accept a [KeyWalletNetwork] array but delegate to the single-network variant without any diagnostic. Unlike NetworkSet.ffiNetwork which at least asserts on count > 1, these methods silently discard extra networks.

If callers pass [.mainnet, .testnet] expecting both, they'll get only the manager's network with no warning. Consider adding an assert or a log when networks.count > 1.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift` around
lines 134 - 147, The overloads
addWallet(mnemonic:passphrase:networks:accountOptions:) and
addWalletAndSerialize(...networks:...) currently drop all networks after the
first without any diagnostic; update both methods to detect when networks.count
> 1 and emit a clear diagnostic (e.g., assertionFailure or a Logger.warn)
stating that only the first network will be used, mirroring the behavior of
NetworkSet.ffiNetwork, then continue delegating to the single-network variant
with networks.first; ensure the message mentions the method name and the
provided networks to aid debugging.
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/WalletService.swift (1)

566-568: Log message is now slightly misleading when fullReset is false.

Since both code paths now call clearStorage(), the "sync-state" description in the log no longer accurately reflects the clearing operation — only the post-clear reset behavior differs.

✏️ Suggested log clarification
-        let modeDescription = fullReset ? "full storage" : "sync-state"
-        print("[SPV][Clear] Completed \(modeDescription) reset for \(currentNetwork.rawValue)")
+        let resetDescription = fullReset ? "full" : "partial"
+        print("[SPV][Clear] Completed storage clear + \(resetDescription) UI reset for \(currentNetwork.rawValue)")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/WalletService.swift`
around lines 566 - 568, The log currently sets modeDescription based on
fullReset but both branches call clearStorage(), making "sync-state" misleading;
update the log in WalletService (around clearStorage()/fullReset) to accurately
reflect that storage was cleared and indicate the post-reset behavior (e.g.,
"storage cleared; post-reset: full storage" vs "storage cleared; post-reset:
sync-state") or emit two logs: one confirming clearStorage() completed and one
describing the subsequent reset mode, referencing the fullReset variable,
clearStorage(), modeDescription (or renamed variable), and
currentNetwork.rawValue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift`:
- Around line 147-182: In Wallet.init(xpub:network:) there is a double-free
risk: after assigning self.handle and self.ownsHandle = true, the catch block of
addAccount(...) calls wallet_free(handle) which will be freed again in deinit;
to fix, either remove the manual wallet_free(handle) call and let deinit handle
cleanup, or if you must free immediately, set self.ownsHandle = false before
calling wallet_free(handle) (and ensure self.handle remains set or cleared
appropriately); update the catch in init(xpub:network:) accordingly so deinit
won't double-free the handle.

---

Duplicate comments:
In `@packages/rs-sdk-ffi/build_ios.sh`:
- Around line 460-493: The review marks a duplicate comment/state for the
universal simulator SPV merge; remove the duplicated marker and ensure the merge
logic isn't present twice—keep the single block that sets SIM_SPV_LIB (using
SPV_SIM_ARM, SPV_SIM_X86, FAT_SPV_SIM), performs libtool merging into
OUTPUT_DIR/simulator/librs_sdk_ffi.a, filters duplicate-member warnings, and
appends the simulator library to XCFRAMEWORK_CMD; if the same logic appears
elsewhere, consolidate it into one place or extract it to a helper (e.g.,
merge_simulator_spv_libs) and call that instead.

---

Nitpick comments:
In `@packages/rs-sdk-ffi/build_ios.sh`:
- Around line 466-490: When libtool fails while merging simulator libs in the
block that sets SIM_SPV_LIB (using FAT_SPV_SIM and LIBTOOL_LOG), ensure the
temporary fat archive (FAT_SPV_SIM /
"$OUTPUT_DIR/simulator/libdash_spv_ffi_fat.a") is removed before exiting: after
printing LIBTOOL_LOG and before exit 1 in the libtool failure branch, rm -f the
FAT_SPV_SIM path (and still remove LIBTOOL_LOG), so the temporary fat SPV
archive is cleaned up on error.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swift`:
- Around line 24-28: The current ffiNetwork computed property uses assert (in
KeyWalletTypes.swift) so in release builds a NetworkSet with multiple networks
will silently use the first element; change the check to a precondition (or
otherwise enforce/fail) so it triggers in release: in the ffiNetwork getter
replace the assert(networks.count <= 1, ...) with a precondition or throw/return
error handling that enforces the single-network constraint for NetworkSet,
ensuring the check on networks.count and the selection of networks.first (or
.mainnet) are guarded by that enforced condition.

In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- Around line 249-305: Reinitialize the FFIError before each FFI call to avoid
leaking a previous error message: in getReceiveAddress (and similarly in
getChangeAddress) create/reset the error struct (FFIError()) immediately before
calling wallet_manager_get_managed_wallet_info, then again before
wallet_manager_get_wallet, and again before
managed_wallet_get_next_bip44_receive_address so each call uses a fresh error
instance; keep the existing defer/frees around the specific error.message checks
and thrown KeyWalletError(ffiError: error) behavior intact.
- Around line 134-147: The overloads
addWallet(mnemonic:passphrase:networks:accountOptions:) and
addWalletAndSerialize(...networks:...) currently drop all networks after the
first without any diagnostic; update both methods to detect when networks.count
> 1 and emit a clear diagnostic (e.g., assertionFailure or a Logger.warn)
stating that only the first network will be used, mirroring the behavior of
NetworkSet.ffiNetwork, then continue delegating to the single-network variant
with networks.first; ensure the message mentions the method name and the
provided networks to aid debugging.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/WalletService.swift`:
- Around line 566-568: The log currently sets modeDescription based on fullReset
but both branches call clearStorage(), making "sync-state" misleading; update
the log in WalletService (around clearStorage()/fullReset) to accurately reflect
that storage was cleared and indicate the post-reset behavior (e.g., "storage
cleared; post-reset: full storage" vs "storage cleared; post-reset: sync-state")
or emit two logs: one confirming clearStorage() completed and one describing the
subsequent reset mode, referencing the fullReset variable, clearStorage(),
modeDescription (or renamed variable), and currentNetwork.rawValue.

@PastaPastaPasta
Copy link
Member

Below is Claude's review; I agree this feels like a bit much for a fix ci pr.

Overview

This PR fixes the broken CI pipeline for the Swift SDK by addressing 7 distinct root causes: incorrect SPV build artifact paths, inconsistent library naming, SIGPIPE in verification scripts, cbindgen 0.27 Option type bug, libtool duplicate warnings, FFINetworks bitmap removal from rust-dashcore, and SPV FFI API changes. It also upgrades cbindgen from 0.27 to 0.29.2.

The commit history is well-structured with logical, atomic commits - good practice.


Build Script Changes (build_ios.sh) - Solid

  • SPV target directory fix is correct and important — workspace member crates build to the workspace root target/, not their own.
  • libtool error handling is a meaningful improvement: captures stderr, filters expected "duplicate member name" warnings, and fails on real errors.
  • Consistent library naming simplification (librs_sdk_ffi_merged.a → replaces original) removes branching complexity.

Minor Issues

  1. RUST_DASHCORE_PATH defined 3 times with the same value ("$PROJECT_ROOT/../rust-dashcore"). Should be defined once at the top.
  2. Hardcoded sibling path assumption (../rust-dashcore). Consider defaulting from an env var: RUST_DASHCORE_PATH="${RUST_DASHCORE_PATH:-$PROJECT_ROOT/../rust-dashcore}".

Rust FFI Changes - Well-Structured but Significant

Good

  • Optional function pointer type aliases for cbindgen are the correct pattern. pub type OptionalHasPendingFn = Option<unsafe extern "C" fn(...)> compiles to nullable C function pointers properly, working around cbindgen's inability to resolve Option<TypeAlias>.
  • Signer callback redesign — adding a const void *signer context pointer and DestroyCallback is the right approach (non-global signer state).
  • cbindgen upgrade from 0.27 → 0.29.2 with removal of the old version is clean.

Issues

  1. dash_sdk_dpns_register_name uses const void* for typed handlesidentity, identity_public_key, and signer parameters are all const void* instead of their proper typed handles (IdentityHandle*, IdentityPublicKeyHandle*, SignerHandle*). This loses all C-level type safety and enables silent misuse.

  2. Dead type aliases in address_sync/provider.rsHasPendingFn, GetHighestFoundIndexFn, and DestroyProviderFn are still defined but unused (the VTable now uses the Optional* variants). Should be removed.

  3. unsafe impl Send for AddressProviderFFI — The safety depends entirely on the C caller providing thread-safe callbacks, since the Rust async runtime may move tasks between threads. The comment acknowledges this but it should be prominently documented for consumers.

  4. Duplicate # Safety block on dash_sdk_create_trusted in the generated header and test_header.h. The first block is a leftover.

  5. dash_sdk_test_identity_transfer_crash is still exposed as a public FFI function. This looks like a diagnostic function that shouldn't ship in release builds.


Breaking API Changes — Significant

This is labeled as a "build fix" but includes multiple ABI-breaking changes that warrant careful attention:

Change Impact
DashSDKNetwork enum renumbered (added SDKRegtest, shifted SDKDevnet 2→3, SDKLocal 3→4) Silent wrong-network behavior if old callers pass numeric values
DashSDKResultDataType_NoneDashSDKResultDataType_NoData Compile error for old callers (safe)
Signer callbacks gain const void *signer parameter Crash if old callbacks used without update
Document functions switch from handle-based to string-based IDs API redesign, not backwards compatible
CoreSDKHandle.client changed from FFIDashSpvClient* to void* Loses type safety
Entire dash_core_sdk_*, dash_key_*, dash_tx_* families removed Moved to dash_spv_ffi.h

The Swift SDK changes (6 wrapper files + SPVClient.swift + WalletService.swift + example WalletManager.swift) appear to cover the consumer-side updates, which is good. The PR description documents the breaking changes.


Cargo.lock — Unusual Windows-sys Downgrades

Multiple dependencies had their windows-sys version downgraded:

  • anstyle-wincon: 0.61.2 → 0.60.2
  • errno, is-terminal, rustix, tempfile: 0.61.2 → 0.52.0
  • mio: 0.61.2 → 0.59.0

This looks like a lockfile regeneration artifact rather than intentional. While irrelevant for iOS builds, it could affect Windows development environments. The heck crate consolidation (0.4.1 + 0.5.0 → just 0.5.0) is a clean improvement.


Summary

Strengths:

  • Build script fixes are correct and well-reasoned
  • Commit history is clean and atomic
  • cbindgen Optional workaround is the right pattern
  • Signer API redesign properly adds context pointers
  • PR description is thorough and documents breaking changes

Should Fix:

  • dash_sdk_dpns_register_name should use typed pointers, not void*
  • Remove dead type aliases (HasPendingFn, etc.)
  • Remove duplicate # Safety doc block on dash_sdk_create_trusted
  • Investigate the windows-sys downgrades in Cargo.lock

Should Consider:

  • RUST_DASHCORE_PATH defined once, optionally from env var
  • dash_sdk_test_identity_transfer_crash shouldn't be in release builds
  • Document the unsafe impl Send requirement for callback thread safety

Overall this is a well-executed PR that does more than the title suggests. The build fixes are solid and the FFI API evolution is reasonable, but the scope of breaking changes is significant and reviewers should verify all downstream Swift consumers are updated.

lklimek and others added 7 commits February 23, 2026 09:33
…tory

If a CI environment or developer sets CARGO_TARGET_DIR, the hardcoded
SPV_TARGET_DIR path silently diverges from where Cargo actually writes
artifacts. Use parameter expansion to respect the override while falling
back to the workspace root target directory.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e-aware

The fallback branch in the SPV lib selection could pick an arm64 SPV
library even when BUILD_ARCH=x86, if a stale arm64 build artifact
existed. Gate the arm64 fallback on BUILD_ARCH != x86 to prevent
merging mismatched architectures via libtool.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The FAT_SPV_SIM archive was only removed on the success path. If
libtool failed, the stale file was left behind. Clean it up in the
error path as well, using ${FAT_SPV_SIM:-} to safely handle the case
where the variable is unset (non-universal builds).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When addAccount threw, the catch block called wallet_free(handle)
manually, but ownsHandle was already true and deinit would free the
handle again—classic double-free. Since Swift calls deinit on a
partially-initialised object when a designated init throws (after all
stored properties are set), the manual free is unnecessary. Remove the
do/catch wrapper and let deinit handle cleanup.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Both the fullReset and partial code paths now call clearStorage(), so
the old "sync-state" vs "full storage" wording was inaccurate. Update
the log to reflect that storage is always cleared and only the
post-clear UI reset scope differs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
getReceiveAddress and getChangeAddress reuse a single FFIError across
three sequential FFI calls. If an FFI function ever set a non-null
error message on a successful return, the prior message would leak.
Reinitialise the error struct before each call to make the contract
explicit and prevent potential memory leaks.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…p networks

addWallet(networks:) and addWalletAndSerialize(networks:) accept an
array of KeyWalletNetwork but delegate to the single-network variant,
silently discarding extras. Add a runtime log when networks.count > 1
so callers get a visible diagnostic instead of silent data loss.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lklimek
Copy link
Contributor Author

lklimek commented Feb 23, 2026

Noe to self: plan to split this PR into two (fix+refactor) is plans/soft-stirring-plum.md.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants