chore(swift-sdk): clean up core transactions, wallet, balance, acccounts, etc in swift sdk#3079
Open
chore(swift-sdk): clean up core transactions, wallet, balance, acccounts, etc in swift sdk#3079
Conversation
Contributor
📝 WalkthroughWalkthroughA significant architectural refactoring removes legacy transaction and filter-matching types, restructures WalletService from singleton to dependency-injected initialization, and simplifies HDWallet data model. SPVClient pointers are made optional with nil-cleanup, and public APIs for transaction/address management are consolidated. Multiple UI views are updated or removed to reflect the new service architecture. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
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: "27b315aadd0b2282dc995986a2b21c483f346b3634a4c5936cdf9cccee040ce4"
)Xcode manual integration:
|
ZocoLini
changed the title
ZocoLini
force-pushed
the
chore/core-swift-sdk-cleanup
branch
from
2aaca9d to
d37de8f
Compare
![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: 10
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/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift (1)
21-26:⚠️ Potential issue | 🟠 MajorHardcoded
99,999duffs breaks amount validation and shows a fake balance.This caps sends at
0.00099999 DASHregardless of actual wallet funds, and the “Insufficient balance” state is no longer tied to real account balance.Also applies to: 51-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift` around lines 21 - 26, The canSend computed property (and the related validation at lines 51-59) uses a hardcoded 99999 duffs cap which prevents sending based on the real wallet balance; replace that hardcoded limit with a check against the actual account/wallet balance (e.g., compare amount to a Wallet.balance or Account.availableBalance value), ensure amount is safely unwrapped (use optional binding) and greater than zero, and update the “Insufficient balance” logic to derive from the same real balance variable so the UI reflects true funds rather than the fixed 99,999 duffs ceiling.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift (1)
256-265:⚠️ Potential issue | 🟠 MajorValidate mnemonic content before creation call.
At Line 257-265, validation does not check mnemonic content (empty/whitespace/word count). Add explicit mnemonic validation before entering the async create path.
Suggested fix
private func createWallet(using mnemonic: String) { + let normalizedMnemonic = mnemonic + .trimmingCharacters(in: .whitespacesAndNewlines) + .lowercased() + let wordCount = normalizedMnemonic.split(whereSeparator: \.isWhitespace).count + guard !walletLabel.isEmpty, + !normalizedMnemonic.isEmpty, + [12, 15, 18, 21, 24].contains(wordCount), walletPin == confirmPin, walletPin.count >= 4 && walletPin.count <= 6 else { print("=== WALLET CREATION VALIDATION FAILED ===") print("Label empty: \(walletLabel.isEmpty)") print("PINs match: \(walletPin == confirmPin)") print("PIN length valid: \(walletPin.count >= 4 && walletPin.count <= 6)") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift` around lines 256 - 265, The createWallet(using:) method currently only validates walletLabel, walletPin and confirmPin but never checks the mnemonic; add explicit validation in createWallet(using:) that the mnemonic string is non-empty, not just whitespace, and meets expected word count (e.g., 12 or 24 words) before proceeding to the async creation path—use the existing parameters walletLabel, walletPin, confirmPin and the mnemonic argument to perform these checks and return early with the same failure logging if the mnemonic is invalid.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift (2)
607-615:⚠️ Potential issue | 🟠 MajorValidate derived private key against the address public key before display.
At Line 612–615, the derived private key is accepted and rendered without a private/public key consistency check. Add
KeyValidation.validatePrivateKeyForPublicKey(whendetail.publicKeyis present) before settingprivateKeyToShow.As per coding guidelines, "Always validate private keys match their public keys using KeyValidation.validatePrivateKeyForPublicKey before cryptographic operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift` around lines 607 - 615, The derived WIF private key must be validated against the address public key before being shown: after calling walletService.walletManager.derivePrivateKeyAsWIF(for:accountInfo:addressIndex:) and before setting showingPrivateKey/privateKeyToShow, if detail.publicKey is non-nil convert the derived WIF to the raw private key form expected by KeyValidation.validatePrivateKeyForPublicKey (or derive hex via the wallet manager if available) and call KeyValidation.validatePrivateKeyForPublicKey(privateKey: , publicKey: detail.publicKey); only set self.privateKeyToShow and self.showingPrivateKey when validation succeeds, and otherwise handle failure (clear vars and surface an error/alert or log) so invalid private keys are never displayed.
604-611:⚠️ Potential issue | 🔴 CriticalEnforce PIN before private-key derivation.
At Line 607,
pinis not used at all, so the PIN prompt currently does not protect key export. Add explicit PIN verification (or pass the PIN into the FFI call if that is the intended contract) before deriving the WIF key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift` around lines 604 - 611, The derivePrivateKeyWithPIN function currently ignores the supplied pin and directly calls walletService.walletManager.derivePrivateKeyAsWIF, so the PIN prompt does not protect key export; fix by verifying the PIN before derivation (e.g., call the existing PIN-check API such as walletService.verifyPIN(pin) or walletService.walletManager.verifyPIN(pin) if available) or, if the FFI requires it, call a derivePrivateKeyAsWIF overload that accepts the pin (e.g., derivePrivateKeyAsWIF(for: wallet, accountInfo: account, addressIndex: detail.index, pin: pin)); ensure you use the AddressDetail (detail), wallet, and account identifiers already present and only proceed to call derivePrivateKeyAsWIF after successful PIN verification.
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift (1)
389-402: Consolidate duplicatedgetAccounts(for:)calls.Line 389, Line 395, and Line 401 call the same async method in separate branches. Fetch once, then assign to the enabled network slot(s). This reduces repeated FFI calls and simplifies maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift` around lines 389 - 402, The code currently calls walletService.walletManager.getAccounts(for:) three times; instead, call it once (await try? walletService.walletManager.getAccounts(for: wallet)) store the result in a local variable (e.g., accountsList) and then set mainnetAccountCount/testnetAccountCount/devnetAccountCount from accountsList.count only when the corresponding flags (testnetEnabled, devnetEnabled, and mainnetEnabled if present) are true, otherwise set them to nil; preserve the same error-tolerant behavior (use try? semantics) and ensure you only perform the single async call in the scope where those counts are assigned.
🤖 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/Services/WalletService.swift`:
- Around line 118-129: The use of try! when constructing SPVClient and
CoreWalletManager in WalletService.init (creating spvClient and walletManager)
can crash the host app; update WalletService.init to either be a throwing
initializer or catch these errors and store them in a recoverable property
(e.g., lastSyncError) instead of force-unwrapping. Locate the SPVClient(...) and
CoreWalletManager(spvClient:..., modelContainer:...) calls and replace try! with
proper do/catch handling that assigns the caught Error to lastSyncError (and
leaves walletManager/spvClient nil) or change the init signature to throws and
propagate the errors upward, applying the same pattern for the similar block at
the other occurrence (around lines 153-171).
- Around line 237-247: switchNetwork currently logs the wrong "from" network and
recreates the SPV client twice; move the print of "Switching from ..." to before
assigning self.network (so it reports the prior network), and remove the
redundant initializeNewSPVClient() call because stopSync() already recreates the
client; update the switchNetwork function to log first, then set self.network,
call stopSync(), and do not call initializeNewSPVClient() again (references:
switchNetwork, stopSync, initializeNewSPVClient).
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift`:
- Around line 266-271: The destroy() method currently calls
dash_spv_ffi_client_destroy(client) and dash_spv_ffi_config_destroy(config)
unconditionally, which can double-free when destroy() is invoked multiple times
(explicitly or from deinit); make destroy() idempotent by guarding the optional
handles: check if client and config are non-nil before calling
dash_spv_ffi_client_destroy and dash_spv_ffi_config_destroy respectively, call
each FFI destructor only once, then set client = nil and config = nil; ensure
the same guard protects any callers from WalletService or deinit
double-invocation.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- Around line 175-177: The getReceiveAddress(for:accountIndex:) implementation
incorrectly hardcodes accountIndex to 0; update the call to
sdkWalletManager.getReceiveAddress(walletId:account.walletId, accountIndex:
accountIndex) so it passes the provided accountIndex parameter (reference:
function getReceiveAddress(for: HDWallet, accountIndex: UInt32) and
sdkWalletManager.getReceiveAddress(walletId:accountIndex:) and use
wallet.walletId). Ensure the passed accountIndex variable (not literal 0) is
used so non-default accounts return the correct address.
- Around line 151-173: The public methods getTransactions(for:accountIndex:) and
getBalance(for:accountIndex:) currently use force-try (try!) and can crash;
change their signatures to throw and propagate errors (or translate to a
WalletError) instead of force-unwrapping. Replace try!
sdkWalletManager.getManagedAccount(...) and try!
managedAccount.getTransactions(...) / try! managedAccount.getBalance() with
do/catch or direct throws so failures from sdkWalletManager.getManagedAccount
and ManagedAccount.getTransactions/getBalance are returned to callers (or mapped
to a WalletError enum) rather than terminating the app.
- Around line 441-443: The warning print uses a nil-coalescing operator on a
non-optional property causing a compile error; in CoreWalletManager (the
restoration block where wallet.walletId is compared to restoredWalletId) remove
the invalid "?? \"nil\"" from wallet.walletId.hexString and just use
wallet.walletId.hexString (and similarly ensure restoredWalletId.hexString is
used directly) so the print call becomes a plain interpolation of the two
hexString values.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/HDWallet.swift`:
- Line 7: The Example app breaks because removed types HDAddress and HDUTXO are
still referenced; either reintroduce them as compatibility typealiases/structs
in the SDK (e.g., add public typealias HDAddress = NewAddressType and public
typealias HDUTXO = NewUTXOType or lightweight wrapper structs) within
HDWallet.swift or update the Example and test code to use the new canonical
types and shapes used by HDWallet (replace HDAddress/HDUTXO usages in
WalletDetailView and TransactionTests with the new types and adjust any
property/initializer names accordingly), and remove or update the comment in
TransactionTests that refers to HDUTXO. Ensure the chosen fix preserves API
visibility (public) so the Example compiles.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift`:
- Around line 273-275: Remove the sensitive logging: delete the print that
outputs the recovery phrase (the local variable mnemonic computed from
showImportOption/importMnemonic/mnemonic) and also avoid logging wallet PIN
details (walletPin or its length) in CreateWalletView; if you need diagnostic
info keep only non-sensitive flags (e.g., "hasMnemonic: true/false") or use
sanitized booleans, and ensure any debugging messages referencing mnemonic or
walletPin are removed or replaced with safe indicators.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- Around line 125-127: The sendTransaction() method is a TODO and currently a
no-op; wire it to the existing wallet service to perform a send and show
user-facing errors: call walletService.walletManager.sendTransaction(...) (or
the appropriate send method on walletManager) to submit the transaction, handle
the async result with success and failure branches, update UI state on success,
and on failure present an Alert or error label with the
NSError.localizedDescription; also add a temporary fallback path that validates
required fields (amount, recipient) and returns a clear error if validation
fails before calling walletManager so taps always produce feedback.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- Line 459: Replace the force-try call to
walletService.walletManager.deleteWallet(wallet) with an explicit do-catch
around the await so failures don't crash; call try await
walletService.walletManager.deleteWallet(wallet) inside a do block and in catch
assign the caught Error to the view's existing error UI state (for example by
setting the viewModel/errorMessage state or calling the view's presentError(_:)
helper) so the error is surfaced to the UI instead of terminating the app.
---
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift`:
- Around line 607-615: The derived WIF private key must be validated against the
address public key before being shown: after calling
walletService.walletManager.derivePrivateKeyAsWIF(for:accountInfo:addressIndex:)
and before setting showingPrivateKey/privateKeyToShow, if detail.publicKey is
non-nil convert the derived WIF to the raw private key form expected by
KeyValidation.validatePrivateKeyForPublicKey (or derive hex via the wallet
manager if available) and call
KeyValidation.validatePrivateKeyForPublicKey(privateKey: , publicKey:
detail.publicKey); only set self.privateKeyToShow and self.showingPrivateKey
when validation succeeds, and otherwise handle failure (clear vars and surface
an error/alert or log) so invalid private keys are never displayed.
- Around line 604-611: The derivePrivateKeyWithPIN function currently ignores
the supplied pin and directly calls
walletService.walletManager.derivePrivateKeyAsWIF, so the PIN prompt does not
protect key export; fix by verifying the PIN before derivation (e.g., call the
existing PIN-check API such as walletService.verifyPIN(pin) or
walletService.walletManager.verifyPIN(pin) if available) or, if the FFI requires
it, call a derivePrivateKeyAsWIF overload that accepts the pin (e.g.,
derivePrivateKeyAsWIF(for: wallet, accountInfo: account, addressIndex:
detail.index, pin: pin)); ensure you use the AddressDetail (detail), wallet, and
account identifiers already present and only proceed to call
derivePrivateKeyAsWIF after successful PIN verification.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift`:
- Around line 256-265: The createWallet(using:) method currently only validates
walletLabel, walletPin and confirmPin but never checks the mnemonic; add
explicit validation in createWallet(using:) that the mnemonic string is
non-empty, not just whitespace, and meets expected word count (e.g., 12 or 24
words) before proceeding to the async creation path—use the existing parameters
walletLabel, walletPin, confirmPin and the mnemonic argument to perform these
checks and return early with the same failure logging if the mnemonic is
invalid.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- Around line 21-26: The canSend computed property (and the related validation
at lines 51-59) uses a hardcoded 99999 duffs cap which prevents sending based on
the real wallet balance; replace that hardcoded limit with a check against the
actual account/wallet balance (e.g., compare amount to a Wallet.balance or
Account.availableBalance value), ensure amount is safely unwrapped (use optional
binding) and greater than zero, and update the “Insufficient balance” logic to
derive from the same real balance variable so the UI reflects true funds rather
than the fixed 99,999 duffs ceiling.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- Around line 389-402: The code currently calls
walletService.walletManager.getAccounts(for:) three times; instead, call it once
(await try? walletService.walletManager.getAccounts(for: wallet)) store the
result in a local variable (e.g., accountsList) and then set
mainnetAccountCount/testnetAccountCount/devnetAccountCount from
accountsList.count only when the corresponding flags (testnetEnabled,
devnetEnabled, and mainnetEnabled if present) are true, otherwise set them to
nil; preserve the same error-tolerant behavior (use try? semantics) and ensure
you only perform the single async call in the scope where those counts are
assigned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36421e20-01fb-4bc3-83dc-4859f8a500dd
📒 Files selected for processing (26)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Models/CoreTransaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Models/CoreTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Models/FilterMatch.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/FilterMatchService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Utils/ModelContainerHelper.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/HDTransaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/HDWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/TransactionErrors.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/TransactionService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Shared/UnifiedStateManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AddressManagementView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/FilterMatchesView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ReceiveAddressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/UnifiedAppState.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletTests/TransactionTests.swift
💤 Files with no reviewable changes (13)
- packages/swift-sdk/Sources/SwiftDashSDK/Shared/UnifiedStateManager.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/HDTransaction.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Models/CoreTypes.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/TransactionService.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletTests/TransactionTests.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Models/FilterMatch.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Models/CoreTransaction.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Utils/ModelContainerHelper.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AddressManagementView.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/FilterMatchService.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/FilterMatchesView.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/TransactionErrors.swift
Comment on lines
+118
to
+129
| let spvClient = try! SPVClient( | ||
| network: network.sdkNetwork, | ||
| dataDir: dataDir, | ||
| startHeight: 0, | ||
| ) | ||
|
|
||
| // Just sync the current wallet from WalletManager | ||
| if let walletManager = self.walletManager { | ||
| Task { | ||
| // WalletManager's loadWallets() is called in its init | ||
| // We just need to sync the current wallet | ||
| if let wallet = walletManager.currentWallet { | ||
| self.currentWallet = wallet | ||
| await loadWallet(wallet) | ||
| } else if let firstWallet = walletManager.wallets.first { | ||
| self.currentWallet = firstWallet | ||
| await loadWallet(firstWallet) | ||
| } | ||
| } | ||
| } | ||
| self.spvClient = spvClient | ||
|
|
||
| SDKLogger.log("=== WalletService.configure END ===", minimumLevel: .medium) | ||
| // Create the SDK wallet manager by reusing the SPV client's shared manager | ||
| // TODO: Investigate this error | ||
| self.walletManager = try! CoreWalletManager(spvClient: spvClient, modelContainer: modelContainer) | ||
|
|
Contributor
There was a problem hiding this comment.
Avoid try! in SDK service initialization paths.
A recoverable startup error currently crashes the host app. This should either throw from WalletService.init or be converted to lastSyncError.
Proposed fix direction
-public init(modelContainer: ModelContainer, network: AppNetwork) {
+public init(modelContainer: ModelContainer, network: AppNetwork) throws {
...
- let spvClient = try! SPVClient(
+ let spvClient = try SPVClient(
network: network.sdkNetwork,
dataDir: dataDir,
startHeight: 0,
)
...
- self.walletManager = try! CoreWalletManager(spvClient: spvClient, modelContainer: modelContainer)
+ self.walletManager = try CoreWalletManager(spvClient: spvClient, modelContainer: modelContainer)
}Also applies to: 153-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift`
around lines 118 - 129, The use of try! when constructing SPVClient and
CoreWalletManager in WalletService.init (creating spvClient and walletManager)
can crash the host app; update WalletService.init to either be a throwing
initializer or catch these errors and store them in a recoverable property
(e.g., lastSyncError) instead of force-unwrapping. Locate the SPVClient(...) and
CoreWalletManager(spvClient:..., modelContainer:...) calls and replace try! with
proper do/catch handling that assigns the caught Error to lastSyncError (and
leaves walletManager/spvClient nil) or change the init signature to throws and
propagate the errors upward, applying the same pattern for the similar block at
the other occurrence (around lines 153-171).
packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
Show resolved
Hide resolved
Comment on lines
+151
to
+173
| public func getTransactions(for wallet: HDWallet, accountIndex: UInt32 = 0) -> [WalletTransaction] { | ||
| // Get managed account | ||
| let managedAccount = try sdkWalletManager.getManagedAccount( | ||
| walletId: walletId, | ||
| let managedAccount = try! sdkWalletManager.getManagedAccount( | ||
| walletId: wallet.walletId, | ||
| accountIndex: accountIndex, | ||
| accountType: .standardBIP44 | ||
| ) | ||
|
|
||
| // Get current height (TODO: get from SPV client when available) | ||
| let currentHeight: UInt32 = 0 | ||
|
|
||
| return try managedAccount.getTransactions(currentHeight: currentHeight) | ||
| return try! managedAccount.getTransactions(currentHeight: currentHeight) | ||
| } | ||
|
|
||
| // MARK: - Account Management | ||
|
|
||
| public func getBalance(for wallet: HDWallet, accountIndex: UInt32 = 0) -> Balance { | ||
| let managedAccount = try! sdkWalletManager.getManagedAccount( | ||
| walletId: wallet.walletId, | ||
| accountIndex: accountIndex, | ||
| accountType: .standardBIP44 | ||
| ) | ||
|
|
||
| return try! managedAccount.getBalance() | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid try! in public wallet query APIs.
These methods can terminate the app on normal failure paths (missing account, FFI read error). Convert to throws (or map to WalletError) instead of force-try.
Proposed fix direction
-public func getTransactions(for wallet: HDWallet, accountIndex: UInt32 = 0) -> [WalletTransaction] {
- let managedAccount = try! sdkWalletManager.getManagedAccount(
+public func getTransactions(for wallet: HDWallet, accountIndex: UInt32 = 0) throws -> [WalletTransaction] {
+ let managedAccount = try sdkWalletManager.getManagedAccount(
walletId: wallet.walletId,
accountIndex: accountIndex,
accountType: .standardBIP44
)
let currentHeight: UInt32 = 0
- return try! managedAccount.getTransactions(currentHeight: currentHeight)
+ return try managedAccount.getTransactions(currentHeight: currentHeight)
}
-public func getBalance(for wallet: HDWallet, accountIndex: UInt32 = 0) -> Balance {
- let managedAccount = try! sdkWalletManager.getManagedAccount(
+public func getBalance(for wallet: HDWallet, accountIndex: UInt32 = 0) throws -> Balance {
+ let managedAccount = try sdkWalletManager.getManagedAccount(
walletId: wallet.walletId,
accountIndex: accountIndex,
accountType: .standardBIP44
)
- return try! managedAccount.getBalance()
+ return try managedAccount.getBalance()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public func getTransactions(for wallet: HDWallet, accountIndex: UInt32 = 0) -> [WalletTransaction] { | |
| // Get managed account | |
| let managedAccount = try sdkWalletManager.getManagedAccount( | |
| walletId: walletId, | |
| let managedAccount = try! sdkWalletManager.getManagedAccount( | |
| walletId: wallet.walletId, | |
| accountIndex: accountIndex, | |
| accountType: .standardBIP44 | |
| ) | |
| // Get current height (TODO: get from SPV client when available) | |
| let currentHeight: UInt32 = 0 | |
| return try managedAccount.getTransactions(currentHeight: currentHeight) | |
| return try! managedAccount.getTransactions(currentHeight: currentHeight) | |
| } | |
| // MARK: - Account Management | |
| public func getBalance(for wallet: HDWallet, accountIndex: UInt32 = 0) -> Balance { | |
| let managedAccount = try! sdkWalletManager.getManagedAccount( | |
| walletId: wallet.walletId, | |
| accountIndex: accountIndex, | |
| accountType: .standardBIP44 | |
| ) | |
| return try! managedAccount.getBalance() | |
| } | |
| public func getTransactions(for wallet: HDWallet, accountIndex: UInt32 = 0) throws -> [WalletTransaction] { | |
| // Get managed account | |
| let managedAccount = try sdkWalletManager.getManagedAccount( | |
| walletId: wallet.walletId, | |
| accountIndex: accountIndex, | |
| accountType: .standardBIP44 | |
| ) | |
| // Get current height (TODO: get from SPV client when available) | |
| let currentHeight: UInt32 = 0 | |
| return try managedAccount.getTransactions(currentHeight: currentHeight) | |
| } | |
| public func getBalance(for wallet: HDWallet, accountIndex: UInt32 = 0) throws -> Balance { | |
| let managedAccount = try sdkWalletManager.getManagedAccount( | |
| walletId: wallet.walletId, | |
| accountIndex: accountIndex, | |
| accountType: .standardBIP44 | |
| ) | |
| return try managedAccount.getBalance() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`
around lines 151 - 173, The public methods getTransactions(for:accountIndex:)
and getBalance(for:accountIndex:) currently use force-try (try!) and can crash;
change their signatures to throw and propagate errors (or translate to a
WalletError) instead of force-unwrapping. Replace try!
sdkWalletManager.getManagedAccount(...) and try!
managedAccount.getTransactions(...) / try! managedAccount.getBalance() with
do/catch or direct throws so failures from sdkWalletManager.getManagedAccount
and ManagedAccount.getTransactions/getBalance are returned to callers (or mapped
to a WalletError enum) rather than terminating the app.
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift
Show resolved
Hide resolved
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift
Show resolved
Hide resolved
|
|
||
| @Model | ||
| public final class HDWallet: HDWalletModels { | ||
| public final class HDWallet { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find remaining usages in Example app.
rg -nP '\b(HDAddress|HDUTXO|HDAccount|HDWatchedAddress)\b' packages/swift-sdk/SwiftExampleApp --type swift
# Confirm type definitions are no longer present in SDK sources.
rg -nP '\b(class|struct)\s+(HDAddress|HDUTXO|HDAccount|HDWatchedAddress)\b' packages/swift-sdk/Sources/SwiftDashSDK --type swiftRepository: dashpay/platform
Length of output: 709
Breaking type removals are still referenced by the Example app.
HDAddress and HDUTXO types are removed from SDK sources but remain in use within packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift (lines 637, 656, 685, 704). This creates a compile blocker. Additionally, HDUTXO is referenced in a comment at packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletTests/TransactionTests.swift:21.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/HDWallet.swift` at line
7, The Example app breaks because removed types HDAddress and HDUTXO are still
referenced; either reintroduce them as compatibility typealiases/structs in the
SDK (e.g., add public typealias HDAddress = NewAddressType and public typealias
HDUTXO = NewUTXOType or lightweight wrapper structs) within HDWallet.swift or
update the Example and test code to use the new canonical types and shapes used
by HDWallet (replace HDAddress/HDUTXO usages in WalletDetailView and
TransactionTests with the new types and adjust any property/initializer names
accordingly), and remove or update the comment in TransactionTests that refers
to HDUTXO. Ensure the chosen fix preserves API visibility (public) so the
Example compiles.
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift
Show resolved
Hide resolved
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Show resolved
Hide resolved
| showError = true | ||
| } | ||
| } | ||
| try! await walletService.walletManager.deleteWallet(wallet) |
Contributor
There was a problem hiding this comment.
Replace try! in wallet deletion path (crash risk).
At Line 459, deletion failure will hard-crash the app. Handle the error explicitly and route it through existing error UI state.
Suggested fix
- try! await walletService.walletManager.deleteWallet(wallet)
+ do {
+ try await walletService.walletManager.deleteWallet(wallet)
+ } catch {
+ await MainActor.run {
+ errorMessage = "Failed to delete wallet: \(error.localizedDescription)"
+ showError = true
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try! await walletService.walletManager.deleteWallet(wallet) | |
| do { | |
| try await walletService.walletManager.deleteWallet(wallet) | |
| } catch { | |
| await MainActor.run { | |
| errorMessage = "Failed to delete wallet: \(error.localizedDescription)" | |
| showError = true | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`
at line 459, Replace the force-try call to
walletService.walletManager.deleteWallet(wallet) with an explicit do-catch
around the await so failures don't crash; call try await
walletService.walletManager.deleteWallet(wallet) inside a do block and in catch
assign the caught Error to the view's existing error UI state (for example by
setting the viewModel/errorMessage state or calling the view's presentError(_:)
helper) so the error is surfaced to the UI instead of terminating the app.
added 14 commits
March 9, 2026 15:11
…t functionalities
ZocoLini
force-pushed
the
chore/core-swift-sdk-cleanup
branch
from
f23769a to
acd0c33
Compare
ZocoLini
had a problem deploying
to
test-suite-approval
GitHub Actions
Error
ZocoLini
had a problem deploying
to
test-suite-approval
GitHub Actions
Error
ZocoLini
had a problem deploying
to
test-suite-approval
GitHub Actions
Error
ZocoLini
had a problem deploying
to
test-suite-approval
GitHub Actions
Error
ZocoLini
force-pushed
the
chore/core-swift-sdk-cleanup
branch
from
fdddf5d to
ccee368
Compare
ZocoLini
had a problem deploying
to
test-suite-approval
GitHub Actions
Error
ZocoLini
had a problem deploying
to
test-suite-approval
GitHub Actions
Error
ZocoLini
force-pushed
the
chore/core-swift-sdk-cleanup
branch
from
ccee368 to
5b55768
Compare
ZocoLini
requested a deployment
to
test-suite-approval
GitHub Actions
Waiting
ZocoLini
requested a deployment
to
test-suite-approval
GitHub Actions
Waiting
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
1 participant
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.
Balances where not being shown correctly and the main issue was the enormous amount of logic, the underlying Rust lib contains the logic and the sdk only needs to wrap it, dropped all logic that was unnecessary and added one method to CoreWalletManager wrapper:
This follows the same pattern the transactions follow, asking directly the ffi for the balance.
Note that this PR is a cleanup, I drop unused stuff and logic layers that are not useful, the PR fixes the mentioned issue, and a couple more found by cleaning, like wallets being persisted twice, SPVclient reference being hold somewhere preventing it from de-initializing, wallet crashes after delete and maybe more
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
Refactor
Breaking Changes
CoreTransaction, filter matching, and transaction service APIs; wallet creation now requires explicit mnemonic parameter.