chore(swift-sdk): swift-sdk test updated#3479
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request consolidates Swift SDK test infrastructure by migrating from a separate SwiftTests package with C FFI mocks to an integrated test suite within the main SwiftDashSDK package. The manifest is updated to link SystemConfiguration and declare a test target; new tests exercise wallet initialization, identity manager flows, and data parsing; a test runner script and updated CI workflow orchestrate the build and test execution. ChangesSwift SDK Test Infrastructure Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
✅ Review complete (commit 65be5fe) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (2)
71-93:⚠️ Potential issue | 🟠 MajorMisleading per-network caching when FFI doesn't support it.
The
identityManagersdictionary caches byPlatformNetwork, but the FFI functionplatform_wallet_info_get_identity_manager(per context snippet atPlatformWalletFFI.swift:26-38) doesn't accept a network parameter—it returns the same manager for all networks. This creates a misleading API where callers expect network isolation that doesn't exist.Consider either:
- Removing the
networkparameter and caching a single manager, or- Documenting that network-specific managers are a planned future feature
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift` around lines 71 - 93, The getIdentityManager(for network: PlatformNetwork) method currently caches IdentityManager instances per PlatformNetwork but the FFI call platform_wallet_info_get_identity_manager (see PlatformWalletFFI.swift) returns a single global manager, so remove the misleading per-network behavior by changing getIdentityManager to not take a network parameter, replace the identityManagers dictionary with a single cached property (e.g., identityManager: IdentityManager?), call platform_wallet_info_get_identity_manager once to populate that single cache (use the existing managerHandle/error handling and IdentityManager(handle:)), update any callers to use the new parameterless getter, and remove any network-based caching logic to ensure the API reflects the FFI capability.
18-48:⚠️ Potential issue | 🟠 MajorInconsistent Network types used in the same class.
fromSeedandfromMnemonicuseNetwork(alias forDashSDKNetwork), whilegetIdentityManagerandsetIdentityManagerusePlatformNetwork. These are different enums with different raw values (see issue inPlatformWalletTypes.swift), which will cause confusion and potential bugs when users work with both APIs.Consider unifying to a single network type throughout this class.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift` around lines 18 - 48, The class mixes two different enums (Network and PlatformNetwork) causing inconsistency; update the class to use a single network type (preferably Network, the alias for DashSDKNetwork) throughout: change the signatures and internal usage of getIdentityManager and setIdentityManager to accept Network instead of PlatformNetwork, and update any FFI calls inside those methods to pass the network raw value expected by the C layer (map or cast to the FFI/platform enum as needed), or vice versa if you choose PlatformNetwork—ensure all methods (fromSeed, fromMnemonic, getIdentityManager, setIdentityManager) reference the same enum and add conversion logic where the FFI requires the other enum type so rawValue alignment is preserved.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift (1)
101-110:⚠️ Potential issue | 🟠 MajorNetwork enum values mismatch with
DashSDKNetwork.
PlatformNetworkdefines:
devnet = 2,local = 3But
DashSDKNetwork(inSwiftDashSDK.swift) and the Rust FFI (packages/rs-sdk-ffi/src/types.rs) define:
regtest = 2,devnet = 3,local = 4This inconsistency will cause incorrect network selection when
PlatformNetworkvalues are passed to FFI calls expectingDashSDKNetworkvalues. The PR objectives mention unifying multiple Network enums as a follow-up, but this discrepancy could cause bugs in the meantime.🔧 Suggested fix to align with DashSDKNetwork
public enum PlatformNetwork: UInt32 { case mainnet = 0 case testnet = 1 - case devnet = 2 - case local = 3 + case regtest = 2 + case devnet = 3 + case local = 4 var ffiValue: NetworkType { NetworkType(self.rawValue) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift` around lines 101 - 110, PlatformNetwork's raw values don't match the FFI/DashSDKNetwork ordering; update the enum to align with DashSDKNetwork by inserting a regtest case and shifting devnet and local values (so: mainnet=0, testnet=1, regtest=2, devnet=3, local=4) in the PlatformNetwork declaration and ensure the ffiValue mapping (var ffiValue: NetworkType) continues to convert using NetworkType(self.rawValue); adjust any code that constructs PlatformNetwork from integers if present.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swift (1)
26-38:⚠️ Potential issue | 🟠 MajorFFI identity manager functions are network-agnostic, incompatible with per-network caching in Swift wrapper.
platform_wallet_info_get_identity_managerandplatform_wallet_info_set_identity_managerlack network parameters. The RustPlatformWalletInfostruct stores a singleidentity_managerfield shared across all networks. The Swift wrapper'sgetIdentityManager(for:)caches managers per network but calls the FFI function without network context—receiving the same manager handle regardless of which network is requested. This causestestMultipleNetworkIdentityManagersto fail, as it expects different handles for different networks but receives identical handles.Either implement network-specific managers in the Rust FFI layer, or redesign the Swift wrapper to acknowledge that a single shared manager serves all networks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swift` around lines 26 - 38, The FFI functions platform_wallet_info_get_identity_manager and platform_wallet_info_set_identity_manager are network-agnostic but the Swift wrapper's getIdentityManager(for:) caches managers per-network, causing identical handles to be returned for different networks; fix by either (A) adding a network parameter to the FFI signatures and underlying Rust PlatformWalletInfo to store per-network identity managers so platform_wallet_info_get_identity_manager/set_identity_manager accept a network identifier and return/set network-specific handles, or (B) change the Swift wrapper (getIdentityManager(for:), any per-network cache) to treat the identity manager as global/shared (remove per-network caching and use a single cached manager handle), and update related code/comments/tests (e.g., testMultipleNetworkIdentityManagers) to reflect the chosen behavior.
🧹 Nitpick comments (5)
packages/swift-sdk/Tests/SwiftDashSDKTests/IdentityManagerTests.swift (1)
4-16: Consider addingtearDown()to clean up resources.If
IdentityManagerholds FFI handles or other resources, adding atearDown()method to setidentityManager = nilensures proper cleanup between tests.♻️ Suggested addition
override func setUp() { super.setUp() do { identityManager = try IdentityManager.create() } catch { XCTFail("Failed to set up test: \(error)") } } + + override func tearDown() { + identityManager = nil + super.tearDown() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Tests/SwiftDashSDKTests/IdentityManagerTests.swift` around lines 4 - 16, Tests lack a tearDown to release resources held by IdentityManager; add an override func tearDown() in the IdentityManagerTests class that clears the identityManager reference (e.g., set identityManager = nil) and calls super.tearDown() so any FFI handles or other resources get cleaned between tests after using IdentityManager.create().packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift (1)
52-59: Clarify the expected behavior for odd-length hex strings.The comment notes the implementation "treats this as 1 byte," but actually it parses
"12"(first 2 chars) and silently drops the trailing"3". This truncation behavior may be intentional but could mask bugs if callers accidentally pass odd-length strings.Consider documenting this behavior more explicitly or updating the implementation to return
nilfor odd-length inputs if strict validation is preferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift` around lines 52 - 59, The test and implementation are ambiguous about how Data(hexString:) handles odd-length hex strings—currently it silently truncates the last nibble (parses "12" and drops "3"). Update the behavior and tests to be explicit: either (A) make Data.init(hexString:) perform strict validation and return nil for odd-length inputs (adjust or replace testDataFromOddLengthHexString to assert nil), or (B) document the truncation behavior in the Data(hexString:) initializer and update testDataFromOddLengthHexString to assert the exact parsed result (e.g., that "123" yields Data([0x12]) and not just non-nil). Locate the logic in the Data(hexString:) extension/initializer and the test testDataFromOddLengthHexString to apply the change and ensure unit tests reflect the chosen behavior.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift (1)
37-51:ErrorMemoryAllocationis not handled in the switch statement.The constant
ErrorMemoryAllocation = 11is defined but thePlatformWalletError.init(result:error:)switch (lines 71-96) doesn't have a case for it, causing memory allocation errors to fall through to.unknown. Consider adding a dedicated case if this error type should be distinguished.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift` around lines 37 - 51, The init switch in PlatformWalletError.init(result:error:) currently maps result codes but omits the ErrorMemoryAllocation (11) constant, so memory allocation errors fall through to .unknown; update the switch to handle ErrorMemoryAllocation by mapping it to a distinct PlatformWalletError case (e.g., .memoryAllocation) or add that enum case to PlatformWalletError if it doesn't exist, and ensure the case uses the provided error string (error) like the other cases for consistent messaging.packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift (2)
85-100: Inconsistent error type assertions.The invalid seed test (lines 93-99) properly validates the specific
PlatformWalletError.invalidParametercase, but the invalid mnemonic test (lines 87-89) only checks that the error isPlatformWalletErrorwithout verifying which case. Consider making both assertions consistent for better test coverage.♻️ Proposed fix
// Test invalid mnemonic XCTAssertThrowsError(try PlatformWallet.fromMnemonic("invalid mnemonic phrase")) { error in - XCTAssertTrue(error is PlatformWalletError) + if case PlatformWalletError.invalidParameter = error { + // Expected + } else { + XCTFail("Expected invalidParameter error, got \(error)") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift` around lines 85 - 100, In testWalletCreationErrorHandling update the invalid mnemonic assertion to mirror the seed test by checking for the specific PlatformWalletError case instead of only asserting the error type; modify the XCTAssertThrowsError closure around PlatformWallet.fromMnemonic("invalid mnemonic phrase") to pattern-match the thrown error and assert it equals the expected PlatformWalletError.invalidParameter (or the appropriate specific case) so both PlatformWallet.fromMnemonic and PlatformWallet.fromSeed checks consistently validate the exact PlatformWalletError case.
79-81: Redundant assertion adds no value.If execution reaches line 80, the stress test has already completed successfully. The
XCTAssertTrue(true, ...)is a no-op.♻️ Proposed fix
XCTAssertGreaterThanOrEqual(count, 0) } - - // If this completes without crashing, memory management is working - XCTAssertTrue(true, "Stress test completed") + // Test passes if loop completes without crashing - memory management is working }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift` around lines 79 - 81, The test contains a no-op assertion XCTAssertTrue(true, "Stress test completed") which adds no value; remove that line from the test (the one calling XCTAssertTrue(true, ...)) in PlatformWalletIntegrationTests so the test simply completes without the redundant assertion, or replace it with a meaningful assertion if there is an actual post-condition to verify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift`:
- Around line 39-47: The test passes only because
IDENTITY_MANAGER_STORAGE.insert() generates unique handles, but
getIdentityManager() / platform_wallet_info_get_identity_manager() are returning
clones of the same underlying identity_manager for different networks—masking
that the wallet is single-network. Fix by enforcing a single-network invariant
in the wrapper: update getIdentityManager(for:) to check the requested Network
against the wallet's configured network (e.g., compare requestedNetwork to
wallet.network) and either return the existing identity_manager handle stored on
the wallet (do not call IDENTITY_MANAGER_STORAGE.insert() to fabricate new
handles) or throw/reject when networks differ; alternatively, if true
multi-network support is desired, extend
platform_wallet_info_get_identity_manager() and the underlying wallet to accept
a Network parameter and return distinct per-network identity_manager instances.
Ensure references: getIdentityManager,
platform_wallet_info_get_identity_manager, IDENTITY_MANAGER_STORAGE.insert, and
identity_manager are updated accordingly.
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:
- Around line 71-93: The getIdentityManager(for network: PlatformNetwork) method
currently caches IdentityManager instances per PlatformNetwork but the FFI call
platform_wallet_info_get_identity_manager (see PlatformWalletFFI.swift) returns
a single global manager, so remove the misleading per-network behavior by
changing getIdentityManager to not take a network parameter, replace the
identityManagers dictionary with a single cached property (e.g.,
identityManager: IdentityManager?), call
platform_wallet_info_get_identity_manager once to populate that single cache
(use the existing managerHandle/error handling and IdentityManager(handle:)),
update any callers to use the new parameterless getter, and remove any
network-based caching logic to ensure the API reflects the FFI capability.
- Around line 18-48: The class mixes two different enums (Network and
PlatformNetwork) causing inconsistency; update the class to use a single network
type (preferably Network, the alias for DashSDKNetwork) throughout: change the
signatures and internal usage of getIdentityManager and setIdentityManager to
accept Network instead of PlatformNetwork, and update any FFI calls inside those
methods to pass the network raw value expected by the C layer (map or cast to
the FFI/platform enum as needed), or vice versa if you choose
PlatformNetwork—ensure all methods (fromSeed, fromMnemonic, getIdentityManager,
setIdentityManager) reference the same enum and add conversion logic where the
FFI requires the other enum type so rawValue alignment is preserved.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swift`:
- Around line 26-38: The FFI functions platform_wallet_info_get_identity_manager
and platform_wallet_info_set_identity_manager are network-agnostic but the Swift
wrapper's getIdentityManager(for:) caches managers per-network, causing
identical handles to be returned for different networks; fix by either (A)
adding a network parameter to the FFI signatures and underlying Rust
PlatformWalletInfo to store per-network identity managers so
platform_wallet_info_get_identity_manager/set_identity_manager accept a network
identifier and return/set network-specific handles, or (B) change the Swift
wrapper (getIdentityManager(for:), any per-network cache) to treat the identity
manager as global/shared (remove per-network caching and use a single cached
manager handle), and update related code/comments/tests (e.g.,
testMultipleNetworkIdentityManagers) to reflect the chosen behavior.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift`:
- Around line 101-110: PlatformNetwork's raw values don't match the
FFI/DashSDKNetwork ordering; update the enum to align with DashSDKNetwork by
inserting a regtest case and shifting devnet and local values (so: mainnet=0,
testnet=1, regtest=2, devnet=3, local=4) in the PlatformNetwork declaration and
ensure the ffiValue mapping (var ffiValue: NetworkType) continues to convert
using NetworkType(self.rawValue); adjust any code that constructs
PlatformNetwork from integers if present.
---
Nitpick comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift`:
- Around line 37-51: The init switch in PlatformWalletError.init(result:error:)
currently maps result codes but omits the ErrorMemoryAllocation (11) constant,
so memory allocation errors fall through to .unknown; update the switch to
handle ErrorMemoryAllocation by mapping it to a distinct PlatformWalletError
case (e.g., .memoryAllocation) or add that enum case to PlatformWalletError if
it doesn't exist, and ensure the case uses the provided error string (error)
like the other cases for consistent messaging.
In `@packages/swift-sdk/Tests/SwiftDashSDKTests/IdentityManagerTests.swift`:
- Around line 4-16: Tests lack a tearDown to release resources held by
IdentityManager; add an override func tearDown() in the IdentityManagerTests
class that clears the identityManager reference (e.g., set identityManager =
nil) and calls super.tearDown() so any FFI handles or other resources get
cleaned between tests after using IdentityManager.create().
In
`@packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift`:
- Around line 85-100: In testWalletCreationErrorHandling update the invalid
mnemonic assertion to mirror the seed test by checking for the specific
PlatformWalletError case instead of only asserting the error type; modify the
XCTAssertThrowsError closure around PlatformWallet.fromMnemonic("invalid
mnemonic phrase") to pattern-match the thrown error and assert it equals the
expected PlatformWalletError.invalidParameter (or the appropriate specific case)
so both PlatformWallet.fromMnemonic and PlatformWallet.fromSeed checks
consistently validate the exact PlatformWalletError case.
- Around line 79-81: The test contains a no-op assertion XCTAssertTrue(true,
"Stress test completed") which adds no value; remove that line from the test
(the one calling XCTAssertTrue(true, ...)) in PlatformWalletIntegrationTests so
the test simply completes without the redundant assertion, or replace it with a
meaningful assertion if there is an actual post-condition to verify.
In `@packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift`:
- Around line 52-59: The test and implementation are ambiguous about how
Data(hexString:) handles odd-length hex strings—currently it silently truncates
the last nibble (parses "12" and drops "3"). Update the behavior and tests to be
explicit: either (A) make Data.init(hexString:) perform strict validation and
return nil for odd-length inputs (adjust or replace
testDataFromOddLengthHexString to assert nil), or (B) document the truncation
behavior in the Data(hexString:) initializer and update
testDataFromOddLengthHexString to assert the exact parsed result (e.g., that
"123" yields Data([0x12]) and not just non-nil). Locate the logic in the
Data(hexString:) extension/initializer and the test
testDataFromOddLengthHexString to apply the change and ensure unit tests reflect
the chosen behavior.
🪄 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: 3a0b7f49-cc75-4b45-9455-dd96a52db433
📒 Files selected for processing (31)
packages/swift-sdk/Package.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SwiftDashSDK.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/TEST_FIX_SUMMARY.mdpackages/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/IdentityManagerTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/WalletSerializationTests.swiftpackages/swift-sdk/example/SwiftSDKExample.swiftpackages/swift-sdk/run_tests.sh
💤 Files with no reviewable changes (21)
- packages/swift-sdk/SwiftTests/TEST_FIX_SUMMARY.md
- packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/EstablishedContactTests.swift
- packages/swift-sdk/SwiftTests/Package.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DocumentTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ManagedIdentityTests.swift
- packages/swift-sdk/SwiftTests/run_tests.sh
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SwiftConstants.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityManagerTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ContactRequestTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/TokenTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DataContractTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDK.h
- packages/swift-sdk/example/SwiftSDKExample.swift
- packages/swift-sdk/SwiftTests/SwiftDashSDK.h
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDKMock.c
|
✅ 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: "94f01982bd5a53a32305a026d2f1eed1bb54b0c5e961e611cfbc4ae94baf2a7b"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR mostly cleans up the Swift package/test layout and aligns parts of the Platform Wallet bridge with the current Rust FFI, but two wrapper changes still leave the Swift API out of sync with the actual platform-wallet ABI. The most serious issue is the new networked wallet-creation API: it now forwards the unified SDK network enum directly into the platform-wallet FFI even though that FFI uses a different network enum layout and valid value set. Separately, the Swift layer still exposes per-network identity-manager methods even though the FFI calls are now wallet-global, so the local cache can diverge from the Rust backend state.
Reviewed commit: dab130e
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding
🟡 suggestion: The Swift API still pretends identity managers are network-scoped after the FFI removed network scoping
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (lines 71-109)
getIdentityManager(for:) / setIdentityManager(_:for:) still cache managers under separate PlatformNetwork keys, but this PR removed the network argument from the actual FFI calls. On the Rust side, platform_wallet_info_get_identity_manager and platform_wallet_info_set_identity_manager now operate on the wallet's single stored identity_manager, so the Swift cache is manufacturing per-network separation that no longer exists in the backend. That means one call can update the wallet-global manager while another network key continues returning a stale cached wrapper, and the new testMultipleNetworkManagers tests can still pass because they compare handle identity rather than verifying backend isolation. The wrapper should either restore explicit network scoping in the FFI or collapse this Swift API/cache to a single wallet-global manager so callers cannot observe inconsistent per-network state.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:
- [BLOCKING] lines 18-58: Wallet creation now sends the wrong network enum into the platform-wallet FFI
The new `network:` overloads are typed as `SwiftDashSDK.Network` (`DashSDKNetwork`) and pass that value straight into `platform_wallet_info_create_from_seed` / `platform_wallet_info_create_from_mnemonic`. That is not the same enum the platform-wallet FFI expects. `DashSDKNetwork` uses `mainnet=0, testnet=1, regtest=2, devnet=3, local=4` in `packages/rs-sdk-ffi/src/types.rs`, while the Rust platform-wallet side uses `dashcore::Network`, whose discriminants are `Dash=0, Testnet=1, Devnet=2, Regtest=3` (see the checked-out dash-network dependency at `~/.cargo/git/checkouts/rust-dashcore-a35422abb966ea16/6affdaa/dash-network/src/lib.rs`). So `.regtest` now creates a devnet wallet, `.devnet` creates a regtest wallet, and `.local` passes an invalid discriminant across the FFI boundary. Before this PR the Swift bridge used `PlatformNetwork.ffiValue`, which matched the platform-wallet network layout. The fix here is to keep using the platform-wallet network enum/mapping for these entry points instead of reusing the unified SDK enum directly.
- [SUGGESTION] lines 71-109: The Swift API still pretends identity managers are network-scoped after the FFI removed network scoping
`getIdentityManager(for:)` / `setIdentityManager(_:for:)` still cache managers under separate `PlatformNetwork` keys, but this PR removed the network argument from the actual FFI calls. On the Rust side, `platform_wallet_info_get_identity_manager` and `platform_wallet_info_set_identity_manager` now operate on the wallet's single stored `identity_manager`, so the Swift cache is manufacturing per-network separation that no longer exists in the backend. That means one call can update the wallet-global manager while another network key continues returning a stale cached wrapper, and the new `testMultipleNetworkManagers` tests can still pass because they compare handle identity rather than verifying backend isolation. The wrapper should either restore explicit network scoping in the FFI or collapse this Swift API/cache to a single wallet-global manager so callers cannot observe inconsistent per-network state.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The Swift wrapper feeds a 4-byte DashSDKNetwork (mainnet=0, testnet=1, regtest=2, devnet=3, local=4) into Rust FFI functions that take dashcore::Network (#[repr(u8)], mainnet=0, testnet=1, devnet=2, regtest=3, no Local). Confirmed against rs-platform-wallet-ffi/src/types.rs:3 (pub use dashcore::Network) and rust-dashcore/dash-network/src/lib.rs. The default .testnet happens to coincide, masking the bug — every other network value either selects the wrong chain or invokes UB. The identity-manager API still presents per-network behavior over an FFI/Rust layer that is no longer network-aware. The C header is stale relative to the Rust source.
Reviewed commit: dab130e
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
4 additional findings
🔴 blocking: Network parameter ABI/discriminant mismatch — `.regtest`/`.devnet` swapped, `.local` is UB
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swift (lines 8-24)
The new network parameter in platform_wallet_info_create_from_seed and platform_wallet_info_create_from_mnemonic is typed as Network (typealias for DashSDKNetwork in Sources/SwiftDashSDK/SwiftDashSDK.swift:5). DashSDKNetwork is #[repr(C)] (4-byte int) with Mainnet=0, Testnet=1, Regtest=2, Devnet=3, Local=4 (packages/rs-sdk-ffi/src/types.rs:39-52). The Rust callees take crate::types::Network, which is pub use dashcore::Network (packages/rs-platform-wallet-ffi/src/types.rs:3); dashcore::Network is #[repr(u8)] with Mainnet=0, Testnet=1, Devnet=2, Regtest=3 and has no Local variant.
Consequences when called from Swift:
.regtest(raw 2) → Rust seesDevnet→key_wallet::Wallet::from_seed_bytesderives keys for the wrong network..devnet(raw 3) → Rust seesRegtest→ same problem in the opposite direction..local(raw 4) → no validdashcore::Networkdiscriminant; constructing the enum value from foreign code is undefined behavior, and any subsequentmatchis unsound.- ABI size mismatch (4-byte vs 1-byte) is masked on AArch64/x86_64 register-passed arguments but is still a contract violation.
For a wallet SDK, silently mis-deriving keys onto the wrong chain is a correctness/security hazard — funds or identities created "on regtest" are actually on devnet. The default .testnet happens to coincide on both sides, which is why the new tests pass.
Fix requires a dedicated FFI Network type that is byte-compatible with dashcore::Network (e.g., #[repr(u8)] with the same variant order) and an explicit Swift-side conversion from DashSDKNetwork, not just renaming the parameter.
🔴 blocking: `getIdentityManager(for:)` / `setIdentityManager(_,for:)` falsely advertise per-network scoping
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (lines 70-110)
The Swift API takes a PlatformNetwork and uses it as a key in the identityManagers cache, but the underlying FFI is no longer network-aware: platform_wallet_info_get_identity_manager/_set_identity_manager take only a wallet+manager handle (packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:207-271), and PlatformWalletInfo stores exactly one identity_manager field (packages/rs-platform-wallet/src/platform_wallet_info/mod.rs).
Observable consequences:
setIdentityManager(m, for: .mainnet)followed bygetIdentityManager(for: .testnet)returns a fresh clone of the same Rust-side slot; setting one network silently overwrites every other network's manager.- The Swift cache and the Rust state diverge: subsequent gets for an uncached network produce a different Swift handle representing the same Rust state, so equality checks on
IdentityManagerinstances are misleading. - The new
testMultipleNetworkManagerstest only passes because each FFI call clones into a new handle inIDENTITY_MANAGER_STORAGE— it does not demonstrate per-network state.
Fix: drop the for: PlatformNetwork parameter to match the FFI, or restore per-network storage in Rust. Leaving the parameter as a Swift-only cache key creates exactly the kind of cross-language semantic drift that surfaces as identities appearing under the wrong network.
🟡 suggestion: C header is out of sync with the Rust signatures
packages/rs-platform-wallet-ffi/platform_wallet_ffi.h (lines 128-178)
Verified: the header still declares platform_wallet_info_create_from_seed(seed_bytes, seed_len, out_handle, out_error) and _create_from_mnemonic(mnemonic, passphrase, out_handle, out_error) — neither takes the new leading network parameter present in src/platform_wallet_info.rs:11-17,81-87. Conversely, platform_wallet_info_get_identity_manager/_set_identity_manager in the header still take NetworkType network (lines 158-163), which the Rust functions no longer accept (lines 207-211, 245-249). Any C consumer, or a Swift consumer using the modulemap rather than @_silgen_name, will compile against a wrong ABI. Regenerate or hand-edit the header to match the Rust source before merge.
🟡 suggestion: `PlatformNetwork` and `DashSDKNetwork` use inconsistent numbering — both are public and both are used here
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift (lines 100-110)
PlatformNetwork is mainnet=0, testnet=1, devnet=2, local=3 (no regtest). DashSDKNetwork (exposed as Network in SwiftDashSDK.swift:5) is mainnet=0, testnet=1, regtest=2, devnet=3, local=4. Neither matches dashcore::Network (mainnet=0, testnet=1, devnet=2, regtest=3).
This PR introduces a public API where wallet creation takes Network (DashSDKNetwork) and the identity-manager getters take PlatformNetwork, in the same module. Callers can easily pick the wrong type — and because PlatformNetwork.devnet (2) collides numerically with DashSDKNetwork.regtest (2), there is no compiler protection against this kind of mistake when raw values cross any boundary. Either delete one of the two enums or document the mapping explicitly; ideally land the unification before changing the FFI surface.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swift`:
- [BLOCKING] lines 8-24: Network parameter ABI/discriminant mismatch — `.regtest`/`.devnet` swapped, `.local` is UB
The new `network` parameter in `platform_wallet_info_create_from_seed` and `platform_wallet_info_create_from_mnemonic` is typed as `Network` (typealias for `DashSDKNetwork` in `Sources/SwiftDashSDK/SwiftDashSDK.swift:5`). `DashSDKNetwork` is `#[repr(C)]` (4-byte int) with `Mainnet=0, Testnet=1, Regtest=2, Devnet=3, Local=4` (`packages/rs-sdk-ffi/src/types.rs:39-52`). The Rust callees take `crate::types::Network`, which is `pub use dashcore::Network` (`packages/rs-platform-wallet-ffi/src/types.rs:3`); `dashcore::Network` is `#[repr(u8)]` with `Mainnet=0, Testnet=1, Devnet=2, Regtest=3` and has no Local variant.
Consequences when called from Swift:
- `.regtest` (raw 2) → Rust sees `Devnet` → `key_wallet::Wallet::from_seed_bytes` derives keys for the wrong network.
- `.devnet` (raw 3) → Rust sees `Regtest` → same problem in the opposite direction.
- `.local` (raw 4) → no valid `dashcore::Network` discriminant; constructing the enum value from foreign code is undefined behavior, and any subsequent `match` is unsound.
- ABI size mismatch (4-byte vs 1-byte) is masked on AArch64/x86_64 register-passed arguments but is still a contract violation.
For a wallet SDK, silently mis-deriving keys onto the wrong chain is a correctness/security hazard — funds or identities created "on regtest" are actually on devnet. The default `.testnet` happens to coincide on both sides, which is why the new tests pass.
Fix requires a dedicated FFI Network type that is byte-compatible with `dashcore::Network` (e.g., `#[repr(u8)]` with the same variant order) and an explicit Swift-side conversion from `DashSDKNetwork`, not just renaming the parameter.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:
- [BLOCKING] lines 70-110: `getIdentityManager(for:)` / `setIdentityManager(_,for:)` falsely advertise per-network scoping
The Swift API takes a `PlatformNetwork` and uses it as a key in the `identityManagers` cache, but the underlying FFI is no longer network-aware: `platform_wallet_info_get_identity_manager`/`_set_identity_manager` take only a wallet+manager handle (`packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:207-271`), and `PlatformWalletInfo` stores exactly one `identity_manager` field (`packages/rs-platform-wallet/src/platform_wallet_info/mod.rs`).
Observable consequences:
1. `setIdentityManager(m, for: .mainnet)` followed by `getIdentityManager(for: .testnet)` returns a fresh clone of the same Rust-side slot; setting one network silently overwrites every other network's manager.
2. The Swift cache and the Rust state diverge: subsequent gets for an uncached network produce a *different* Swift handle representing the same Rust state, so equality checks on `IdentityManager` instances are misleading.
3. The new `testMultipleNetworkManagers` test only passes because each FFI call clones into a new handle in `IDENTITY_MANAGER_STORAGE` — it does not demonstrate per-network state.
Fix: drop the `for: PlatformNetwork` parameter to match the FFI, or restore per-network storage in Rust. Leaving the parameter as a Swift-only cache key creates exactly the kind of cross-language semantic drift that surfaces as identities appearing under the wrong network.
In `packages/rs-platform-wallet-ffi/platform_wallet_ffi.h`:
- [SUGGESTION] lines 128-178: C header is out of sync with the Rust signatures
Verified: the header still declares `platform_wallet_info_create_from_seed(seed_bytes, seed_len, out_handle, out_error)` and `_create_from_mnemonic(mnemonic, passphrase, out_handle, out_error)` — neither takes the new leading `network` parameter present in `src/platform_wallet_info.rs:11-17,81-87`. Conversely, `platform_wallet_info_get_identity_manager`/`_set_identity_manager` in the header still take `NetworkType network` (lines 158-163), which the Rust functions no longer accept (lines 207-211, 245-249). Any C consumer, or a Swift consumer using the modulemap rather than `@_silgen_name`, will compile against a wrong ABI. Regenerate or hand-edit the header to match the Rust source before merge.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift`:
- [SUGGESTION] lines 100-110: `PlatformNetwork` and `DashSDKNetwork` use inconsistent numbering — both are public and both are used here
`PlatformNetwork` is `mainnet=0, testnet=1, devnet=2, local=3` (no regtest). `DashSDKNetwork` (exposed as `Network` in `SwiftDashSDK.swift:5`) is `mainnet=0, testnet=1, regtest=2, devnet=3, local=4`. Neither matches `dashcore::Network` (`mainnet=0, testnet=1, devnet=2, regtest=3`).
This PR introduces a public API where wallet creation takes `Network` (`DashSDKNetwork`) and the identity-manager getters take `PlatformNetwork`, in the same module. Callers can easily pick the wrong type — and because `PlatformNetwork.devnet` (2) collides numerically with `DashSDKNetwork.regtest` (2), there is no compiler protection against this kind of mistake when raw values cross any boundary. Either delete one of the two enums or document the mapping explicitly; ideally land the unification before changing the FFI surface.
In `packages/swift-sdk/Package.swift`:
- [SUGGESTION] lines 31-36: Test reorganization narrows wrapper coverage and never exercises non-default networks
The new `SwiftDashSDKTests` target replaces the old standalone `SwiftTests` package, dropping `SDKTests`, `IdentityTests`, `DataContractTests`, `DocumentTests`, `TokenTests`, `ContactRequestTests`, `EstablishedContactTests`, `ManagedIdentityTests`, and `MemoryManagementTests`. The deleted suite ran against a mock C shim (`SwiftDashSDKMock.c`), so its loss is partially mitigated, but the surviving tests are nearly all `PlatformWallet`/serialization. Combined with the Network ABI bug above, this is consequential: every wallet creation test relies on the default `network: Network = .testnet`, which happens to be the only value that maps correctly across the FFI. Adding tests for `.regtest`, `.devnet`, and `.local` (e.g., comparing derived addresses against fixtures) would have surfaced the swap and the invalid `.local` discriminant.
ZocoLini
force-pushed
the
chore/swift-sdk-tests
branch
2 times, most recently
from
24bc648 to
93a26b0
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
`@packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift`:
- Around line 11-21: The setUp currently swallows errors with XCTFail and allows
subsequent IUO accesses to crash; change setUp to either override
setUpWithError() throws and perform wallet = try
PlatformWallet.fromMnemonic(...) and identityManager = try
wallet.getIdentityManager(for: .testnet) so failures throw a test error, or if
you must keep override func setUp(), after catching the error call XCTFail(...)
and immediately return to avoid accessing wallet and identityManager; target
symbols: setUp / setUpWithError, wallet, identityManager,
PlatformWallet.fromMnemonic, getIdentityManager.
🪄 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: 0543cbbc-e696-4470-a2b0-93d3787599b0
📒 Files selected for processing (29)
.codecov.yml.serena/project.ymlpackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/swift-sdk/Package.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/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/IdentityManagerTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swiftpackages/swift-sdk/example/SwiftSDKExample.swiftpackages/swift-sdk/run_tests.sh
💤 Files with no reviewable changes (21)
- packages/swift-sdk/SwiftTests/Package.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityManagerTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SwiftConstants.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/EstablishedContactTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/TokenTests.swift
- packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ManagedIdentityTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ContactRequestTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swift
- packages/swift-sdk/SwiftTests/run_tests.sh
- packages/swift-sdk/Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swift
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDK.h
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityTests.swift
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDKMock.c
- packages/swift-sdk/example/SwiftSDKExample.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DocumentTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DataContractTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift
- packages/swift-sdk/SwiftTests/SwiftDashSDK.h
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swift
✅ Files skipped from review due to trivial changes (3)
- packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs
- .serena/project.yml
- packages/swift-sdk/Package.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/swift-sdk/run_tests.sh
- packages/swift-sdk/Tests/SwiftDashSDKTests/IdentityManagerTests.swift
- packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift
ZocoLini
force-pushed
the
chore/swift-sdk-tests
branch
from
93a26b0 to
c3d7bb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift (1)
40-47: ⚡ Quick winMake odd-length hex behavior deterministic in the assertion.
Current check (
XCTAssertNotNil) allows multiple behaviors and can mask regressions.Proposed tightening
- // Should handle gracefully (depends on implementation) - // Current implementation treats this as 1 byte - XCTAssertNotNil(data) + // Current behavior: odd trailing nibble is ignored, resulting in 1 byte. + XCTAssertEqual(data?.count, 1)🤖 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/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift` around lines 40 - 47, The test testDataFromOddLengthHexString is non-deterministic because XCTAssertNotNil allows multiple behaviors; change it to assert the exact expected Data value from Data(hexString:), e.g. replace XCTAssertNotNil(data) with an equality assertion that data == Data([0x01, 0x23]) (or the exact byte sequence your Data(hexString:) implementation produces) so the test fails on regressions and documents the intended odd-length-hex handling.
🤖 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/Tests/SwiftDashSDKTests/IdentityManagerTests.swift`:
- Around line 8-16: The test setup should throw on failure instead of swallowing
errors; replace the current override func setUp() with override func
setUpWithError() throws, call try super.setUpWithError(), and assign
identityManager with identityManager = try IdentityManager.create() (remove the
do/catch and XCTFail), so any creation error is propagated and the test setup
fails cleanly; keep the identityManager property usage unchanged in tests.
---
Nitpick comments:
In `@packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift`:
- Around line 40-47: The test testDataFromOddLengthHexString is
non-deterministic because XCTAssertNotNil allows multiple behaviors; change it
to assert the exact expected Data value from Data(hexString:), e.g. replace
XCTAssertNotNil(data) with an equality assertion that data == Data([0x01, 0x23])
(or the exact byte sequence your Data(hexString:) implementation produces) so
the test fails on regressions and documents the intended odd-length-hex
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2873c1c2-f30d-49ff-aacb-056b9a84e9d3
📒 Files selected for processing (30)
.codecov.yml.serena/project.ymlpackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/swift-sdk/Package.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/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/IdentityManagerTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swiftpackages/swift-sdk/build_ios.shpackages/swift-sdk/example/SwiftSDKExample.swiftpackages/swift-sdk/run_tests.sh
💤 Files with no reviewable changes (21)
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SwiftConstants.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DataContractTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift
- packages/swift-sdk/Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityManagerTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/EstablishedContactTests.swift
- packages/swift-sdk/SwiftTests/run_tests.sh
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ContactRequestTests.swift
- packages/swift-sdk/example/SwiftSDKExample.swift
- packages/swift-sdk/SwiftTests/Package.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DocumentTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/TokenTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ManagedIdentityTests.swift
- packages/swift-sdk/SwiftTests/SwiftDashSDK.h
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDK.h
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift
- packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDKMock.c
✅ Files skipped from review due to trivial changes (3)
- .serena/project.yml
- packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs
- .codecov.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swift-sdk/run_tests.sh
- packages/swift-sdk/Package.swift
ZocoLini
force-pushed
the
chore/swift-sdk-tests
branch
3 times, most recently
from
0f85135 to
4262808
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR is a Swift SDK test reorganization. The new tests surface a real correctness bug: PlatformWallet.getIdentityManager(for:) / setIdentityManager(_:for:) accept a Network argument that is dropped at the FFI boundary (the Rust ABI takes no network and operates on a single wallet_info.identity_manager). Multiple new tests pass for the wrong reason — either via the Swift-side cache or because each FFI call mints a fresh handle around a clone of the same Rust manager — giving false confidence in per-network isolation. The PlatformWalletError enum-coverage test is also stale (12 of 14 cases). Codex's run_tests.sh finding is valid.
Reviewed commit: c3d7bb9
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 2 nitpick(s)
2 additional findings
🔴 blocking: `network` parameter is silently dropped at the FFI boundary in get/setIdentityManager
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (lines 56-74)
getIdentityManager(for: Network) (line 56) and setIdentityManager(_:for: Network) (line 71) accept a Network, but neither value is passed across the FFI. The Rust entry points platform_wallet_info_get_identity_manager and platform_wallet_info_set_identity_manager (packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:87-117) take no network argument and read/write a single wallet_info.identity_manager field. Consequences: (1) setIdentityManager(..., for: .mainnet) overwrites the same Rust slot returned for .testnet / .devnet; (2) each get call runs IDENTITY_MANAGER_STORAGE.insert(manager.clone()), minting a fresh handle around a clone of the SAME underlying manager, so mutations through one Swift wrapper are not observable through another even though they logically share Rust state. The new tests in this PR exercise this API as if it were network-scoped, locking in a contract the FFI does not implement. Either remove the Network parameter from the Swift API or extend the Rust FFI to key managers per network and update callers.
💬 nitpick: Use `withCString` instead of `NSString.utf8String` at the FFI boundary
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (lines 44-50)
(mnemonic as NSString).utf8String returns a const char * whose backing buffer is owned by an autoreleased NSString. In a single synchronous FFI call this normally outlives the call, but the contract is "valid until the autorelease pool is drained", not call-scoped — and Swift bridging optimizations can shorten the effective lifetime. The lifetime-guaranteed primitive at a C-string boundary is mnemonic.withCString { cStr in try platform_wallet_info_create_from_mnemonic(network.ffiValue, cStr, &handle).check() }. The Rust side (CStr::from_ptr(...).to_str() at platform_wallet_info.rs:65) reads the bytes before returning, so withCString is sufficient and strictly safer.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:
- [BLOCKING] lines 56-74: `network` parameter is silently dropped at the FFI boundary in get/setIdentityManager
`getIdentityManager(for: Network)` (line 56) and `setIdentityManager(_:for: Network)` (line 71) accept a `Network`, but neither value is passed across the FFI. The Rust entry points `platform_wallet_info_get_identity_manager` and `platform_wallet_info_set_identity_manager` (packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:87-117) take no network argument and read/write a single `wallet_info.identity_manager` field. Consequences: (1) `setIdentityManager(..., for: .mainnet)` overwrites the same Rust slot returned for `.testnet` / `.devnet`; (2) each `get` call runs `IDENTITY_MANAGER_STORAGE.insert(manager.clone())`, minting a fresh handle around a clone of the SAME underlying manager, so mutations through one Swift wrapper are not observable through another even though they logically share Rust state. The new tests in this PR exercise this API as if it were network-scoped, locking in a contract the FFI does not implement. Either remove the `Network` parameter from the Swift API or extend the Rust FFI to key managers per network and update callers.
In `packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift`:
- [SUGGESTION] lines 61-80: `testSetIdentityManager` and `testMultipleNetworkManagers` pass for the wrong reason
`testSetIdentityManager` (line 61) succeeds only because `setIdentityManager` populates `identityManagers[network]` (PlatformWallet.swift:73) and the subsequent `getIdentityManager` short-circuits on that dictionary at line 58-60 — the FFI set/get pair is never actually round-tripped. `testMultipleNetworkManagers` (line 71) asserts handle inequality across `.mainnet`/`.testnet`/`.devnet`, but every call to `platform_wallet_info_get_identity_manager` runs `IDENTITY_MANAGER_STORAGE.insert(...)` (platform_wallet_info.rs:97), so handles are *always* distinct regardless of whether per-network semantics work. Both tests therefore give false confidence: they would still pass after the underlying behavior regressed. To make them meaningful, invalidate the Swift cache (or compare against a fresh `PlatformWallet` wrapping the same FFI handle) and assert behavioral isolation — e.g., write through one network's manager and observe the change/absence-of-change through another. The same pattern applies to `PlatformWalletIntegrationTests.testMultipleNetworkIdentityManagers` at PlatformWalletIntegrationTests.swift:39-47.
In `packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift`:
- [SUGGESTION] lines 51-72: `PlatformWalletError` coverage test is stale and won't fail when new cases are added
`PlatformWalletError` (PlatformWalletResult.swift:114-128) declares 14 cases. The test constructs only 12, omitting `memoryAllocation` and `notFound`, then asserts `errors.count == 12` with the message "All error cases should be defined". The assertion is hard-coded against the literal list, so adding or removing a case in the enum will not break this test. Either add the two missing cases (and update the count), or have the enum conform to `CaseIterable` and iterate over `PlatformWalletError.allCases` so the test is genuinely exhaustive.
In `packages/swift-sdk/run_tests.sh`:
- [SUGGESTION] line 10: Test runner unnecessarily requires the iOS-simulator Rust toolchain
`run_tests.sh` runs `swift test`, which executes on the host macOS target. It does not need the `aarch64-apple-ios-sim` slice. Building `--target sim` here means the script fails on environments (CI images, contributor machines) that have a working macOS Rust toolchain but no iOS-simulator target installed, even though the tests themselves could run successfully. Drop `--target sim` so the runner only builds the slice it actually needs.
73b1cc2 to
555d97b
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior network ABI/discriminant blockers are resolved on this head: FFINetwork is now repr(C) and the Swift Network enum mirrors it through an explicit ffiValue switch. One real blocker persists: PlatformWallet.getIdentityManager(for:) / setIdentityManager(_:for:) take a Network argument that is never forwarded to Rust — the FFI is wallet-global, with no per-network keying, and the new tests added in this PR pass for cache-related reasons rather than exercising actual per-network behavior. A couple of nitpicks around test assertion strength and FFI string-lifetime hygiene remain.
Reviewed commit: 555d97b
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 additional findings
🔴 blocking: `network` parameter is silently dropped at the FFI in get/setIdentityManager
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (lines 55-74)
getIdentityManager(for: Network) (line 56) and setIdentityManager(_:for: Network) (line 71) accept a Network and key the Swift identityManagers cache on it, but the value never crosses the FFI. The Rust entry points platform_wallet_info_get_identity_manager(wallet_handle, out_handle) and platform_wallet_info_set_identity_manager(wallet_handle, manager_handle) take only handles (packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:87-117) and operate on the single wallet_info.identity_manager field — there is no per-network keying on the Rust side.
Observable consequences:
- Cross-network corruption on set.
setIdentityManager(m, for: .mainnet)overwrites the single Rust-sideidentity_manager. A subsequentgetIdentityManager(for: .testnet)that misses the Swift cache fetches that mainnet-derived manager and presents it as the testnet manager. The Swift API promises isolation the Rust state cannot provide. - Swift cache vs. Rust state drift.
getIdentityManagershort-circuits onidentityManagers[network](lines 58-60), so once a network slot is cached, the wrapper hands back stale objects that no longer reflect mutations made through any other network key. - Each cache-miss fetch mints a fresh handle around a clone (
IDENTITY_MANAGER_STORAGE.insert(wallet_info.identity_manager.clone()), line 97), so distinct Swift handles can wrap independent clones of the same Rust state — mutations on one are invisible through the other.
For a wallet SDK this is exactly the kind of cross-language semantic drift that surfaces as identities appearing under the wrong network or silently being lost when one network's manager is replaced. Pick one: drop the for: Network parameter (and the identityManagers dictionary) to match the FFI's actual single-manager semantics, or extend the Rust side so PlatformWalletInfo stores per-network identity managers and the FFI takes an FFINetwork argument, then thread it through Swift.
💬 nitpick: Use `withCString` instead of `NSString.utf8String` at the FFI boundary
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (lines 44-50)
(mnemonic as NSString).utf8String returns a const char * whose backing buffer is owned by the bridged NSString, with a lifetime tied to the autorelease pool rather than the call scope. The Rust side reads the bytes synchronously via CStr::from_ptr(...).to_str() (platform_wallet_info.rs:64-65), so today this works, but String.withCString is the lifetime-guaranteed primitive for this boundary and avoids relying on Foundation bridging/autorelease behavior. The seed path already uses withUnsafeBytes for the same reason — match that hygiene here with mnemonic.withCString { cStr in try platform_wallet_info_create_from_mnemonic(network.ffiValue, cStr, &handle).check() }.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:
- [BLOCKING] lines 55-74: `network` parameter is silently dropped at the FFI in get/setIdentityManager
`getIdentityManager(for: Network)` (line 56) and `setIdentityManager(_:for: Network)` (line 71) accept a `Network` and key the Swift `identityManagers` cache on it, but the value never crosses the FFI. The Rust entry points `platform_wallet_info_get_identity_manager(wallet_handle, out_handle)` and `platform_wallet_info_set_identity_manager(wallet_handle, manager_handle)` take only handles (packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:87-117) and operate on the single `wallet_info.identity_manager` field — there is no per-network keying on the Rust side.
Observable consequences:
1. Cross-network corruption on set. `setIdentityManager(m, for: .mainnet)` overwrites the single Rust-side `identity_manager`. A subsequent `getIdentityManager(for: .testnet)` that misses the Swift cache fetches that mainnet-derived manager and presents it as the testnet manager. The Swift API promises isolation the Rust state cannot provide.
2. Swift cache vs. Rust state drift. `getIdentityManager` short-circuits on `identityManagers[network]` (lines 58-60), so once a network slot is cached, the wrapper hands back stale objects that no longer reflect mutations made through any other network key.
3. Each cache-miss fetch mints a fresh handle around a clone (`IDENTITY_MANAGER_STORAGE.insert(wallet_info.identity_manager.clone())`, line 97), so distinct Swift handles can wrap independent clones of the same Rust state — mutations on one are invisible through the other.
For a wallet SDK this is exactly the kind of cross-language semantic drift that surfaces as identities appearing under the wrong network or silently being lost when one network's manager is replaced. Pick one: drop the `for: Network` parameter (and the `identityManagers` dictionary) to match the FFI's actual single-manager semantics, or extend the Rust side so `PlatformWalletInfo` stores per-network identity managers and the FFI takes an `FFINetwork` argument, then thread it through Swift.
In `packages/swift-sdk/run_tests.sh`:
- [SUGGESTION] lines 29-31: CI no longer builds or tests the SwiftExampleApp scheme
Before this PR, `.github/workflows/swift-sdk-build.yml` had explicit `xcodebuild build` steps for both the `SwiftDashSDK` scheme inside `SwiftExampleApp.xcodeproj` and the `SwiftExampleApp` scheme itself. After this PR the workflow delegates entirely to `run_tests.sh`, which only does `swift build` / `swift test` (Swift Package Manager) and `xcodebuild test -scheme SwiftDashSDK` against the SwiftPM-autogenerated scheme. The only checked-in Xcode scheme in `packages/swift-sdk` is `SwiftExampleApp.xcscheme` (`packages/swift-sdk/SwiftExampleApp/SwiftExampleApp.xcodeproj/xcshareddata/xcschemes/SwiftExampleApp.xcscheme`), and it is never invoked — neither for build nor for its `SwiftExampleAppTests` / `SwiftExampleAppUITests` targets. The Example app build coverage that previously caught warnings-as-errors regressions in the SwiftUI sample is now gone. If the intent is to drop SwiftExampleApp from CI, say so explicitly; otherwise add a `xcodebuild -project SwiftExampleApp/SwiftExampleApp.xcodeproj -scheme SwiftExampleApp` build (and optionally `test`) step to `run_tests.sh`.
ZocoLini
force-pushed
the
chore/swift-sdk-tests
branch
from
555d97b to
65be5fe
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:
- Around line 85-87: The workflow advertises warnings-as-errors but doesn't
enforce them; update the Swift test invocation to enforce it by exporting
SWIFT_TREAT_WARNINGS_AS_ERRORS=YES (or adding the equivalent -warnings-as-errors
flag) before calling packages/swift-sdk/run_tests.sh, or alternatively change
the job/workflow names ("Swift SDK build + tests (warnings as errors)" / "Build
and test Swift SDK (no warnings)") to remove the warnings-as-errors claim;
reference the run_tests.sh invocation in .github/workflows/swift-sdk-build.yml
and the SWIFT_TREAT_WARNINGS_AS_ERRORS / -warnings-as-errors flags when making
the change.
🪄 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: 4cf95182-bb41-4b6d-968f-705593edff20
📒 Files selected for processing (31)
.codecov.yml.github/workflows/swift-sdk-build.yml.serena/project.ymlpackages/swift-sdk/Package.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/KeyManagerTests.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/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/IdentityManagerTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swiftpackages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swiftpackages/swift-sdk/build_ios.shpackages/swift-sdk/example/SwiftSDKExample.swiftpackages/swift-sdk/run_tests.sh
💤 Files with no reviewable changes (21)
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift
- packages/swift-sdk/example/SwiftSDKExample.swift
- packages/swift-sdk/SwiftTests/Package.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ManagedIdentityTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swift
- packages/swift-sdk/Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/EstablishedContactTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DocumentTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/TokenTests.swift
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDK.h
- packages/swift-sdk/SwiftTests/run_tests.sh
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDKMock.c
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ContactRequestTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityManagerTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SwiftConstants.swift
- packages/swift-sdk/SwiftTests/SwiftDashSDK.h
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DataContractTests.swift
- packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift
✅ Files skipped from review due to trivial changes (2)
- .serena/project.yml
- .codecov.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/swift-sdk/Tests/SwiftDashSDKTests/IdentityManagerTests.swift
- packages/swift-sdk/Package.swift
- packages/swift-sdk/build_ios.sh
- packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Network ABI/discriminant blockers from prior reviews remain resolved on 65be5fe. One real correctness blocker persists: PlatformWallet.getIdentityManager(for:) / setIdentityManager(_:for:) accept a Network that is never forwarded to the Rust FFI, which only stores a single wallet-global identity_manager. The new tests entrench this broken contract by passing for cache/handle-allocation reasons. CI also now uploads a debug-built XCFramework artifact (run_tests.sh runs build_ios.sh --profile dev, regressing the prior --profile release). Minor FFI/test hygiene findings (NSString.utf8String lifetime, odd-length hex test) also persist.
Reviewed commit: 65be5fe
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
2 additional findings
🔴 blocking: `network` parameter is silently dropped at the FFI in get/setIdentityManager
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (lines 55-74)
getIdentityManager(for: Network) (line 56) and setIdentityManager(_:for: Network) (line 71) accept a Network and key the Swift identityManagers dictionary on it, but network never crosses the FFI. The Rust callees platform_wallet_info_get_identity_manager(wallet_handle, out_handle) and platform_wallet_info_set_identity_manager(wallet_handle, manager_handle) at packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:87-117 take only handles and operate on a single wallet_info.identity_manager field. There is no per-network keying anywhere on the Rust side.
Observable consequences across the boundary:
- Cross-network corruption on set.
setIdentityManager(m, for: .mainnet)overwrites the single Rust-sideidentity_manager(line 113). A subsequent uncachedgetIdentityManager(for: .testnet)returns a clone of that mainnet-derived manager and presents it as the testnet manager. - Swift cache vs. Rust state drift.
getIdentityManagershort-circuits onidentityManagers[network](lines 58-60), so once a network slot is populated, the wrapper continues handing back stale objects even after Rust state has been mutated through a different network key. - Handle/clone semantics hide it. Each cache-miss fetch runs
IDENTITY_MANAGER_STORAGE.insert(wallet_info.identity_manager.clone())(platform_wallet_info.rs:97), minting a fresh handle around an independent clone of the same logical manager. Mutations through one Swift handle are invisible through another even when both "belong" to the same network.
For a wallet SDK this is exactly the kind of cross-language semantic drift that surfaces as identities appearing under the wrong network or one network's manager being silently lost. Pick one: drop the for: Network parameter (and the identityManagers dictionary) to match the FFI's actual single-manager semantics, or extend PlatformWalletInfo and the FFI so identity managers are keyed by network and thread FFINetwork through these two entry points.
💬 nitpick: Use `withCString` instead of `NSString.utf8String` at the FFI boundary
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (lines 44-50)
(mnemonic as NSString).utf8String returns a const char * whose backing buffer is owned by the bridged NSString, with a lifetime tied to the autorelease pool rather than the call scope. The Rust side reads the bytes synchronously via CStr::from_ptr(...).to_str() (packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:64-65), so today this works, but String.withCString is the lifetime-guaranteed primitive for this boundary and avoids depending on Foundation bridging/autorelease semantics at an extern "C" boundary. The seed path already uses withUnsafeBytes for the same hygiene reason — match it here.
💡 Suggested change
var handle: Handle = NULL_HANDLE
try mnemonic.withCString { mnemonicCStr in
try platform_wallet_info_create_from_mnemonic(
network.ffiValue,
mnemonicCStr,
&handle
).check()
}
return PlatformWallet(handle: handle)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:
- [BLOCKING] lines 55-74: `network` parameter is silently dropped at the FFI in get/setIdentityManager
`getIdentityManager(for: Network)` (line 56) and `setIdentityManager(_:for: Network)` (line 71) accept a `Network` and key the Swift `identityManagers` dictionary on it, but `network` never crosses the FFI. The Rust callees `platform_wallet_info_get_identity_manager(wallet_handle, out_handle)` and `platform_wallet_info_set_identity_manager(wallet_handle, manager_handle)` at packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:87-117 take only handles and operate on a single `wallet_info.identity_manager` field. There is no per-network keying anywhere on the Rust side.
Observable consequences across the boundary:
1. Cross-network corruption on set. `setIdentityManager(m, for: .mainnet)` overwrites the single Rust-side `identity_manager` (line 113). A subsequent uncached `getIdentityManager(for: .testnet)` returns a clone of that mainnet-derived manager and presents it as the testnet manager.
2. Swift cache vs. Rust state drift. `getIdentityManager` short-circuits on `identityManagers[network]` (lines 58-60), so once a network slot is populated, the wrapper continues handing back stale objects even after Rust state has been mutated through a different network key.
3. Handle/clone semantics hide it. Each cache-miss fetch runs `IDENTITY_MANAGER_STORAGE.insert(wallet_info.identity_manager.clone())` (platform_wallet_info.rs:97), minting a fresh handle around an independent clone of the same logical manager. Mutations through one Swift handle are invisible through another even when both "belong" to the same network.
For a wallet SDK this is exactly the kind of cross-language semantic drift that surfaces as identities appearing under the wrong network or one network's manager being silently lost. Pick one: drop the `for: Network` parameter (and the `identityManagers` dictionary) to match the FFI's actual single-manager semantics, or extend `PlatformWalletInfo` and the FFI so identity managers are keyed by network and thread `FFINetwork` through these two entry points.
In `packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift`:
- [SUGGESTION] lines 52-80: Identity-manager tests pass for the wrong reason and lock in the broken FFI contract
`testGetIdentityManagerCaching`, `testSetIdentityManager`, and `testMultipleNetworkManagers` claim to exercise per-network identity-manager behavior but are insensitive to whether per-network behavior actually exists at the FFI boundary.
- `testSetIdentityManager` (line 61): `setIdentityManager(newManager, for: .mainnet)` populates `identityManagers[.mainnet]` on the Swift side (PlatformWallet.swift:73); the next `getIdentityManager(for: .mainnet)` short-circuits on that dictionary at PlatformWallet.swift:58-60 and never round-trips through the Rust get. The handle-equality assertion is satisfied by the Swift cache, not by Rust state.
- `testMultipleNetworkManagers` (line 71): asserts handle inequality across `.mainnet`/`.testnet`/`.devnet`, but each underlying call runs `IDENTITY_MANAGER_STORAGE.insert(...)` (platform_wallet_info.rs:97), which always returns a fresh handle. The same assertion would still hold if all three networks intentionally shared one underlying manager.
- The same false-positive pattern is in `PlatformWalletIntegrationTests.testMultipleNetworkIdentityManagers` (PlatformWalletIntegrationTests.swift:33-41).
As written these tests entrench the broken contract called out in the blocking finding above and would silently survive a regression that removes per-network scoping. To make them meaningful, either bypass the Swift cache (e.g., wrap a fresh `PlatformWallet` around the same Rust handle) so the FFI round-trip is observed, or test behavioral isolation — write through one network's manager and assert presence/absence through another.
In `packages/swift-sdk/run_tests.sh`:
- [SUGGESTION] line 23: CI now uploads a debug-built XCFramework artifact
`run_tests.sh` builds `DashSDKFFI.xcframework` with `--profile dev` (line 23), and the workflow zips that exact framework and publishes its checksum for downstream SwiftPM/Xcode consumption (.github/workflows/swift-sdk-build.yml:89-103). Verified against the previous workflow content: the prior `Build DashSDKFFI.xcframework and install into Swift package` step ran `bash packages/swift-sdk/build_ios.sh --target all --profile release`. After this PR, anyone downloading the published artifact gets an unoptimized debug Rust build instead of release. That is acceptable for local test execution, but a regression for the published artifact because its performance and debug-assert behavior no longer match the release framework consumers are expected to integrate. Split the test build from the artifact build, or build the framework once with `--profile release` for upload and reuse it for tests.
| XCTAssertNotEqual(mainnetManager.handle, testnetManager.handle, "Different networks should have different managers") | ||
| XCTAssertNotEqual(testnetManager.handle, devnetManager.handle, "Different networks should have different managers") | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Identity-manager tests pass for the wrong reason and lock in the broken FFI contract
testGetIdentityManagerCaching, testSetIdentityManager, and testMultipleNetworkManagers claim to exercise per-network identity-manager behavior but are insensitive to whether per-network behavior actually exists at the FFI boundary.
testSetIdentityManager(line 61):setIdentityManager(newManager, for: .mainnet)populatesidentityManagers[.mainnet]on the Swift side (PlatformWallet.swift:73); the nextgetIdentityManager(for: .mainnet)short-circuits on that dictionary at PlatformWallet.swift:58-60 and never round-trips through the Rust get. The handle-equality assertion is satisfied by the Swift cache, not by Rust state.testMultipleNetworkManagers(line 71): asserts handle inequality across.mainnet/.testnet/.devnet, but each underlying call runsIDENTITY_MANAGER_STORAGE.insert(...)(platform_wallet_info.rs:97), which always returns a fresh handle. The same assertion would still hold if all three networks intentionally shared one underlying manager.- The same false-positive pattern is in
PlatformWalletIntegrationTests.testMultipleNetworkIdentityManagers(PlatformWalletIntegrationTests.swift:33-41).
As written these tests entrench the broken contract called out in the blocking finding above and would silently survive a regression that removes per-network scoping. To make them meaningful, either bypass the Swift cache (e.g., wrap a fresh PlatformWallet around the same Rust handle) so the FFI round-trip is observed, or test behavioral isolation — write through one network's manager and assert presence/absence through another.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift`:
- [SUGGESTION] lines 52-80: Identity-manager tests pass for the wrong reason and lock in the broken FFI contract
`testGetIdentityManagerCaching`, `testSetIdentityManager`, and `testMultipleNetworkManagers` claim to exercise per-network identity-manager behavior but are insensitive to whether per-network behavior actually exists at the FFI boundary.
- `testSetIdentityManager` (line 61): `setIdentityManager(newManager, for: .mainnet)` populates `identityManagers[.mainnet]` on the Swift side (PlatformWallet.swift:73); the next `getIdentityManager(for: .mainnet)` short-circuits on that dictionary at PlatformWallet.swift:58-60 and never round-trips through the Rust get. The handle-equality assertion is satisfied by the Swift cache, not by Rust state.
- `testMultipleNetworkManagers` (line 71): asserts handle inequality across `.mainnet`/`.testnet`/`.devnet`, but each underlying call runs `IDENTITY_MANAGER_STORAGE.insert(...)` (platform_wallet_info.rs:97), which always returns a fresh handle. The same assertion would still hold if all three networks intentionally shared one underlying manager.
- The same false-positive pattern is in `PlatformWalletIntegrationTests.testMultipleNetworkIdentityManagers` (PlatformWalletIntegrationTests.swift:33-41).
As written these tests entrench the broken contract called out in the blocking finding above and would silently survive a regression that removes per-network scoping. To make them meaningful, either bypass the Swift cache (e.g., wrap a fresh `PlatformWallet` around the same Rust handle) so the FFI round-trip is observed, or test behavioral isolation — write through one network's manager and assert presence/absence through another.
| exit 1 | ||
| fi | ||
|
|
||
| bash build_ios.sh --target all --profile dev |
There was a problem hiding this comment.
🟡 Suggestion: CI now uploads a debug-built XCFramework artifact
run_tests.sh builds DashSDKFFI.xcframework with --profile dev (line 23), and the workflow zips that exact framework and publishes its checksum for downstream SwiftPM/Xcode consumption (.github/workflows/swift-sdk-build.yml:89-103). Verified against the previous workflow content: the prior Build DashSDKFFI.xcframework and install into Swift package step ran bash packages/swift-sdk/build_ios.sh --target all --profile release. After this PR, anyone downloading the published artifact gets an unoptimized debug Rust build instead of release. That is acceptable for local test execution, but a regression for the published artifact because its performance and debug-assert behavior no longer match the release framework consumers are expected to integrate. Split the test build from the artifact build, or build the framework once with --profile release for upload and reuse it for tests.
💡 Suggested change
| bash build_ios.sh --target all --profile dev | |
| bash build_ios.sh --target all --profile release |
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/run_tests.sh`:
- [SUGGESTION] line 23: CI now uploads a debug-built XCFramework artifact
`run_tests.sh` builds `DashSDKFFI.xcframework` with `--profile dev` (line 23), and the workflow zips that exact framework and publishes its checksum for downstream SwiftPM/Xcode consumption (.github/workflows/swift-sdk-build.yml:89-103). Verified against the previous workflow content: the prior `Build DashSDKFFI.xcframework and install into Swift package` step ran `bash packages/swift-sdk/build_ios.sh --target all --profile release`. After this PR, anyone downloading the published artifact gets an unoptimized debug Rust build instead of release. That is acceptable for local test execution, but a regression for the published artifact because its performance and debug-assert behavior no longer match the release framework consumers are expected to integrate. Split the test build from the artifact build, or build the framework once with `--profile release` for upload and reuse it for tests.
2 participants
So, I worked on getting the tests in scope, working and updated the script to do what it is supposed to do
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
Tests
Chores