chore: bump rust-dashcore to commit dda1db7a7367bb7a6a48de7f4ed79da708266460#3436
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Swift SDK transaction context/record handling to FFI-backed types, removes several public transaction-check/process APIs, updates SPV event callbacks to pass NotOwnedTransactionRecord, adjusts asset-lock API to accept per-output funding/identity arrays, updates WalletTransaction fields, updates a Rust workspace dependency rev, and extends a Rust instant-lock parameter. Changes
Sequence Diagram(s)sequenceDiagram
participant C as C FFI Shim
participant Swift as Swift SPVEventHandler
participant Wallet as WalletService / CoreWalletManager
participant FFI as Rust FFI
C->>Swift: onSpvTransactionReceived(recordPtr)
Swift->>Swift: assert recordPtr != nil\nlet record = NotOwnedTransactionRecord(handle: recordPtr)
Swift->>Wallet: handler.onTransactionReceived(accountId, network, record)
Wallet->>FFI: forward record or call buildAssetLockTransaction(...) with buffers
FFI-->>Wallet: result / error
Wallet-->>Swift: completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⏳ Review in progress (commit 4aa999b) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift (1)
487-573: Consolidate account-type metadata to avoid switch drift.The same
FFI_ACCOUNT_TYPE_*families are repeated across multiple computed properties. Consider centralizing into a single traits/presentation mapper so future enum additions can’t desynchronize UI behavior.🤖 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 487 - 573, The repeated switch logic across hasInternalExternalAddresses, shouldShowPrivateKeyButton, accountTypeName, and accountTypeColor can drift—create a single mapper (e.g., an AccountTypeTraits struct or extension with a factory like AccountTypeTraits.for(accountType: FFIAccountType) or init(accountType:)), centralizing properties: hasInternalExternalAddresses, shouldShowPrivateKeyButton, displayName, and color; then have the computed properties in AccountDetailView delegate to detailInfo.flatMap { AccountTypeTraits(for: $0.accountType) } so all account-type metadata lives in one place and future FFI_ACCOUNT_TYPE_* additions only need updating in that mapper.packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swift (1)
18-28: Consider Swift naming conventions for properties.Properties use
snake_case(block_hash,context_type,block_info,islock_data) which doesn't follow Swift naming conventions. Consider usingcamelCasefor consistency.Also applies to: 30-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swift` around lines 18 - 28, Rename the snake_case properties to Swift camelCase for consistency: in class BlockInfo change block_hash -> blockHash (keep height and timestamp unchanged), and apply the same pattern to other structs/classes referenced in this file (e.g., context_type -> contextType, block_info -> blockInfo, islock_data -> islockData or isLockData depending on semantics). Update the initializers (e.g., init(ffi: FFIBlockInfo)) to assign the camelCase properties from the FFI fields (using withUnsafeBytes for block_hash -> blockHash), and update all local uses and any public API consumers in this file to the new property names to avoid breaking references.packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift (1)
5-11: Consider Swift naming conventions for properties.The properties use
snake_case(net_amount,tx_data) which doesn't follow Swift naming conventions. Consider usingcamelCase(netAmount,txData) for consistency with idiomatic Swift code.Also, the properties are implicitly
internalwhile the struct ispublic. If these properties need to be accessed by SDK consumers, they should be markedpublic.Proposed refactor
public struct NotOwnedTransactionRecord { - let txid: Data - let net_amount: Int64 - let context: TransactionContext - let fee: UInt64 - let tx_data: Data - let label: String? + public let txid: Data + public let netAmount: Int64 + public let context: TransactionContext + public let fee: UInt64 + public let txData: Data + public let label: String? public init(handle: UnsafePointer<FFITransactionRecord>) { let p = handle.pointee self.txid = withUnsafeBytes(of: p.txid) { Data($0) } - self.net_amount = p.net_amount + self.netAmount = p.net_amount self.fee = p.fee - self.tx_data = p.tx_data != nil + self.txData = p.tx_data != nil ? Data(bytes: p.tx_data, count: p.tx_len) : Data() self.label = p.label != nil ? String(cString: p.label) : nil self.context = TransactionContext(ffi: p.context) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift` around lines 5 - 11, The struct NotOwnedTransactionRecord uses snake_case property names (net_amount, tx_data) and leaves properties implicitly internal while the type is public; rename these to Swift-style camelCase (netAmount, txData), mark properties public if they must be accessible by SDK users, and add/adjust a CodingKeys enum (or other serialization mapping) to map the old JSON keys ("net_amount", "tx_data") to the new property names; also update any references to NotOwnedTransactionRecord.net_amount/tx_data elsewhere in the codebase to use the new names.
🤖 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/TransactionContext.swift`:
- Around line 1-2: TransactionContext.swift is missing the FFI import required
for the FFI types used; add an import DashSDKFFI at the top of the file so
FFITransactionContextType, FFIBlockInfo, and FFITransactionContext resolve
correctly and the file compiles.
- Around line 35-39: The init(ffi: FFITransactionContext) in TransactionContext
can crash when ffi.islock_data is nil but ffi.islock_len > 0; change
construction of self.islock_data to check ffi.islock_data for nil and only call
Data(bytes:count:) when non-nil and count > 0, otherwise set islock_data to
empty Data() — mirror the nil/count guard used for tx_data in
NotOwnedTransactionRecord to safely handle zero-length or nil pointers.
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift`:
- Around line 1-2: The file TransactionRecord.swift is missing the DashSDKFFI
import required for FFI types; add an import DashSDKFFI at the top of
TransactionRecord.swift so uses of FFITransactionRecord (and any other FFI
types) resolve and the module compiles successfully.
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swift`:
- Around line 18-28: Rename the snake_case properties to Swift camelCase for
consistency: in class BlockInfo change block_hash -> blockHash (keep height and
timestamp unchanged), and apply the same pattern to other structs/classes
referenced in this file (e.g., context_type -> contextType, block_info ->
blockInfo, islock_data -> islockData or isLockData depending on semantics).
Update the initializers (e.g., init(ffi: FFIBlockInfo)) to assign the camelCase
properties from the FFI fields (using withUnsafeBytes for block_hash ->
blockHash), and update all local uses and any public API consumers in this file
to the new property names to avoid breaking references.
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift`:
- Around line 5-11: The struct NotOwnedTransactionRecord uses snake_case
property names (net_amount, tx_data) and leaves properties implicitly internal
while the type is public; rename these to Swift-style camelCase (netAmount,
txData), mark properties public if they must be accessible by SDK users, and
add/adjust a CodingKeys enum (or other serialization mapping) to map the old
JSON keys ("net_amount", "tx_data") to the new property names; also update any
references to NotOwnedTransactionRecord.net_amount/tx_data elsewhere in the
codebase to use the new names.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift`:
- Around line 487-573: The repeated switch logic across
hasInternalExternalAddresses, shouldShowPrivateKeyButton, accountTypeName, and
accountTypeColor can drift—create a single mapper (e.g., an AccountTypeTraits
struct or extension with a factory like AccountTypeTraits.for(accountType:
FFIAccountType) or init(accountType:)), centralizing properties:
hasInternalExternalAddresses, shouldShowPrivateKeyButton, displayName, and
color; then have the computed properties in AccountDetailView delegate to
detailInfo.flatMap { AccountTypeTraits(for: $0.accountType) } so all
account-type metadata lives in one place and future FFI_ACCOUNT_TYPE_* additions
only need updating in that mapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4d9412f-faa4-4124-b22e-abce7dd35777
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlpackages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/README.mdpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (6)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift
- packages/swift-sdk/build_ios.sh
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/README.md
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swift
ZocoLini
force-pushed
the
chore/bump-rust-dashcore
branch
from
853063d to
2b197e1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swift (1)
470-475:⚠️ Potential issue | 🟠 MajorPreserve address payload parity in the new transaction callback.
This API change removes
addresses, butNotOwnedTransactionRecordandTransactionContextin this PR context do not expose an address list (packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift:5-27,packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swift:30-44). That can break downstream transaction handling that depended on per-address data.🤖 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/SPVEventHandler.swift` around lines 470 - 475, The onTransactionReceived callback on SPVWalletEventsHandler was changed to drop per-address information but NotOwnedTransactionRecord and TransactionContext do not expose addresses, which will break downstream handlers; restore parity by either (a) adding an addresses: [String] (or appropriate Address type) property to NotOwnedTransactionRecord and TransactionContext and populating it where transactions are created, or (b) reverting/overloading SPVWalletEventsHandler.onTransactionReceived to include an additional parameter for addresses (e.g., _ addresses: [String]) and wire the originating code that constructs the NotOwnedTransactionRecord/TransactionContext to pass the per-address list so downstream consumers continue to receive per-address data. Ensure all callers and implementors of SPVWalletEventsHandler and the factories that build NotOwnedTransactionRecord/TransactionContext are updated to supply the address list.packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift (1)
172-194:⚠️ Potential issue | 🔴 CriticalCorrect the
timestamptype mismatch at the FFI boundary: Swift expectsUInt32but FFI providesu64.The Swift wrapper incorrectly declares
timestamp: UInt32in bothBlockInfoandWalletTransaction, but the underlying FFI type (defined inrs-platform-wallet-ffi/src/types.rs) providesBlockTime.timestamp: u64. WhenTransactionContext.swiftassignsffi.timestamp(a 64-bit unsigned integer) to aUInt32property, this performs unsafe data truncation, silently discarding the upper 32 bits. This violates proper FFI memory management—Swift must match the actual FFI type or explicitly handle the conversion. Either align the Swift type toUInt64or add explicit truncation logic with documented justification if a 32-bit value is actually intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift` around lines 172 - 194, The Swift structs use timestamp: UInt32 but the FFI provides a u64; update all Swift declarations and uses to UInt64 (e.g., in ManagedAccount.swift for WalletTransaction/BlockInfo, their initializers and the stored property timestamp) and adjust the TransactionContext.swift assignment to accept ffi.timestamp as UInt64 (or explicitly convert with safe truncation if you intentionally want 32-bit, documenting why). Also update the date computed property to convert TimeInterval(TimeInterval(timestamp)) or cast as needed, and search for other references to timestamp to ensure consistent UInt64 usage across FFI boundary and initializers.packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift (2)
5-52:⚠️ Potential issue | 🟠 MajorPublic API removal needs a migration path.
Transaction.check(...)appears to have been removed from a public type. That is source-breaking for SDK consumers; please add a deprecation/unavailable shim and migration note, or explicitly mark/document this as a breaking change in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift` around lines 5 - 52, You removed the public Transaction.check(...) API which is a breaking change; restore a compatibility shim on the Transaction class: add a public static func check(_ ... ) marked deprecated (using `@available`(*, deprecated, message: "... migrate to Transaction.classify(...)")) that forwards to the new implementation or translates parameters/return values to the new classify(...) semantics, and include an unavailable overload if you want to force migration with a clear message; also add a brief migration note to the PR description referencing Transaction.check and the new Transaction.classify symbol so consumers know how to update.
17-23:⚠️ Potential issue | 🟠 MajorMemory leak in
Transaction.Output.toFFI()—strdupallocation never freed.The
addresspointer allocated viastrdupintoFFI()(line 20) is passed intoFFITxOutputand returned without a guaranteed cleanup path. WhenbuildSignedTransactioncallsffiOutputs.map { $0.toFFI() }(WalletManager.swift line 374) and passes the array towallet_build_and_sign_transaction, the strdup-allocated memory remains on the C heap after the Swift array scope ends. The code itself contains a TODO acknowledgment of this at lines 394-395:// TODO: Memory leak, FFI doesnt expose a way to free the address.Either establish ownership and implement deterministic cleanup on the Swift side (e.g., expose and call a transaction output free function), or document that the Rust FFI takes ownership and guarantees cleanup. Per the coding guideline, Swift FFI wrappers must codify and enforce deterministic memory management across the boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift` around lines 17 - 23, Transaction.Output.toFFI() uses strdup(address) but never frees the C string, leaking memory when the FFITxOutput array is passed to wallet_build_and_sign_transaction (via buildSignedTransaction); fix by codifying ownership: either (A) ensure the Rust FFI function wallet_build_and_sign_transaction documents/takes ownership and frees the addr, and reflect that in Swift docs, or (B) add a Rust FFI cleanup function (e.g., wallet_free_tx_output or wallet_free_tx_outputs) that frees FFITxOutput.address and call it from Swift after wallet_build_and_sign_transaction returns (update buildSignedTransaction to map and call the new free function), or (C) change to allocate in a way that the Swift side can free deterministically; implement one of these and update Transaction.Output.toFFI, FFITxOutput usage, and buildSignedTransaction to perform the matching free so strdup memory is released.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swift`:
- Around line 470-475: The onTransactionReceived callback on
SPVWalletEventsHandler was changed to drop per-address information but
NotOwnedTransactionRecord and TransactionContext do not expose addresses, which
will break downstream handlers; restore parity by either (a) adding an
addresses: [String] (or appropriate Address type) property to
NotOwnedTransactionRecord and TransactionContext and populating it where
transactions are created, or (b) reverting/overloading
SPVWalletEventsHandler.onTransactionReceived to include an additional parameter
for addresses (e.g., _ addresses: [String]) and wire the originating code that
constructs the NotOwnedTransactionRecord/TransactionContext to pass the
per-address list so downstream consumers continue to receive per-address data.
Ensure all callers and implementors of SPVWalletEventsHandler and the factories
that build NotOwnedTransactionRecord/TransactionContext are updated to supply
the address list.
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift`:
- Around line 172-194: The Swift structs use timestamp: UInt32 but the FFI
provides a u64; update all Swift declarations and uses to UInt64 (e.g., in
ManagedAccount.swift for WalletTransaction/BlockInfo, their initializers and the
stored property timestamp) and adjust the TransactionContext.swift assignment to
accept ffi.timestamp as UInt64 (or explicitly convert with safe truncation if
you intentionally want 32-bit, documenting why). Also update the date computed
property to convert TimeInterval(TimeInterval(timestamp)) or cast as needed, and
search for other references to timestamp to ensure consistent UInt64 usage
across FFI boundary and initializers.
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift`:
- Around line 5-52: You removed the public Transaction.check(...) API which is a
breaking change; restore a compatibility shim on the Transaction class: add a
public static func check(_ ... ) marked deprecated (using `@available`(*,
deprecated, message: "... migrate to Transaction.classify(...)")) that forwards
to the new implementation or translates parameters/return values to the new
classify(...) semantics, and include an unavailable overload if you want to
force migration with a clear message; also add a brief migration note to the PR
description referencing Transaction.check and the new Transaction.classify
symbol so consumers know how to update.
- Around line 17-23: Transaction.Output.toFFI() uses strdup(address) but never
frees the C string, leaking memory when the FFITxOutput array is passed to
wallet_build_and_sign_transaction (via buildSignedTransaction); fix by codifying
ownership: either (A) ensure the Rust FFI function
wallet_build_and_sign_transaction documents/takes ownership and frees the addr,
and reflect that in Swift docs, or (B) add a Rust FFI cleanup function (e.g.,
wallet_free_tx_output or wallet_free_tx_outputs) that frees FFITxOutput.address
and call it from Swift after wallet_build_and_sign_transaction returns (update
buildSignedTransaction to map and call the new free function), or (C) change to
allocate in a way that the Swift side can free deterministically; implement one
of these and update Transaction.Output.toFFI, FFITxOutput usage, and
buildSignedTransaction to perform the matching free so strdup memory is
released.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c754264d-a891-4f58-823d-3b77b9867f43
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlpackages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/README.mdpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (6)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift
- packages/swift-sdk/build_ios.sh
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/README.md
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swift
✅ Files skipped from review due to trivial changes (2)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
- packages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rs
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift
- Cargo.toml
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swift
ZocoLini
force-pushed
the
chore/bump-rust-dashcore
branch
from
2b197e1 to
6930eca
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift (1)
176-190: Breaking API change:isOursproperty removed fromWalletTransaction.The removal of the
isOurs: Boolproperty and initializer parameter is a breaking change for any consumers constructing or accessing this property. This appears intentional as part of the rust-dashcore API evolution.Consider documenting this breaking change in the PR description or changelog to help downstream consumers migrate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift` around lines 176 - 190, The public API removed the isOurs: Bool property from WalletTransaction (the init in ManagedAccount.swift no longer takes isOurs), which is a breaking change for consumers; to fix, either restore backward compatibility by adding an overload/init that accepts isOurs (or add a deprecated public isOurs property with a default) on the WalletTransaction/ManagedAccount initializer, or if intentional, add a clear changelog/PR note documenting the breaking change and migration steps referencing the WalletTransaction type and the init in ManagedAccount.swift so downstream callers know how to update their code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift`:
- Around line 176-190: The public API removed the isOurs: Bool property from
WalletTransaction (the init in ManagedAccount.swift no longer takes isOurs),
which is a breaking change for consumers; to fix, either restore backward
compatibility by adding an overload/init that accepts isOurs (or add a
deprecated public isOurs property with a default) on the
WalletTransaction/ManagedAccount initializer, or if intentional, add a clear
changelog/PR note documenting the breaking change and migration steps
referencing the WalletTransaction type and the init in ManagedAccount.swift so
downstream callers know how to update their code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83a2c08a-0015-4334-9e6a-c5a965d4a020
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlpackages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/README.mdpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (6)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/README.md
- packages/swift-sdk/build_ios.sh
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift
✅ Files skipped from review due to trivial changes (2)
- Cargo.toml
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swift
- packages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rs
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift
ZocoLini
force-pushed
the
chore/bump-rust-dashcore
branch
from
6930eca to
d32bb58
Compare
|
✅ 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: "ed1aabc2ff8098e53d5d32906f2cad3eefb19e94ce566c346005d1539af03e46"
)Xcode manual integration:
|
There was a problem hiding this comment.
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/SPVEventHandler.swift (1)
566-571:⚠️ Potential issue | 🟡 MinorRemove trailing comma to fix potential build warning.
Line 570 has a trailing comma after the last parameter, which is inconsistent with other method signatures in this file (e.g.,
onBalanceUpdatedat lines 573-579). This may be contributing to the CI build failure mentioned in the PR ("warnings treated as errors").🔧 Suggested fix
private final class DummySPVWalletEventsHandler: SPVWalletEventsHandler { func onTransactionReceived( _: String, _: UInt32, - _: NotOwnedTransactionRecord, + _: NotOwnedTransactionRecord ) {}🤖 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/SPVEventHandler.swift` around lines 566 - 571, The method signature for onTransactionReceived in DummySPVWalletEventsHandler has a trailing comma after the last parameter which causes a warning; edit the DummySPVWalletEventsHandler implementation of onTransactionReceived to remove the trailing comma so the parameter list matches other handlers (e.g., onBalanceUpdated) and rebuild to clear the warnings-as-errors CI failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swift`:
- Around line 566-571: The method signature for onTransactionReceived in
DummySPVWalletEventsHandler has a trailing comma after the last parameter which
causes a warning; edit the DummySPVWalletEventsHandler implementation of
onTransactionReceived to remove the trailing comma so the parameter list matches
other handlers (e.g., onBalanceUpdated) and rebuild to clear the
warnings-as-errors CI failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed2a3c7a-517f-4930-9b85-cbf004acd9a3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlpackages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/README.mdpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (6)
- packages/swift-sdk/build_ios.sh
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/README.md
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift
- packages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rs
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swift
ZocoLini
force-pushed
the
chore/bump-rust-dashcore
branch
from
d32bb58 to
4aa999b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift (1)
26-29:⚠️ Potential issue | 🟡 MinorUpdate docs to remove stale
Transaction.check(...)usage.With the public API now centered on
Transaction.classify(_:), the example inpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/README.md(Line 106–117 in the provided snippet) still callingTransaction.check(...)is now stale and non-compilable for consumers. Please update or replace that snippet with the current flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift` around lines 26 - 29, Update the README example that still calls the removed Transaction.check(...) to use the current public API Transaction.classify(_:) instead: replace the outdated call with a try call to Transaction.classify(transactionData), handle the thrown error with do/try/catch (or propagate the error) and use the returned String to drive routing logic; mention Transaction.classify(_:) and show using the returned value (e.g., let type = try Transaction.classify(txData)) in the example so it compiles with the new SDK.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- Line 191: Change outputIndexOut from an immutable constant to a mutable
variable and pass it by reference into the FFI call so the Rust code can
populate the real output index: replace the declaration "let outputIndexOut:
UInt32 = 0" with a mutable "var outputIndexOut: UInt32 = 0" and include
"&outputIndexOut" in the same FFI invocation where feeOut, txBytesOut, txLenOut,
and privateKeyOut are passed so the returned result uses the populated
outputIndexOut value instead of always 0.
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift`:
- Around line 26-29: Update the README example that still calls the removed
Transaction.check(...) to use the current public API Transaction.classify(_:)
instead: replace the outdated call with a try call to
Transaction.classify(transactionData), handle the thrown error with do/try/catch
(or propagate the error) and use the returned String to drive routing logic;
mention Transaction.classify(_:) and show using the returned value (e.g., let
type = try Transaction.classify(txData)) in the example so it compiles with the
new SDK.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b98cd8c-ddd6-442a-900c-95f08b498550
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlpackages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/README.mdpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swiftpackages/swift-sdk/build_ios.sh
💤 Files with no reviewable changes (5)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/README.md
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedWallet.swift
✅ Files skipped from review due to trivial changes (4)
- Cargo.toml
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR bumps rust-dashcore but bundles a significant Swift SDK refactor (WalletService singleton removal, InstantLockStore, new callback signatures). Verification confirmed 4 blocking bugs: a pre-existing bytePtrIntoData function that silently truncates variable-length data to 32 bytes, a hardcoded outputIndexOut = 0 that is no longer populated by FFI, a build script verification step that breaks non-simulator and Intel targets, and a local/regtest SDK initialization path that fails deterministically when the Docker toggle is off. Six additional suggestions and nitpicks address dead code, crash-prone force-unwraps, and dropped callback data.
Reviewed commit: 4aa999b
🔴 4 blocking | 🟡 4 suggestion(s) | 💬 2 nitpick(s)
9 additional findings
🔴 blocking: bytePtrIntoData ignores its count parameter, always reads 32 bytes
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swift (lines 588-596)
The helper bytePtrIntoData declares its second parameter as unnamed _: Int and hardcodes count: 32. This silently truncates or over-reads any data that isn't exactly 32 bytes. Verified callers affected:
- Line 270: chain lock signatures pass 96 but only 32 bytes are read — signatures are truncated
- Line 285: instant lock data passes
instantlockDataLenbut it's discarded — variable-length IS lock data is truncated or read out-of-bounds
Lines 249, 269, 283 pass 32 and happen to be correct by coincidence. This is pre-existing but the PR touches this file and adds new callback signatures, making it the right time to fix.
💡 Suggested change
private func bytePtrIntoData(_ ptr: UnsafeRawPointer?, _ count: Int) -> Data {
guard let ptr else {
assertionFailure("Byte pointer is nil!")
return Data()
}
return Data(bytes: ptr, count: count)
}
🔴 blocking: Asset lock outputIndex hardcoded to 0, never populated by FFI
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift (lines 191-266)
Line 191 declares let outputIndexOut: UInt32 = 0 (immutable). The old code passed &outputIndexOut to the FFI function, but after the rust-dashcore bump the parameter was removed from wallet_build_and_sign_asset_lock_transaction (lines 217-233 show no &outputIndexOut argument). However, the value is still returned at line 263 via outputIndex: outputIndexOut and consumed downstream by SendViewModel to build outpoints for asset lock proofs. The new multi-output API (arrays of fundingTypes/identityIndices) suggests the upstream FFI now supports multiple outputs, making a hardcoded index of 0 a silent data-correctness bug. Either the FFI should return the index through a new mechanism, or outputIndex should be removed from AssetLockTransactionResult and callers updated.
🔴 blocking: Swift verification step breaks non-simulator targets and Intel Macs
packages/swift-sdk/build_ios.sh (lines 232-261)
Two issues in the same code block: (1) The verification build at lines 241-258 runs unconditionally with -sdk iphonesimulator, even when only --target mac or --target intel-mac was requested. Without a simulator slice in the XCFramework, this verification fails — making the advertised mac-only target modes unusable. (2) Line 235 hardcodes EXCLUDED_ARCHS="x86_64". On Intel Macs where the iOS simulator architecture is x86_64, this excludes the only viable architecture, causing the build to fail even when --target sim is correctly requested.
💡 Suggested change
if $BUILD_SIM && command -v xcodebuild >/dev/null 2>&1; then
SWIFT_PROJECT="$SCRIPT_DIR/SwiftExampleApp/SwiftExampleApp.xcodeproj"
SWIFT_SCHEME="SwiftExampleApp"
SWIFT_DESTINATION="generic/platform=iOS Simulator"
if [[ "$(uname -m)" == "arm64" ]]; then
EXCLUDED_ARCHS="x86_64"
else
EXCLUDED_ARCHS=""
fi
OTHER_SWIFT_FLAGS="-warnings-as-errors"
SWIFT_TREAT_WARNINGS_AS_ERRORS=YES
SWIFT_SUPPRESS_WARNINGS=NO
set +e
xcodebuild -project "$SWIFT_PROJECT" \
-scheme "$SWIFT_SCHEME" \
-sdk iphonesimulator \
-destination "$SWIFT_DESTINATION" \
EXCLUDED_ARCHS="$EXCLUDED_ARCHS" \
OTHER_SWIFT_FLAGS="$OTHER_SWIFT_FLAGS" \
SWIFT_TREAT_WARNINGS_AS_ERRORS=$SWIFT_TREAT_WARNINGS_AS_ERRORS \
SWIFT_SUPPRESS_WARNINGS=$SWIFT_SUPPRESS_WARNINGS \
build
XC_STATUS=$?
set -e
if [[ $XC_STATUS -ne 0 ]]; then
log_error "Swift/Xcode build failed"
exit $XC_STATUS
fi
log_info "Swift/Xcode build succeeded"
else
if ! $BUILD_SIM; then
log_info "Skipping Swift verification (no simulator target built)"
else
echo "xcodebuild not found; skipping Swift verification."
fi
fi
🔴 blocking: Regtest network without Docker toggle guarantees SDK initialization failure
packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift (lines 167-177)
When useDockerSetup is false, SDK.init(network:) calls dash_sdk_create_trusted with dapi_addresses = nil. On the Rust side (rs-sdk-ffi/src/sdk.rs:379-448), the DAPI address resolver only has defaults for Testnet and Mainnet — the _ => catch-all at line 439 returns InvalidParameter for Regtest. The comment at line 164 ("Rust side auto-detects local/regtest") is only true for the trusted context provider (lines 335-376), not for DAPI address resolution. The UI still allows selecting regtest with Docker toggle off, so this path fails deterministically instead of being prevented or auto-configured.
🟡 suggestion: InstantLockStore has dead continuation code and unbounded memory growth
packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift (lines 417-453)
Two issues: (1) The continuations dictionary (line 419) is checked and drained in store() (line 427) but waitForLock() never registers a continuation — it uses polling instead (lines 440-447). The continuations dict and resume logic in store() are dead code. (2) The locks dictionary grows without bound; every IS lock received during the app's lifetime is retained with no eviction, leaking memory proportional to transaction count.
💡 Suggested change
internal final class InstantLockStore: @unchecked Sendable {
private var locks: [Data: Data] = [:]
private let queue = DispatchQueue(label: "com.dash.instantlock-store")
/// Store an IS lock.
func store(txid: Data, lockData: Data) {
queue.sync {
locks[txid] = lockData
}
}
/// Wait for an IS lock for a specific txid.
/// Returns immediately if already cached, otherwise polls until received or timeout.
func waitForLock(txid: Data, timeout: TimeInterval = 30) async throws -> Data {
// Check if already available
if let existing = queue.sync(execute: { locks.removeValue(forKey: txid) }) {
return existing
}
// Poll-based approach — simpler and avoids continuation resume races
let deadline = Date().addingTimeInterval(timeout)
while Date() < deadline {
try await Task.sleep(nanoseconds: 250_000_000) // 250ms
if let existing = queue.sync(execute: { locks.removeValue(forKey: txid) }) {
return existing
}
}
throw SPVError.transactionBroadcastFailed(
"InstantSend lock timeout after \(Int(timeout))s for txid \(txid.hexString)"
)
}
}
🟡 suggestion: Multiple try! force-unwraps in WalletService.init crash on any initialization failure
packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift (lines 125-156)
Lines 125, 135, 148, and 156 all use try!. The TODO comment at line 134 acknowledges uncertainty. In a shipping iOS app, crashing on a storage, SPV, or wallet manager initialization error provides no recovery path. I/O errors, corrupted state directories, or resource exhaustion would crash the app instead of presenting an error state. The class comment at line 87 notes this "should be in the example app" — even so, propagating errors would make the SDK more robust.
🟡 suggestion: WalletService.init creates and destroys a dummy SPVClient to work around initialization ordering
packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift (lines 123-156)
The init creates a temporary SPVClient (line 125) to extract the wallet manager, destroys it (line 138), then creates a second SPVClient with real event handlers (line 148). This means the SPV subsystem is initialized and torn down twice on every WalletService creation. If SPVClient initialization has side effects (file locks, network connections, logging setup), double-init could cause subtle issues. A two-phase init or a closure/builder pattern for handlers would avoid this.
💬 nitpick: New confirmedTxids/confirmedTxidCount callback parameters are silently dropped
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swift (lines 241-252)
The onSpvBlockProcessedCallbackC callback now receives confirmedTxids and confirmedTxidCount from the FFI (lines 245-246) but neither value is propagated to the SPVSyncEventsHandler.onBlocksProcessed protocol method (line 251). These contain the txids confirmed in the block — useful for updating transaction state. While matching the FFI signature is required, dropping this data means the Swift SDK has no way to notify callers which transactions were just confirmed.
💬 nitpick: Scope of WalletService refactor exceeds PR title
packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift (line 84)
The PR title is 'bump rust-dashcore' but WalletService.swift has ~300 lines of non-bump changes: singleton removal, API surface reduction (createWallet, sendTransaction, getNewAddress all removed), new InstantLockStore, new broadcastTransaction/waitForInstantLock methods. These are significant architectural changes that deserve their own review attention and changelog entry rather than being buried in a dependency bump. Consider splitting the refactor into a separate PR or updating the PR title/description to reflect the actual scope.
🤖 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/SPVEventHandler.swift`:
- [BLOCKING] lines 588-596: bytePtrIntoData ignores its count parameter, always reads 32 bytes
The helper `bytePtrIntoData` declares its second parameter as unnamed `_: Int` and hardcodes `count: 32`. This silently truncates or over-reads any data that isn't exactly 32 bytes. Verified callers affected:
- Line 270: chain lock signatures pass 96 but only 32 bytes are read — signatures are truncated
- Line 285: instant lock data passes `instantlockDataLen` but it's discarded — variable-length IS lock data is truncated or read out-of-bounds
Lines 249, 269, 283 pass 32 and happen to be correct by coincidence. This is pre-existing but the PR touches this file and adds new callback signatures, making it the right time to fix.
In `packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- [BLOCKING] lines 191-266: Asset lock outputIndex hardcoded to 0, never populated by FFI
Line 191 declares `let outputIndexOut: UInt32 = 0` (immutable). The old code passed `&outputIndexOut` to the FFI function, but after the rust-dashcore bump the parameter was removed from `wallet_build_and_sign_asset_lock_transaction` (lines 217-233 show no `&outputIndexOut` argument). However, the value is still returned at line 263 via `outputIndex: outputIndexOut` and consumed downstream by `SendViewModel` to build outpoints for asset lock proofs. The new multi-output API (arrays of `fundingTypes`/`identityIndices`) suggests the upstream FFI now supports multiple outputs, making a hardcoded index of 0 a silent data-correctness bug. Either the FFI should return the index through a new mechanism, or `outputIndex` should be removed from `AssetLockTransactionResult` and callers updated.
In `packages/swift-sdk/build_ios.sh`:
- [BLOCKING] lines 232-261: Swift verification step breaks non-simulator targets and Intel Macs
Two issues in the same code block: (1) The verification build at lines 241-258 runs unconditionally with `-sdk iphonesimulator`, even when only `--target mac` or `--target intel-mac` was requested. Without a simulator slice in the XCFramework, this verification fails — making the advertised mac-only target modes unusable. (2) Line 235 hardcodes `EXCLUDED_ARCHS="x86_64"`. On Intel Macs where the iOS simulator architecture is x86_64, this excludes the only viable architecture, causing the build to fail even when `--target sim` is correctly requested.
In `packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift`:
- [BLOCKING] lines 167-177: Regtest network without Docker toggle guarantees SDK initialization failure
When `useDockerSetup` is false, `SDK.init(network:)` calls `dash_sdk_create_trusted` with `dapi_addresses = nil`. On the Rust side (`rs-sdk-ffi/src/sdk.rs:379-448`), the DAPI address resolver only has defaults for Testnet and Mainnet — the `_ =>` catch-all at line 439 returns `InvalidParameter` for Regtest. The comment at line 164 ("Rust side auto-detects local/regtest") is only true for the trusted context provider (lines 335-376), not for DAPI address resolution. The UI still allows selecting regtest with Docker toggle off, so this path fails deterministically instead of being prevented or auto-configured.
In `packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift`:
- [SUGGESTION] lines 417-453: InstantLockStore has dead continuation code and unbounded memory growth
Two issues: (1) The `continuations` dictionary (line 419) is checked and drained in `store()` (line 427) but `waitForLock()` never registers a continuation — it uses polling instead (lines 440-447). The `continuations` dict and resume logic in `store()` are dead code. (2) The `locks` dictionary grows without bound; every IS lock received during the app's lifetime is retained with no eviction, leaking memory proportional to transaction count.
- [SUGGESTION] lines 125-156: Multiple try! force-unwraps in WalletService.init crash on any initialization failure
Lines 125, 135, 148, and 156 all use `try!`. The TODO comment at line 134 acknowledges uncertainty. In a shipping iOS app, crashing on a storage, SPV, or wallet manager initialization error provides no recovery path. I/O errors, corrupted state directories, or resource exhaustion would crash the app instead of presenting an error state. The class comment at line 87 notes this "should be in the example app" — even so, propagating errors would make the SDK more robust.
- [SUGGESTION] lines 123-156: WalletService.init creates and destroys a dummy SPVClient to work around initialization ordering
The init creates a temporary SPVClient (line 125) to extract the wallet manager, destroys it (line 138), then creates a second SPVClient with real event handlers (line 148). This means the SPV subsystem is initialized and torn down twice on every WalletService creation. If SPVClient initialization has side effects (file locks, network connections, logging setup), double-init could cause subtle issues. A two-phase init or a closure/builder pattern for handlers would avoid this.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swift`:
- [SUGGESTION] lines 14-16: Unknown FFI enum value silently falls back to .mempool
If the FFI returns a `TransactionContextType` raw value outside {0,1,2,3}, the `?? .mempool` fallback at line 15 silently assigns `.mempool`. This could mask protocol evolution bugs — if a new context type is added upstream, transactions of that type would appear as mempool transactions without any diagnostic signal.
| init(ffiContext: FFITransactionContextType) { | ||
| self = TransactionContextType(rawValue: ffiContext.rawValue) ?? .mempool | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Unknown FFI enum value silently falls back to .mempool
If the FFI returns a TransactionContextType raw value outside {0,1,2,3}, the ?? .mempool fallback at line 15 silently assigns .mempool. This could mask protocol evolution bugs — if a new context type is added upstream, transactions of that type would appear as mempool transactions without any diagnostic signal.
💡 Suggested change
| init(ffiContext: FFITransactionContextType) { | |
| self = TransactionContextType(rawValue: ffiContext.rawValue) ?? .mempool | |
| } | |
| init(ffiContext: FFITransactionContextType) { | |
| if let known = TransactionContextType(rawValue: ffiContext.rawValue) { | |
| self = known | |
| } else { | |
| assertionFailure("Unknown TransactionContextType raw value: \(ffiContext.rawValue)") | |
| self = .mempool | |
| } | |
| } |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swift`:
- [SUGGESTION] lines 14-16: Unknown FFI enum value silently falls back to .mempool
If the FFI returns a `TransactionContextType` raw value outside {0,1,2,3}, the `?? .mempool` fallback at line 15 silently assigns `.mempool`. This could mask protocol evolution bugs — if a new context type is added upstream, transactions of that type would appear as mempool transactions without any diagnostic signal.
3 participants
Summary by CodeRabbit
API Changes
New Features
Improvements
Chores