Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds transaction building and broadcasting APIs and types: new buildSignedTransaction in wallet managers (one missing return, one duplicated), broadcastTransaction on SPV client (FFI call + SPVError.transactionBroadcastFailed), renames SPVBlockHeadersProgress.currentHeight → tipHeight, replaces BuildAndSignResult with FeeRate, and updates example send UI to build and broadcast transactions. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as SendTransactionView
participant WalletMgr as WalletManager
participant SPV as SPVClient
participant FFI as FFI Layer
User->>UI: enter recipient, amount, choose fee rate
UI->>WalletMgr: buildSignedTransaction(wallet, accIndex, outputs)
WalletMgr->>FFI: prepare outputs & call build/sign FFI
FFI-->>WalletMgr: (txBytes, fee) or error
WalletMgr-->>UI: return (transactionData, fee) or throw
UI->>UI: validate amount + fee ≤ spendable balance
User->>UI: confirm send
UI->>SPV: broadcastTransaction(transactionData)
SPV->>FFI: call broadcast FFI
FFI-->>SPV: success or error
SPV-->>UI: success / throws transactionBroadcastFailed
UI-->>User: show success or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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 |
ZocoLini
force-pushed
the
feat/send-transaction-swift
branch
5 times, most recently
from
e52c1dc to
9d6e256
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swift (1)
54-66:⚠️ Potential issue | 🟠 MajorKeep a deprecated
currentHeightshim for this public rename.Changing a public SDK property from
currentHeighttotipHeightis source-breaking for any downstream app that has not been updated with this PR. Unless this is intentionally shipping as a breaking release, add a deprecated alias so existing callers keep compiling.Compatibility shim
public struct SPVBlockHeadersProgress: Sendable { public let state: SPVSyncState public let tipHeight: UInt32 + `@available`(*, deprecated, renamed: "tipHeight") + public var currentHeight: UInt32 { tipHeight } public let targetHeight: UInt32🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swift` around lines 54 - 66, The public rename removed the previously exposed currentHeight and will break downstream code; add a deprecated computed property currentHeight to SPVBlockHeadersProgress that returns tipHeight (mark it `@available`(*, deprecated, message: "Use tipHeight instead")) so existing callers continue to compile, and ensure the init and other code continue to set tipHeight as before; reference struct SPVBlockHeadersProgress and property tipHeight when adding the shim.
🤖 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/KeyWallet/WalletManager.swift`:
- Line 407: The send flow in WalletManager currently force-unwraps the result of
getWallet(id:) (let wallet = try self.getWallet(id: wallet.walletId)!), which
can be nil after a client reset or during re-import and will crash; change this
to safely handle the nil by using optional binding (guard/if let) or by throwing
a descriptive error (e.g., WalletNotFound or WalletUnavailable) from the send
method when getWallet(id:) returns nil, and ensure downstream code uses the
non-optional wallet only after the safe check so the send becomes a recoverable
error path instead of a runtime crash.
- Around line 409-432: The FFI outputs created by outputs.map { $0.toFFI() }
allocate duplicated C strings for each output address via strdup(), but they are
never freed; in the defer block (after the wallet_build_and_sign_transaction
call) iterate the ffiOutputs array and free each duplicated address pointer
(e.g. call free on each ffiOutput.address or the actual field name returned by
Transaction.Output.toFFI()), then proceed to free error.message and txBytesPtr
as already done so no C string leaks remain.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- Around line 130-148: recalculateTransaction() currently swallows errors by
clearing fee and tx on failure; add a stored error state (e.g.,
transactionError: Error?) and set it in the catch block instead of silently
clearing so the view can present the failure; keep clearing transactionError
only when inputs are empty (the guard branch) or when a subsequent successful
createTransaction() returns, and update the UI bindings to show transactionError
alongside fee/tx (references: recalculateTransaction(), createTransaction(),
fee, tx, recipientAddress, amount, balance.spendable).
- Around line 21-24: The computed property amount must stop using Double; parse
amountString into a Decimal, validate that the fractional scale is at most 8 (no
more than 8 decimal places), multiply the Decimal by 100_000_000 exactly (use
Decimal arithmetic/NSDecimalNumber to avoid floating-point rounding), round if
necessary using .plain rounding only after validating scale, then safely convert
the resulting integral Decimal to UInt64; update the amount computed property
(referencing amountString and amount) to return 0 or handle parse/precision
failures consistently.
- Around line 33-35: The balance computed property uses
walletService.walletManager.getBalance with incorrect parameter names; update
the call in the balance getter to call CoreWalletManager.getBalance(for: wallet,
accountIndex: 0) (replace accType:/.standardBIP44 and accIndex: with the single
accountIndex: parameter) so it matches the getBalance(for:accountIndex:)
signature.
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swift`:
- Around line 54-66: The public rename removed the previously exposed
currentHeight and will break downstream code; add a deprecated computed property
currentHeight to SPVBlockHeadersProgress that returns tipHeight (mark it
`@available`(*, deprecated, message: "Use tipHeight instead")) so existing callers
continue to compile, and ensure the init and other code continue to set
tipHeight as before; reference struct SPVBlockHeadersProgress and property
tipHeight when adding the shim.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c37e876f-9947-4e1d-b010-187d9c0b977d
📒 Files selected for processing (9)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
💤 Files with no reviewable changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
Outdated
Show resolved
Hide resolved
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Show resolved
Hide resolved
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Show resolved
Hide resolved
thepastaclaw
suggested changes
Mar 17, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR adds transaction sending to the Swift SDK with FFI-based transaction building/signing, fee rate selection, and broadcast. Three blocking issues prevent merge: a memory leak from strdup'd C strings never freed (leaking on every keystroke), a force-unwrap that crashes when a wallet isn't found, and a call to a getBalance overload that doesn't exist (won't compile).
Reviewed commit: 9d6e256
🔴 3 blocking | 🟡 2 suggestion(s) | 💬 2 nitpick(s)
🤖 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/KeyWallet/Transaction.swift`:
- [BLOCKING] lines 31-34: Memory leak: strdup'd C strings in toFFI() are never freed
`strdup(address)` at line 32 allocates heap memory for each `FFITxOutput`. In `WalletManager.buildSignedTransaction` (line 409), `outputs.map { $0.toFFI() }` creates these allocations, but the `defer` block only frees `error.message` and `txBytesPtr` — the `ffiOutputs[].address` pointers are never freed.
`SendTransactionView.recalculateTransaction()` fires on every keystroke via `.onChange` handlers (lines 109-117), so each character typed leaks one strdup allocation per output.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- [BLOCKING] line 407: Force-unwrap on getWallet() will crash when wallet is not found
`let wallet = try self.getWallet(id: wallet.walletId)!` — `getWallet(id:)` returns `Wallet?` and explicitly returns `nil` for NOT_FOUND errors (line 163). Force-unwrapping crashes the app. The parameter name `wallet` (an `HDWallet`) also shadows the result, making the force-unwrap easy to miss during review.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- [BLOCKING] lines 33-35: getBalance call uses a non-existent method signature — won't compile
Line 34 calls `walletService.walletManager.getBalance(for: wallet, accType: .standardBIP44, accIndex: 0)` but `CoreWalletManager.getBalance` (line 171) has the signature `getBalance(for wallet: HDWallet, accountIndex: UInt32 = 0) -> Balance`. There is no `accType` parameter. Other call sites in this repo (CoreContentView.swift:460, WalletDetailView.swift:487) use the correct signature. This will fail to compile.
- [SUGGESTION] lines 21-23: Floating-point arithmetic causes off-by-one duff rounding errors
`UInt64(double * 100_000_000)` uses IEEE 754 double-precision which cannot exactly represent many decimal fractions. For example, `Double("0.29")! * 100_000_000` evaluates to `28999999.999999996`, truncated to `28999999` duffs — one duff less than intended. Use `Decimal` for exact base-10 arithmetic.
- [SUGGESTION] lines 109-117: Full transaction build+sign fires on every keystroke
`recalculateTransaction()` fires on every character typed in address and amount fields. Each call goes through FFI to `wallet_build_and_sign_transaction`, which selects UTXOs, constructs, and signs a transaction. Consider debouncing (e.g., with a short `Task.sleep` delay that cancels on the next keystroke) to avoid unnecessary FFI calls and the associated strdup leaks.
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
Outdated
Show resolved
Hide resolved
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Show resolved
Hide resolved
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Outdated
Show resolved
Hide resolved
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Outdated
Show resolved
Hide resolved
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift
Outdated
Show resolved
Hide resolved
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Show resolved
Hide resolved
ZocoLini
force-pushed
the
feat/send-transaction-swift
branch
from
9d6e256 to
9d604c0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift (2)
21-24:⚠️ Potential issue | 🟠 MajorDon't convert DASH amounts through
Double— useDecimalfor exact arithmetic.IEEE 754 double-precision cannot exactly represent many decimal fractions. For example,
Double("0.29")! * 100_000_000evaluates to28999999.999999996, which truncates to28999999duffs — one duff less than intended.🔧 Proposed fix using Decimal
private var amount: UInt64 { - guard let double = Double(amountString) else { return 0 } - return UInt64(double * 100_000_000) // Convert DASH to duffs + guard let decimal = Decimal(string: amountString) else { return 0 } + let duffs = decimal * 100_000_000 + return NSDecimalNumber(decimal: duffs).uint64Value }🤖 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 - 24, The computed property `amount` currently parses `amountString` to a Double which causes precision loss; change it to parse using Decimal (e.g., Decimal(string: amountString)) and perform exact multiplication by 100_000_000 with appropriate rounding (use .plain or .bankers as desired) before converting to UInt64, and ensure you handle nil/invalid input and overflow safely; update the `amount` getter to use `Decimal` arithmetic and rounding rather than `Double` to produce the correct duffs value.
140-148:⚠️ Potential issue | 🟠 MajorSurface transaction-build failures to the user.
When
createTransaction()fails, the error is swallowed and the user only sees a disabled Send button with no indication of what went wrong (invalid address, insufficient balance, signing failure, etc.). The view already has anerrorstate — use it here.🔧 Proposed fix
+ `@State` private var transactionError: Error? = nil + private func recalculateTransaction() { guard !recipientAddress.isEmpty, amount > 0, amount <= balance.spendable else { self.fee = 0 self.tx = nil + self.transactionError = nil return } do { let (tx, fee) = try createTransaction() self.fee = fee self.tx = tx + self.transactionError = nil } catch { self.fee = 0 self.tx = nil + self.transactionError = error } }Then display
transactionErrorin the UI when non-nil (e.g., as footer text below the fee section).As per coding guidelines, "Implement proper loading and error states in SwiftUI views".
🤖 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 140 - 148, The catch block for createTransaction() currently swallows errors; update it to assign the thrown error to the view's error state (e.g., set self.error or self.transactionError = error), and still clear self.fee and self.tx as before. Then ensure the view displays that error state (render transactionError/error as footer text under the fee section) so users see a meaningful message (invalid address, insufficient balance, signing failure) instead of only a disabled Send button.packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (2)
409-432:⚠️ Potential issue | 🟠 MajorFree
strdup-allocated output addresses in the defer cleanup.
outputs.map { $0.toFFI() }allocates C strings for each output address viastrdup, but Line 425-432 does not release them. This leaks memory on each transaction build.#!/bin/bash # Verify allocation in Transaction.Output.toFFI and missing free in buildSignedTransaction cleanup. rg -n -C2 'func toFFI\(\)|strdup\(' packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift rg -n -C5 'let ffiOutputs = outputs\.map \{ \$0\.toFFI\(\) \}|defer \{|transaction_bytes_free|error_message_free|free\(' packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftProposed fix
defer { if error.message != nil { error_message_free(error.message) } + for output in ffiOutputs { + if let address = output.address { + free(UnsafeMutableRawPointer(address)) + } + } if let ptr = txBytesPtr { transaction_bytes_free(ptr) } }As per coding guidelines, Swift code must properly wrap FFI functions with correct memory management across Swift/Rust boundaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift` around lines 409 - 432, The defer block after calling wallet_build_and_sign_transaction leaks strdup-allocated C strings created by Transaction.Output.toFFI; update the cleanup to iterate ffiOutputs and free each allocated address string (the C pointer inside each FFI output struct produced by outputs.map { $0.toFFI() }) before or inside the existing defer that already frees error.message and txBytesPtr so all FFI-allocated memory is released; ensure you reference the same ffiOutputs array used to build outputsPtr and call the appropriate C free function for those strdup pointers.
407-407:⚠️ Potential issue | 🔴 CriticalReplace force-unwrap with a recoverable wallet-not-found error.
Line 407 can crash when
getWallet(id:)returnsnil(which is an expected path for NOT_FOUND). This should be guarded and thrown as a wallet error instead of force-unwrapped.#!/bin/bash # Verify nil-return behavior and force-unwrap use in this file. rg -n -C2 'func getWallet\(id walletId: Data\) throws -> Wallet\?' packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift rg -n -C2 'NOT_FOUND|return nil' packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift rg -n -C2 'getWallet\(id: wallet\.walletId\)!\b' packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftProposed fix
- let wallet = try self.getWallet(id: wallet.walletId)! + guard let sdkWallet = try self.getWallet(id: wallet.walletId) else { + throw KeyWalletError.walletError("Wallet not found in manager") + } @@ - wallet.ffiHandle, + sdkWallet.ffiHandle,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift` at line 407, The force-unwrap on the result of getWallet(id:) can crash when it returns nil; replace the force-unwrap with a guard that throws a recoverable wallet-not-found error (e.g. throw WalletError.notFound or WalletManagerError.walletNotFound) including the walletId for context: call let wallet = try self.getWallet(id: wallet.walletId) and if it is nil throw the new/appropriate error; ensure the error type you use exists or add it to the WalletManager/KeyWallet error enum and keep call sites that handle thrown errors unchanged.
🤖 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/KeyWallet/WalletManager.swift`:
- Around line 390-396: The docstring for
WalletManager.buildSignedTransaction(for: accIndex: outputs: feeRate:) is out of
sync: update the parameter list and description to reflect the actual API
(rename or document accIndex as accountIndex if you prefer consistent naming),
replace the stale `feePerKB` entry with `feeRate` (describe FeeRate
units/meaning), change the function summary to say it builds and returns a
signed transaction, and update the Returns line to indicate it returns signed
transaction bytes (Data) and the fee (UInt64). Ensure the parameter names in the
doc tags match the function signature (wallet, accIndex/accountIndex, outputs,
feeRate) and the wording makes clear the payload is signed.
---
Duplicate comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- Around line 409-432: The defer block after calling
wallet_build_and_sign_transaction leaks strdup-allocated C strings created by
Transaction.Output.toFFI; update the cleanup to iterate ffiOutputs and free each
allocated address string (the C pointer inside each FFI output struct produced
by outputs.map { $0.toFFI() }) before or inside the existing defer that already
frees error.message and txBytesPtr so all FFI-allocated memory is released;
ensure you reference the same ffiOutputs array used to build outputsPtr and call
the appropriate C free function for those strdup pointers.
- Line 407: The force-unwrap on the result of getWallet(id:) can crash when it
returns nil; replace the force-unwrap with a guard that throws a recoverable
wallet-not-found error (e.g. throw WalletError.notFound or
WalletManagerError.walletNotFound) including the walletId for context: call let
wallet = try self.getWallet(id: wallet.walletId) and if it is nil throw the
new/appropriate error; ensure the error type you use exists or add it to the
WalletManager/KeyWallet error enum and keep call sites that handle thrown errors
unchanged.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- Around line 21-24: The computed property `amount` currently parses
`amountString` to a Double which causes precision loss; change it to parse using
Decimal (e.g., Decimal(string: amountString)) and perform exact multiplication
by 100_000_000 with appropriate rounding (use .plain or .bankers as desired)
before converting to UInt64, and ensure you handle nil/invalid input and
overflow safely; update the `amount` getter to use `Decimal` arithmetic and
rounding rather than `Double` to produce the correct duffs value.
- Around line 140-148: The catch block for createTransaction() currently
swallows errors; update it to assign the thrown error to the view's error state
(e.g., set self.error or self.transactionError = error), and still clear
self.fee and self.tx as before. Then ensure the view displays that error state
(render transactionError/error as footer text under the fee section) so users
see a meaningful message (invalid address, insufficient balance, signing
failure) instead of only a disabled Send button.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9e83c22-e335-4a37-b8d8-683ba156a5a0
📒 Files selected for processing (9)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
💤 Files with no reviewable changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
Outdated
Show resolved
Hide resolved
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: "3598343e6b170dea4b4050bd3be7f80ae17199a7b92b7c83d95db8226bbbaf62"
)Xcode manual integration:
|
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Trivial incremental push: 1-line change in example app (SendTransactionView.swift) adapting getBalance() call to updated API signature — removes accType parameter, renames accIndex to accountIndex. No new issues introduced.
Reviewed commit: 9d604c0
ZocoLini
force-pushed
the
feat/send-transaction-swift
branch
from
9d604c0 to
063b7cc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift (1)
145-153:⚠️ Potential issue | 🟡 MinorSync the doc comment with the new API surface.
The comment still documents
accountIndex/feePerKBand only mentions returning transaction bytes, but the public signature takesaccIndex/feeRateand returns(Data, UInt64). That mismatch will mislead callers and generated docs.Suggested fix
/// Build a signed transaction /// - Parameters: - /// - accountIndex: The account index to use + /// - wallet: The wallet to build the transaction for + /// - accIndex: The account index to use /// - outputs: The transaction outputs - /// - feePerKB: Fee per kilobyte in satoshis - /// - Returns: The signed transaction bytes + /// - feeRate: The fee priority to use for fee estimation + /// - Returns: The signed transaction bytes and computed fee in duffs🤖 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 145 - 153, Update the doc comment on CoreWalletManager.buildSignedTransaction to match the new API: replace outdated parameter names/accountation (accountIndex, feePerKB) with accIndex and feeRate and clearly describe what each represents (accIndex = account index used; feeRate = FeeRate object used for fee calculation), and update the Returns section to state it returns a tuple (Data, UInt64) where Data is the signed transaction bytes and UInt64 is the computed fee in satoshis; ensure the param and returns tags reference accIndex, outputs, feeRate and the (Data, UInt64) return shape.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift (2)
141-149:⚠️ Potential issue | 🟠 MajorDon't collapse build failures into a disabled button.
This catch path clears
feeandtxand drops the actual error, so invalid addresses, fee-too-high cases, and missing wallet state all look the same to the user. Keep a transaction-build error in view state and render it inline or through the existing alert.As per coding guidelines, "Implement proper loading and error states in SwiftUI views".
🤖 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 141 - 149, The catch currently swallows createTransaction() errors and resets fee/tx, making all failures invisible to the UI; instead add and set a view state like transactionError (e.g. transactionBuildError: Error? or String?) inside the catch and assign the caught error to it, and only clear fee/tx if truly invalid—do not drop the error. Update the catch block that surrounds createTransaction() to set self.transactionError = error (and optionally self.tx = nil), leave or set fee appropriately, and make the UI render transactionError inline or via the existing alert so the user sees specific build failures.
21-24:⚠️ Potential issue | 🔴 CriticalReject inputs that are not an exact duff value.
This still accepts more than 8 fractional digits and then silently changes the amount during
uint64Valueconversion. In a send flow, values like0.123456789should fail validation, not turn into a smaller payment.Suggested fix
private var amount: UInt64 { - guard let dashAmount = Decimal(string: amountString) else { return 0 } - let duffAmount = dashAmount * 100_000_000 - return NSDecimalNumber(decimal: duffAmount).uint64Value + guard let dashAmount = Decimal(string: amountString), + dashAmount >= 0 + else { return 0 } + + var value = dashAmount + var rounded = Decimal() + NSDecimalRound(&rounded, &value, 8, .plain) + guard rounded == dashAmount else { return 0 } + + let duffAmount = rounded * 100_000_000 + let number = NSDecimalNumber(decimal: duffAmount) + guard number != NSDecimalNumber.notANumber, + number.compare(NSDecimalNumber(value: UInt64.max)) != .orderedDescending + else { return 0 } + + return number.uint64Value }🤖 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 - 24, The computed property amount currently accepts amountString values with >8 fractional digits and silently truncates when converting to UInt64; update SendTransactionView.amount to explicitly validate that Decimal(string: amountString) converts exactly to an integer number of duffs (i.e., fractional digits ≤ 8 and decimal * 100_000_000 has no fractional part) and reject/flag the input when it does not (for example by returning nil or setting a validation error instead of silently returning 0). Use the existing symbols amount, amountString and the duff multiplier (100_000_000) to locate the logic and perform the exactness check before creating NSDecimalNumber.uint64Value.packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (1)
411-437:⚠️ Potential issue | 🔴 CriticalFree the duplicated output addresses in Swift.
Transaction.Output.toFFI()now allocates each address withstrdup(), but this defer never releases those pointers. Because Swift performed the allocation, Swift shouldfreethem afterwallet_build_and_sign_transactionreturns.Suggested fix
defer { if error.message != nil { error_message_free(error.message) } for output in ffiOutputs { - // TODO: Memory leak, FFI doesnt expose a way to free the address + if let address = output.address { + free(UnsafeMutableRawPointer(address)) + } } if let ptr = txBytesPtr { transaction_bytes_free(ptr) } }As per coding guidelines, "Swift code must properly wrap FFI functions with correct memory management across Swift/Rust boundaries".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift` around lines 411 - 437, The ffiOutputs array contains FFI structs produced by Transaction.Output.toFFI() that strdup()'d the address string, but the defer currently doesn't free those duplicated address pointers; after the wallet_build_and_sign_transaction call you should free each allocated address (e.g. iterate ffiOutputs and call free(...) on the struct's address field) inside the defer (alongside the existing error_message_free and transaction_bytes_free) so Swift reclaims the strdup()'d memory — use the platform free (Darwin.C.free or Glibc.free) or libc.free as appropriate.
🤖 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/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- Around line 27-31: The canSend computed property risks overflow by evaluating
amount + fee; change its funds check to a subtraction-based guard such as
ensuring fee <= balance.spendable and amount <= balance.spendable - fee (or
extract a shared hasSufficientFunds(amount:fee:) helper and use it in canSend
and the footer) so the button gating and footer share the same safe logic;
update usages of canSend/footer insufficient-funds logic to call that helper or
the new subtraction-style condition and keep recipientAddress, amount, fee,
balance.spendable and tx checks intact.
---
Duplicate comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- Around line 145-153: Update the doc comment on
CoreWalletManager.buildSignedTransaction to match the new API: replace outdated
parameter names/accountation (accountIndex, feePerKB) with accIndex and feeRate
and clearly describe what each represents (accIndex = account index used;
feeRate = FeeRate object used for fee calculation), and update the Returns
section to state it returns a tuple (Data, UInt64) where Data is the signed
transaction bytes and UInt64 is the computed fee in satoshis; ensure the param
and returns tags reference accIndex, outputs, feeRate and the (Data, UInt64)
return shape.
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- Around line 411-437: The ffiOutputs array contains FFI structs produced by
Transaction.Output.toFFI() that strdup()'d the address string, but the defer
currently doesn't free those duplicated address pointers; after the
wallet_build_and_sign_transaction call you should free each allocated address
(e.g. iterate ffiOutputs and call free(...) on the struct's address field)
inside the defer (alongside the existing error_message_free and
transaction_bytes_free) so Swift reclaims the strdup()'d memory — use the
platform free (Darwin.C.free or Glibc.free) or libc.free as appropriate.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- Around line 141-149: The catch currently swallows createTransaction() errors
and resets fee/tx, making all failures invisible to the UI; instead add and set
a view state like transactionError (e.g. transactionBuildError: Error? or
String?) inside the catch and assign the caught error to it, and only clear
fee/tx if truly invalid—do not drop the error. Update the catch block that
surrounds createTransaction() to set self.transactionError = error (and
optionally self.tx = nil), leave or set fee appropriately, and make the UI
render transactionError inline or via the existing alert so the user sees
specific build failures.
- Around line 21-24: The computed property amount currently accepts amountString
values with >8 fractional digits and silently truncates when converting to
UInt64; update SendTransactionView.amount to explicitly validate that
Decimal(string: amountString) converts exactly to an integer number of duffs
(i.e., fractional digits ≤ 8 and decimal * 100_000_000 has no fractional part)
and reject/flag the input when it does not (for example by returning nil or
setting a validation error instead of silently returning 0). Use the existing
symbols amount, amountString and the duff multiplier (100_000_000) to locate the
logic and perform the exactness check before creating
NSDecimalNumber.uint64Value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89c037cd-2969-41c4-9d1f-c6327baa9102
📒 Files selected for processing (9)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
💤 Files with no reviewable changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Show resolved
Hide resolved
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental push addressing 5 of 7 prior findings. The force-unwrap crash is fixed (guard let), floating-point rounding now uses Decimal, doc comments corrected, and the ~ prefix properly scoped to fee display only.
The memory leak in toFFI() is acknowledged with TODO comments but not actually freed — the for output in ffiOutputs { } loop body is empty. This is a deliberate deferral since the FFI doesn't expose a free method, which is reasonable, but the allocation still leaks on every recalculateTransaction() call.
The keystroke debouncing suggestion remains unaddressed.
Reviewed commit: 063b7cc
ZocoLini
force-pushed
the
feat/send-transaction-swift
branch
2 times, most recently
from
30e0c0e to
a5a1e31
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift (1)
132-133:⚠️ Potential issue | 🟠 Major
destroy()must be idempotent to prevent double-teardown in the SPVClient lifecycle.When
WalletService.initializeNewSPVClient()callsdestroy()at line 148 and then reassignsspvClient, the old instance deinitializes and callsdestroy()again at line 133. The current implementation unconditionally passes pointers to FFI functions without nil checks, so the second call invokes FFI teardown with nil pointers, causing undefined behavior. Guard the FFI calls withif letchecks to makedestroy()safe to call multiple times.Proposed fix
func destroy() { - dash_spv_ffi_client_destroy(client) - dash_spv_ffi_config_destroy(config) - - client = nil - config = nil + if let client { + dash_spv_ffi_client_destroy(client) + self.client = nil + } + if let config { + dash_spv_ffi_config_destroy(config) + self.config = nil + } }As per coding guidelines, Swift code must properly wrap FFI functions with correct memory management across Swift/Rust boundaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift` around lines 132 - 133, The destroy() implementation in SPVClient and its deinit must be made idempotent: guard against nil/invalid pointers before calling any FFI teardown functions so a second invocation is a no-op; update destroy() to check optional FFI handles (e.g., the SPVClient's underlying pointer(s) and any related handles used by the FFI teardown functions) with if let / guard let and only call the FFI free/teardown once, then nil out/clear those stored pointers, and leave deinit calling destroy() unchanged so WalletService.initializeNewSPVClient() can safely call destroy() and the subsequent deinit won’t re-invoke FFI with nil pointers.
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (1)
390-425: Either exposefeeRatehere or drop the new abstraction for now.Line 420 still hardcodes
1000, so.economyand.prioritynever influence transaction building even thoughTransaction.swiftnow definesFeeRateand this doc block still describes afeeRateinput. ThreadfeeRatethrough this API, or keep a fixed-fee API until that end-to-end wiring exists.Possible fix
- public func buildSignedTransaction(for wallet: HDWallet, accIndex: UInt32, outputs: [Transaction.Output]) throws -> (Data, UInt64) { + public func buildSignedTransaction( + for wallet: HDWallet, + accIndex: UInt32, + outputs: [Transaction.Output], + feeRate: FeeRate = .normal + ) throws -> (Data, UInt64) { @@ - 1000, + feeRate.intoDuffPerKB(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift` around lines 390 - 425, The doc and Transaction.FeeRate enum indicate feeRate should be configurable, but buildSignedTransaction(for:accIndex:outputs:) still hardcodes 1000 when calling wallet_build_and_sign_transaction; thread a FeeRate parameter through this API (e.g., add feeRate: Transaction.FeeRate to buildSignedTransaction) and map the enum to the corresponding duffs-per-kb numeric value to replace the hardcoded 1000 in the wallet_build_and_sign_transaction call, update the function signature and all callers, and adjust the doc comment to reflect the new parameter; alternatively, if you want fixed-fee behavior, remove the feeRate mention from the doc and keep the hardcoded value.
🤖 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 107-128: Replace the force-throws in initializeNewSPVClient by
wrapping the SPVClient(...) construction, CoreWalletManager(spvClient:...,
modelContainer:...), and any setMasternodeSyncEnabled(...) calls in do-catch
blocks: call them with try, catch the thrown Error, assign it to lastSyncError,
and return/abort the rest of initialization so the app doesn’t crash; keep
existing stopSync()/clearSpvStorage()/switchNetwork paths intact but ensure
initializeNewSPVClient sets lastSyncError instead of crashing. Target the
initializeNewSPVClient function and the specific constructors/methods SPVClient,
CoreWalletManager, and setMasternodeSyncEnabled when applying the changes.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- Around line 20-24: The computed amount property silently truncates precision
beyond 8 fractional DASH digits and allows negative inputs; update the amount
getter (which parses amountString into a Decimal and multiplies by 100_000_000)
to validate before converting: first ensure the parsed Decimal is non‑negative,
then multiply by 100_000_000 into a Decimal/NSDecimalNumber and reject the input
if that product has any fractional part (i.e., more precision than whole duffs)
or if the original string has >8 fractional digits; if validation fails return 0
(or signal invalid input) rather than truncating, and only then safely convert
NSDecimalNumber(...).uint64Value. Use the existing amountString, amount getter,
dashAmount/duffAmount variables to locate where to add these checks.
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift`:
- Around line 132-133: The destroy() implementation in SPVClient and its deinit
must be made idempotent: guard against nil/invalid pointers before calling any
FFI teardown functions so a second invocation is a no-op; update destroy() to
check optional FFI handles (e.g., the SPVClient's underlying pointer(s) and any
related handles used by the FFI teardown functions) with if let / guard let and
only call the FFI free/teardown once, then nil out/clear those stored pointers,
and leave deinit calling destroy() unchanged so
WalletService.initializeNewSPVClient() can safely call destroy() and the
subsequent deinit won’t re-invoke FFI with nil pointers.
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- Around line 390-425: The doc and Transaction.FeeRate enum indicate feeRate
should be configurable, but buildSignedTransaction(for:accIndex:outputs:) still
hardcodes 1000 when calling wallet_build_and_sign_transaction; thread a FeeRate
parameter through this API (e.g., add feeRate: Transaction.FeeRate to
buildSignedTransaction) and map the enum to the corresponding duffs-per-kb
numeric value to replace the hardcoded 1000 in the
wallet_build_and_sign_transaction call, update the function signature and all
callers, and adjust the doc comment to reflect the new parameter; alternatively,
if you want fixed-fee behavior, remove the feeRate mention from the doc and keep
the hardcoded value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32c4cb22-636e-41ab-a00f-24a44ef402ef
📒 Files selected for processing (9)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
💤 Files with no reviewable changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift
packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
Show resolved
Hide resolved
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift (1)
127-135:⚠️ Potential issue | 🟠 MajorDo not silently swallow transaction-build failures in recalculation.
When build/sign fails, Lines 133-134 clear
fee/txbut do not surface any reason, leaving users with a disabled Send button and no actionable feedback.Suggested direction
+ `@State` private var transactionBuildError: String? @@ do { let (tx, fee) = try createTransaction() self.fee = fee self.tx = tx + self.transactionBuildError = nil } catch { self.fee = 0 self.tx = nil + self.transactionBuildError = error.localizedDescription }As per coding guidelines, "Implement proper loading and error states in SwiftUI views".
🤖 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 127 - 135, The recalculation catch block currently swallows errors and clears fee/tx; instead capture and surface the error so the UI can show a proper loading/error state: in SendTransactionView add or use an `@State` transactionError (or errorMessage) and set it inside the catch with the localized error string, keep previous fee/tx or set them to nil intentionally, and trigger an alert/inline error view and disable the Send button based on that error state; ensure the catch for createTransaction() stores error.localizedDescription (or Error) so build/sign failures are visible to the user.packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (1)
411-433:⚠️ Potential issue | 🟠 MajorFree FFI output-address buffers during cleanup (currently left as TODO).
Line 432 leaves address cleanup unimplemented. If
Transaction.Output.toFFI()allocates address C strings, this leaks one buffer per output per transaction build.Proposed fix
defer { if error.message != nil { error_message_free(error.message) } for output in ffiOutputs { - // TODO: Memory leak, FFI doesnt expose a way to free the address + if let address = output.address { + free(UnsafeMutableRawPointer(mutating: address)) + } } if let ptr = txBytesPtr { transaction_bytes_free(ptr) } }#!/bin/bash # Verify allocation/free symmetry for transaction output FFI structs. rg -nP --type swift -C4 'func\s+toFFI\s*\(|strdup\s*\(' packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift rg -nP --type swift -C6 'ffiOutputs|wallet_build_and_sign_transaction|transaction_bytes_free|free\s*\(' packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftAs per coding guidelines, "Swift code must properly wrap FFI functions with correct memory management across Swift/Rust boundaries".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift` around lines 411 - 433, The ffiOutputs produced by outputs.map { $0.toFFI() } allocate C string memory for each output address which is not freed in the defer block; update the cleanup to free those buffers by invoking the appropriate C free on each ffiOutput.address (i.e., free(output.address) or the matching FFI-provided deallocator if one exists) for every element of ffiOutputs inside the defer (alongside the existing error_message_free); ensure you reference Transaction.Output.toFFI(), the ffiOutputs array, and the wallet_build_and_sign_transaction call so the free is colocated with the existing cleanup for txBytesPtr/error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- Around line 411-433: The ffiOutputs produced by outputs.map { $0.toFFI() }
allocate C string memory for each output address which is not freed in the defer
block; update the cleanup to free those buffers by invoking the appropriate C
free on each ffiOutput.address (i.e., free(output.address) or the matching
FFI-provided deallocator if one exists) for every element of ffiOutputs inside
the defer (alongside the existing error_message_free); ensure you reference
Transaction.Output.toFFI(), the ffiOutputs array, and the
wallet_build_and_sign_transaction call so the free is colocated with the
existing cleanup for txBytesPtr/error.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- Around line 127-135: The recalculation catch block currently swallows errors
and clears fee/tx; instead capture and surface the error so the UI can show a
proper loading/error state: in SendTransactionView add or use an `@State`
transactionError (or errorMessage) and set it inside the catch with the
localized error string, keep previous fee/tx or set them to nil intentionally,
and trigger an alert/inline error view and disable the Send button based on that
error state; ensure the catch for createTransaction() stores
error.localizedDescription (or Error) so build/sign failures are visible to the
user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 835777ca-5dee-49c8-9ebe-17e6f8cb57c2
📒 Files selected for processing (9)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
💤 Files with no reviewable changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
ZocoLini
force-pushed
the
feat/send-transaction-swift
branch
from
a5a1e31 to
c8acaef
Compare
thepastaclaw
suggested changes
Mar 18, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This push simplifies the API surface (async→sync, throws→optional) and switches to whole-wallet balance aggregation, but introduces a correctness regression: the send flow validates against the aggregate wallet balance while still building transactions from account 0 only. The preconditionFailure replacing throw at the FFI boundary is also problematic — one corrupt account handle crashes the entire app.
Reviewed commit: c8acaef
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 additional findings
🔴 blocking: preconditionFailure on FFI failure crashes the app with no recovery
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift (lines 117-125)
managed_core_account_get_balance is a C FFI function — failure modes depend on the Rust implementation and handle validity. Replacing throw with preconditionFailure means any FFI failure (dangling handle, Rust-side panic, memory corruption) terminates the process. This function is called in a loop over all account types in getAccounts() — a single bad account handle takes down the entire wallet UI.
The previous code used try? at call sites to gracefully skip failing accounts. Now those same code paths crash instead. Consider returning a zero Balance on failure (or Balance?) to maintain the non-throwing simplification without the crash risk.
💡 Suggested change
public func getBalance() -> Balance {
var ffiBalance = FFIBalance()
let success = managed_core_account_get_balance(handle, &ffiBalance)
guard success else {
// Return zero balance if FFI call fails unexpectedly
return Balance(confirmed: 0, unconfirmed: 0, immature: 0, locked: 0)
}
return Balance(ffiBalance: ffiBalance)
}
🟡 suggestion: getManagedAccountCollection silently discards FFI error information
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (lines 539-561)
Changed from throwing KeyWalletError(ffiError: error) to returning nil. The FFI error message is freed (line 553) without being logged or surfaced. Callers now return empty arrays (getAccounts), nil (getAccountDetails), or zero balances (getBalance) — all indistinguishable from 'wallet has no accounts'. A corrupted wallet, locked database, or Rust-side failure becomes invisible.
Consider at minimum logging the FFI error before returning nil to preserve diagnostic information.
🤖 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/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- [BLOCKING] lines 29-35: Send form validates against whole-wallet balance but builds transaction from account 0 only
`balance` (line 34) now calls `getBalance(for: wallet)` which aggregates confirmed/unconfirmed/immature/locked across **every** managed account (BIP44, BIP32, CoinJoin, Identity, Provider). But `createTransaction()` (line 158) still passes `accIndex: 0` to `buildSignedTransaction`, which only selects UTXOs from a single BIP44 account.
In a wallet with funds split across accounts, the UI shows enough balance and enables the Send button, but the FFI transaction builder fails or builds a transaction the user didn't intend because account 0 may not have sufficient funds.
Either restrict `getBalance()` to the same account used for signing, or update the transaction builder to support whole-wallet coin selection.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift`:
- [BLOCKING] lines 117-125: preconditionFailure on FFI failure crashes the app with no recovery
`managed_core_account_get_balance` is a C FFI function — failure modes depend on the Rust implementation and handle validity. Replacing `throw` with `preconditionFailure` means any FFI failure (dangling handle, Rust-side panic, memory corruption) terminates the process. This function is called in a loop over all account types in `getAccounts()` — a single bad account handle takes down the entire wallet UI.
The previous code used `try?` at call sites to gracefully skip failing accounts. Now those same code paths crash instead. Consider returning a zero `Balance` on failure (or `Balance?`) to maintain the non-throwing simplification without the crash risk.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- [SUGGESTION] lines 539-561: getManagedAccountCollection silently discards FFI error information
Changed from throwing `KeyWalletError(ffiError: error)` to returning `nil`. The FFI error message is freed (line 553) without being logged or surfaced. Callers now return empty arrays (`getAccounts`), `nil` (`getAccountDetails`), or zero balances (`getBalance`) — all indistinguishable from 'wallet has no accounts'. A corrupted wallet, locked database, or Rust-side failure becomes invisible.
Consider at minimum logging the FFI error before returning `nil` to preserve diagnostic information.
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Show resolved
Hide resolved
QuantumExplorer
previously approved these changes
Mar 18, 2026
ZocoLini
force-pushed
the
feat/send-transaction-swift
branch
2 times, most recently
from
a9870fe to
7440763
Compare
thepastaclaw
suggested changes
Mar 18, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The send-transaction feature has a confirmed memory leak at the Swift→FFI boundary that is amplified by per-keystroke transaction rebuilding. The FeeRate enum described in the PR is defined but never wired into the actual transaction-building path. No critical security or correctness issues beyond the memory leak.
Reviewed commit: 7440763
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 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/KeyWallet/WalletManager.swift`:
- [BLOCKING] lines 426-436: strdup'd address strings are never freed — memory leak on every call
`toFFI()` allocates each output address with `strdup` (Transaction.swift:34), but the defer block at lines 430-432 iterates `ffiOutputs` with an empty body — nothing calls `free()`. The TODO comment says "FFI doesnt expose a way to free the address", but since `strdup` allocates on the C/Swift heap, plain `free()` is the correct deallocator. Combined with the per-keystroke rebuild from `SendTransactionView`, this leaks one C string per output on every character typed.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift`:
- [SUGGESTION] lines 4-16: FeeRate enum is defined but never used — fee hardcoded to 1000
The `FeeRate` enum with `.economy`/`.normal`/`.priority` variants is added but never referenced. `buildSignedTransaction` hardcodes `1000` (WalletManager.swift:419) and doesn't accept a `FeeRate` parameter. Either wire the enum into the build path or remove it to avoid dead code.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- [SUGGESTION] lines 99-104: Transaction build+sign on every keystroke with no debounce
`.onChange(of: recipientAddress)` and `.onChange(of: amountString)` call `recalculateTransaction()` on every character. Each invocation crosses into Rust for UTXO selection and cryptographic signing — CPU-intensive work on the main thread. Consider debouncing (e.g., a short `Task.sleep` cancellation pattern) or triggering recalculation only when both fields have plausible values.
In `packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift`:
- [SUGGESTION] lines 210-212: Synchronous broadcast on @MainActor may freeze UI
`WalletService` is `@MainActor` and `broadcastTransaction` is synchronous. The underlying FFI call `dash_spv_ffi_client_broadcast_transaction` may perform blocking I/O. If it does, the UI thread freezes until the broadcast completes. Consider making this `async` and dispatching the FFI call off the main actor.
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift
Outdated
Show resolved
Hide resolved
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Show resolved
Hide resolved
packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
Show resolved
Hide resolved
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
Show resolved
Hide resolved
ZocoLini
force-pushed
the
feat/send-transaction-swift
branch
from
7440763 to
5adc466
Compare
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental push removes the unused FeeRate enum from Transaction.swift (14 lines deleted, 0 added). This directly addresses the prior suggestion about dead code. No new code introduced — remaining prior findings (memory leak in strdup, no debounce on keystroke builds, synchronous broadcast on @MainActor, silent error swallowing) are still valid from the previous review and carry forward.
Reviewed commit: 5adc466
ZocoLini
force-pushed
the
feat/send-transaction-swift
branch
from
5adc466 to
e86f385
Compare
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The delta fixes a sighash mismatch in bundle building, realigns the Swift SPVSyncState enum with the updated dash-spv-ffi dependency (rev 42eb1d6, updated in commit c4e64a7), refactors the wallet list UI, and adds the empty-loop suppression in WalletManager. The codex-ffi BLOCKING claim that raw values were remapped before the FFI producer changed is a false positive — the dependency was updated earlier in the same PR. No blocking issues remain in this delta.
Reviewed commit: e86f385
🟡 1 suggestion(s) | 💬 2 nitpick(s)
2 additional findings
🟡 suggestion: waitingForConnections is no longer treated as an active sync state
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVTypes.swift (lines 28-44)
Both isSyncing() and isRunning() now return false for .waitingForConnections. This was intentional (commit a0efb0c says "match dash-spv implementation"), but it changes behavior for callers: WalletService.clearSpvStorage() gates on isRunning(), so storage can now be cleared while a sync task is alive but waiting for peer connections. Similarly, the UI "Clear" button in CoreContentView becomes enabled during this state. If the upstream Rust semantics treat this state as inactive, this is correct — but worth confirming the SPV client safely handles a storage clear while in waitingForConnections.
💬 nitpick: navigationTitle("Wallets") removed — navigation bar has no title
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift (lines 60-215)
The refactor from toolbar-based "+" button to section-header-based "+" button also removed .navigationTitle("Wallets") and the .toolbar modifier from the List. Since CoreContentView is embedded in a NavigationStack (via CoreWalletView in ContentView.swift:158), the navigation bar will render without a title. The section header text "Wallets (network)" partially replaces the visual function, but the nav bar itself is blank.
🤖 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/Core/SPV/SPVTypes.swift`:
- [SUGGESTION] lines 28-44: waitingForConnections is no longer treated as an active sync state
Both `isSyncing()` and `isRunning()` now return `false` for `.waitingForConnections`. This was intentional (commit a0efb0ca8 says "match dash-spv implementation"), but it changes behavior for callers: `WalletService.clearSpvStorage()` gates on `isRunning()`, so storage can now be cleared while a sync task is alive but waiting for peer connections. Similarly, the UI "Clear" button in CoreContentView becomes enabled during this state. If the upstream Rust semantics treat this state as inactive, this is correct — but worth confirming the SPV client safely handles a storage clear while in `waitingForConnections`.
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.
Need other branch to get merged first and to update the rust-dashcore targeted revision once the open PRS with the needed changes gets merge
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes / Improvements