test(swift-sdk): swift-sdk test updated and added to CI#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 | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit 4832bec) |
![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: "2837c2f54333a6ee59f8e59389941e9dbd0b4640d532e15fc73417bb7892b195"
)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`.
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.
65be5fe to
4832bec
Compare
ZocoLini
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 4832bec. The Network ABI/discriminant fixes remain in place, but one real correctness blocker persists: PlatformWallet.getIdentityManager(for:)/setIdentityManager(_:for:) accept a Network argument that is never forwarded across the FFI, and the Rust side stores a single wallet-global identity_manager. The new tests entrench this broken contract by passing for Swift-cache / fresh-handle-allocation reasons rather than exercising actual per-network isolation. Carry-forward issues include a debug-profile XCFramework artifact upload in CI, removed wallet serialization test coverage, an NSString.utf8String FFI lifetime nit, and an under-asserting odd-length hex test.
Reviewed commit: 4832bec
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
3 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) (PlatformWallet.swift:56) and setIdentityManager(_:for: Network) (PlatformWallet.swift: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) (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 (line 94 and line 113). 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(platform_wallet_info.rs: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](PlatformWallet.swift:58-60), so once a network slot is populated, the wrapper keeps 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.
Pick one resolution: 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.
🟡 suggestion: Test reorganization drops all coverage for wallet serialization/import APIs that still ship
packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift (lines 1-82)
The prior Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swift (176 lines) is deleted in this PR, but the corresponding production APIs remain at packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift:594 (addWalletAndSerialize) and :655 (importWallet). The package test target now contains only PlatformWalletTests.swift, IdentityManagerTests.swift, PlatformWalletIntegrationTests.swift, and PlatformWalletTypesTests.swift, and none of them exercise wallet-serialization round-trips, watch-only imports, externally signable imports, or invalid-byte import errors. Since this PR is specifically about rewiring Swift SDK tests into CI, removing the only checked-in coverage for a user-visible persistence surface is a regression unless equivalent replacement tests are added.
💬 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 on lines 26-33 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)` (PlatformWallet.swift:56) and `setIdentityManager(_:for: Network)` (PlatformWallet.swift: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)` (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 (line 94 and line 113). 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` (platform_wallet_info.rs: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]` (PlatformWallet.swift:58-60), so once a network slot is populated, the wrapper keeps 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.
Pick one resolution: 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 1-82: Test reorganization drops all coverage for wallet serialization/import APIs that still ship
The prior `Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swift` (176 lines) is deleted in this PR, but the corresponding production APIs remain at `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift:594` (`addWalletAndSerialize`) and `:655` (`importWallet`). The package test target now contains only `PlatformWalletTests.swift`, `IdentityManagerTests.swift`, `PlatformWalletIntegrationTests.swift`, and `PlatformWalletTypesTests.swift`, and none of them exercise wallet-serialization round-trips, watch-only imports, externally signable imports, or invalid-byte import errors. Since this PR is specifically about rewiring Swift SDK tests into CI, removing the only checked-in coverage for a user-visible persistence surface is a regression unless equivalent replacement tests are added.
4832bec to
d93586d
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-verified all six prior findings against worktree at d93586d; every prior finding is STILL VALID. The latest delta (d93586d17a swift-sdk test adjusted) did not touch PlatformWallet.swift, PlatformWalletTests.swift, PlatformWalletTypesTests.swift, run_tests.sh, or platform_wallet_info.rs. No new findings from the latest delta. One blocking FFI contract bug (Swift exposes per-network identity manager API that the Rust FFI does not honor), plus three test/CI suggestions and two nitpicks carried forward.
Reviewed commit: d93586d
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 2 nitpick(s)
6 additional finding(s) omitted from inline comments / attached in the review body.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
ZocoLini
force-pushed
the
chore/swift-sdk-tests
branch
2 times, most recently
from
3d5e96a to
456ca5d
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Current head still carries five prior issues: the Swift PlatformWallet API still advertises per-network identity managers even though the Rust FFI stores a single manager slot, the related tests still cannot detect that mismatch, the public wallet serialization/import FFI remains uncovered, fromMnemonic still marshals through NSString.utf8String, and the odd-length hex test still does not lock in the helper’s behavior. Prior finding prior-3 is outdated on 3d5e96a9ff2608879ff602be947afe34439f4f6a: the consumer XCFramework is released by .github/workflows/swift-sdk-release.yml with --profile release, while run_tests.sh only feeds the PR build-and-test workflow. New in the d93586d17aca094b288a978d0476cb21d8ebf5f3..3d5e96a9ff2608879ff602be947afe34439f4f6a delta, restorableWalletIds() is not network-scoped even though loadWalletList() is, and the new core-address-pool restore marshalling landed without automated round-trip coverage.
Note: Inline posting failed (command failed (1): gh api /repos/dashpay/platform/pulls/3479/reviews --method POST --input -
STDOUT:
{"message":"Unprocessable Entity","errors":["Variable $threads of type [DraftPullRequestReviewThread] was provided invalid value for 0.commitId (Field is not defined on DraftPullRequestReviewThread)), so I posted the same verified findings as a top-level review body.
🔴 1 blocking | 🟡 4 suggestion(s) | 💬 2 nitpick(s)
7 additional finding(s)
blocking: `PlatformWallet` still promises per-network identity managers that the FFI does not implement
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (line 55)
getIdentityManager(for:) and setIdentityManager(_:for:) key Swift-side state by Network, but neither FFI call transmits that parameter. On the Rust side, platform_wallet_info_get_identity_manager and platform_wallet_info_set_identity_manager still read and write the single wallet_info.identity_manager field in packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:87-116. That means .mainnet, .testnet, and .devnet are all aliases for the same underlying manager even though the public Swift API advertises separate per-network behavior.
suggestion: The identity-manager tests still codify the broken single-manager contract
packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift (line 52)
These tests still cannot distinguish a correct per-network implementation from the current single-slot Rust FFI. testGetIdentityManagerCaching only proves the Swift dictionary cache works for one key, testSetIdentityManager only checks that the .mainnet cache entry comes back, and testMultipleNetworkManagers compares handle values even though platform_wallet_info_get_identity_manager allocates a fresh handle every time it clones the same underlying manager. The suite will keep passing while the public API remains semantically wrong.
suggestion: The public wallet serialization and import FFI paths are still untested
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (line 594)
addWalletAndSerialize and importWallet remain shipping APIs, but the current packages/swift-sdk/Tests/SwiftDashSDKTests target does not exercise them. That leaves normal round-trips, watch-only and externally-signable restore flows, and malformed-byte failures uncovered even though these methods cross the FFI boundary, copy heap buffers, and free Rust-allocated memory manually.
nitpick: `fromMnemonic` still relies on bridged `NSString.utf8String` instead of a call-scoped C string
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (line 42)
The mnemonic pointer currently comes from Objective-C bridging rather than Swift’s withCString lifetime guarantee. Rust reads it synchronously today, so this usually works, but withCString is the correct FFI boundary primitive because it guarantees the NUL-terminated UTF-8 buffer stays valid for the exact duration of the call.
var handle: Handle = NULL_HANDLE
return try mnemonic.withCString { mnemonicCStr in
try platform_wallet_info_create_from_mnemonic(
network.ffiValue,
mnemonicCStr,
&handle
).check()
return PlatformWallet(handle: handle)
}
nitpick: The odd-length hex test still does not specify the helper’s contract
packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift (line 27)
Data(hexString:) currently truncates the trailing nibble because it iterates hexString.count / 2 byte pairs. This test still asserts only non-nil, so it will not catch a future change to reject odd-length input, zero-pad it, or preserve the current truncation behavior.
func testDataFromOddLengthHexString() {
let oddHex = "123"
let data = Data(hexString: oddHex)
XCTAssertEqual(data, Data([0x12]))
}
suggestion: `restorableWalletIds()` now disagrees with the network-scoped restore callback
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3377)
loadWalletList() was updated to fetch only wallets for self.network, but restorableWalletIds() still fetches every restorable wallet in the store. PlatformWalletManager.loadFromPersistor() then asks Rust for each returned ID, including wallets that were never restored into this manager. In a mixed-network store that produces predictable invalid-handle errors and sets lastError even though the extra wallets were intentionally filtered out earlier in the same restore flow.
public func restorableWalletIds() -> [Data] {
onQueue {
let descriptor: FetchDescriptor<PersistentWallet>
if let network = self.network {
let raw = network.rawValue
descriptor = FetchDescriptor<PersistentWallet>(
predicate: #Predicate { $0.networkRaw == raw }
)
} else {
descriptor = FetchDescriptor<PersistentWallet>()
}
guard let wallets = try? backgroundContext.fetch(descriptor) else {
return []
}
return wallets
.filter { w in
w.accounts.contains { ($0.accountExtendedPubKeyBytes?.isEmpty == false) }
}
.map { $0.walletId }
}
}
suggestion: The new core-address-pool restore marshalling shipped without automated restart-path coverage
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 2921)
This delta adds a new buildCoreAddressPoolBuffer path that reconstructs persisted PersistentCoreAddress rows into AccountAddressPoolFFI and feeds them back into Rust during restore, but no test under packages/swift-sdk/Tests exercises a save/load round-trip through that path. The new code now carries address indexes, balances, derivation paths, and corruption abort behavior across the FFI boundary, so leaving it untested makes restart regressions easy to miss.
🤖 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.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:55-73: `PlatformWallet` still promises per-network identity managers that the FFI does not implement
`getIdentityManager(for:)` and `setIdentityManager(_:for:)` key Swift-side state by `Network`, but neither FFI call transmits that parameter. On the Rust side, `platform_wallet_info_get_identity_manager` and `platform_wallet_info_set_identity_manager` still read and write the single `wallet_info.identity_manager` field in `packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:87-116`. That means `.mainnet`, `.testnet`, and `.devnet` are all aliases for the same underlying manager even though the public Swift API advertises separate per-network behavior.
- [SUGGESTION] In `packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift`:52-80: The identity-manager tests still codify the broken single-manager contract
These tests still cannot distinguish a correct per-network implementation from the current single-slot Rust FFI. `testGetIdentityManagerCaching` only proves the Swift dictionary cache works for one key, `testSetIdentityManager` only checks that the `.mainnet` cache entry comes back, and `testMultipleNetworkManagers` compares handle values even though `platform_wallet_info_get_identity_manager` allocates a fresh handle every time it clones the same underlying manager. The suite will keep passing while the public API remains semantically wrong.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:594-684: The public wallet serialization and import FFI paths are still untested
`addWalletAndSerialize` and `importWallet` remain shipping APIs, but the current `packages/swift-sdk/Tests/SwiftDashSDKTests` target does not exercise them. That leaves normal round-trips, watch-only and externally-signable restore flows, and malformed-byte failures uncovered even though these methods cross the FFI boundary, copy heap buffers, and free Rust-allocated memory manually.
- [NITPICK] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:42-50: `fromMnemonic` still relies on bridged `NSString.utf8String` instead of a call-scoped C string
The mnemonic pointer currently comes from Objective-C bridging rather than Swift’s `withCString` lifetime guarantee. Rust reads it synchronously today, so this usually works, but `withCString` is the correct FFI boundary primitive because it guarantees the NUL-terminated UTF-8 buffer stays valid for the exact duration of the call.
- [NITPICK] In `packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift`:27-33: The odd-length hex test still does not specify the helper’s contract
`Data(hexString:)` currently truncates the trailing nibble because it iterates `hexString.count / 2` byte pairs. This test still asserts only non-nil, so it will not catch a future change to reject odd-length input, zero-pad it, or preserve the current truncation behavior.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3377-3388: `restorableWalletIds()` now disagrees with the network-scoped restore callback
`loadWalletList()` was updated to fetch only wallets for `self.network`, but `restorableWalletIds()` still fetches every restorable wallet in the store. `PlatformWalletManager.loadFromPersistor()` then asks Rust for each returned ID, including wallets that were never restored into this manager. In a mixed-network store that produces predictable invalid-handle errors and sets `lastError` even though the extra wallets were intentionally filtered out earlier in the same restore flow.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:2921-2997: The new core-address-pool restore marshalling shipped without automated restart-path coverage
This delta adds a new `buildCoreAddressPoolBuffer` path that reconstructs persisted `PersistentCoreAddress` rows into `AccountAddressPoolFFI` and feeds them back into Rust during restore, but no test under `packages/swift-sdk/Tests` exercises a save/load round-trip through that path. The new code now carries address indexes, balances, derivation paths, and corruption abort behavior across the FFI boundary, so leaving it untested makes restart regressions easy to miss.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The delta between 3d5e96a and 456ca5d is a one-line nonisolated annotation in CreateIdentityView.swift and introduces no new issues. All seven prior findings remain valid on the current head: the per-network identity-manager API is still mismatched with the single-slot Rust FFI, the tests still codify the broken contract, wallet serialization/import and core-address-pool restore paths still lack coverage, restorableWalletIds() still disagrees with the network-scoped restore callback, and the two nitpicks (bridged NSString.utf8String, odd-length hex contract) are unchanged.
Note: Inline posting failed (command failed (1): gh api /repos/dashpay/platform/pulls/3479/reviews --method POST --input -
STDOUT:
{"message":"Unprocessable Entity","errors":["Variable $threads of type [DraftPullRequestReviewThread] was provided invalid value for 0.commitId (Field is not defined on DraftPullRequestReviewThread)), so I posted the same verified findings as a top-level review body.
🔴 1 blocking | 🟡 4 suggestion(s) | 💬 2 nitpick(s)
7 additional finding(s)
blocking: `PlatformWallet` advertises per-network identity managers, but the FFI stores only one wallet-global slot
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (line 55)
getIdentityManager(for:) and setIdentityManager(_:for:) key Swift-side state by Network, but neither FFI call transmits that parameter. On the Rust side, 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) only take a wallet_handle and manager_handle and read/write the single wallet_info.identity_manager field. A caller that sets a manager for .mainnet and then asks for .testnet will currently get back the same Rust-side manager once the Swift cache is bypassed (e.g. on a fresh PlatformWallet after restore). Either thread the Network discriminator across the FFI and store a per-network map in Rust, or restrict the Swift API surface to a single manager per wallet.
suggestion: `restorableWalletIds()` disagrees with the network-scoped restore callback
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3377)
loadWalletList() was updated to scope its fetch to self.network (lines 2404-2421), so Rust now only reconstructs wallets for the current manager's network. restorableWalletIds() still fetches every restorable PersistentWallet across all networks with an unconstrained FetchDescriptor<PersistentWallet>(). PlatformWalletManager.loadFromPersistor() (line 288) iterates that unfiltered list and calls platform_wallet_manager_get_wallet for each ID, asking Rust for handles to wallets it was never told to restore for this manager — yielding NotFound and making the Swift-side ID list disagree with the load callback payload. Filter restorableWalletIds() by self.network the same way loadWalletList() does.
suggestion: Identity-manager tests still codify the broken single-slot FFI contract
packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift (line 52)
These tests do not distinguish a correct per-network implementation from the single-slot Rust FFI. testGetIdentityManagerCaching only exercises one key; testSetIdentityManager only round-trips .mainnet; testMultipleNetworkManagers is satisfied by distinct Swift handle allocations even though every uncached getIdentityManager(for:) wraps the same Rust slot in a freshly inserted IDENTITY_MANAGER_STORAGE handle, so XCTAssertNotEqual(handle, handle) passes by accident. A meaningful test must set a known manager for one network, set a different one for another, and verify the Rust-side state for each network is independent (e.g. by reading manager-owned state, not handle equality).
suggestion: Public wallet serialization/import FFI paths ship without any test coverage
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (line 594)
addWalletAndSerialize and importWallet are still public API that marshal raw byte buffers across the FFI and rely on the Rust-allocated wallet_manager_free_wallet_bytes ownership contract, but no test in packages/swift-sdk/Tests/SwiftDashSDKTests (4 files total: PlatformWalletTests, PlatformWalletIntegrationTests, PlatformWalletTypesTests, IdentityManagerTests) references either method. The previous WalletSerializationTests.swift was deleted in this PR rather than migrated, leaving normal round-trips, watch-only restore, externally-signable restore, and malformed-byte failure modes uncovered. Add back coverage that exercises a save-then-restore round-trip through both APIs.
suggestion: New core-address-pool restore marshalling has no restart-path coverage
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 2923)
buildCoreAddressPoolBuffer allocates nested AccountAddressPoolFFI and CoreAddressEntryFFI buffers and feeds them back to Rust via WalletRestoreEntryFFI.core_address_pools. The boundary depends on field ordering, pool grouping, borrowed C-string lifetimes, and the load-callback free contract, but no test exercises a full save → restart → reload round-trip through this path. A marshalling or ownership bug would only surface after app restart. Add a SwiftData-backed test that persists a wallet with core address pools and asserts the restored Rust-side wallet sees the same addresses, change indices, and use flags.
nitpick: `fromMnemonic` uses bridged `NSString.utf8String` instead of `withCString`
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (line 38)
The mnemonic pointer is obtained via (mnemonic as NSString).utf8String, which returns an Objective-C-bridged pointer with implicit lifetime. Rust reads the string synchronously today (CStr::from_ptr then to_str in platform_wallet_info_create_from_mnemonic), so this currently works, but withCString is the idiomatic Swift FFI primitive and guarantees the NUL-terminated UTF-8 buffer is valid for exactly the call body — matching the pattern used elsewhere in this SDK.
public static func fromMnemonic(
_ mnemonic: String,
network: Network = .testnet
) throws -> PlatformWallet {
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)
}
nitpick: Odd-length hex test does not pin `Data(hexString:)` contract
packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift (line 27)
testDataFromOddLengthHexString only asserts non-nil with a comment that the behavior is implementation-dependent. The current implementation silently truncates the trailing nibble (1 byte from "123"), but Data(hexString:) is used to construct binary values that ultimately cross into Rust. The test should specify the intended contract — reject odd-length, zero-pad, or truncate — so a future change to parsing semantics does not silently alter the exact bytes passed across the FFI without a failing test.
🤖 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.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:55-74: `PlatformWallet` advertises per-network identity managers, but the FFI stores only one wallet-global slot
`getIdentityManager(for:)` and `setIdentityManager(_:for:)` key Swift-side state by `Network`, but neither FFI call transmits that parameter. On the Rust side, `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`) only take a `wallet_handle` and `manager_handle` and read/write the single `wallet_info.identity_manager` field. A caller that sets a manager for `.mainnet` and then asks for `.testnet` will currently get back the same Rust-side manager once the Swift cache is bypassed (e.g. on a fresh `PlatformWallet` after restore). Either thread the `Network` discriminator across the FFI and store a per-network map in Rust, or restrict the Swift API surface to a single manager per wallet.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3377-3389: `restorableWalletIds()` disagrees with the network-scoped restore callback
`loadWalletList()` was updated to scope its fetch to `self.network` (lines 2404-2421), so Rust now only reconstructs wallets for the current manager's network. `restorableWalletIds()` still fetches every restorable `PersistentWallet` across all networks with an unconstrained `FetchDescriptor<PersistentWallet>()`. `PlatformWalletManager.loadFromPersistor()` (line 288) iterates that unfiltered list and calls `platform_wallet_manager_get_wallet` for each ID, asking Rust for handles to wallets it was never told to restore for this manager — yielding `NotFound` and making the Swift-side ID list disagree with the load callback payload. Filter `restorableWalletIds()` by `self.network` the same way `loadWalletList()` does.
- [SUGGESTION] In `packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTests.swift`:52-80: Identity-manager tests still codify the broken single-slot FFI contract
These tests do not distinguish a correct per-network implementation from the single-slot Rust FFI. `testGetIdentityManagerCaching` only exercises one key; `testSetIdentityManager` only round-trips `.mainnet`; `testMultipleNetworkManagers` is satisfied by distinct Swift handle allocations even though every uncached `getIdentityManager(for:)` wraps the same Rust slot in a freshly inserted `IDENTITY_MANAGER_STORAGE` handle, so `XCTAssertNotEqual(handle, handle)` passes by accident. A meaningful test must set a known manager for one network, set a different one for another, and verify the Rust-side state for each network is independent (e.g. by reading manager-owned state, not handle equality).
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:594-684: Public wallet serialization/import FFI paths ship without any test coverage
`addWalletAndSerialize` and `importWallet` are still public API that marshal raw byte buffers across the FFI and rely on the Rust-allocated `wallet_manager_free_wallet_bytes` ownership contract, but no test in `packages/swift-sdk/Tests/SwiftDashSDKTests` (4 files total: `PlatformWalletTests`, `PlatformWalletIntegrationTests`, `PlatformWalletTypesTests`, `IdentityManagerTests`) references either method. The previous `WalletSerializationTests.swift` was deleted in this PR rather than migrated, leaving normal round-trips, watch-only restore, externally-signable restore, and malformed-byte failure modes uncovered. Add back coverage that exercises a save-then-restore round-trip through both APIs.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:2923-2997: New core-address-pool restore marshalling has no restart-path coverage
`buildCoreAddressPoolBuffer` allocates nested `AccountAddressPoolFFI` and `CoreAddressEntryFFI` buffers and feeds them back to Rust via `WalletRestoreEntryFFI.core_address_pools`. The boundary depends on field ordering, pool grouping, borrowed C-string lifetimes, and the load-callback `free` contract, but no test exercises a full save → restart → reload round-trip through this path. A marshalling or ownership bug would only surface after app restart. Add a SwiftData-backed test that persists a wallet with core address pools and asserts the restored Rust-side wallet sees the same addresses, change indices, and use flags.
- [NITPICK] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:38-53: `fromMnemonic` uses bridged `NSString.utf8String` instead of `withCString`
The mnemonic pointer is obtained via `(mnemonic as NSString).utf8String`, which returns an Objective-C-bridged pointer with implicit lifetime. Rust reads the string synchronously today (`CStr::from_ptr` then `to_str` in `platform_wallet_info_create_from_mnemonic`), so this currently works, but `withCString` is the idiomatic Swift FFI primitive and guarantees the NUL-terminated UTF-8 buffer is valid for exactly the call body — matching the pattern used elsewhere in this SDK.
- [NITPICK] In `packages/swift-sdk/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift`:27-34: Odd-length hex test does not pin `Data(hexString:)` contract
`testDataFromOddLengthHexString` only asserts non-nil with a comment that the behavior is implementation-dependent. The current implementation silently truncates the trailing nibble (1 byte from `"123"`), but `Data(hexString:)` is used to construct binary values that ultimately cross into Rust. The test should specify the intended contract — reject odd-length, zero-pad, or truncate — so a future change to parsing semantics does not silently alter the exact bytes passed across the FFI without a failing test.
ZocoLini
force-pushed
the
chore/swift-sdk-tests
branch
from
456ca5d to
79ffa47
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I revalidated the full current head 79ffa472988cdf5dce469f74e998570463eb2f87 against all seven prior findings. The latest delta is effectively a test-path relocation plus README updates, and none of the underlying Swift/Rust contract, restore logic, or test-coverage gaps were addressed, so all seven prior findings remain STILL VALID. I did not confirm any additional defect introduced specifically by 456ca5daf53fef9d970ed926f21b9f308e6e1973..79ffa472988cdf5dce469f74e998570463eb2f87.
Note: Inline posting via scripts/review_poster.py hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
🔴 1 blocking | 🟡 4 suggestion(s) | 💬 2 nitpick(s)
7 additional finding(s)
blocking: `PlatformWallet` still exposes network-scoped identity managers on top of a wallet-global FFI slot
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (line 55)
getIdentityManager(for:) and setIdentityManager(_:for:) cache state by Network, but neither FFI call passes a network argument. The Rust side still reads and writes the single wallet_info.identity_manager field in packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:87-116. That means the public Swift API promises per-network isolation that the backing store does not implement: once the Swift cache is cold, any wrapper recreated around the same wallet can observe whichever manager was last written globally.
suggestion: The relocated identity-manager tests still bless the broken single-slot contract
packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTests.swift (line 52)
These tests moved to SwiftTests/ without changing their assertions. testGetIdentityManagerCaching() only proves the one-key Swift cache, testSetIdentityManager() only round-trips .mainnet, and testMultipleNetworkManagers() is satisfied by distinct handle allocations even if Rust keeps one wallet-global manager slot. The same weak assertion pattern also remains in packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift:31-38, so CI still cannot catch the cross-network aliasing bug above.
suggestion: Wallet serialization and import FFI paths still ship without direct test coverage
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (line 594)
addWalletAndSerialize and importWallet are still public APIs that marshal raw byte buffers across FFI, but the current Swift test target contains no calls to either method. That leaves the main serialize/import round-trip, watch-only and external-signing restore modes, and malformed-byte failure handling unverified. Because these paths depend on correct length handling and paired allocation/free behavior, the lack of direct coverage is still a real gap.
suggestion: `restorableWalletIds()` still disagrees with the network-scoped restore callback
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3377)
loadWalletList() now filters PersistentWallet rows to self.network at packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:2413-2418, but restorableWalletIds() still fetches every restorable wallet in the store with no network predicate. PlatformWalletManager.loadFromPersistor() then iterates that global ID list at packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:288-313 and asks Rust for wallets that were never restored into the current manager. In a multi-network store this leaves the callback payload and follow-up lookup list inconsistent and can produce spurious get_wallet failures.
suggestion: The core-address-pool restore marshalling still has no restart-path coverage
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 2923)
buildCoreAddressPoolBuffer is still responsible for reconstructing nested AccountAddressPoolFFI and CoreAddressEntryFFI buffers from persisted PersistentCoreAddress rows during restore, but the current Swift tests do not drive a save/restart/load round-trip through that path. That leaves ordering, pool grouping, string duplication, and nested-buffer lifetime unverified at the exact point where Swift hands reconstructed address state back to Rust. A regression here will typically surface only after restart, and there is still no test that exercises it.
nitpick: `fromMnemonic` still relies on bridged `NSString.utf8String` instead of a scoped Swift C-string
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift (line 38)
The FFI call currently uses (mnemonic as NSString).utf8String and stores the pointer in a local before calling Rust. Rust dereferences that pointer immediately in packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:54-67, so this works today, but withCString is the correct boundary primitive because it ties the UTF-8 buffer lifetime to the exact call scope instead of relying on bridged Objective-C storage behavior.
return try mnemonic.withCString { mnemonicCStr in
try platform_wallet_info_create_from_mnemonic(
network.ffiValue,
mnemonicCStr,
&handle
).check()
return PlatformWallet(handle: handle)
}
nitpick: The odd-length hex test still does not pin `Data(hexString:)` semantics
packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTypesTests.swift (line 24)
Data(hexString:) still truncates the trailing nibble because it iterates hexString.count / 2 byte pairs in packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift:80-95, but testDataFromOddLengthHexString() only asserts non-nil. That test would also pass if the helper started rejecting odd-length input or zero-padding it, so it still does not lock down the actual byte contract used by callers that parse identifiers and addresses.
let oddHex = "123"
let data = Data(hexString: oddHex)
XCTAssertEqual(data, Data([0x12]))
🤖 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.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:55-72: `PlatformWallet` still exposes network-scoped identity managers on top of a wallet-global FFI slot
`getIdentityManager(for:)` and `setIdentityManager(_:for:)` cache state by `Network`, but neither FFI call passes a network argument. The Rust side still reads and writes the single `wallet_info.identity_manager` field in `packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:87-116`. That means the public Swift API promises per-network isolation that the backing store does not implement: once the Swift cache is cold, any wrapper recreated around the same wallet can observe whichever manager was last written globally.
- [SUGGESTION] In `packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTests.swift`:52-79: The relocated identity-manager tests still bless the broken single-slot contract
These tests moved to `SwiftTests/` without changing their assertions. `testGetIdentityManagerCaching()` only proves the one-key Swift cache, `testSetIdentityManager()` only round-trips `.mainnet`, and `testMultipleNetworkManagers()` is satisfied by distinct handle allocations even if Rust keeps one wallet-global manager slot. The same weak assertion pattern also remains in `packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift:31-38`, so CI still cannot catch the cross-network aliasing bug above.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:594-684: Wallet serialization and import FFI paths still ship without direct test coverage
`addWalletAndSerialize` and `importWallet` are still public APIs that marshal raw byte buffers across FFI, but the current Swift test target contains no calls to either method. That leaves the main serialize/import round-trip, watch-only and external-signing restore modes, and malformed-byte failure handling unverified. Because these paths depend on correct length handling and paired allocation/free behavior, the lack of direct coverage is still a real gap.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3377-3388: `restorableWalletIds()` still disagrees with the network-scoped restore callback
`loadWalletList()` now filters `PersistentWallet` rows to `self.network` at `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:2413-2418`, but `restorableWalletIds()` still fetches every restorable wallet in the store with no network predicate. `PlatformWalletManager.loadFromPersistor()` then iterates that global ID list at `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:288-313` and asks Rust for wallets that were never restored into the current manager. In a multi-network store this leaves the callback payload and follow-up lookup list inconsistent and can produce spurious `get_wallet` failures.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:2923-2997: The core-address-pool restore marshalling still has no restart-path coverage
`buildCoreAddressPoolBuffer` is still responsible for reconstructing nested `AccountAddressPoolFFI` and `CoreAddressEntryFFI` buffers from persisted `PersistentCoreAddress` rows during restore, but the current Swift tests do not drive a save/restart/load round-trip through that path. That leaves ordering, pool grouping, string duplication, and nested-buffer lifetime unverified at the exact point where Swift hands reconstructed address state back to Rust. A regression here will typically surface only after restart, and there is still no test that exercises it.
- [NITPICK] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWallet.swift`:38-49: `fromMnemonic` still relies on bridged `NSString.utf8String` instead of a scoped Swift C-string
The FFI call currently uses `(mnemonic as NSString).utf8String` and stores the pointer in a local before calling Rust. Rust dereferences that pointer immediately in `packages/rs-platform-wallet-ffi/src/platform_wallet_info.rs:54-67`, so this works today, but `withCString` is the correct boundary primitive because it ties the UTF-8 buffer lifetime to the exact call scope instead of relying on bridged Objective-C storage behavior.
- [NITPICK] In `packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTypesTests.swift`:24-31: The odd-length hex test still does not pin `Data(hexString:)` semantics
`Data(hexString:)` still truncates the trailing nibble because it iterates `hexString.count / 2` byte pairs in `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletTypes.swift:80-95`, but `testDataFromOddLengthHexString()` only asserts non-nil. That test would also pass if the helper started rejecting odd-length input or zero-padding it, so it still does not lock down the actual byte contract used by callers that parse identifiers and addresses.
4 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