feat(swift-sdk): add mnemonic storage to WalletStorage#3477
Merged
Conversation
Package.swift declares `.iOS(.v17)` as the minimum, but build_ios.sh
hardcoded IPHONEOS/IPHONESIMULATOR_DEPLOYMENT_TARGET=18.0, so the
DashSDKFFI.xcframework it produced was stamped iOS 18.0. Downstream
consumers targeting iOS 17 hit link-time warnings like:
ld: warning: object file (.../librs_unified_sdk_ffi.a(...)) was
built for newer iOS Simulator version (18.0) than being linked (17.0)
Replace the hardcoded exports with `${VAR:-17.0}` defaults so the
deployment target matches Package.swift by default while still being
overridable via env var.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Storage dashwallet-ios needs to construct a CoreWalletManager from outside the SwiftDashSDK module to perform a one-shot wallet key migration from DashSync's legacy keychain layout. The existing designated init takes an internal SPVClient, and WalletStorage's synthesized init defaulted to internal — both unreachable from external callers. Add a public convenience init on CoreWalletManager that hides SPV plumbing behind a KeyWalletNetwork parameter (constructs an SPVClient with no on-disk data dir and an in-process SwiftData ModelContainer), and add a public init() to WalletStorage so external callers can construct it directly. Both changes are additive and minimal — no behavior change, no breaking change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ger convenience init The earlier convenience init `CoreWalletManager(keyWalletNetwork:)` added in 223dda6 had two real problems: 1. Use-after-free. The convenience init constructed an SPVClient as a local variable and chained to the existing internal designated init `init(spvClient:modelContainer:)`. That designated init only stores `self.sdkWalletManager = try spvClient.getWalletManager()` and discards the SPVClient parameter. Because `WalletManager.init(handle:)` sets `ownsHandle = false` (the handle is non-owning, owned by the SPVClient), dropping the local SPVClient at end-of-init triggered `SPVClient.deinit -> destroy() -> dash_spv_ffi_client_destroy`, leaving `self.sdkWalletManager.handle` dangling. Build succeeded because this is a runtime ARC bug; never observed at runtime due to a separate iPhone 17 + iOS 26.3 [DSChain retrieveWallets] crash. 2. Unverified `dataDir: nil` semantics. The convenience init passed `dataDir: nil` to SPVClient, which simply skips `dash_spv_ffi_config_set_data_dir`. Every other call site in the SDK passes an explicit Documents-based path; the FFI behavior with no dataDir is unknown and the SPVClient header comment hints at a directory lock that may persist even with nil. Revert the convenience init. Instead, expose `HDWallet.init` publicly so external callers can use the lower-level public API surface (`WalletManager` standalone init + `Mnemonic` + `WalletStorage` + direct `HDWallet` construction inserted into a fresh `ModelContext`) without any SPVClient ceremony. This eliminates the lifetime bug, the dataDir uncertainty, the inner-Task lifetime risk from `CoreWalletManager.init`, and the @mainactor + async constraints — all in a single visibility flip. The `public init() {}` on `WalletStorage` from 223dda6 is still required and stays. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Out-of-module consumers (dashwallet-ios) need to construct SPVClient
directly with their own event handler implementations to drive SPV
chain sync from outside the SDK. Today everything in Core/SPV is
internal — SPVClient class, all 5 event handler protocols, the
SPVEventHandlers bundle, and every method on the client.
Mark them public:
- SPVClient class + init + getSyncProgress + setMasternodeSyncEnabled
+ clearStorage + destroy + broadcastTransaction + startSync +
stopSync + getWalletManager.
- SPVEventHandlers struct + var fields + a new public memberwise init
(the synthesized init defaults to internal on a public struct).
- SPVProgressUpdateEventHandler / SPVSyncEventsHandler /
SPVNetworkEventsHandler / SPVWalletEventsHandler /
SPVClientErrorEventsHandler protocols.
- SPVSyncManager enum (referenced in public protocol method
signatures, must itself be public).
Strictly additive — same risk profile as the prior HDWallet.init /
WalletStorage.init / CoreWalletManager visibility patches in
223dda6 and b3bb5fc. No behavior change. The internal
intoFFIEventCallbacks bridging stays internal.
Verified: dashwallet and dashpay targets in dashwallet-ios both
build clean against the patched SwiftDashSDK via local SPM.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Follow-up to 400b4b2. The 5 SPV event handler protocols are now public, but their `intoFFI*Callbacks()` default implementations in the protocol extensions were still internal-scoped. That meant out-of-module conformers (dashwallet-ios) saw the protocol requirement but could NOT see the default implementation, forcing them to either implement the FFI bridge themselves or fail to compile. Mark the 5 default implementations public so out-of-module conformers get the bridge for free: - SPVProgressUpdateEventHandler.intoFFIProgressCallback - SPVSyncEventsHandler.intoFFISyncEventCallbacks - SPVNetworkEventsHandler.intoFFINetworkEventCallbacks - SPVClientErrorEventsHandler.intoFFIClientErrorCallback - SPVWalletEventsHandler.intoFFIWalletEventCallbacks The FFI types these methods reference (FFIProgressCallback, FFISyncEventCallbacks, etc.) are already visible to out-of-module consumers because SwiftDashSDK.swift uses `@_exported import DashSDKFFI`. No new surface area is being added on top of what 400b4b2 already exposed. Verified: dashwallet-ios still builds clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add storeMnemonic/retrieveMnemonic/deleteMnemonic methods to WalletStorage for persisting the BIP39 mnemonic phrase in iOS keychain. Plain keychain with kSecAttrAccessibleWhenUnlocked ThisDeviceOnly — no PIN-derived encryption (iOS keychain hardware protection is the security boundary, parity with how DashSync stores mnemonics). New keychain account: "wallet.mnemonic" under the existing "org.dash.wallet" service. Idempotent writes (delete-before-add), idempotent deletes (accepts errSecItemNotFound). This enables dashwallet-ios to store and retrieve the human-readable 12-word phrase independently of DashSync, which is needed to migrate the backup seed phrase display away from DashSync's seedPhraseIfAuthenticated API. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t-sdk-mnemonic-storage
Contributor
📝 WalkthroughWalkthroughAdded mnemonic keychain support to WalletStorage (store, retrieve, delete). Also adjusted keychain mutation and retrieval paths to validate deletion results and map keychain errors to explicit WalletStorageError cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Collaborator
Review GateCommit:
|
Contributor
|
✅ 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: "9a3d3409968528001cd357b0253ff151920b1091a22982a08714cfb27afefaf0"
)Xcode manual integration:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift`:
- Around line 105-121: In storeMnemonic(), don't reuse the add-query for
SecItemDelete; build a separate deleteQuery containing only identifying
attributes (kSecClass/kSecAttrService/kSecAttrAccount using keychainService and
mnemonicKeychainAccount) and call SecItemDelete with that, verify the result
allows errSecSuccess or errSecItemNotFound before proceeding; then construct the
add-query with kSecValueData and kSecAttrAccessible and call SecItemAdd,
throwing WalletStorageError.keychainError(status) on failure.
- Around line 124-140: The retrieveMnemonic() function currently maps all
SecItemCopyMatching failures to WalletStorageError.mnemonicNotFound; change it
to distinguish not-found vs other Keychain errors by checking the returned
status: if status == errSecSuccess proceed as before, if status ==
errSecItemNotFound throw WalletStorageError.mnemonicNotFound, otherwise throw or
map the status into a Keychain-specific error (e.g.,
WalletStorageError.keychainError(status)) similar to how deleteMnemonic()
handles non-found vs other errors; update references in retrieveMnemonic to use
SecItemCopyMatching, errSecSuccess, errSecItemNotFound and WalletStorageError to
locate and implement the fix.
🪄 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: e4c3e4b2-e70c-4e0c-860d-ad332596cb5c
📒 Files selected for processing (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift
- Separate delete and add queries in all store methods (storeSeed, storeMnemonic, storePINHash, enableBiometricProtection) so SecItemDelete uses only identifying attributes (class/service/account) - Verify delete status before proceeding with add - Distinguish errSecItemNotFound from other keychain errors in retrieveSeed and retrieveMnemonic - Add docstrings to mnemonic and seed storage methods Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
llbartekll
changed the title
llbartekll
changed the title
QuantumExplorer
approved these changes
Apr 14, 2026
QuantumExplorer
pushed a commit
that referenced
this pull request
Apr 19, 2026
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
dashwallet-ios is migrating from DashSync to SwiftDashSDK. The backup seed phrase display (Settings → View Recovery Phrase) needs to read the 12-word mnemonic
from SwiftDashSDK instead of DashSync. Today WalletStorage only stores the post-PBKDF2 64-byte seed via storeSeed, which is one-way — you cannot recover the
human-readable words from it. This PR adds the missing mnemonic storage capability.
What was done?
Added three public methods to WalletStorage:
Idempotent (delete-before-add).
Added WalletStorageError.mnemonicNotFound enum case.
Storage uses plain keychain with kSecAttrAccessibleWhenUnlockedThisDeviceOnly — no PIN-derived encryption. This is parity with how DashSync stores mnemonics
(plaintext in keychain with OS-level protection). iOS keychain is hardware-backed (Secure Enclave), encrypted at rest, device-locked, and never backed up to
iCloud. Adding PIN encryption on top provides marginal value given a 4-digit PIN is brute-forceable in seconds at 10k PBKDF2 iterations.
How Has This Been Tested?
testing deferred to a device that doesn't hit the known [DSChain retrieveWallets] host-app crash on iPhone 17 + iOS 26.3.
Breaking Changes
None. Additive only — three new public methods and one new error case. No existing API signatures changed.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability