test(swift-sdk): first swift sdk integration tests with local network#3712
test(swift-sdk): first swift sdk integration tests with local network#3712ZocoLini wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMigrates Swift SDK testing from many mock/unit tests to a devnet-backed integration test harness: deletes old test suites and headers, adds integration helpers and tests, updates package manifest and build scripts, and changes CI workflow to invoke the new runners. ChangesSwift SDK Integration Test Migration
Sequence DiagramsequenceDiagram
participant Developer as Dev / CI
participant CI as GitHub Actions
participant Runner as run_tests.sh
participant IntegrationRunner as run_integration_tests.sh
participant Dashmate as dashmate (local devnet)
participant Core as dashd (Core RPC)
participant SwiftTests as SwiftDashSDKIntegrationTests
CI->>Runner: run unit build & tests
Runner->>SwiftTests: swift build/test, xcodebuild test
CI->>IntegrationRunner: run integration flow
IntegrationRunner->>Dashmate: reset/start devnet
IntegrationRunner->>Core: Core RPC calls via CoreRPCClient (getnewaddress, sendtoaddress, generatetoaddress)
IntegrationRunner->>SwiftTests: RUN_INTEGRATION_TESTS=1 swift test SwiftDashSDKIntegrationTests
IntegrationRunner->>Dashmate: stop (always)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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
changed the title
ZocoLini
force-pushed
the
feat/swift-sdk-integration-tests
branch
5 times, most recently
from
e6cadad to
b16e06f
Compare
ZocoLini
force-pushed
the
feat/swift-sdk-integration-tests
branch
from
b16e06f to
361114b
Compare
ZocoLini
force-pushed
the
feat/swift-sdk-integration-tests
branch
from
361114b to
59ef0d4
Compare
|
✅ 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: "b12b8793f11e97a6829590cab1d11aa9e71a103876c4b44407b2d45c93ccdbb1"
)Xcode manual integration:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTypesTests.swift (1)
27-34: ⚡ Quick winMake odd-length hex behavior explicit and test exact expected output.
This test currently validates an implementation-dependent outcome with only
XCTAssertNotNil, which can hide parsing regressions. Please assert a deterministic contract for odd-length input (reject withnil, or accept with a specific byte result) and verify that exact result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTypesTests.swift` around lines 27 - 34, The test testDataFromOddLengthHexString currently only asserts non-nil for an implementation-dependent outcome; change it to assert a deterministic contract: decide whether Data(hexString:) should reject odd-length hex (preferred) or normalize it, then update the test to assert that exact behavior—for example, if you choose rejection, replace XCTAssertNotNil(data) with XCTAssertNil(data) and optionally assert a specific error/path; if you choose acceptance, assert the exact expected bytes (e.g., XCTAssertEqual(data, expectedData)). Ensure the assertion references the Data(hexString:) initializer and the test method name testDataFromOddLengthHexString so the behavior is explicit and reproducible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/swift-sdk/run_tests.sh`:
- Around line 11-17: The script run_tests.sh currently sets SIM_NAME by parsing
device display names which can be ambiguous; instead derive a SIM_UDID and use
-destination "id=$SIM_UDID". Update run_tests.sh to introduce a SIM_UDID
variable (e.g., SIM_UDID="${SIM_UDID:-}") and, when empty, run xcrun simctl list
devices available to pick the first matching iPhone device UDID (not the display
name). Replace uses of SIM_NAME/-destination "platform=iOS
Simulator,name=$SIM_NAME" with -destination "id=$SIM_UDID" so xcodebuild targets
the exact simulator UDID.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/CoreRPCClient.swift`:
- Around line 74-76: CoreRPCClient.call builds a URLRequest but doesn't set a
timeout, leaving the default (~60s idle) which can hang tests; set an explicit
timeoutInterval on the URLRequest (e.g., request.timeoutInterval = 30) before
using URLSession to bound RPC call time, and ensure any related helper paths in
CoreRPCClient.call respect this timeout when creating the request.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTypesTests.swift`:
- Around line 27-34: The test testDataFromOddLengthHexString currently only
asserts non-nil for an implementation-dependent outcome; change it to assert a
deterministic contract: decide whether Data(hexString:) should reject odd-length
hex (preferred) or normalize it, then update the test to assert that exact
behavior—for example, if you choose rejection, replace XCTAssertNotNil(data)
with XCTAssertNil(data) and optionally assert a specific error/path; if you
choose acceptance, assert the exact expected bytes (e.g., XCTAssertEqual(data,
expectedData)). Ensure the assertion references the Data(hexString:) initializer
and the test method name testDataFromOddLengthHexString so the behavior is
explicit and reproducible.
🪄 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: 0a0c245b-b203-4fe5-8d83-2191641ba8ff
📒 Files selected for processing (40)
.github/workflows/swift-sdk-build.yml.serena/project.ymlpackages/swift-sdk/Package.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/README.mdpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swiftpackages/swift-sdk/SwiftTests/Package.swiftpackages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDK.hpackages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDKMock.cpackages/swift-sdk/SwiftTests/SwiftDashSDK.hpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Platform/SDKBootstrapIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/CoreRPCClient.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestEnv.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/LocalDevnet.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/Shell.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/TestWallet.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityManagerTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTypesTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ContactRequestTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DataContractTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DocumentTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/EstablishedContactTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityManagerTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ManagedIdentityTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SwiftConstants.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/TokenTests.swiftpackages/swift-sdk/SwiftTests/run_tests.shpackages/swift-sdk/Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swiftpackages/swift-sdk/build_ios.shpackages/swift-sdk/example/SwiftSDKExample.swiftpackages/swift-sdk/run_integration_tests.shpackages/swift-sdk/run_tests.sh
💤 Files with no reviewable changes (21)
- packages/swift-sdk/Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swift
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDK.h
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SwiftConstants.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DocumentTests.swift
- packages/swift-sdk/example/SwiftSDKExample.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swift
- packages/swift-sdk/SwiftTests/Package.swift
- packages/swift-sdk/SwiftTests/SwiftDashSDK.h
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/EstablishedContactTests.swift
- packages/swift-sdk/SwiftTests/run_tests.sh
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDKMock.c
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ManagedIdentityTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityManagerTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ContactRequestTests.swift
- packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/TokenTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DataContractTests.swift
| # Pick a concrete iOS Simulator for the `xcodebuild test` run. A name | ||
| # can be pinned via `SIM_NAME`; otherwise grab the first available | ||
| SIM_NAME="${SIM_NAME:-}" | ||
| if [ -z "$SIM_NAME" ]; then | ||
| SIM_NAME="$(xcrun simctl list devices available \ | ||
| | grep -oE 'iPhone [0-9][^(]*' | head -1 | sed 's/ *$//')" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp)"
xcrun simctl list devices available > "$tmp"
echo "Current regex output from script:"
grep -oE 'iPhone [0-9][^(]*' "$tmp" | sed 's/[[:space:]]*$//' | sort -u || true
echo
echo "All available iPhone names:"
grep -oE 'iPhone [^()]+' "$tmp" | sed 's/[[:space:]]*$//' | sort -u || true
echo
echo "Duplicate iPhone names across runtimes (ambiguous for name=):"
grep -oE 'iPhone [^()]+' "$tmp" | sed 's/[[:space:]]*$//' | sort | uniq -cd || trueRepository: dashpay/platform
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' packages/swift-sdk/run_tests.sh | nl -baRepository: dashpay/platform
Length of output: 103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File: packages/swift-sdk/run_tests.sh =="
if command -v sed >/dev/null 2>&1; then
awk '{printf "%d\t%s\n", NR, $0}' packages/swift-sdk/run_tests.sh | sed -n '1,200p'
else
cat packages/swift-sdk/run_tests.sh
fi
echo
echo "== Grep SIM_NAME / destination =="
rg -n --no-heading 'SIM_NAME|destination .*name=|destination .*id=|xcodebuild test' packages/swift-sdk/run_tests.sh || trueRepository: dashpay/platform
Length of output: 1601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp)"
xcrun simctl list devices available > "$tmp"
echo "Current regex output (the script’s SIM_NAME fallback):"
grep -oE 'iPhone [0-9][^(]*' "$tmp" | sed 's/[[:space:]]*$//' | sort -u || true
echo
echo "iPhone device names with UDIDs:"
awk -F'[()]' '/iPhone/ {gsub(/^[[:space:]]+|[[:space:]]+$/, "", $1); print $1 "\t" $2}' "$tmp" \
| sort -k1,1 -k2,2 || true
echo
echo "Duplicate iPhone names across UDIDs (ambiguous for name=):"
awk -F'[()]' '/iPhone/ {gsub(/^[[:space:]]+|[[:space:]]+$/, "", $1); print $1}' "$tmp" \
| sort | uniq -c | awk '$1>1{print}' || trueRepository: dashpay/platform
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp)"
xcrun simctl list devices available > "$tmp"
echo "Current regex output from script:"
grep -oE 'iPhone [0-9][^(]*' "$tmp" | sed 's/[[:space:]]*$//' | sort -u || true
echo
echo "All available iPhone names:"
grep -oE 'iPhone [^()]+' "$tmp" | sed 's/[[:space:]]*$//' | sort -u || true
echo
echo "Duplicate iPhone names across runtimes (ambiguous for name=):"
grep -oE 'iPhone [^()]+' "$tmp" | sed 's/[[:space:]]*$//' | sort | uniq -cd || trueRepository: dashpay/platform
Length of output: 106
🌐 Web query:
xcrun simctl list devices available output format parentheses UUID field position
💡 Result:
The command xcrun simctl list devices available is not a documented/standard simctl subcommand form; simctl list supports listing devices, and “available” is typically expressed as filtering by state (e.g., excluding “unavailable”) or using the JSON output and filtering fields like isAvailable. [1][2] In the default (human-readable) output of xcrun simctl list devices, each available device line includes the UUID/UDID in parentheses immediately after the device name, before the trailing status parentheses such as (Shutdown). Example format: iPhone 12 (31891A98-684F-4C82-9767-EF09FD91F678) (Shutdown) [2][3] So, “UUID field position” in that output is: 1) Device name (e.g., iPhone 12) 2) A space 3) The UUID inside parentheses (e.g., (31891A98-684F-4C82-9767-EF09FD91F678)) [2][3] 4) A space 5) The status in parentheses (e.g., (Shutdown)) [2][3] If you want structured filtering and don’t care about “field position” in text, use xcrun simctl list devices --json (or -j) and filter by JSON keys like udid and isAvailable. Example JSON shows entries containing both udid and isAvailable. [4][1]
Citations:
- 1: https://stackoverflow.com/questions/58806843/how-to-get-list-of-available-ios-simulators
- 2: https://github.com/NSHipster/articles/blob/master/2018-11-26-simctl.md
- 3: https://www.andyibanez.com/posts/getting-to-know-the-simulator-better/
- 4: https://gist.github.com/erikkerber/630e00c1afebd3ef6881a60a22e2e15b
🌐 Web query:
xcrun simctl list devices --json support devices available
💡 Result:
The command xcrun simctl list devices --json is used to retrieve a structured, machine-readable list of all configured iOS, watchOS, tvOS, and visionOS simulators in JSON format [1][2][3]. Breakdown of the command: - xcrun: A utility that locates and executes developer tools (in this case, simctl) within the active Xcode installation [4][3]. - simctl: The command-line utility used to control the iOS Simulator [5][6]. - list devices: The subcommand that retrieves the list of simulator devices [7][5]. - --json: An optional flag that formats the output as a JSON object, which is highly useful for parsing in scripts or automated workflows [2][3]. Adding the available argument (e.g., xcrun simctl list devices --json available) filters the output to show only those simulators that are currently available for use on your system [2]. Usage examples: - To list all simulators: xcrun simctl list devices - To list all simulators in JSON format: xcrun simctl list devices --json - To list only available simulators in JSON: xcrun simctl list devices --json available If you encounter errors when running these commands, ensure that your command-line tools are correctly configured by running xcode-select -s /path/to/your/Xcode.app and verifying your installation via xcodebuild -runFirstLaunch [8][9].
Citations:
- 1: https://gist.github.com/MHabiib/d4dfe30aee4d87e27b168fdd49a456ea
- 2: https://stackoverflow.com/questions/58806843/how-to-get-list-of-available-ios-simulators
- 3: https://github.com/NSHipster/articles/blob/master/2018-11-26-simctl.md
- 4: https://dev.to/lanycrost/macos-workers-or-how-i-built-my-own-mac-cloud-bhm
- 5: https://suelan.github.io/2020/02/05/iOS-Simulator-from-the-Command-Line/
- 6: https://medium.com/xcblog/simctl-control-ios-simulators-from-command-line-78b9006a20dc
- 7: https://github.com/banghuazhao/Swift-iOS-CLI-Build-Cheat-Sheet/blob/main/xcrun.md
- 8: https://stackoverflow.com/questions/74784060/could-not-get-the-simulator-list-from-xcode-please-open-xcode-and-try-running-p
- 9: https://developer.apple.com/documentation/xcode/downloading-and-installing-additional-xcode-components
🌐 Web query:
xcodebuild -destination platform=iOS Simulator id= UDID example
💡 Result:
To use the xcodebuild -destination flag with a specific iOS Simulator identified by its unique identifier (UDID), you must provide the platform and the id in a comma-separated key-value pair string [1][2][3]. The correct syntax is: xcodebuild -destination 'platform=iOS Simulator,id=YOUR_SIMULATOR_UDID' [1][4] Example command: xcodebuild test -workspace MyApp.xcworkspace -scheme MyApp -destination 'platform=iOS Simulator,id=5BBA1401-0C74-47A7-8709-9F6C1D9C9CBB' [1][3] Key points to remember: 1. Mutually Exclusive: The destination specifier for an iOS Simulator allows you to use either an id or a name, but you should not provide both in the same specifier [1][5][3]. 2. Finding your UDID: If you are unsure of your simulator's UDID, you can run xcodebuild with an invalid destination (e.g., -destination 'platform=iOS Simulator') [1][6]. The resulting error message will list all available destinations for your scheme, including their respective names and UDIDs [1][6]. 3. Additional Parameters: While id is sufficient, you can optionally include other parameters such as OS version if needed (e.g., 'platform=iOS Simulator,id=...,OS=latest') [1][3]. Note that this flag is specifically for targeting a simulator or device; if omitted, xcodebuild may default to building for the macOS platform [3].
Citations:
- 1: https://mokacoding.com/blog/xcodebuild-destination-options/
- 2: https://keith.github.io/xcode-man-pages/xcodebuild.1.html
- 3: https://danfabulich.medium.com/xcodebuild-cli-cheat-sheet-b7ee7b3d5fc6
- 4: https://gist.github.com/mtauraso/95a49eeb8e5b4335dd2b?permalink_comment_id=3936803
- 5: https://developer.apple.com/forums/thread/764408
- 6: https://stackoverflow.com/questions/43989967/xcodebuild-error-option-destination-requires-at-least-one-parameter-of-the-f
🌐 Web query:
xcrun simctl list devices available output format parentheses UUID field position
💡 Result:
You can extract the UDID directly from the non-JSON output of xcrun simctl list devices by using the fact that each device line formats the UDID inside parentheses, right before the availability status parentheses. Example device line (from typical output): “iPad (5th generation) (1D9E3D9C-7715-4742-A9DC-6096BCE95B64) (Booted)”. The UDID is the substring in the first pair of parentheses and it appears before the second “(Booted)”/“(Shutdown)” parentheses.[1][2] 1) “available output format parentheses UUID field position” (string-field position) - If you split a line on parentheses as awk -F'[()]' and then print the field after the device name/space, the UDID is field $4 in the pattern “Device Name (UDID) (Status)”. This is because the parentheses content becomes separate fields: field order corresponds to: $3 = first parentheses content (UDID) only if there is no extra leading/trailing spaces; the StackOverflow answer explicitly suggests $4 for the general case and notes you may need to adjust.[2] Practical, more robust option (recommended): don’t depend on awk field numbers; instead regex-match the UDID. 2) Robust UDID extraction (regex) Use a UUID-matching regex against the “(Booted)” lines: xcrun simctl list devices | grep "(Booted)" | grep -E -o -i "([0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12})" This returns only the UDID(s) for booted simulators.[2] 3) JSON alternative (if you want “field” clearly defined) xcrun simctl list --json / xcrun simctl list devices --json structures include a udid field in each device object.[3][4] This avoids parsing the parentheses-formatted plain text. In short: - Plain text format: “Device Name (UDID) (Status)”, where UDID is inside the first parentheses and appears immediately before the “(Booted)/(Shutdown)” parentheses.[1][2] - For field position in awk: the UDID corresponds to the parentheses content; one common approach prints $4 with awk -F'[()]' for lines containing “(Booted)”, but you might need to adjust the field number depending on exact spacing.[2] - Best practice: regex-extract the UUID from the “(Booted)” lines, or use --json and read the udid field.[2][3]
Citations:
- 1: https://suelan.github.io/2020/02/05/iOS-Simulator-from-the-Command-Line/
- 2: https://stackoverflow.com/questions/55787980/is-there-anyway-to-get-the-udid-of-a-booted-simulator-in-mac-via-terminal
- 3: Unable to find iOS Simulators (Post Xcode 10.2 install) Carthage/Carthage#2691
- 4: https://stackoverflow.com/questions/58806843/how-to-get-list-of-available-ios-simulators
🌐 Web query:
xcrun simctl list devices --json support devices available
💡 Result:
To list Simulator devices in JSON (including availability), use: xcrun simctl list devices --json [1] This command produces a JSON object that includes per-device fields such as state and isAvailable, which you can use to filter for devices that are available (i.e., “usable” for launching). One common pattern is to parse the output with jq and select entries where isAvailable is true [2]. Example (macOS with jq): xcrun simctl list devices --json | jq '.devices | to_entries[] |.value[] | select(.isAvailable==true) | {name, udid, state}' [2] Notes: - “--json or -j” is the option for JSON output for simctl list [1][3]. - If you want all runtimes/devices structure, you can also run xcrun simctl list --json, which outputs JSON for the full list (device types, runtimes, devices, etc.) [3].
Citations:
- 1: https://www.iosdev.recipes/simctl/
- 2: https://stackoverflow.com/questions/58806843/how-to-get-list-of-available-ios-simulators
- 3: https://github.com/NSHipster/articles/blob/master/2018-11-26-simctl.md
🌐 Web query:
xcodebuild -destination platform=iOS Simulator id= UDID example
💡 Result:
When using xcodebuild, the -destination argument accepts a destination specifier, which is a comma-separated list of key=value pairs [1][2][3]. To target a specific iOS Simulator by its unique identifier (UDID), use the id key within the destination specifier [1][4][5]. The basic syntax for targeting a simulator via UDID is [1][3]: xcodebuild -destination 'platform=iOS Simulator,id=YOUR_SIMULATOR_UDID' [command] Replace YOUR_SIMULATOR_UDID with the actual UUID of the simulator (e.g., 5BBA1401-0C74-47A7-8709-9F6C1D9C9CBB) [1][5]. Key points regarding this usage: - Mutually Exclusive: The id and name keys are mutually exclusive; you must provide one or the other, but not both [1][4][2]. - Additional Keys: When using platform=iOS Simulator, you can also include an optional OS key to specify the runtime version (e.g., OS=latest or OS=18.6) [1][3]. - Finding UDIDs: You can list available simulators and their corresponding UDIDs by running the command xcrun simctl list devices [5]. - Error Handling: If you provide an incorrect specifier, xcodebuild will typically fail and output a list of valid destination specifiers for your current project, which can help you verify the correct ID and platform string [1][6].
Citations:
- 1: https://mokacoding.com/blog/xcodebuild-destination-options/
- 2: https://keith.github.io/xcode-man-pages/xcodebuild.1.html
- 3: https://danfabulich.medium.com/xcodebuild-cli-cheat-sheet-b7ee7b3d5fc6
- 4: https://developer.apple.com/forums/thread/764408
- 5: https://gist.github.com/mtauraso/95a49eeb8e5b4335dd2b?permalink_comment_id=3936803
- 6: https://stackoverflow.com/questions/43989967/xcodebuild-error-option-destination-requires-at-least-one-parameter-of-the-f
🏁 Script executed:
#!/bin/bash
set -euo pipefail
xcrun simctl list devices --json > /tmp/simctl_devices.json
python3 - <<'PY'
import json, re
data=json.load(open('/tmp/simctl_devices.json'))
name_to_udids={}
for runtime, devs in data.get('devices', {}).items():
for d in devs:
if not d.get('isAvailable', False):
continue
name=d.get('name','')
udid=d.get('udid','')
if name.startswith('iPhone '):
name_to_udids.setdefault(name,set()).add(udid)
dupes={k:v for k,v in name_to_udids.items() if len(v)>1}
print("Ambiguous iPhone display names (multiple UDIDs):")
for k in sorted(dupes):
print(f"- {k}: {len(dupes[k])} UDIDs")
rx=re.compile(r"^iPhone [0-9]")
lost=[k for k in name_to_udids.keys() if not rx.match(k)]
print("\niPhone names not matching current fallback regex (would be missed by SIM_NAME fallback):")
for k in sorted(lost):
print(f"- {k}")
PYRepository: dashpay/platform
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp)"
xcrun simctl list devices available > "$tmp"
echo "Current regex output from script:"
grep -oE 'iPhone [0-9][^(]*' "$tmp" | sed 's/[[:space:]]*$//' | sort -u || true
echo
echo "All available iPhone names:"
grep -oE 'iPhone [^()]+' "$tmp" | sed 's/[[:space:]]*$//' | sort -u || true
echo
echo "Duplicate iPhone names across runtimes (ambiguous for name=):"
grep -oE 'iPhone [^()]+' "$tmp" | sed 's/[[:space:]]*$//' | sort | uniq -cd || trueRepository: dashpay/platform
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp)"
xcrun simctl list devices available > "$tmp"
echo "Current regex output from script:"
grep -oE 'iPhone [0-9][^(]*' "$tmp" | sed 's/[[:space:]]*$//' | sort -u || true
echo
echo "All available iPhone names:"
grep -oE 'iPhone [^()]+' "$tmp" | sed 's/[[:space:]]*$//' | sort -u || true
echo
echo "Duplicate iPhone names across runtimes (ambiguous for name=):"
grep -oE 'iPhone [^()]+' "$tmp" | sed 's/[[:space:]]*$//' | sort | uniq -cd || trueRepository: dashpay/platform
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp)"
xcrun simctl list devices --json > "$tmp"
python3 - <<'PY' "$tmp"
import json, re, collections, sys
data = json.load(open(sys.argv[1]))
rx = re.compile(r"^iPhone [0-9]") # matches current SIM_NAME fallback
name_to_udids = collections.defaultdict(set)
for runtime, devs in data.get("devices", {}).items():
for d in devs:
if not d.get("isAvailable", False):
continue
name = d.get("name", "")
udid = d.get("udid", "")
if name.startswith("iPhone "):
name_to_udids[name].add(udid)
ambiguous = {k:v for k,v in name_to_udids.items() if len(v) > 1}
print("Duplicate iPhone display names across available UDIDs (ambiguous for name=):")
for name in sorted(ambiguous):
print(f"- {name}: {len(ambiguous[name])} UDIDs")
missed_by_regex = sorted([k for k in name_to_udids if not rx.match(k)])
print("\nIPhone names that current fallback regex would miss:")
for name in missed_by_regex[:50]:
print(f"- {name}")
if len(missed_by_regex) > 50:
print(f"... and {len(missed_by_regex)-50} more")
PYRepository: dashpay/platform
Length of output: 106
Select iOS Simulator by UDID (id=) instead of parsed name=
packages/swift-sdk/run_tests.sh derives SIM_NAME via grep -oE 'iPhone [0-9][^(]*' (numeric-only, can miss valid iPhone simulator names), then passes -destination "platform=iOS Simulator,name=$SIM_NAME", which is prone to ambiguity across multiple runtimes/simulators with the same display name.
Proposed fix
# Pick a concrete iOS Simulator for the `xcodebuild test` run. A name
-# can be pinned via `SIM_NAME`; otherwise grab the first available
+# can be pinned via `SIM_NAME`; otherwise grab the first available iPhone UDID.
SIM_NAME="${SIM_NAME:-}"
-if [ -z "$SIM_NAME" ]; then
- SIM_NAME="$(xcrun simctl list devices available \
- | grep -oE 'iPhone [0-9][^(]*' | head -1 | sed 's/ *$//')"
-fi
-if [ -z "$SIM_NAME" ]; then
+SIM_UDID=""
+if [ -n "$SIM_NAME" ]; then
+ SIM_UDID="$(xcrun simctl list devices available \
+ | awk -F '[()]' -v target="$SIM_NAME" '
+ /iPhone/ {
+ name=$1; gsub(/^[[:space:]]+|[[:space:]]+$/, "", name);
+ if (name == target) { print $2; exit }
+ }')"
+else
+ SIM_UDID="$(xcrun simctl list devices available \
+ | awk -F '[()]' '/iPhone/ {print $2; exit}')"
+fi
+if [ -z "$SIM_UDID" ]; then
echo "No available iPhone simulator found for the simulator test run" >&2
exit 1
fi
@@
xcodebuild test \
-project SwiftExampleApp/SwiftExampleApp.xcodeproj \
-scheme SwiftExampleApp \
-skip-testing:SwiftExampleAppUITests \
- -destination "platform=iOS Simulator,name=$SIM_NAME"
+ -destination "platform=iOS Simulator,id=$SIM_UDID"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/swift-sdk/run_tests.sh` around lines 11 - 17, The script
run_tests.sh currently sets SIM_NAME by parsing device display names which can
be ambiguous; instead derive a SIM_UDID and use -destination "id=$SIM_UDID".
Update run_tests.sh to introduce a SIM_UDID variable (e.g.,
SIM_UDID="${SIM_UDID:-}") and, when empty, run xcrun simctl list devices
available to pick the first matching iPhone device UDID (not the display name).
Replace uses of SIM_NAME/-destination "platform=iOS Simulator,name=$SIM_NAME"
with -destination "id=$SIM_UDID" so xcodebuild targets the exact simulator UDID.
| var request = URLRequest(url: url) | ||
| request.httpMethod = "POST" | ||
| request.setValue("application/json", forHTTPHeaderField: "Content-Type") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the default timeout behaviors of URLSession.shared and URLRequest.timeoutInterval in Swift Foundation, and best practices for setting explicit timeouts for JSON-RPC calls?
💡 Result:
In Swift Foundation, URLSession and URLRequest manage timeouts primarily through an idle timer mechanism [1][2]. Default Timeout Behaviors: - URLRequest.timeoutInterval: This property has a default value of 60 seconds [3][4]. It represents the maximum idle time allowed between receiving packets of data [2][4]. The timer resets whenever new data is received [1][4]. If the interval passes without new data, the request times out [1][2]. - URLSessionConfiguration.timeoutIntervalForRequest: When using a custom configuration, this property similarly defaults to 60 seconds [1][5]. It dictates the default timeout behavior for all tasks created within that session [1][5]. - URLSessionConfiguration.timeoutIntervalForResource: This property defaults to 7 days [2]. It defines the maximum total duration allowed for a resource to be retrieved, regardless of idle periods [2]. Best Practices for JSON-RPC Timeouts: - Use Explicit URLRequest Timeouts: While session configurations set defaults, explicitly set the timeoutInterval on each URLRequest object [6]. This ensures your JSON-RPC calls adhere to the specific latency requirements of your API endpoints [6]. - Avoid Overly Aggressive Timeouts: Do not set timeouts too low, as this can cause frequent failures on high-latency or unstable cellular networks [3][7]. - Distinguish Connection vs. Application Logic: Remember that these properties are "idle" timeouts [2][7]. If a server is slow but continuously streaming data, these timeouts will not trigger [1][2]. For strict total-execution deadlines, implement application-level timeout logic (e.g., using Swift Concurrency Task cancellation) alongside URLSession timeouts [8][9]. - Implement Infrastructure Design: Treat timeouts as a core component of your network infrastructure [10]. Define consistent boundaries for different types of calls (e.g., fast API calls vs. long-running operations) and ensure appropriate error handling and retry mechanisms are in place [8][10].
Citations:
- 1: https://developer.apple.com/documentation/foundation/urlsessionconfiguration/timeoutintervalforrequest?language=objc
- 2: https://stackoverflow.com/questions/46084747/nsurlsession-sharedsession-default-timeout
- 3: https://developer.apple.com/documentation/foundation/urlrequest/timeoutinterval
- 4: https://fuchsia.googlesource.com/third_party/swift-corelibs-foundation/+/refs/tags/swift-4.0-DEVELOPMENT-SNAPSHOT-2017-11-08-a/Foundation/URLRequest.swift
- 5: https://developer.apple.com/documentation/foundation/urlsessionconfiguration/timeoutintervalforrequest
- 6: https://stackoverflow.com/questions/23428793/nsurlsession-how-to-increase-time-out-for-url-requests
- 7: https://stackoverflow.com/questions/43760007/urlsession-is-not-getting-timeout-after-specified-interval-in-swift-3-on-ios-10
- 8: https://dev.to/mukundakatta/per-tool-timeouts-stop-one-slow-api-from-hanging-your-agent-548n
- 9: https://dev.to/dtisch/how-i-implemented-ping-and-traceroute-on-ios-without-entitlements-2fm
- 10: https://dev.to/jayesh_pamnani_4ecb1c7338/why-timeout-handling-matters-more-than-most-backend-logic-2me9
Add explicit timeout for JSON-RPC requests
CoreRPCClient.call builds a URLRequest without setting a timeout; Foundation’s default URLRequest.timeoutInterval / URLSessionConfiguration.timeoutIntervalForRequest is an ~60s idle timeout, which can still cause long hangs when the devnet/network is unhealthy. Set a shorter explicit request.timeoutInterval (e.g., 30s) to bound integration test runtime.
Suggested fix
func call<T: Decodable>(_ method: String, params: [Param] = []) async throws -> T {
var request = URLRequest(url: url)
request.httpMethod = "POST"
request.setValue("application/json", forHTTPHeaderField: "Content-Type")
+ request.timeoutInterval = 30📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var request = URLRequest(url: url) | |
| request.httpMethod = "POST" | |
| request.setValue("application/json", forHTTPHeaderField: "Content-Type") | |
| var request = URLRequest(url: url) | |
| request.httpMethod = "POST" | |
| request.setValue("application/json", forHTTPHeaderField: "Content-Type") | |
| request.timeoutInterval = 30 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/CoreRPCClient.swift`
around lines 74 - 76, CoreRPCClient.call builds a URLRequest but doesn't set a
timeout, leaving the default (~60s idle) which can hang tests; set an explicit
timeoutInterval on the URLRequest (e.g., request.timeoutInterval = 30) before
using URLSession to bound RPC call time, and ensure any related helper paths in
CoreRPCClient.call respect this timeout when creating the request.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR adds a Swift SDK integration-test framework against a local dashmate devnet. Validation confirmed: the shared SwiftData container makes the global transaction-count assertion brittle, the missing README pointer is real, the DASHMATE_CONFIG env var is read by the test binary but ignored by the lifecycle/probe in the runner, and ensureSPVStarted/trackedWalletIds have unsynchronized check-then-act on nonisolated(unsafe) state. No in-scope blocker; recommending COMMENT.
🟡 3 suggestion(s) | 💬 3 nitpick(s)
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Net loss of Swift unit-test coverage from mock-based suite removal — The PR deletes ~2500 lines of older mock-based unit tests (Document/Identity/MemoryManagement/Token/ContactRequest/DataContract/EstablishedContact/ManagedIdentity/SDK/WalletSerialization) and replaces them with ~140 lines of trivial getter/error-path coverage. The deletions are justified — the mocks didn't reflect the real FFI — but the replacement is much thinner. Out of scope because recreating that coverage against real FFI is a substantial separate effort.
- Follow-up: Open a follow-up issue enumerating behaviors the deleted unit tests covered (document CRUD, opaque-handle memory management, token ops) and decide which deserve real-FFI unit or integration coverage.
- Integration suite is skipped in CI by design — can rot silently —
run_tests.shdoes not setRUN_INTEGRATION_TESTS=1, so the new integration tests all hitXCTSkipUnlessin CI. This is the intended split (CI runs unit + xcodebuild example app; dashmate-dependent suite runs locally), but it means the framework itself is never exercised in CI and regressions can land unnoticed.- Follow-up: Track adding a self-hosted/nightly job that provisions dashmate and runs
run_integration_tests.sh.
- Follow-up: Track adding a self-hosted/nightly job that provisions dashmate and runs
| try await MainActor.run { | ||
| let context = ModelContext(container) | ||
| let allTxCount = try context.fetchCount(FetchDescriptor<PersistentTransaction>()) | ||
| XCTAssertEqual(allTxCount, expectedTotalTxs) | ||
|
|
||
| let aliceTxoCount = try context.fetchCount(FetchDescriptor<PersistentTxo>( | ||
| predicate: #Predicate<PersistentTxo>{ | ||
| $0.walletId == aliceWalletId | ||
| } | ||
| )) | ||
| let bobTxoCount = try context.fetchCount(FetchDescriptor<PersistentTxo>( | ||
| predicate: #Predicate<PersistentTxo>{ | ||
| $0.walletId == bobWalletId | ||
| } | ||
| )) | ||
|
|
||
| XCTAssertEqual(aliceTxoCount, 1 + iterations) | ||
| XCTAssertEqual(bobTxoCount, 1 + iterations) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Global PersistentTransaction count is coupled to a process-shared SwiftData store
IntegrationTestCase.sharedEnv() memoizes a single IntegrationTestEnv for the whole process, and IntegrationTestEnv owns one in-memory ModelContainer reused across every test (only Keychain mnemonics get cleaned up in tearDown). The assertion at line 61 runs an unfiltered fetchCount(FetchDescriptor<PersistentTransaction>()) == 2 + iterations over that shared store. Today only one SPV test exists so it happens to pass, but the moment a second SPV-using test (or a re-run within the same process) lands, prior transactions persist and this assertion flips to a confusing failure even when wallet-to-wallet send still works. The framework is explicitly meant to host more tests — fix the coupling now. Either filter PersistentTransaction by walletId (the way the PersistentTxo queries below already do) or hand each test a fresh ModelContainer from IntegrationTestEnv.
source: ['claude', 'codex']
| DASHMATE_CONFIG="${DASHMATE_CONFIG:-local_seed}" | ||
|
|
||
| cd "$SCRIPT_DIR" | ||
|
|
||
| node_major=$(node -p "process.versions.node.split('.')[0]" 2>/dev/null || echo 0) | ||
| if [ "$node_major" -gt 22 ] || [ "$node_major" -lt 20 ]; then | ||
| echo "Node $(node --version 2>/dev/null || echo '<missing>') is not supported by dashmate." | ||
| echo "Use Node 20–22 (e.g. \`nvm use 22\` or \`brew link node@22\`) and re-run." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! ls -d "$REPO_ROOT"/.yarn/unplugged/systeminformation-* >/dev/null 2>&1; then | ||
| echo "yarn install (materialising unplugged packages)…" | ||
| (cd "$REPO_ROOT" && yarn install) | ||
| fi | ||
| if [ ! -f "$REPO_ROOT/packages/wasm-dpp/dist/index.js" ]; then | ||
| # Apple's bundled clang lacks the wasm32 target; cc-rs picks it up | ||
| # via `CC_wasm32_unknown_unknown` (not `CC`, which only affects the | ||
| # host build). Wire in Homebrew's LLVM automatically when present. | ||
| if [ -z "${CC_wasm32_unknown_unknown:-}" ] && command -v brew >/dev/null 2>&1; then | ||
| brew_llvm="$(brew --prefix llvm 2>/dev/null || true)" | ||
| if [ -x "$brew_llvm/bin/clang" ]; then | ||
| export CC_wasm32_unknown_unknown="$brew_llvm/bin/clang" | ||
| export AR_wasm32_unknown_unknown="$brew_llvm/bin/llvm-ar" | ||
| fi | ||
| fi | ||
| echo "yarn build (~few min — builds wasm-dpp etc.)…" | ||
| (cd "$REPO_ROOT" && yarn build) | ||
| if [ ! -f "$REPO_ROOT/packages/wasm-dpp/dist/index.js" ]; then | ||
| echo "yarn build finished but wasm-dpp/dist is still missing." | ||
| echo " Likely a wasm32 toolchain issue. If Homebrew's LLVM isn't" | ||
| echo " installed, run \`brew install llvm\` and try again." | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| DAPI_PORT="${DAPI_PORT:-2443}" | ||
| dapi_reachable() { | ||
| nc -z 127.0.0.1 "$DAPI_PORT" >/dev/null 2>&1 | ||
| } | ||
|
|
||
| if dapi_reachable; then | ||
| echo "dashmate devnet already running" | ||
| else | ||
| echo "starting dashmate devnet group (this can take ~1-2 min)…" | ||
| (cd "$REPO_ROOT" && yarn dashmate group stop --group=local 2>/dev/null || true) | ||
| (cd "$REPO_ROOT" && yarn dashmate group start --group=local --wait-for-readiness) | ||
| fi |
There was a problem hiding this comment.
🟡 Suggestion: DASHMATE_CONFIG only flows into the test binary, not into the stack the runner manages
The test binary reads DASHMATE_CONFIG (defaulting to local_seed) in IntegrationTestEnv.bootstrap() to drive LocalDevnet endpoint discovery, but the runner hard-codes DAPI_PORT=2443 and dashmate group {stop,start} --group=local. A user who sets DASHMATE_CONFIG=something_else (or who has a non-default DAPI port in local_seed) will have the script probing/starting the wrong stack while the tests query the configured one. Default usage works, but the env var is misleading as-is. Drive DAPI_PORT from yarn dashmate config get platform.gateway.listeners.dapiAndDrive.port --config="$DASHMATE_CONFIG" and expose a DASHMATE_GROUP (defaulting to local) for the group commands, so the script and the tests target the same stack.
source: ['codex']
| nonisolated(unsafe) private var spvStarted = false | ||
| nonisolated(unsafe) private var trackedWalletIds: [Data] = [] | ||
|
|
||
| private init( | ||
| endpoints: LocalDevnet.Endpoints, | ||
| coreRPC: CoreRPCClient, | ||
| sdk: SDK, | ||
| modelContainer: ModelContainer, | ||
| walletManager: PlatformWalletManager, | ||
| mineRewardAddress: String | ||
| ) { | ||
| self.endpoints = endpoints | ||
| self.coreRPC = coreRPC | ||
| self.sdk = sdk | ||
| self.modelContainer = modelContainer | ||
| self.walletManager = walletManager | ||
| self.mineRewardAddress = mineRewardAddress | ||
| } | ||
|
|
||
| /// Build the env from scratch. `IntegrationTestCase` is responsible | ||
| /// for memoising the result across tests. Assumes dashmate is | ||
| /// already up, `run_integration_tests.sh` handles bringing the | ||
| /// stack online before the test binary runs. | ||
| static func bootstrap() async throws -> IntegrationTestEnv { | ||
| let repoRoot = try locateRepoRoot() | ||
| let configName = ProcessInfo.processInfo.environment["DASHMATE_CONFIG"] ?? "local_seed" | ||
| let devnet = LocalDevnet(repoRoot: repoRoot, configName: configName) | ||
|
|
||
| let endpoints = try devnet.discoverEndpoints() | ||
| let coreRPC = CoreRPCClient( | ||
| port: endpoints.coreRPC, | ||
| username: endpoints.rpcUsername, | ||
| password: endpoints.rpcPassword | ||
| ) | ||
|
|
||
| // The SDK reads `platformDAPIAddresses` from UserDefaults to | ||
| // override its built-in regtest default of 127.0.0.1:2443 | ||
| // (matches the SwiftExampleApp wiring). | ||
| UserDefaults.standard.set( | ||
| "http://127.0.0.1:\(endpoints.platformDAPI)", | ||
| forKey: "platformDAPIAddresses" | ||
| ) | ||
| let sdk = try SDK(network: .regtest) | ||
| let modelContainer = try DashModelContainer.createInMemory() | ||
| // `PlatformWalletManager` is `@MainActor`-isolated. | ||
| let walletManager = try await MainActor.run { | ||
| try PlatformWalletManager(sdk: sdk, modelContainer: modelContainer) | ||
| } | ||
|
|
||
| let mineReward = try await coreRPC.getNewAddress() | ||
|
|
||
| return IntegrationTestEnv( | ||
| endpoints: endpoints, | ||
| coreRPC: coreRPC, | ||
| sdk: sdk, | ||
| modelContainer: modelContainer, | ||
| walletManager: walletManager, | ||
| mineRewardAddress: mineReward | ||
| ) | ||
| } | ||
|
|
||
| /// Start SPV pinned to the devnet's P2P port. Idempotent. | ||
| func ensureSPVStarted() async throws { | ||
| if spvStarted { return } | ||
| let dir = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("swift-sdk-it-spv-\(UUID().uuidString.prefix(8))", isDirectory: true) | ||
| try? FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) | ||
|
|
||
| let manager = walletManager | ||
| let config = PlatformSpvStartConfig( | ||
| dataDir: dir.path, | ||
| network: .regtest, | ||
| userAgent: "swift-sdk-integration-tests/0.1", | ||
| peers: ["127.0.0.1:\(endpoints.coreP2P)"], | ||
| restrictToConfiguredPeers: true, | ||
| startFromHeight: 0 | ||
| ) | ||
| try await MainActor.run { try manager.startSpv(config: config) } | ||
| spvStarted = true | ||
| } | ||
|
|
||
| /// Each call generates a fresh random mnemonic. | ||
| /// The mnemonic is also persisted to Keychain | ||
| func makeTestWallet(name: String? = nil) async throws -> TestWalletWrapper { | ||
| let mnemonic = try Mnemonic.generate(wordCount: 24) | ||
| let manager = walletManager | ||
| let wallet = try await MainActor.run { | ||
| try manager.createWallet( | ||
| mnemonic: mnemonic, | ||
| network: .regtest, | ||
| name: name, | ||
| createDefaultAccounts: true | ||
| ) | ||
| } | ||
| try WalletStorage().storeMnemonic(mnemonic, for: wallet.walletId) | ||
| trackedWalletIds.append(wallet.walletId) |
There was a problem hiding this comment.
🟡 Suggestion: ensureSPVStarted and trackedWalletIds have check-then-act races on nonisolated(unsafe) state
spvStarted and trackedWalletIds are declared nonisolated(unsafe). ensureSPVStarted reads spvStarted, hops to MainActor to call startSpv, then writes it; two concurrent callers can both pass the guard and try to start SPV twice (each with a unique temp dir) against the same wallet manager. makeTestWallet similarly appends to trackedWalletIds without synchronization. XCTest runs tests serially by default so the current single SPV test is safe, but swift test --parallel and test plans are supported and this will bite once a second test enters either path concurrently. Either serialize through an actor / DispatchQueue, or document that parallel execution is unsupported for this target.
source: ['claude']
| # See SwiftTests/SwiftDashSDKIntegrationTests/README.md for one-time setup | ||
| # (`yarn dashmate setup --preset local`). |
There was a problem hiding this comment.
💬 Nitpick: Referenced README does not exist
The header comment points users at SwiftTests/SwiftDashSDKIntegrationTests/README.md for one-time setup, but that directory contains only Core/, Platform/, and Support/ — no README. First-time users hitting dashmate config get failures will follow the pointer and find nothing. Either inline the yarn dashmate setup --preset local instruction into the script's failure message or add the README the comment promises.
source: ['claude']
| /// Placeholder until real Platform-side integration coverage lands | ||
| final class SDKBootstrapIntegrationTests: IntegrationTestCase { | ||
| func testSDKInitializes() { | ||
| _ = env.sdk | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Bootstrap test does not exercise the local DAPI endpoint
testSDKInitializes only references env.sdk, which was already constructed during suite bootstrap, so a broken local DAPI/gateway would still leave this test green. The file's own comment labels it a placeholder, so this is acknowledged — but as long as the file exists in the suite, one cheap call (e.g. try env.sdk.getStatus()) would make it actually validate the Platform-side path it's meant to guard.
source: ['codex']
| if let timeout { | ||
| let deadline = Date().addingTimeInterval(timeout) | ||
| while process.isRunning && Date() < deadline { | ||
| Thread.sleep(forTimeInterval: 0.05) | ||
| } | ||
| if process.isRunning { | ||
| process.terminate() | ||
| Thread.sleep(forTimeInterval: 0.5) | ||
| if process.isRunning { kill(process.processIdentifier, SIGKILL) } | ||
| } | ||
| } | ||
|
|
||
| process.waitUntilExit() |
There was a problem hiding this comment.
💬 Nitpick: Shell.run blocks a concurrency thread with Thread.sleep + waitUntilExit
Shell.run is invoked transitively from IntegrationTestEnv.bootstrap() (an async throws function) via LocalDevnet.readConfigRaw. The timeout loop uses Thread.sleep(0.05) and a synchronous process.waitUntilExit(), blocking a Swift Concurrency cooperative worker for up to the full timeout. Fine for one-time bootstrap; will starve the pool if Shell is later called from inside test bodies. Consider wrapping with Process.terminationHandler + withCheckedContinuation.
source: ['claude']
ZocoLini
force-pushed
the
feat/swift-sdk-integration-tests
branch
from
59ef0d4 to
ea8c626
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/swift-sdk-build.yml:
- Around line 95-98: The workflow currently disables failure handling with "set
+e" and forces success with a trailing "true", causing failures in "yarn
dashmate group stop --group=local --force" and "yarn dashmate group reset
--group=local --hard --verbose" to be ignored; restore strict error handling by
removing "set +e" and the final "true" (or re-enable with "set -e" after the
block) so any non-zero exit from those yarn dashmate commands fails the job, or
explicitly propagate errors by appending "|| exit 1" to the reset command.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swift`:
- Line 9: The shared cached variable bootstrapResult in IntegrationTestCase is
accessed from nonisolated(unsafe) context in sharedEnv() and can race; protect
all reads/writes to bootstrapResult by introducing synchronization (e.g., a
private static serial DispatchQueue or an actor) and marshal all accesses
through it inside sharedEnv() and any other accessors; update sharedEnv() to
perform the check/read/write of bootstrapResult on that queue (or via await if
using an actor) so initialization is atomic and thread-safe while keeping the
existing API.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestEnv.swift`:
- Around line 18-20: IntegrationTestEnv currently exposes nonisolated(unsafe)
shared mutable state via spvStarted and trackedWalletIds and performs an
unsynchronized check/set in ensureSPVStarted(), causing race conditions; fix by
moving these vars into a synchronized context (either make IntegrationTestEnv an
actor or add a private serial synchronization primitive such as a dedicated
DispatchQueue or Lock) and update all accessors (ensureSPVStarted(), any
append/iterate/clear operations on trackedWalletIds) to run on that
synchronization context so the check-and-set in ensureSPVStarted() is atomic (no
await between check and set) and all mutations/read of trackedWalletIds are
serialized.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/Shell.swift`:
- Around line 74-77: The code currently calls process.waitUntilExit() before
reading stdout/stderr which can deadlock; change Shell.run (or the function
creating Process and using stdoutPipe/stderrPipe) to read the pipes
asynchronously: set stdoutPipe.fileHandleForReading.readabilityHandler and
stderrPipe.fileHandleForReading.readabilityHandler to append incoming Data into
local Data buffers, then call process.waitUntilExit(), then clear the
readabilityHandlers and use the accumulated Data (instead of calling
readDataToEndOfFile() after waitUntilExit()). Ensure handlers are removed (set
to nil) to avoid leaks and convert the collected Data to strings as before.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/TestWallet.swift`:
- Around line 44-49: The loop currently checks the deadline before calling
condition(), which can suppress an exception thrown by condition() on the final
attempt; change the loop in the waiting helper (the block that creates deadline,
calls try await condition(), sleeps with Task.sleep using pollInterval, and
throws TimeoutError) so you call try await condition() first, return on success,
then if Date() >= deadline throw the TimeoutError, and only then await
Task.sleep; this ensures any error from condition() on the final attempt is
propagated instead of being replaced by the TimeoutError.
🪄 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: 44a6106d-d8be-4e58-af49-1a307003d13a
📒 Files selected for processing (11)
.github/workflows/swift-sdk-build.ymlpackages/swift-sdk/Package.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Platform/SDKBootstrapIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/CoreRPCClient.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestEnv.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/LocalDevnet.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/Shell.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/TestWallet.swiftpackages/swift-sdk/run_integration_tests.sh
| set +e | ||
| yarn dashmate group stop --group=local --force | ||
| yarn dashmate group reset --group=local --hard --verbose | ||
| true |
There was a problem hiding this comment.
Fail fast when devnet reset fails.
Line 95 disables error handling and Line 98 forces success, so reset failures are silently ignored and integration tests can run against stale state.
Proposed fix
- set +e
- yarn dashmate group stop --group=local --force
- yarn dashmate group reset --group=local --hard --verbose
- true
+ yarn dashmate group stop --group=local --force || true
+ yarn dashmate group reset --group=local --hard --verbose📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set +e | |
| yarn dashmate group stop --group=local --force | |
| yarn dashmate group reset --group=local --hard --verbose | |
| true | |
| yarn dashmate group stop --group=local --force || true | |
| yarn dashmate group reset --group=local --hard --verbose |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/swift-sdk-build.yml around lines 95 - 98, The workflow
currently disables failure handling with "set +e" and forces success with a
trailing "true", causing failures in "yarn dashmate group stop --group=local
--force" and "yarn dashmate group reset --group=local --hard --verbose" to be
ignored; restore strict error handling by removing "set +e" and the final "true"
(or re-enable with "set -e" after the block) so any non-zero exit from those
yarn dashmate commands fails the job, or explicitly propagate errors by
appending "|| exit 1" to the reset command.
|
|
||
| // XCTest serialises class-level setUps; only one path touches | ||
| // this latch. | ||
| nonisolated(unsafe) private static var bootstrapResult: Result<IntegrationTestEnv, Error>? |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect test runner parallelization flags:"
rg -n --iglob '*run*test*.sh' --iglob '*.yml' 'swift test|xcodebuild test|--parallel|--num-workers|parallel-testing-enabled'
echo
echo "Inspect unsafe shared bootstrap state usage:"
rg -n 'nonisolated\(unsafe\)|bootstrapResult|sharedEnv\(' packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swiftRepository: dashpay/platform
Length of output: 836
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant parts of the swift test runner scripts
sed -n '1,120p' packages/swift-sdk/run_tests.sh
echo "-----"
sed -n '1,140p' packages/swift-sdk/run_integration_tests.sh
echo "-----"
# Inspect the integration test base class in question
FILE="packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swift"
nl -ba "$FILE" | sed -n '1,200p'Repository: dashpay/platform
Length of output: 3718
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant parts of the swift test runner scripts
sed -n '1,120p' packages/swift-sdk/run_tests.sh
echo "-----"
sed -n '1,140p' packages/swift-sdk/run_integration_tests.sh
echo "-----"
# Inspect the integration test base class in question
FILE="packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swift"
nl -ba "$FILE" | sed -n '1,200p'Repository: dashpay/platform
Length of output: 3718
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the integration test base class with line numbers (avoid nl)
FILE="packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swift"
echo "===== $FILE (first 220 lines) ====="
cat -n "$FILE" | sed -n '1,220p'
echo
echo "===== Search for parallel-testing flags across repo ====="
rg -n --hidden --glob '!.git' \
'parallel-testing-enabled|parallelTesting|--parallel|num-workers|numWorkers|testWorkers|maximumParallel' \
.
echo
echo "===== Search for xcodebuild test invocation flags ====="
rg -n --hidden --glob '!.git' 'xcodebuild test[^{\n]*' .
echo
echo "===== Search for XCTest parallelization knobs ====="
rg -n --hidden --glob '!.git' 'XCTest.*parallel|Parallel.*XCTest|XCT*Parallel' .Repository: dashpay/platform
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused search: avoid scanning huge .yarn bundles
rg -n --hidden \
--glob '!.yarn/**' --glob '!.git/**' --glob '!node_modules/**' \
--glob '*.sh' --glob '*.yml' --glob '*.yaml' --glob '*.json' --glob '*.md' --glob '*.swift' --glob '*.xcscheme' --glob '*.plist' --glob '*.xcconfig' \
'parallel-testing-enabled|parallelTesting|--parallel|num-workers|numWorkers|testWorkers|maximumParallel|XCT.*parallel|Parallel.*XCTest|worker' \
packages/swift-sdk .github . || true
echo "-----"
# Check for xcodebuild test parallel-testing options specifically
rg -n --hidden \
--glob '!.yarn/**' --glob '!.git/**' --glob '!node_modules/**' \
--glob '*.sh' --glob '*.yml' --glob '*.yaml' --glob '*.md' \
'xcodebuild test[^\n]*parallel' \
packages/swift-sdk .github . || true
echo "-----"
# Inspect the Xcode scheme used by xcodebuild (if present)
fd -e xcscheme -a packages/swift-sdk .github . | head -n 50Repository: dashpay/platform
Length of output: 9903
🌐 Web query:
SwiftPM swift testparallel execution default behavior and relevant flags (e.g.,--parallel, test workers)
💡 Result:
In the Swift Package Manager (SwiftPM), parallel execution behavior for the swift test command depends on the testing framework being used [1][2]. Default Behavior: - Swift Testing: Parallel execution is enabled by default [3][1][2]. - XCTest: Parallel execution is disabled by default [1][2]. Command-Line Flags: You can explicitly control parallelization using the following flags with swift test: - --parallel: Explicitly enables parallel execution [1][2]. - --no-parallel: Explicitly disables parallel execution [4][1][5]. Important Notes: - Framework Discrepancy: Because these two frameworks have different default behaviors, the CLI help documentation for swift test --parallel may sometimes be misleading, as it often displays --no-parallel as the default despite it not applying to all frameworks [1]. - Controlling Concurrency: When running tests in parallel, the testing library (particularly with Swift Testing) manages concurrency via the Swift runtime (e.g., using task groups) rather than relying on explicit "worker" counts that are common in other CLI test runners [5]. - Serializing Tests: Even when parallelization is enabled, you can force specific tests or suites to run serially by using the .serialized trait in Swift Testing [5][2]. This is useful for tests that rely on shared state or external resources that cannot be accessed concurrently [5].
Citations:
- 1: Misleading CLI help for
swift test --parallelswiftlang/swift-package-manager#8174 - 2: https://forums.swift.org/t/running-tests-serially-or-in-parallel/72935
- 3: https://developer.apple.com/xcode/swift-testing/
- 4: Parallelize tests by default. swiftlang/swift-testing#174
- 5: https://developer.apple.com/documentation/testing/parallelization
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== Search for parallel-testing / swift test parallel flags (excluding .yarn/.git) ====="
rg -n --hidden \
--glob '!.yarn/**' --glob '!.git/**' --glob '!node_modules/**' \
'parallel-testing-enabled|parallelize.*tests|--parallel(\b|=)|--no-parallel(\b|=)|num-workers|testWorkers|maximumParallel' \
.github packages/swift-sdk . || true
echo
echo "===== Search for SwiftDashSDKIntegrationTests usage in workflows/scripts ====="
rg -n --hidden \
--glob '!.yarn/**' --glob '!.git/**' --glob '!node_modules/**' \
'SwiftDashSDKIntegrationTests' \
.github packages/swift-sdk . || true
echo
echo "===== Find XCTest scheme settings (parallelization) in swift-sdk area ====="
fd --type f -e xcscheme packages/swift-sdk .github 2>/dev/null | head -n 50
# If there are schemes, grep for parallelization keywords inside them
fd -e xcscheme packages/swift-sdk .github 2>/dev/null \
| xargs -r rg -n 'parallelize|parallel-testing-enabled|testParallel' || true
echo
echo "===== Inspect IntegrationTestCase.swift to see if any other caching/serialization exists (beyond bootstrapResult) ====="
FILE="packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swift"
cat -n "$FILE" | sed -n '1,120p'Repository: dashpay/platform
Length of output: 1744
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --glob '!.yarn/**' --glob '!.git/**' --glob '!node_modules/**' \
'run_integration_tests\.sh|RUN_INTEGRATION_TESTS=1|SwiftDashSDKIntegrationTests.*swift test|swift test --filter SwiftDashSDKIntegrationTests' \
.github packages/swift-sdk || true
echo "-----"
# Look for any job-level flags that affect test parallelism (xcodebuild/swift test)
rg -n --hidden --glob '!.yarn/**' --glob '!.git/**' --glob '!node_modules/**' \
'xcodebuild test.*parallel|parallel-testing-enabled|test.*parallel|--parallel-testing|--parallel-testing-enabled|numWorkers|testWorkers|XCTest' \
.github packages/swift-sdk || trueRepository: dashpay/platform
Length of output: 6699
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the swift-sdk build workflow around where it runs integration tests
sed -n '1,220p' .github/workflows/swift-sdk-build.yml
echo
echo "-----"
# Inspect integration test README for any mention of parallelism/serialization
README="packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/README.md"
if [ -f "$README" ]; then
sed -n '1,200p' "$README"
else
echo "Missing $README"
fi
echo
echo "-----"
# Inspect TESTING.md or any suite docs for guidance about parallel execution
TESTING="packages/swift-sdk/TESTING.md"
sed -n '1,200p' "$TESTING" 2>/dev/null || true
echo
echo "-----"
# Inspect Package.swift for the test target type (XCTest-based)
sed -n '1,120p' packages/swift-sdk/Package.swiftRepository: dashpay/platform
Length of output: 11919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect integration test files
fd -e swift packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests | head -n 50
# Show the main integration bootstrap test file(s)
for f in $(fd -e swift packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests | sort); do
echo "===== $f ====="
cat -n "$f" | sed -n '1,220p'
doneRepository: dashpay/platform
Length of output: 598
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'sharedEnv\(|bootstrapResult' packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests
echo "-----"
# List Swift files in that directory
fd -e swift packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests | sort
echo "-----"
# Show first ~200 lines for each .swift file (likely small count)
for f in $(fd -e swift packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests | sort); do
echo "===== $f ====="
sed -n '1,220p' "$f" | cat -n
doneRepository: dashpay/platform
Length of output: 1538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'sharedEnv\(|bootstrapResult' packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests
echo "-----"
fd -e swift packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests | sort
echo "-----"
for f in $(fd -e swift packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests | sort); do
echo "===== $f ====="
sed -n '1,220p' "$f" | cat -n
doneRepository: dashpay/platform
Length of output: 1538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'sharedEnv\(|bootstrapResult' packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests
echo "-----"
fd -e swift packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests | sort
echo "-----"
for f in $(fd -e swift packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests | sort); do
echo "===== $f ====="
sed -n '1,220p' "$f" | cat -n
doneRepository: dashpay/platform
Length of output: 1538
Synchronize bootstrapResult cache access in sharedEnv()
packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swift uses nonisolated(unsafe) private static var bootstrapResult and reads/writes it in sharedEnv() (lines 9, 30-41) without synchronization. This can race if sharedEnv() is invoked concurrently (e.g., if XCTest parallel execution is enabled later or if concurrent tasks call into this during a test).
Proposed fix
open class IntegrationTestCase: XCTestCase {
+ private static let bootstrapLock = NSLock()
private(set) var env: IntegrationTestEnv!
@@
- nonisolated(unsafe) private static var bootstrapResult: Result<IntegrationTestEnv, Error>?
+ private static var bootstrapResult: Result<IntegrationTestEnv, Error>?
@@
private static func sharedEnv() async throws -> IntegrationTestEnv {
+ bootstrapLock.lock()
if let cached = bootstrapResult {
+ bootstrapLock.unlock()
return try cached.get()
}
+ bootstrapLock.unlock()
do {
let env = try await IntegrationTestEnv.bootstrap()
+ bootstrapLock.lock()
+ defer { bootstrapLock.unlock() }
+ if let cached = bootstrapResult {
+ return try cached.get()
+ }
bootstrapResult = .success(env)
return env
} catch {
+ bootstrapLock.lock()
+ defer { bootstrapLock.unlock() }
bootstrapResult = .failure(error)
throw error
}
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swift`
at line 9, The shared cached variable bootstrapResult in IntegrationTestCase is
accessed from nonisolated(unsafe) context in sharedEnv() and can race; protect
all reads/writes to bootstrapResult by introducing synchronization (e.g., a
private static serial DispatchQueue or an actor) and marshal all accesses
through it inside sharedEnv() and any other accessors; update sharedEnv() to
perform the check/read/write of bootstrapResult on that queue (or via await if
using an actor) so initialization is atomic and thread-safe while keeping the
existing API.
| nonisolated(unsafe) private var spvStarted = false | ||
| nonisolated(unsafe) private var trackedWalletIds: [Data] = [] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate shared mutable state and all access points:"
rg -n 'spvStarted|trackedWalletIds|ensureSPVStarted|makeTestWallet|purgeStoredMnemonics' packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestEnv.swift
echo
echo "Check whether integration test runners enforce serial execution:"
rg -n --iglob 'run_integration_tests.sh' --iglob 'swift-sdk-build.yml' 'swift test|--parallel|--num-workers|parallel-testing-enabled'Repository: dashpay/platform
Length of output: 798
Guard IntegrationTestEnv shared mutable state (spvStarted, trackedWalletIds) with synchronization.
In packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestEnv.swift, spvStarted and trackedWalletIds are nonisolated(unsafe) shared vars, and ensureSPVStarted() performs an unsynchronized check/set across an await boundary while trackedWalletIds is appended/iterated/cleared without synchronization. Concurrent async execution can race (double SPV start, cross-test mnemonic purge interference), leading to flaky integration tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestEnv.swift`
around lines 18 - 20, IntegrationTestEnv currently exposes nonisolated(unsafe)
shared mutable state via spvStarted and trackedWalletIds and performs an
unsynchronized check/set in ensureSPVStarted(), causing race conditions; fix by
moving these vars into a synchronized context (either make IntegrationTestEnv an
actor or add a private serial synchronization primitive such as a dedicated
DispatchQueue or Lock) and update all accessors (ensureSPVStarted(), any
append/iterate/clear operations on trackedWalletIds) to run on that
synchronization context so the check-and-set in ensureSPVStarted() is atomic (no
await between check and set) and all mutations/read of trackedWalletIds are
serialized.
| process.waitUntilExit() | ||
|
|
||
| let stdoutData = stdoutPipe.fileHandleForReading.readDataToEndOfFile() | ||
| let stderrData = stderrPipe.fileHandleForReading.readDataToEndOfFile() |
There was a problem hiding this comment.
Risk of deadlock if subprocess output exceeds pipe buffer capacity.
The current implementation waits for the process to exit (line 74) before reading from the pipes (lines 76-77). If a subprocess writes more data to stdout or stderr than the pipe buffer can hold (~64 KB on macOS), the subprocess will block waiting for the pipe to be drained. Meanwhile, the parent is blocked in waitUntilExit(), creating a deadlock.
To prevent this, read the pipes asynchronously while the process is running.
🔧 Proposed fix using readabilityHandler
let stdoutPipe = Pipe()
let stderrPipe = Pipe()
process.standardOutput = stdoutPipe
process.standardError = stderrPipe
+
+var stdoutData = Data()
+var stderrData = Data()
+stdoutPipe.fileHandleForReading.readabilityHandler = { handle in
+ stdoutData.append(handle.availableData)
+}
+stderrPipe.fileHandleForReading.readabilityHandler = { handle in
+ stderrData.append(handle.availableData)
+}
do {
try process.run()
} catch {
throw Error.spawnFailed(
command: "\(command) \(arguments.joined(separator: " "))",
underlying: error
)
}
if let timeout {
let deadline = Date().addingTimeInterval(timeout)
while process.isRunning && Date() < deadline {
Thread.sleep(forTimeInterval: 0.05)
}
if process.isRunning {
process.terminate()
Thread.sleep(forTimeInterval: 0.5)
if process.isRunning { kill(process.processIdentifier, SIGKILL) }
}
}
process.waitUntilExit()
+stdoutPipe.fileHandleForReading.readabilityHandler = nil
+stderrPipe.fileHandleForReading.readabilityHandler = nil
-let stdoutData = stdoutPipe.fileHandleForReading.readDataToEndOfFile()
-let stderrData = stderrPipe.fileHandleForReading.readDataToEndOfFile()
+// Capture any remaining data
+stdoutData.append(stdoutPipe.fileHandleForReading.availableData)
+stderrData.append(stderrPipe.fileHandleForReading.availableData)
return Result(
exitCode: process.terminationStatus,
stdout: String(data: stdoutData, encoding: .utf8) ?? "",
stderr: String(data: stderrData, encoding: .utf8) ?? ""
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/Shell.swift`
around lines 74 - 77, The code currently calls process.waitUntilExit() before
reading stdout/stderr which can deadlock; change Shell.run (or the function
creating Process and using stdoutPipe/stderrPipe) to read the pipes
asynchronously: set stdoutPipe.fileHandleForReading.readabilityHandler and
stderrPipe.fileHandleForReading.readabilityHandler to append incoming Data into
local Data buffers, then call process.waitUntilExit(), then clear the
readabilityHandlers and use the accumulated Data (instead of calling
readDataToEndOfFile() after waitUntilExit()). Ensure handlers are removed (set
to nil) to avoid leaks and convert the collected Data to strings as before.
| let deadline = Date().addingTimeInterval(timeout) | ||
| while Date() < deadline { | ||
| if try await condition() { return } | ||
| try await Task.sleep(nanoseconds: UInt64(pollInterval * 1_000_000_000)) | ||
| } | ||
| throw TimeoutError(description: "Timed out after \(timeout)s waiting for: \(message())") |
There was a problem hiding this comment.
Timeout error may obscure the actual failure reason.
If the condition throws an exception on the final iteration (when Date() >= deadline), the loop exits at line 45 and throws TimeoutError at line 49, hiding the underlying error from the condition. This can make debugging harder when a test fails due to an unexpected exception rather than a simple timeout.
Consider checking the deadline after attempting the condition so that errors from the final attempt are surfaced.
🛠️ Proposed fix to surface condition errors
static func until(
_ message: `@autoclosure` () -> String,
timeout: TimeInterval = 60,
pollInterval: TimeInterval = 0.5,
_ condition: () async throws -> Bool
) async throws {
let deadline = Date().addingTimeInterval(timeout)
- while Date() < deadline {
- if try await condition() { return }
+ while true {
+ if try await condition() { return }
+ guard Date() < deadline else {
+ throw TimeoutError(description: "Timed out after \(timeout)s waiting for: \(message())")
+ }
try await Task.sleep(nanoseconds: UInt64(pollInterval * 1_000_000_000))
}
- throw TimeoutError(description: "Timed out after \(timeout)s waiting for: \(message())")
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/TestWallet.swift`
around lines 44 - 49, The loop currently checks the deadline before calling
condition(), which can suppress an exception thrown by condition() on the final
attempt; change the loop in the waiting helper (the block that creates deadline,
calls try await condition(), sleeps with Task.sleep using pollInterval, and
throws TimeoutError) so you call try await condition() first, return on success,
then if Date() >= deadline throw the TimeoutError, and only then await
Task.sleep; this ensures any error from condition() on the final attempt is
propagated instead of being replaced by the TimeoutError.
ea8c626 to
def4241
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta is CI-only: three new workflow steps that reset the local dashmate stack, run the new Swift integration suite via run_integration_tests.sh, and always stop dashmate on exit. No FFI, consensus, crypto, or production paths are touched. All six prior findings against 361114b still apply verbatim at HEAD (none were addressed in the delta), plus the new CI wiring introduces two related concerns around error swallowing on reset and the duplicated --group=local ownership between the workflow and the runner script.
🟡 4 suggestion(s) | 💬 4 nitpick(s)
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Prior
SwiftTestsSwift Package and example app cleanup — The cumulative diff againstv3.1-devdeletespackages/swift-sdk/SwiftTests/Package.swift,SwiftDashSDK.h,run_tests.sh, andexample/SwiftSDKExample.swift. These removals look intentional given the test reorganisation, but they break previously documented entry points that may be referenced from external docs or downstream consumers.- Follow-up: Author-requested follow-up PR: sweep README/docs for references to
SwiftTests/Package.swift,SwiftDashSDK.h, andexample/SwiftSDKExample.swiftand update or repoint them.
- Follow-up: Author-requested follow-up PR: sweep README/docs for references to
| try await MainActor.run { | ||
| let context = ModelContext(container) | ||
| let allTxCount = try context.fetchCount(FetchDescriptor<PersistentTransaction>()) | ||
| XCTAssertEqual(allTxCount, expectedTotalTxs) | ||
|
|
||
| let aliceTxoCount = try context.fetchCount(FetchDescriptor<PersistentTxo>( | ||
| predicate: #Predicate<PersistentTxo>{ | ||
| $0.walletId == aliceWalletId | ||
| } | ||
| )) | ||
| let bobTxoCount = try context.fetchCount(FetchDescriptor<PersistentTxo>( | ||
| predicate: #Predicate<PersistentTxo>{ | ||
| $0.walletId == bobWalletId | ||
| } | ||
| )) | ||
|
|
||
| XCTAssertEqual(aliceTxoCount, 1 + iterations) | ||
| XCTAssertEqual(bobTxoCount, 1 + iterations) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior finding: global PersistentTransaction count is coupled to the process-shared SwiftData store
IntegrationTestCase.sharedEnv() memoises a single IntegrationTestEnv for the whole test process and IntegrationTestEnv.bootstrap() builds one in-memory ModelContainer reused across every test. Only Keychain mnemonics are cleared in tearDown; SwiftData rows persist in the shared container. The assertion on line 61 (fetchCount(FetchDescriptor<PersistentTransaction>())) is unfiltered, so the moment a second integration test writes a transaction into the same container the equality check either fails spuriously or — worse — masks a regression because the global count happens to land on the expected value. The TXO counts on lines 63-75 are correctly scoped by walletId; the same scoping should apply to the transaction count.
| try await MainActor.run { | |
| let context = ModelContext(container) | |
| let allTxCount = try context.fetchCount(FetchDescriptor<PersistentTransaction>()) | |
| XCTAssertEqual(allTxCount, expectedTotalTxs) | |
| let aliceTxoCount = try context.fetchCount(FetchDescriptor<PersistentTxo>( | |
| predicate: #Predicate<PersistentTxo>{ | |
| $0.walletId == aliceWalletId | |
| } | |
| )) | |
| let bobTxoCount = try context.fetchCount(FetchDescriptor<PersistentTxo>( | |
| predicate: #Predicate<PersistentTxo>{ | |
| $0.walletId == bobWalletId | |
| } | |
| )) | |
| XCTAssertEqual(aliceTxoCount, 1 + iterations) | |
| XCTAssertEqual(bobTxoCount, 1 + iterations) | |
| } | |
| try await MainActor.run { | |
| let context = ModelContext(container) | |
| let aliceTxCount = try context.fetchCount(FetchDescriptor<PersistentTransaction>( | |
| predicate: #Predicate<PersistentTransaction>{ | |
| $0.walletId == aliceWalletId | |
| } | |
| )) | |
| let bobTxCount = try context.fetchCount(FetchDescriptor<PersistentTransaction>( | |
| predicate: #Predicate<PersistentTransaction>{ | |
| $0.walletId == bobWalletId | |
| } | |
| )) | |
| XCTAssertEqual(aliceTxCount + bobTxCount, expectedTotalTxs) | |
| let aliceTxoCount = try context.fetchCount(FetchDescriptor<PersistentTxo>( | |
| predicate: #Predicate<PersistentTxo>{ | |
| $0.walletId == aliceWalletId | |
| } | |
| )) | |
| let bobTxoCount = try context.fetchCount(FetchDescriptor<PersistentTxo>( | |
| predicate: #Predicate<PersistentTxo>{ | |
| $0.walletId == bobWalletId | |
| } | |
| )) | |
| XCTAssertEqual(aliceTxoCount, 1 + iterations) | |
| XCTAssertEqual(bobTxoCount, 1 + iterations) | |
| } |
source: ['claude', 'codex']
| DASHMATE_CONFIG="${DASHMATE_CONFIG:-local_seed}" | ||
|
|
||
| cd "$SCRIPT_DIR" | ||
|
|
||
| node_major=$(node -p "process.versions.node.split('.')[0]" 2>/dev/null || echo 0) | ||
| if [ "$node_major" -gt 22 ] || [ "$node_major" -lt 20 ]; then | ||
| echo "Node $(node --version 2>/dev/null || echo '<missing>') is not supported by dashmate." | ||
| echo "Use Node 20–22 (e.g. \`nvm use 22\` or \`brew link node@22\`) and re-run." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! ls -d "$REPO_ROOT"/.yarn/unplugged/systeminformation-* >/dev/null 2>&1; then | ||
| echo "yarn install (materialising unplugged packages)…" | ||
| (cd "$REPO_ROOT" && yarn install) | ||
| fi | ||
| if [ ! -f "$REPO_ROOT/packages/wasm-dpp/dist/index.js" ]; then | ||
| # Apple's bundled clang lacks the wasm32 target; cc-rs picks it up | ||
| # via `CC_wasm32_unknown_unknown` (not `CC`, which only affects the | ||
| # host build). Wire in Homebrew's LLVM automatically when present. | ||
| if [ -z "${CC_wasm32_unknown_unknown:-}" ] && command -v brew >/dev/null 2>&1; then | ||
| brew_llvm="$(brew --prefix llvm 2>/dev/null || true)" | ||
| if [ -x "$brew_llvm/bin/clang" ]; then | ||
| export CC_wasm32_unknown_unknown="$brew_llvm/bin/clang" | ||
| export AR_wasm32_unknown_unknown="$brew_llvm/bin/llvm-ar" | ||
| fi | ||
| fi | ||
| echo "yarn build (~few min — builds wasm-dpp etc.)…" | ||
| (cd "$REPO_ROOT" && yarn build) | ||
| if [ ! -f "$REPO_ROOT/packages/wasm-dpp/dist/index.js" ]; then | ||
| echo "yarn build finished but wasm-dpp/dist is still missing." | ||
| echo " Likely a wasm32 toolchain issue. If Homebrew's LLVM isn't" | ||
| echo " installed, run \`brew install llvm\` and try again." | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| DAPI_PORT="${DAPI_PORT:-2443}" | ||
| dapi_reachable() { | ||
| nc -z 127.0.0.1 "$DAPI_PORT" >/dev/null 2>&1 | ||
| } | ||
|
|
||
| if dapi_reachable; then | ||
| echo "dashmate devnet already running" | ||
| else | ||
| echo "starting dashmate devnet group (this can take ~1-2 min)…" | ||
| (cd "$REPO_ROOT" && yarn dashmate group stop --group=local 2>/dev/null || true) | ||
| (cd "$REPO_ROOT" && yarn dashmate group start --group=local --wait-for-readiness) | ||
| fi |
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior finding: DASHMATE_CONFIG only flows into the test binary, not into the devnet lifecycle the runner manages
Line 14 honours DASHMATE_CONFIG (default local_seed) and the test binary uses the same variable in IntegrationTestEnv.bootstrap() (line 43) to drive LocalDevnet endpoint discovery via yarn dashmate config get. But this script hard-codes DAPI_PORT=2443 for the reachability probe (line 50) and yarn dashmate group {stop,start} --group=local for lifecycle (lines 59-60). With DASHMATE_CONFIG=something_else, the runner probes/starts/stops one stack while the test binary inspects another — leading to false failures or accidentally exercising the wrong devnet. The probe port should be derived from dashmate config get <chosen-config> platform.dapi.envoy.http.port, and the group name should be configurable (or derived from the config).
source: ['claude', 'codex']
| nonisolated(unsafe) private var spvStarted = false | ||
| nonisolated(unsafe) private var trackedWalletIds: [Data] = [] | ||
|
|
||
| private init( | ||
| endpoints: LocalDevnet.Endpoints, | ||
| coreRPC: CoreRPCClient, | ||
| sdk: SDK, | ||
| modelContainer: ModelContainer, | ||
| walletManager: PlatformWalletManager, | ||
| mineRewardAddress: String | ||
| ) { | ||
| self.endpoints = endpoints | ||
| self.coreRPC = coreRPC | ||
| self.sdk = sdk | ||
| self.modelContainer = modelContainer | ||
| self.walletManager = walletManager | ||
| self.mineRewardAddress = mineRewardAddress | ||
| } | ||
|
|
||
| /// Build the env from scratch. `IntegrationTestCase` is responsible | ||
| /// for memoising the result across tests. Assumes dashmate is | ||
| /// already up, `run_integration_tests.sh` handles bringing the | ||
| /// stack online before the test binary runs. | ||
| static func bootstrap() async throws -> IntegrationTestEnv { | ||
| let repoRoot = try locateRepoRoot() | ||
| let configName = ProcessInfo.processInfo.environment["DASHMATE_CONFIG"] ?? "local_seed" | ||
| let devnet = LocalDevnet(repoRoot: repoRoot, configName: configName) | ||
|
|
||
| let endpoints = try devnet.discoverEndpoints() | ||
| let coreRPC = CoreRPCClient( | ||
| port: endpoints.coreRPC, | ||
| username: endpoints.rpcUsername, | ||
| password: endpoints.rpcPassword | ||
| ) | ||
|
|
||
| // The SDK reads `platformDAPIAddresses` from UserDefaults to | ||
| // override its built-in regtest default of 127.0.0.1:2443 | ||
| // (matches the SwiftExampleApp wiring). | ||
| UserDefaults.standard.set( | ||
| "http://127.0.0.1:\(endpoints.platformDAPI)", | ||
| forKey: "platformDAPIAddresses" | ||
| ) | ||
| let sdk = try SDK(network: .regtest) | ||
| let modelContainer = try DashModelContainer.createInMemory() | ||
| // `PlatformWalletManager` is `@MainActor`-isolated. | ||
| let walletManager = try await MainActor.run { | ||
| try PlatformWalletManager(sdk: sdk, modelContainer: modelContainer) | ||
| } | ||
|
|
||
| let mineReward = try await coreRPC.getNewAddress() | ||
|
|
||
| return IntegrationTestEnv( | ||
| endpoints: endpoints, | ||
| coreRPC: coreRPC, | ||
| sdk: sdk, | ||
| modelContainer: modelContainer, | ||
| walletManager: walletManager, | ||
| mineRewardAddress: mineReward | ||
| ) | ||
| } | ||
|
|
||
| /// Start SPV pinned to the devnet's P2P port. Idempotent. | ||
| func ensureSPVStarted() async throws { | ||
| if spvStarted { return } | ||
| let dir = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("swift-sdk-it-spv-\(UUID().uuidString.prefix(8))", isDirectory: true) | ||
| try? FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) | ||
|
|
||
| let manager = walletManager | ||
| let config = PlatformSpvStartConfig( | ||
| dataDir: dir.path, | ||
| network: .regtest, | ||
| userAgent: "swift-sdk-integration-tests/0.1", | ||
| peers: ["127.0.0.1:\(endpoints.coreP2P)"], | ||
| restrictToConfiguredPeers: true, | ||
| startFromHeight: 0 | ||
| ) | ||
| try await MainActor.run { try manager.startSpv(config: config) } | ||
| spvStarted = true | ||
| } | ||
|
|
||
| /// Each call generates a fresh random mnemonic. | ||
| /// The mnemonic is also persisted to Keychain | ||
| func makeTestWallet(name: String? = nil) async throws -> TestWalletWrapper { | ||
| let mnemonic = try Mnemonic.generate(wordCount: 24) | ||
| let manager = walletManager | ||
| let wallet = try await MainActor.run { | ||
| try manager.createWallet( | ||
| mnemonic: mnemonic, | ||
| network: .regtest, | ||
| name: name, | ||
| createDefaultAccounts: true | ||
| ) | ||
| } | ||
| try WalletStorage().storeMnemonic(mnemonic, for: wallet.walletId) | ||
| trackedWalletIds.append(wallet.walletId) | ||
| return TestWalletWrapper(wallet: wallet, core: try wallet.coreWallet()) | ||
| } | ||
|
|
||
| /// Delete every mnemonic this env wrote to Keychain during the | ||
| /// current test | ||
| func purgeStoredMnemonics() { | ||
| let storage = WalletStorage() | ||
| for id in trackedWalletIds { | ||
| try? storage.deleteMnemonic(for: id) | ||
| } | ||
| trackedWalletIds.removeAll() | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior finding: ensureSPVStarted and trackedWalletIds have check-then-act races on nonisolated(unsafe) state
spvStarted and trackedWalletIds are declared nonisolated(unsafe) (lines 18-19). ensureSPVStarted (lines 80-97) reads spvStarted, awaits a MainActor hop to manager.startSpv(config:), then writes spvStarted = true. Two concurrent callers can both pass the guard and both invoke platform_wallet_manager_spv_start on the same wallet manager — the second start has nondeterministic semantics on the Rust side (SpvRuntime::spawn_in_background cancels any prior token and the duplicate-start error is only logged in the spawned task, so Swift still observes success). makeTestWallet (line 113) and purgeStoredMnemonics (lines 119-125) similarly mutate trackedWalletIds without synchronisation. Today XCTest serialises tests per class so this is latent, but nonisolated(unsafe) explicitly opts out of compiler race checks for state that is shared and mutable. Mark the env @MainActor, wrap these in a small actor, or guard with OSAllocatedUnfairLock.
source: ['claude', 'codex']
| # See SwiftTests/SwiftDashSDKIntegrationTests/README.md for one-time setup | ||
| # (`yarn dashmate setup --preset local`). |
There was a problem hiding this comment.
💬 Nitpick: Carried-forward prior finding: referenced README does not exist
The header comment points users at SwiftTests/SwiftDashSDKIntegrationTests/README.md for one-time setup, but only Core/, Platform/, and Support/ subdirectories exist there — no README. Either drop the reference or add the file with the yarn dashmate setup --preset local instructions it claims to host.
source: ['claude', 'codex']
| /// Placeholder until real Platform-side integration coverage lands | ||
| final class SDKBootstrapIntegrationTests: IntegrationTestCase { | ||
| func testSDKInitializes() { | ||
| _ = env.sdk | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Carried-forward prior finding: bootstrap test does not exercise the local DAPI endpoint
testSDKInitializes only evaluates env.sdk, which is already constructed during suite bootstrap. A broken local DAPI / gateway can still leave this test green, since no DAPI request is ever issued from the test body. The file's own comment labels it a placeholder, but as long as it appears in the integration suite it provides misleading positive Platform-side signal in CI. Either delete it until a real Platform integration test lands, or add at least one DAPI round-trip (e.g. fetching a system contract or known identity) so it actually verifies the platform endpoint discovered by dashmate config get.
source: ['claude', 'codex']
| if let timeout { | ||
| let deadline = Date().addingTimeInterval(timeout) | ||
| while process.isRunning && Date() < deadline { | ||
| Thread.sleep(forTimeInterval: 0.05) | ||
| } | ||
| if process.isRunning { | ||
| process.terminate() | ||
| Thread.sleep(forTimeInterval: 0.5) | ||
| if process.isRunning { kill(process.processIdentifier, SIGKILL) } | ||
| } | ||
| } | ||
|
|
||
| process.waitUntilExit() |
There was a problem hiding this comment.
💬 Nitpick: Carried-forward prior finding: Shell.run blocks a Swift Concurrency worker with Thread.sleep + waitUntilExit
Shell.run is invoked transitively from async IntegrationTestEnv.bootstrap() via LocalDevnet.readConfigRaw and from coreRPC paths. The timeout loop uses Thread.sleep(forTimeInterval: 0.05) plus a synchronous process.waitUntilExit(), blocking whichever Swift Concurrency cooperative thread happens to run it for up to the timeout. The dashmate config get invocations use a 30-second timeout, so a single hang can stall the cooperative pool for half a minute. Consider wrapping Process with a terminationHandler + CheckedContinuation, or move the blocking work onto DispatchQueue.global().
source: ['claude', 'codex']
| - name: Reset dashmate local devnet to a clean state | ||
| run: | | ||
| bash packages/swift-sdk/build_ios.sh --target all --profile release | ||
| set +e | ||
| yarn dashmate group stop --group=local --force | ||
| yarn dashmate group reset --group=local --hard --verbose | ||
| true | ||
| timeout-minutes: 5 |
There was a problem hiding this comment.
🟡 Suggestion: New finding in latest delta: reset step swallows all errors, hiding broken-devnet failures from CI
The new "Reset dashmate local devnet to a clean state" step uses set +e plus a trailing true so the entire block always exits 0. If yarn dashmate group reset --group=local --hard --verbose fails (corrupt state, missing config, permission issue), the next step (run_integration_tests.sh) will still run — and because the runner's dapi_reachable probe falls through to dashmate group start when DAPI is unreachable, a failed reset silently turns into "reuse whatever was there", which is exactly what this step is meant to prevent. Make reset fatal; stop can legitimately no-op when nothing is running.
| - name: Reset dashmate local devnet to a clean state | |
| run: | | |
| bash packages/swift-sdk/build_ios.sh --target all --profile release | |
| set +e | |
| yarn dashmate group stop --group=local --force | |
| yarn dashmate group reset --group=local --hard --verbose | |
| true | |
| timeout-minutes: 5 | |
| - name: Reset dashmate local devnet to a clean state | |
| run: | | |
| set -euo pipefail | |
| yarn dashmate group stop --group=local --force || true | |
| yarn dashmate group reset --group=local --hard --verbose | |
| timeout-minutes: 5 |
source: ['claude']
| - name: Reset dashmate local devnet to a clean state | ||
| run: | | ||
| bash packages/swift-sdk/build_ios.sh --target all --profile release | ||
| set +e | ||
| yarn dashmate group stop --group=local --force | ||
| yarn dashmate group reset --group=local --hard --verbose | ||
| true | ||
| timeout-minutes: 5 | ||
|
|
||
| - name: Run Swift SDK integration tests | ||
| run: bash packages/swift-sdk/run_integration_tests.sh | ||
| timeout-minutes: 10 | ||
|
|
||
| - name: Stop dashmate (always) | ||
| if: always() | ||
| run: yarn dashmate group stop --group=local --force || true | ||
| timeout-minutes: 5 |
There was a problem hiding this comment.
💬 Nitpick: New finding in latest delta: CI workflow and runner script duplicate (and can conflict on) dashmate lifecycle
The workflow now resets and explicitly stops dashmate around the integration run, while run_integration_tests.sh (lines 55-61) still has its own "start if not reachable" branch that stops + starts --group=local. The end-to-end sequence in CI becomes: workflow stop → workflow reset → script-side stop (DAPI unreachable post-reset) → script-side start → tests → workflow stop. It works, but it pins --group=local and the DAPI port assumption in two places — and combined with prior finding 2, both copies need to be parameterised together if DASHMATE_CONFIG is ever changed. Pick one owner: either skip the runner's auto-start in CI (e.g. via SKIP_DASHMATE_LIFECYCLE=1) or move the start call entirely out of the script so the workflow owns lifecycle.
source: ['claude', 'codex']
ZocoLini
force-pushed
the
feat/swift-sdk-integration-tests
branch
2 times, most recently
from
9031b50 to
9069b49
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/swift-sdk-build.yml:
- Line 94: The workflow is using floating refs (actions/setup-node@v4 and
cargo-bins/cargo-binstall@v1.3.1); pin both Actions to immutable commit SHAs
instead of version tags to harden supply-chain integrity — locate the
corresponding repository commit SHAs for actions/setup-node and
cargo-bins/cargo-binstall (from their GitHub repos or Marketplace pages) and
replace the usages "actions/setup-node@v4" and
"cargo-bins/cargo-binstall@v1.3.1" with "actions/setup-node@<commit-sha>" and
"cargo-bins/cargo-binstall@<commit-sha>" respectively, keeping the rest of the
workflow unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d295ba4-9bf0-4735-bc72-4c3159f6e942
📒 Files selected for processing (11)
.github/workflows/swift-sdk-build.ymlpackages/swift-sdk/Package.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Platform/SDKBootstrapIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/CoreRPCClient.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestEnv.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/LocalDevnet.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/Shell.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/TestWallet.swiftpackages/swift-sdk/run_integration_tests.sh
| bash packages/swift-sdk/run_tests.sh | ||
|
|
||
| - name: Pin Node 22 for dashmate | ||
| uses: actions/setup-node@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Resolve commit SHAs for currently referenced tags:"
gh api repos/actions/setup-node/commits/v4 --jq '.sha'
gh api repos/cargo-bins/cargo-binstall/commits/v1.3.1 --jq '.sha'
echo
echo "Confirm locations in workflow:"
rg -n 'uses:\s*(actions/setup-node|cargo-bins/cargo-binstall)@' .github/workflows/swift-sdk-build.ymlRepository: dashpay/platform
Length of output: 315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/swift-sdk-build.yml"
echo "Context around line 94 and 102:"
nl -ba "$FILE" | sed -n '80,115p'
echo
echo "All occurrences of the targeted actions in this workflow:"
rg -n 'uses:\s*(actions/setup-node|cargo-bins/cargo-binstall)@' "$FILE"Repository: dashpay/platform
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/swift-sdk-build.yml"
echo "Context around lines ~90-105:"
sed -n '85,110p' "$FILE"
echo
echo "All occurrences of the targeted actions in this workflow:"
rg -n 'uses:\s*(actions/setup-node|cargo-bins/cargo-binstall)@' "$FILE"Repository: dashpay/platform
Length of output: 1038
Pin GitHub Actions to immutable commit SHAs.
Line 94 (actions/setup-node@v4) and line 102 (cargo-bins/cargo-binstall@v1.3.1) use floating refs, weakening supply-chain integrity.
Suggested hardening change
- - name: Pin Node 22 for dashmate
- uses: actions/setup-node@v4
+ - name: Pin Node 22 for dashmate
+ uses: actions/setup-node@<SETUP_NODE_COMMIT_SHA>
@@
- - name: Install cargo-binstall
- uses: cargo-bins/cargo-binstall@v1.3.1
+ - name: Install cargo-binstall
+ uses: cargo-bins/cargo-binstall@<CARGO_BINSTALL_COMMIT_SHA>🧰 Tools
🪛 zizmor (1.25.2)
[error] 94-94: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/swift-sdk-build.yml at line 94, The workflow is using
floating refs (actions/setup-node@v4 and cargo-bins/cargo-binstall@v1.3.1); pin
both Actions to immutable commit SHAs instead of version tags to harden
supply-chain integrity — locate the corresponding repository commit SHAs for
actions/setup-node and cargo-bins/cargo-binstall (from their GitHub repos or
Marketplace pages) and replace the usages "actions/setup-node@v4" and
"cargo-bins/cargo-binstall@v1.3.1" with "actions/setup-node@<commit-sha>" and
"cargo-bins/cargo-binstall@<commit-sha>" respectively, keeping the rest of the
workflow unchanged.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta only adds a Node 22 pinning step and a corepack-enable step to the CI workflow before the dashmate lifecycle. None of the eight previously reported findings were addressed; all are reproducible at def4241 and carried forward. No new issues introduced by the delta itself.
🟡 4 suggestion(s) | 💬 4 nitpick(s)
2 additional finding(s) omitted (not in diff).
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Self-hosted runner global-state side effects from setup-node + corepack — The newly added
actions/setup-node@v4+corepack enablesteps run on a self-hosted macOS ARM64 runner. setup-node mutates PATH and corepack-managed yarn shims for the runner user; on long-lived self-hosted runners this can leave state behind between jobs (e.g. pinning Node 22 for subsequent unrelated workflows that expect system Node). Worth tracking but not blocking this PR.- Follow-up: Create a separate issue to audit self-hosted runner global-state interactions from setup-node and corepack across the platform CI matrix.
| try await MainActor.run { | ||
| let context = ModelContext(container) | ||
| let allTxCount = try context.fetchCount(FetchDescriptor<PersistentTransaction>()) | ||
| XCTAssertEqual(allTxCount, expectedTotalTxs) |
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior finding: global PersistentTransaction count is coupled to the process-shared SwiftData store
IntegrationTestCase.sharedEnv() memoises one IntegrationTestEnv per process, and IntegrationTestEnv.bootstrap() creates a single in-memory ModelContainer that every test reuses. tearDown() only purges Keychain mnemonics — SwiftData rows accumulate across tests. The assertion XCTAssertEqual(allTxCount, expectedTotalTxs) counts every PersistentTransaction in the shared store, so as soon as any second test persists transactions (or this test is rerun in the same process), it flakes. The per-wallet TXO counts below (lines 63–75) already filter by walletId and are the correct pattern — scope the total similarly (e.g. walletId == aliceWalletId || walletId == bobWalletId) or otherwise restrict to wallets created by this test.
source: ['claude', 'codex']
| DASHMATE_CONFIG="${DASHMATE_CONFIG:-local_seed}" | ||
|
|
||
| cd "$SCRIPT_DIR" | ||
|
|
||
| node_major=$(node -p "process.versions.node.split('.')[0]" 2>/dev/null || echo 0) | ||
| if [ "$node_major" -gt 22 ] || [ "$node_major" -lt 20 ]; then | ||
| echo "Node $(node --version 2>/dev/null || echo '<missing>') is not supported by dashmate." | ||
| echo "Use Node 20–22 (e.g. \`nvm use 22\` or \`brew link node@22\`) and re-run." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! ls -d "$REPO_ROOT"/.yarn/unplugged/systeminformation-* >/dev/null 2>&1; then | ||
| echo "yarn install (materialising unplugged packages)…" | ||
| (cd "$REPO_ROOT" && yarn install) | ||
| fi | ||
| if [ ! -f "$REPO_ROOT/packages/wasm-dpp/dist/index.js" ]; then | ||
| # Apple's bundled clang lacks the wasm32 target; cc-rs picks it up | ||
| # via `CC_wasm32_unknown_unknown` (not `CC`, which only affects the | ||
| # host build). Wire in Homebrew's LLVM automatically when present. | ||
| if [ -z "${CC_wasm32_unknown_unknown:-}" ] && command -v brew >/dev/null 2>&1; then | ||
| brew_llvm="$(brew --prefix llvm 2>/dev/null || true)" | ||
| if [ -x "$brew_llvm/bin/clang" ]; then | ||
| export CC_wasm32_unknown_unknown="$brew_llvm/bin/clang" | ||
| export AR_wasm32_unknown_unknown="$brew_llvm/bin/llvm-ar" | ||
| fi | ||
| fi | ||
| echo "yarn build (~few min — builds wasm-dpp etc.)…" | ||
| (cd "$REPO_ROOT" && yarn build) | ||
| if [ ! -f "$REPO_ROOT/packages/wasm-dpp/dist/index.js" ]; then | ||
| echo "yarn build finished but wasm-dpp/dist is still missing." | ||
| echo " Likely a wasm32 toolchain issue. If Homebrew's LLVM isn't" | ||
| echo " installed, run \`brew install llvm\` and try again." | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| DAPI_PORT="${DAPI_PORT:-2443}" | ||
| dapi_reachable() { | ||
| nc -z 127.0.0.1 "$DAPI_PORT" >/dev/null 2>&1 | ||
| } | ||
|
|
||
| if dapi_reachable; then | ||
| echo "dashmate devnet already running" | ||
| else | ||
| echo "starting dashmate devnet group (this can take ~1-2 min)…" | ||
| (cd "$REPO_ROOT" && yarn dashmate group stop --group=local 2>/dev/null || true) | ||
| (cd "$REPO_ROOT" && yarn dashmate group start --group=local --wait-for-readiness) | ||
| fi |
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior finding: DASHMATE_CONFIG does not flow into the devnet lifecycle the runner manages
Line 14 honours DASHMATE_CONFIG (default local_seed) and exports it for the test binary, which uses it in IntegrationTestEnv.bootstrap() to discover endpoints via yarn dashmate config get --config=.... But the runner hard-codes DAPI_PORT=2443 for the reachability probe (line 50) and uses yarn dashmate group stop/start --group=local unconditionally (lines 59–60). With a non-default DASHMATE_CONFIG (e.g. a different node in the group or a different group), the runner will start/probe one devnet while the tests query another — a confusing failure mode that depends on local machine state. Either drive the port and group selection from DASHMATE_CONFIG, or document that the env var is test-binary-only.
source: ['claude', 'codex']
| nonisolated(unsafe) private var spvStarted = false | ||
| nonisolated(unsafe) private var trackedWalletIds: [Data] = [] | ||
|
|
||
| private init( | ||
| endpoints: LocalDevnet.Endpoints, | ||
| coreRPC: CoreRPCClient, | ||
| sdk: SDK, | ||
| modelContainer: ModelContainer, | ||
| walletManager: PlatformWalletManager, | ||
| mineRewardAddress: String | ||
| ) { | ||
| self.endpoints = endpoints | ||
| self.coreRPC = coreRPC | ||
| self.sdk = sdk | ||
| self.modelContainer = modelContainer | ||
| self.walletManager = walletManager | ||
| self.mineRewardAddress = mineRewardAddress | ||
| } | ||
|
|
||
| /// Build the env from scratch. `IntegrationTestCase` is responsible | ||
| /// for memoising the result across tests. Assumes dashmate is | ||
| /// already up, `run_integration_tests.sh` handles bringing the | ||
| /// stack online before the test binary runs. | ||
| static func bootstrap() async throws -> IntegrationTestEnv { | ||
| let repoRoot = try locateRepoRoot() | ||
| let configName = ProcessInfo.processInfo.environment["DASHMATE_CONFIG"] ?? "local_seed" | ||
| let devnet = LocalDevnet(repoRoot: repoRoot, configName: configName) | ||
|
|
||
| let endpoints = try devnet.discoverEndpoints() | ||
| let coreRPC = CoreRPCClient( | ||
| port: endpoints.coreRPC, | ||
| username: endpoints.rpcUsername, | ||
| password: endpoints.rpcPassword | ||
| ) | ||
|
|
||
| // The SDK reads `platformDAPIAddresses` from UserDefaults to | ||
| // override its built-in regtest default of 127.0.0.1:2443 | ||
| // (matches the SwiftExampleApp wiring). | ||
| UserDefaults.standard.set( | ||
| "http://127.0.0.1:\(endpoints.platformDAPI)", | ||
| forKey: "platformDAPIAddresses" | ||
| ) | ||
| let sdk = try SDK(network: .regtest) | ||
| let modelContainer = try DashModelContainer.createInMemory() | ||
| // `PlatformWalletManager` is `@MainActor`-isolated. | ||
| let walletManager = try await MainActor.run { | ||
| try PlatformWalletManager(sdk: sdk, modelContainer: modelContainer) | ||
| } | ||
|
|
||
| let mineReward = try await coreRPC.getNewAddress() | ||
|
|
||
| return IntegrationTestEnv( | ||
| endpoints: endpoints, | ||
| coreRPC: coreRPC, | ||
| sdk: sdk, | ||
| modelContainer: modelContainer, | ||
| walletManager: walletManager, | ||
| mineRewardAddress: mineReward | ||
| ) | ||
| } | ||
|
|
||
| /// Start SPV pinned to the devnet's P2P port. Idempotent. | ||
| func ensureSPVStarted() async throws { | ||
| if spvStarted { return } | ||
| let dir = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("swift-sdk-it-spv-\(UUID().uuidString.prefix(8))", isDirectory: true) | ||
| try? FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) | ||
|
|
||
| let manager = walletManager | ||
| let config = PlatformSpvStartConfig( | ||
| dataDir: dir.path, | ||
| network: .regtest, | ||
| userAgent: "swift-sdk-integration-tests/0.1", | ||
| peers: ["127.0.0.1:\(endpoints.coreP2P)"], | ||
| restrictToConfiguredPeers: true, | ||
| startFromHeight: 0 | ||
| ) | ||
| try await MainActor.run { try manager.startSpv(config: config) } | ||
| spvStarted = true | ||
| } | ||
|
|
||
| /// Each call generates a fresh random mnemonic. | ||
| /// The mnemonic is also persisted to Keychain | ||
| func makeTestWallet(name: String? = nil) async throws -> TestWalletWrapper { | ||
| let mnemonic = try Mnemonic.generate(wordCount: 24) | ||
| let manager = walletManager | ||
| let wallet = try await MainActor.run { | ||
| try manager.createWallet( | ||
| mnemonic: mnemonic, | ||
| network: .regtest, | ||
| name: name, | ||
| createDefaultAccounts: true | ||
| ) | ||
| } | ||
| try WalletStorage().storeMnemonic(mnemonic, for: wallet.walletId) | ||
| trackedWalletIds.append(wallet.walletId) | ||
| return TestWalletWrapper(wallet: wallet, core: try wallet.coreWallet()) | ||
| } | ||
|
|
||
| /// Delete every mnemonic this env wrote to Keychain during the | ||
| /// current test | ||
| func purgeStoredMnemonics() { | ||
| let storage = WalletStorage() | ||
| for id in trackedWalletIds { | ||
| try? storage.deleteMnemonic(for: id) | ||
| } | ||
| trackedWalletIds.removeAll() | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior finding: ensureSPVStarted and trackedWalletIds have check-then-act races on nonisolated(unsafe) state
spvStarted and trackedWalletIds are declared nonisolated(unsafe) (lines 18–19). ensureSPVStarted reads spvStarted (line 81), awaits a MainActor hop to manager.startSpv(config:) (line 95), then writes spvStarted = true (line 96) — two concurrent callers can both pass the guard and start SPV twice. Similarly makeTestWallet/purgeStoredMnemonics (lines 113, 119–125) mutate the shared [Data] without synchronization. Today XCTest's default serial scheduling masks this; the moment async parallelism is enabled, the env starts duplicating SPV or losing cleanup entries. Isolate to @MainActor, gate mutations through an actor, or wrap with a lock.
source: ['claude', 'codex']
| /// Placeholder until real Platform-side integration coverage lands | ||
| final class SDKBootstrapIntegrationTests: IntegrationTestCase { | ||
| func testSDKInitializes() { | ||
| _ = env.sdk | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Carried-forward prior finding: bootstrap test does not exercise the local DAPI endpoint
testSDKInitializes only evaluates env.sdk, which was already constructed during suite bootstrap — it's a no-op. The local DAPI gateway can be unreachable, mis-configured, or pointed at the wrong port and this test still passes. Make it a real smoke test by issuing a single proof-verified DAPI request (e.g. getStatus() or fetch network status) so a broken devnet/SDK wiring actually fails the test.
source: ['claude', 'codex']
| if let timeout { | ||
| let deadline = Date().addingTimeInterval(timeout) | ||
| while process.isRunning && Date() < deadline { | ||
| Thread.sleep(forTimeInterval: 0.05) | ||
| } | ||
| if process.isRunning { | ||
| process.terminate() | ||
| Thread.sleep(forTimeInterval: 0.5) | ||
| if process.isRunning { kill(process.processIdentifier, SIGKILL) } | ||
| } | ||
| } | ||
|
|
||
| process.waitUntilExit() |
There was a problem hiding this comment.
💬 Nitpick: Carried-forward prior finding: Shell.run blocks a Swift Concurrency worker with Thread.sleep + waitUntilExit
Shell.run is invoked transitively from async paths (IntegrationTestEnv.bootstrap() → LocalDevnet.readConfigRaw, and CoreRPC helpers). The timeout loop uses Thread.sleep(forTimeInterval: 0.05) plus a synchronous process.waitUntilExit(), blocking whichever Swift Concurrency cooperative-pool worker entered the async chain. Under XCTest this normally works because the pool is large, but a future change that fans out shell calls with async let could exhaust the pool. Either expose Shell as sync-only and dispatch from async callers onto a dedicated background queue, or implement the wait with Process.terminationHandler + a continuation.
source: ['claude', 'codex']
| # See SwiftTests/SwiftDashSDKIntegrationTests/README.md for one-time setup | ||
| # (`yarn dashmate setup --preset local`). |
There was a problem hiding this comment.
💬 Nitpick: Carried-forward prior finding: referenced README does not exist
The header comment points contributors to SwiftTests/SwiftDashSDKIntegrationTests/README.md for one-time setup, but only Core/, Platform/, and Support/ subdirectories exist there — no README. Either drop the reference or add the file with the setup instructions (yarn dashmate setup --preset local, supported Node version, env vars like DASHMATE_CONFIG and PLATFORM_REPO_ROOT).
source: ['claude', 'codex']
There was a problem hiding this comment.
Code Review
Verified all 8 prior findings against current head 9031b50 — all remain STILL VALID and are carried forward. The latest delta (def4241..9031b50) only adds CI provisioning of cargo-binstall, wasm-bindgen-cli@0.2.108, and wasm-pack to .github/workflows/swift-sdk-build.yml. One new finding: wasm-pack is installed unpinned alongside a pinned wasm-bindgen-cli, which weakens CI reproducibility. No blocking issues; all findings are suggestions/nitpicks against new test/CI infrastructure.
🟡 4 suggestion(s) | 💬 5 nitpick(s)
Most findings were posted inline. GitHub could not place the workflow findings on the current diff, so I’m including those carried-forward/latest-delta items here instead of letting them disappear:
Additional verified findings not posted inline
🟡 Suggestion: Carried-forward prior finding: reset step swallows all errors, hiding broken-devnet failures from CI
.github/workflows/swift-sdk-build.yml lines 110-116
The block uses set +e and a trailing true, so the step always exits 0 — even if yarn dashmate group reset --group=local --hard --verbose fails (corrupt state, missing config, permission issue). The integration-tests step then runs against stale or broken devnet state and can produce misleading failures or false greens. The wasm tooling steps added at lines 101–108 do not change this. Fail loudly on reset errors, or scope set +e to only the expected 'nothing to stop' case from group stop.
💬 Nitpick: New finding in latest delta: wasm-pack version is unpinned in CI
.github/workflows/swift-sdk-build.yml lines 107-108
The delta adds cargo binstall wasm-pack -y without a version pin, while the sibling step pins wasm-bindgen-cli@0.2.108. cargo binstall resolves to the latest published wasm-pack each time CI runs, so an upstream release can break the integration-test job on a green PR with no change in this repo. Pin wasm-pack to a known-good version (consistent with the wasm-bindgen-cli pin) so CI builds remain reproducible.
- name: Install wasm-pack
run: cargo binstall wasm-pack@0.13.1 -y
💬 Nitpick: Carried-forward prior finding: CI workflow and runner script duplicate (and can conflict on) dashmate lifecycle
.github/workflows/swift-sdk-build.yml lines 110-125
The workflow resets dashmate (lines 110–116), runs run_integration_tests.sh (118–120), then unconditionally stops dashmate (122–125). The script itself has its own 'start if not reachable' branch (run_integration_tests.sh:55–61) that also calls group stop then group start for --group=local. Two independent lifecycle owners increase the chance of ordering / config drift between local and CI, and make failures harder to diagnose. Consolidate ownership in one layer.
| func ensureSPVStarted() async throws { | ||
| if spvStarted { return } | ||
| let dir = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("swift-sdk-it-spv-\(UUID().uuidString.prefix(8))", isDirectory: true) | ||
| try? FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) | ||
|
|
||
| let manager = walletManager | ||
| let config = PlatformSpvStartConfig( | ||
| dataDir: dir.path, | ||
| network: .regtest, | ||
| userAgent: "swift-sdk-integration-tests/0.1", | ||
| peers: ["127.0.0.1:\(endpoints.coreP2P)"], | ||
| restrictToConfiguredPeers: true, | ||
| startFromHeight: 0 | ||
| ) | ||
| try await MainActor.run { try manager.startSpv(config: config) } | ||
| spvStarted = true | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior finding: ensureSPVStarted and trackedWalletIds have check-then-act races on nonisolated(unsafe) state
spvStarted (line 18) and trackedWalletIds (line 19) are declared nonisolated(unsafe). ensureSPVStarted() reads spvStarted, awaits a MainActor hop to manager.startSpv(config:), then writes spvStarted = true — two concurrent callers can both pass the guard and start SPV twice, each generating its own fresh temp dataDir and crossing the FFI boundary into platform_wallet_manager_spv_start, which doesn't return an 'already starting' error. makeTestWallet/purgeStoredMnemonics (lines 101–125) likewise mutate trackedWalletIds without synchronization. With XCTest's parallel execution this can corrupt state or double-start SPV mid-test. Isolate this state to @MainActor (the wallet manager already is) or guard with an actor / lock.
source: ['claude', 'codex']
| try await MainActor.run { | ||
| let context = ModelContext(container) | ||
| let allTxCount = try context.fetchCount(FetchDescriptor<PersistentTransaction>()) | ||
| XCTAssertEqual(allTxCount, expectedTotalTxs) |
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior finding: global PersistentTransaction count is coupled to the process-shared SwiftData store
IntegrationTestCase.sharedEnv() memoises a single IntegrationTestEnv per process, and IntegrationTestEnv.bootstrap() builds one in-memory ModelContainer reused across every test. tearDown only calls purgeStoredMnemonics() (Keychain), so SwiftData rows accumulate. As soon as another test class writes PersistentTransaction rows to the same shared container, XCTAssertEqual(allTxCount, expectedTotalTxs) becomes order-dependent. Scope the assertion to the wallets under test (filter by walletId in [aliceWalletId, bobWalletId]) the same way the TXO counts on lines 63–75 already do.
source: ['claude', 'codex']
| DASHMATE_CONFIG="${DASHMATE_CONFIG:-local_seed}" | ||
|
|
||
| cd "$SCRIPT_DIR" | ||
|
|
||
| node_major=$(node -p "process.versions.node.split('.')[0]" 2>/dev/null || echo 0) | ||
| if [ "$node_major" -gt 22 ] || [ "$node_major" -lt 20 ]; then | ||
| echo "Node $(node --version 2>/dev/null || echo '<missing>') is not supported by dashmate." | ||
| echo "Use Node 20–22 (e.g. \`nvm use 22\` or \`brew link node@22\`) and re-run." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! ls -d "$REPO_ROOT"/.yarn/unplugged/systeminformation-* >/dev/null 2>&1; then | ||
| echo "yarn install (materialising unplugged packages)…" | ||
| (cd "$REPO_ROOT" && yarn install) | ||
| fi | ||
| if [ ! -f "$REPO_ROOT/packages/wasm-dpp/dist/index.js" ]; then | ||
| # Apple's bundled clang lacks the wasm32 target; cc-rs picks it up | ||
| # via `CC_wasm32_unknown_unknown` (not `CC`, which only affects the | ||
| # host build). Wire in Homebrew's LLVM automatically when present. | ||
| if [ -z "${CC_wasm32_unknown_unknown:-}" ] && command -v brew >/dev/null 2>&1; then | ||
| brew_llvm="$(brew --prefix llvm 2>/dev/null || true)" | ||
| if [ -x "$brew_llvm/bin/clang" ]; then | ||
| export CC_wasm32_unknown_unknown="$brew_llvm/bin/clang" | ||
| export AR_wasm32_unknown_unknown="$brew_llvm/bin/llvm-ar" | ||
| fi | ||
| fi | ||
| echo "yarn build (~few min — builds wasm-dpp etc.)…" | ||
| (cd "$REPO_ROOT" && yarn build) | ||
| if [ ! -f "$REPO_ROOT/packages/wasm-dpp/dist/index.js" ]; then | ||
| echo "yarn build finished but wasm-dpp/dist is still missing." | ||
| echo " Likely a wasm32 toolchain issue. If Homebrew's LLVM isn't" | ||
| echo " installed, run \`brew install llvm\` and try again." | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| DAPI_PORT="${DAPI_PORT:-2443}" | ||
| dapi_reachable() { | ||
| nc -z 127.0.0.1 "$DAPI_PORT" >/dev/null 2>&1 | ||
| } | ||
|
|
||
| if dapi_reachable; then | ||
| echo "dashmate devnet already running" | ||
| else | ||
| echo "starting dashmate devnet group (this can take ~1-2 min)…" | ||
| (cd "$REPO_ROOT" && yarn dashmate group stop --group=local 2>/dev/null || true) | ||
| (cd "$REPO_ROOT" && yarn dashmate group start --group=local --wait-for-readiness) | ||
| fi |
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior finding: DASHMATE_CONFIG does not flow into the devnet lifecycle the runner manages
Line 14 reads DASHMATE_CONFIG=${DASHMATE_CONFIG:-local_seed} and IntegrationTestEnv.bootstrap() (line 43) uses it to discover endpoints via yarn dashmate config get --config=.... But the reachability probe (line 50) hard-codes DAPI_PORT=2443 and the start/stop calls (lines 59–60) hard-code --group=local. Any consumer who overrides DASHMATE_CONFIG will have the runner manage one devnet while the test binary discovers endpoints from another. Either drive the group/port from the same config the tests read, or document that the env var is effectively unused.
source: ['claude', 'codex']
| /// Placeholder until real Platform-side integration coverage lands | ||
| final class SDKBootstrapIntegrationTests: IntegrationTestCase { | ||
| func testSDKInitializes() { | ||
| _ = env.sdk | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Carried-forward prior finding: bootstrap test does not exercise the local DAPI endpoint
testSDKInitializes only evaluates env.sdk (line 7), which was already constructed during suite bootstrap by IntegrationTestEnv.bootstrap(). The expression is effectively a no-op — the local DAPI gateway can be unreachable, mis-configured, or pointed at the wrong port and this test still passes. Have it issue at least one round-trip RPC (e.g. getStatus()) so it becomes a real DAPI smoke test, matching the PR's stated goal of validating against a local network.
source: ['claude', 'codex']
| if let timeout { | ||
| let deadline = Date().addingTimeInterval(timeout) | ||
| while process.isRunning && Date() < deadline { | ||
| Thread.sleep(forTimeInterval: 0.05) | ||
| } | ||
| if process.isRunning { | ||
| process.terminate() | ||
| Thread.sleep(forTimeInterval: 0.5) | ||
| if process.isRunning { kill(process.processIdentifier, SIGKILL) } | ||
| } | ||
| } | ||
|
|
||
| process.waitUntilExit() |
There was a problem hiding this comment.
💬 Nitpick: Carried-forward prior finding: Shell.run blocks a Swift Concurrency worker with Thread.sleep + waitUntilExit
Shell.run is reached from async paths (e.g. IntegrationTestEnv.bootstrap() → LocalDevnet.discoverEndpoints → Shell.run, plus CoreRPC helpers). The timeout loop (lines 62–72) busy-waits with Thread.sleep(forTimeInterval: 0.05) and the call ends with synchronous process.waitUntilExit() (line 74), blocking whichever Swift Concurrency worker entered the async path. For a small test harness this is unlikely to deadlock, but it defeats structured concurrency and is fragile under increased concurrency. Wrap subprocess execution in a continuation that resumes on terminationHandler.
source: ['claude', 'codex']
| # See SwiftTests/SwiftDashSDKIntegrationTests/README.md for one-time setup | ||
| # (`yarn dashmate setup --preset local`). |
There was a problem hiding this comment.
💬 Nitpick: Carried-forward prior finding: referenced README does not exist
The header comment points to SwiftTests/SwiftDashSDKIntegrationTests/README.md for one-time setup, but the directory contains only Core/, Platform/, and Support/ — no README. The first people debugging this new harness will start from this script, so a dead documentation pointer is an unnecessary paper cut. Either drop the reference or add the file with the setup instructions.
source: ['claude', 'codex']
ZocoLini
force-pushed
the
feat/swift-sdk-integration-tests
branch
from
9069b49 to
ef01ba9
Compare
2 participants
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Tests
Chores
Documentation