Conversation
📝 WalkthroughWalkthroughThis PR restructures the Swift SDK test infrastructure by consolidating the separate Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 dab130e) |
![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: "3d52b9502667ea0cc1be668dbd335b0ab4a342a33b6b092dc684e8a2e2a75a82"
)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.
| @@ -25,6 +25,7 @@ public class PlatformWallet { | |||
|
|
|||
| let result = seed.withUnsafeBytes { seedPtr in | |||
| platform_wallet_info_create_from_seed( | |||
| network, | |||
| seedPtr.baseAddress?.assumingMemoryBound(to: UInt8.self), | |||
| seed.count, | |||
| &handle, | |||
| @@ -40,14 +41,19 @@ public class PlatformWallet { | |||
| } | |||
|
|
|||
| /// Create a new Platform Wallet from a BIP39 mnemonic phrase | |||
| public static func fromMnemonic(_ mnemonic: String, passphrase: String? = nil) throws -> PlatformWallet { | |||
| public static func fromMnemonic( | |||
| _ mnemonic: String, | |||
| passphrase: String? = nil, | |||
| network: Network = .testnet | |||
| ) throws -> PlatformWallet { | |||
| var handle: Handle = NULL_HANDLE | |||
| var error = PlatformWalletFFIError() | |||
|
|
|||
| let mnemonicCStr = (mnemonic as NSString).utf8String | |||
| let passphraseCStr = passphrase != nil ? (passphrase! as NSString).utf8String : nil | |||
|
|
|||
| let result = platform_wallet_info_create_from_mnemonic( | |||
| network, | |||
| mnemonicCStr, | |||
| passphraseCStr, | |||
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/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.
There was a problem hiding this comment.
tbh, I will ignore this since I am working on a Network unification PR, in case that is possible
2 participants
So, I worked on getting the tests in scope, working and updated the script to do what it is supposed to do
Todo in incoming PRs
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
fromSeedandfromMnemonic) with testnet as default.mainnet,testnet,regtest,devnet,local).Refactor
SystemConfigurationframework to the SDK target.