chore(swift-sdk): add wrappers for the missing TransactionRecord fields in the swift-sdk#3488
chore(swift-sdk): add wrappers for the missing TransactionRecord fields in the swift-sdk#3488ZocoLini wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds new public Swift models for FFI transaction metadata: Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit 0d1b44c) |
|
✅ 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: "caa57cefe58d85138d0c3bd633448166e7a724a95468bc52502e568607f9af40"
)Xcode manual integration:
|
![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 (2)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift (2)
33-48: SamefatalErrorconcern applies here.The
TransactionDirectionandOutputRoleenums (line 70-78) have the same crash risk with unknown FFI values. Consider applying the same fallback pattern to all three enums.🤖 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 33 - 48, The TransactionDirection enum's init(ffi:) currently uses fatalError on unknown FFITransactionDirection values (and OutputRole has the same pattern), which can crash; update both enums (TransactionDirection and OutputRole) to handle unexpected FFI values gracefully by providing a safe fallback case (e.g., .incoming or .unknown) instead of calling fatalError, and log or assert the unexpected value if desired; modify the init(ffi:) initializer implementations for TransactionDirection and OutputRole to map known FFI constants to their cases and return the chosen fallback for any default branch rather than invoking fatalError.
16-30: Consider using a fallback case instead offatalErrorfor unknown FFI values.If the FFI library is updated with new transaction types before the Swift SDK is updated,
fatalErrorwill crash the app. Consider adding anunknowncase or making the initializer failable (init?) to handle this gracefully.♻️ Proposed fix with unknown case
case coinbase case ignored + case unknown init(ffi: FFITransactionType) { switch ffi { case FFI_TRANSACTION_TYPE_STANDARD: self = .standard case FFI_TRANSACTION_TYPE_COIN_JOIN: self = .coinJoin case FFI_TRANSACTION_TYPE_PROVIDER_REGISTRATION: self = .providerRegistration case FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_REGISTRAR: self = .providerUpdateRegister case FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_SERVICE: self = .providerUpdateService case FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_REVOCATION: self = .providerUpdateRevocation case FFI_TRANSACTION_TYPE_ASSET_LOCK: self = .assetLock case FFI_TRANSACTION_TYPE_ASSET_UNLOCK: self = .assetUnlock case FFI_TRANSACTION_TYPE_COINBASE: self = .coinbase case FFI_TRANSACTION_TYPE_IGNORED: self = .ignored - default: fatalError("Unknown FFITransactionType value: \(ffi)") + default: self = .unknown } } }🤖 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 16 - 30, The initializer for mapping FFITransactionType to your Swift enum uses fatalError on default which can crash if the FFI adds new values; change TransactionRecord.init(ffi:) to handle unknown values safely by either adding an explicit .unknown enum case and returning .unknown for the default branch, or make the initializer failable (init?(ffi: FFITransactionType)) and return nil in the default branch; update any callers of init(ffi:) (and the enum definition) to handle the new .unknown case or optional result accordingly so the SDK no longer crashes on unexpected FFITransactionType values.
🤖 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/TransactionRecord.swift`:
- Around line 81-89: OutputDetail's stored properties are internal and must be
exported; change the declarations of index and role to be public (e.g., public
let index: UInt32 and public let role: OutputRole) so SDK consumers can access
them, keeping the existing public init(ffi:) as-is for FFI construction; mirror
the visibility used in InputDetail to ensure consistency.
- Around line 50-62: The InputDetail struct exposes a public initializer but its
stored properties (index, value, address) are internal, preventing SDK consumers
from accessing them; update the property declarations on InputDetail (index,
value, address) to be public so they are visible outside the module, keeping the
existing public init(ffi: FFIInputDetail) and initialization logic unchanged.
- Around line 91-101: The stored properties on the public structs
NotOwnedTransactionRecord, InputDetail, and OutputDetail are currently internal;
change each property declaration from `let` to `public let` so the fields are
publicly visible to SDK consumers—update all properties inside the
NotOwnedTransactionRecord struct (txid, netAmount, context, transactionType,
direction, fee, inputDetails, outputDetails, txData, label) and all properties
inside the InputDetail and OutputDetail structs accordingly to follow the SDK
visibility pattern.
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift`:
- Around line 33-48: The TransactionDirection enum's init(ffi:) currently uses
fatalError on unknown FFITransactionDirection values (and OutputRole has the
same pattern), which can crash; update both enums (TransactionDirection and
OutputRole) to handle unexpected FFI values gracefully by providing a safe
fallback case (e.g., .incoming or .unknown) instead of calling fatalError, and
log or assert the unexpected value if desired; modify the init(ffi:) initializer
implementations for TransactionDirection and OutputRole to map known FFI
constants to their cases and return the chosen fallback for any default branch
rather than invoking fatalError.
- Around line 16-30: The initializer for mapping FFITransactionType to your
Swift enum uses fatalError on default which can crash if the FFI adds new
values; change TransactionRecord.init(ffi:) to handle unknown values safely by
either adding an explicit .unknown enum case and returning .unknown for the
default branch, or make the initializer failable (init?(ffi:
FFITransactionType)) and return nil in the default branch; update any callers of
init(ffi:) (and the enum definition) to handle the new .unknown case or optional
result accordingly so the SDK no longer crashes on unexpected FFITransactionType
values.
🪄 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: 55c6a7c5-9ab4-49df-825b-85b674e6354d
📒 Files selected for processing (1)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR is directionally useful — it adds Swift wrappers for transaction type, direction, and input/output detail metadata — but the current implementation still has two real API/FFI-boundary problems. First, the newly added metadata is not actually readable by external SDK consumers because the stored properties remain internal even though NotOwnedTransactionRecord is handed out through a public callback API. Second, the new enum bridges use fatalError() on unknown FFI values, which turns future ABI evolution or version skew into an app crash instead of a survivable fallback.
Reviewed commit: b6d3abf
🔴 2 blocking
🤖 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/TransactionRecord.swift`:
- [BLOCKING] lines 50-100: The newly added transaction metadata is still inaccessible to external Swift SDK consumers
The new wrapper types are declared `public`, and `NotOwnedTransactionRecord` is delivered through the public `SPVWalletEventsHandler.onTransactionReceived(_:_:_:)` callback, but all of the newly added stored properties in this file still use Swift's default `internal` visibility. That means downstream apps can receive a `NotOwnedTransactionRecord` value but still cannot read `transactionType`, `direction`, `inputDetails`, `outputDetails`, or the fields inside `InputDetail` / `OutputDetail`. Since this file is the entire feature delta, leaving the fields internal makes the newly added metadata effectively unusable outside the module. The new fields should be `public let` if the goal is to expose them through the public callback API.
- [BLOCKING] lines 16-76: Unknown FFI enum values now crash the host app from the callback path
Each new FFI enum bridge (`TransactionType`, `TransactionDirection`, and `OutputRole`) ends in `default: fatalError(...)`. These values come from the external `DashSDKFFI` binary target, so future Rust-side enum growth or any version skew between the Swift wrapper and xcframework will now terminate the host app while handling an SPV wallet callback. The rest of this SDK already uses safer fallback patterns for unknown FFI values — for example `TransactionContextType.init(ffiContext:)` falls back to `.mempool` instead of crashing. These new enum wrappers should follow the same approach by adding an `.unknown` case or using a non-fatal fallback, rather than making callback handling process-fatal on unexpected values.
ZocoLini
force-pushed
the
chore/transaction-record-fields
branch
from
b6d3abf to
0d1b44c
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR usefully exposes the missing FFITransactionRecord fields to Swift and the wrapper still eagerly copies the borrowed FFI data into Swift-owned values, which is the right overall shape. The one real regression is in enum decoding: these new Swift adapters now fatalError on unknown C enum values coming from the XCFramework, so a forward enum extension or any unexpected discriminant will crash transaction decoding instead of degrading safely. The nullable input-address mapping is also weaker than the underlying FFI contract because it erases NULL into an empty string.
Reviewed commit: 0d1b44c
🔴 1 blocking | 🟡 1 suggestion(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/TransactionRecord.swift`:
- [BLOCKING] lines 23-83: Unknown FFI enum values now crash transaction decoding
`TransactionType.init(ffi:)`, `TransactionDirection.init(ffi:)`, and `OutputRole.init(ffi:)` all end in `fatalError` for any discriminant the Swift source does not recognize. That is a bad failure mode on an FFI boundary: `NotOwnedTransactionRecord(handle:)` is used in both `ManagedAccount.getTransactions()` and the SPV event callback, so any enum value added on the Rust/C side before this Swift wrapper is updated will terminate the host app while decoding a transaction record. The rest of this SDK already treats raw FFI enums defensively — for example `TransactionContextType(ffiContext:)` falls back with `rawValue ?? .mempool` and several SPV wrappers map unknown raw values to `.unknown` — so this change introduces a new crash path instead of following the existing boundary contract.
- [SUGGESTION] lines 62-67: `InputDetail.address` erases FFI nullability into an empty string
`FFIInputDetail.address` is an optional C string pointer, but the new wrapper exposes it as a non-optional `String` and substitutes `""` when the pointer is `NULL`. That collapses two different boundary states — 'no address was provided by the FFI record' and 'the address is the empty string' — into the same Swift value. This file already preserves that distinction for sibling fields like `label`, so the safer mapping here is also to keep the address optional on the Swift side instead of silently inventing a value.
| case FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_REVOCATION: self = .providerUpdateRevocation | ||
| case FFI_TRANSACTION_TYPE_ASSET_LOCK: self = .assetLock | ||
| case FFI_TRANSACTION_TYPE_ASSET_UNLOCK: self = .assetUnlock | ||
| case FFI_TRANSACTION_TYPE_COINBASE: self = .coinbase | ||
| case FFI_TRANSACTION_TYPE_IGNORED: self = .ignored | ||
| default: fatalError("Unknown FFITransactionType value: \(ffi)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public enum TransactionDirection { | ||
| case incoming | ||
| case outgoing | ||
| case internalDir | ||
| case coinjoin | ||
|
|
||
| init(ffi: FFITransactionDirection) { | ||
| switch ffi { | ||
| case FFI_TRANSACTION_DIRECTION_INCOMING: self = .incoming | ||
| case FFI_TRANSACTION_DIRECTION_OUTGOING: self = .outgoing | ||
| case FFI_TRANSACTION_DIRECTION_INTERNAL: self = .internalDir | ||
| case FFI_TRANSACTION_DIRECTION_COIN_JOIN: self = .coinjoin | ||
| default: fatalError("Unknown FFITransactionDirection value: \(ffi)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public struct InputDetail { | ||
| public let index: UInt32 | ||
| public let value: UInt64 | ||
| public let address: String | ||
|
|
||
| public init(ffi: FFIInputDetail) { | ||
| self.index = ffi.index | ||
| self.value = ffi.value | ||
| self.address = ffi.address != nil | ||
| ? String(cString: ffi.address) | ||
| : "" | ||
| } | ||
| } | ||
|
|
||
| public enum OutputRole { | ||
| case received | ||
| case change | ||
| case sent | ||
| case unspendable | ||
|
|
||
| init(ffi: FFIOutputRole) { | ||
| switch ffi { | ||
| case FFI_OUTPUT_ROLE_RECEIVED: self = .received | ||
| case FFI_OUTPUT_ROLE_CHANGE: self = .change | ||
| case FFI_OUTPUT_ROLE_SENT: self = .sent | ||
| case FFI_OUTPUT_ROLE_UNSPENDABLE: self = .unspendable | ||
| default: fatalError("Unknown FFIOutputRole value: \(ffi)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public struct OutputDetail { | ||
| public let index: UInt32 | ||
| public let role: OutputRole |
There was a problem hiding this comment.
🔴 Blocking: Unknown FFI enum values now crash transaction decoding
TransactionType.init(ffi:), TransactionDirection.init(ffi:), and OutputRole.init(ffi:) all end in fatalError for any discriminant the Swift source does not recognize. That is a bad failure mode on an FFI boundary: NotOwnedTransactionRecord(handle:) is used in both ManagedAccount.getTransactions() and the SPV event callback, so any enum value added on the Rust/C side before this Swift wrapper is updated will terminate the host app while decoding a transaction record. The rest of this SDK already treats raw FFI enums defensively — for example TransactionContextType(ffiContext:) falls back with rawValue ?? .mempool and several SPV wrappers map unknown raw values to .unknown — so this change introduces a new crash path instead of following the existing boundary contract.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift`:
- [BLOCKING] lines 23-83: Unknown FFI enum values now crash transaction decoding
`TransactionType.init(ffi:)`, `TransactionDirection.init(ffi:)`, and `OutputRole.init(ffi:)` all end in `fatalError` for any discriminant the Swift source does not recognize. That is a bad failure mode on an FFI boundary: `NotOwnedTransactionRecord(handle:)` is used in both `ManagedAccount.getTransactions()` and the SPV event callback, so any enum value added on the Rust/C side before this Swift wrapper is updated will terminate the host app while decoding a transaction record. The rest of this SDK already treats raw FFI enums defensively — for example `TransactionContextType(ffiContext:)` falls back with `rawValue ?? .mempool` and several SPV wrappers map unknown raw values to `.unknown` — so this change introduces a new crash path instead of following the existing boundary contract.
| } | ||
|
|
||
| public enum OutputRole { | ||
| case received | ||
| case change | ||
| case sent |
There was a problem hiding this comment.
🟡 Suggestion: InputDetail.address erases FFI nullability into an empty string
FFIInputDetail.address is an optional C string pointer, but the new wrapper exposes it as a non-optional String and substitutes "" when the pointer is NULL. That collapses two different boundary states — 'no address was provided by the FFI record' and 'the address is the empty string' — into the same Swift value. This file already preserves that distinction for sibling fields like label, so the safer mapping here is also to keep the address optional on the Swift side instead of silently inventing a value.
💡 Suggested change
| } | |
| public enum OutputRole { | |
| case received | |
| case change | |
| case sent | |
| public struct InputDetail { | |
| public let index: UInt32 | |
| public let value: UInt64 | |
| public let address: String? | |
| public init(ffi: FFIInputDetail) { | |
| self.index = ffi.index | |
| self.value = ffi.value | |
| self.address = ffi.address.map { String(cString: $0) } | |
| } | |
| } |
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift`:
- [SUGGESTION] lines 62-67: `InputDetail.address` erases FFI nullability into an empty string
`FFIInputDetail.address` is an optional C string pointer, but the new wrapper exposes it as a non-optional `String` and substitutes `""` when the pointer is `NULL`. That collapses two different boundary states — 'no address was provided by the FFI record' and 'the address is the empty string' — into the same Swift value. This file already preserves that distinction for sibling fields like `label`, so the safer mapping here is also to keep the address optional on the Swift side instead of silently inventing a value.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two convergent FFI-boundary issues in TransactionRecord.swift, both flagged independently by Claude and Codex and verified against the worktree at SHA 0d1b44c. The three new enum bridges (TransactionType/TransactionDirection/OutputRole) terminate the process via fatalError on unknown FFI discriminants while being decoded inside an SPV C-callback frame, and InputDetail.address collapses a nullable C string into an empty Swift string, contradicting the optional-preserving pattern already used for label in the same struct.
Reviewed commit: 0d1b44c
Fresh rerun of the review pipeline for this dispatcher item. An earlier same-SHA automated review already exists, so I am posting this fresh run as a top-level review to avoid duplicating inline threads while still completing the current queue item.
🔴 1 blocking | 🟡 1 suggestion(s)
🔴 blocking: Unknown FFI enum discriminants hard-crash transaction decoding via fatalError
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift:16-78
TransactionType.init(ffi:) (line 28), TransactionDirection.init(ffi:) (line 45), and OutputRole.init(ffi:) (line 76) each end their switch with fatalError("Unknown FFI...") for any discriminant not enumerated in the current Swift source. These enum values originate in the externally-built DashSDKFFI xcframework (Rust side) and are decoded inside NotOwnedTransactionRecord(handle:), which runs both from ManagedAccount.getTransactions() and — more critically — from the SPV on_transaction_received C callback (SPVEventHandler.swift). Two consequences: (1) any future variant added to FFITransactionType/FFITransactionDirection/FFIOutputRole on the Rust side, or any version skew between the Swift wrappers and the shipped binary, will hard-abort the host app while parsing an inbound transaction; (2) the trap fires from inside a Rust-driven extern "C" frame, where Swift unwinding semantics are undefined behavior in addition to terminating the process. The same package already demonstrates the boundary-safe convention at TransactionContext.swift:15, where TransactionContextType.init(ffiContext:) falls back via rawValue ?? .mempool instead of trapping. Mirror that pattern here — add an .unknown case (or make these initializers failable / Optional-returning) so a forward-compatible C enum value degrades gracefully rather than killing the process from an FFI callback.
🟡 suggestion: InputDetail.address erases FFI nullability into an empty string
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift:50-62
FFIInputDetail.address is treated as a nullable C string (the initializer guards ffi.address != nil on line 58), but InputDetail.address is exposed as a non-optional String and "" is substituted when the pointer is NULL. This collapses two distinct boundary states — "FFI did not provide an address" and "the address is the empty string" — into the same Swift value, forcing downstream consumers to treat empty-string as a NULL sentinel. The sibling label field on NotOwnedTransactionRecord at lines 101 and 112–114 already preserves the Optional distinction across the same FFI boundary in the same file. Mirror that pattern: expose address as String? and use ffi.address.map { String(cString: $0) }.
🤖 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/TransactionRecord.swift`:
- [BLOCKING] lines 16-78: Unknown FFI enum discriminants hard-crash transaction decoding via fatalError
TransactionType.init(ffi:) (line 28), TransactionDirection.init(ffi:) (line 45), and OutputRole.init(ffi:) (line 76) each end their switch with `fatalError("Unknown FFI...")` for any discriminant not enumerated in the current Swift source. These enum values originate in the externally-built DashSDKFFI xcframework (Rust side) and are decoded inside NotOwnedTransactionRecord(handle:), which runs both from ManagedAccount.getTransactions() and — more critically — from the SPV `on_transaction_received` C callback (SPVEventHandler.swift). Two consequences: (1) any future variant added to FFITransactionType/FFITransactionDirection/FFIOutputRole on the Rust side, or any version skew between the Swift wrappers and the shipped binary, will hard-abort the host app while parsing an inbound transaction; (2) the trap fires from inside a Rust-driven `extern "C"` frame, where Swift unwinding semantics are undefined behavior in addition to terminating the process. The same package already demonstrates the boundary-safe convention at TransactionContext.swift:15, where TransactionContextType.init(ffiContext:) falls back via `rawValue ?? .mempool` instead of trapping. Mirror that pattern here — add an `.unknown` case (or make these initializers failable / Optional-returning) so a forward-compatible C enum value degrades gracefully rather than killing the process from an FFI callback.
- [SUGGESTION] lines 50-62: InputDetail.address erases FFI nullability into an empty string
FFIInputDetail.address is treated as a nullable C string (the initializer guards `ffi.address != nil` on line 58), but InputDetail.address is exposed as a non-optional `String` and `""` is substituted when the pointer is NULL. This collapses two distinct boundary states — "FFI did not provide an address" and "the address is the empty string" — into the same Swift value, forcing downstream consumers to treat empty-string as a NULL sentinel. The sibling `label` field on NotOwnedTransactionRecord at lines 101 and 112–114 already preserves the Optional distinction across the same FFI boundary in the same file. Mirror that pattern: expose `address` as `String?` and use `ffi.address.map { String(cString: $0) }`.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior visibility blocker on NotOwnedTransactionRecord/InputDetail/OutputDetail is resolved (now public let). Two blocking issues remain: (1) the three new FFI enum bridges still call fatalError on unknown values, diverging from the SDK's established graceful-fallback pattern and turning ABI/version skew into a host-app crash on the SPV callback path; and (2) the newly added TransactionContext and BlockInfo are public class but every stored property defaults to internal, so consumers receive record.context but cannot read any of its fields. Two minor suggestions (nil-vs-empty address modeling, missing tests) round out the review.
Reviewed commit: 0d1b44c
🔴 2 blocking | 🟡 2 suggestion(s)
1 additional finding
🔴 blocking: New TransactionContext and BlockInfo wrappers expose no readable fields to SDK consumers
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swift (lines 19-44)
NotOwnedTransactionRecord.context is now public let, but the wrapper types it points at hide every payload field. In TransactionContext.swift:
BlockInfo(line 19) ispublic class, yetheight,block_hash, andtimestamp(lines 20–22) have no access modifier and therefore default tointernal.TransactionContext(line 31) ispublic class, yetcontext_type,block_info, andislock_data(lines 32–34) likewise default tointernal.
Downstream apps receiving record.context through the public SPVWalletEventsHandler.onTransactionReceived callback can hold the object but cannot inspect whether the transaction is mempool vs in-block, read block metadata, or access the IS lock bytes — defeating the purpose of exposing the field. Because this file is new in this PR, it's part of the feature delta and the stored properties should be declared public let (and the initializers reconsidered for public if external construction is intended).
🤖 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/TransactionRecord.swift`:
- [BLOCKING] lines 16-78: Unknown FFI enum values crash the host app from the SPV transaction callback
All three new FFI-to-Swift enum bridges terminate the process via `fatalError(...)` on any unrecognized discriminant:
- `TransactionType.init(ffi:)` line 28
- `TransactionDirection.init(ffi:)` line 45
- `OutputRole.init(ffi:)` line 76
These values originate in the externally-built `DashSDKFFI.xcframework` (a separately-compiled Rust target). They are consumed inside `NotOwnedTransactionRecord.init(handle:)` (line 117–118 and via `OutputDetail.init(ffi:)` at line 87), which is invoked synchronously from `onSpvTransactionReceivedCallbackC` at `SPVEventHandler.swift:532` — the public `SPVWalletEventsHandler.onTransactionReceived` callback path. Any future Rust-side enum growth, version skew between this Swift wrapper and the bundled xcframework, or any unexpected sentinel value will hard-crash the embedding application while delivering a wallet event.
This diverges from the SDK's established convention for FFI enum bridges. `TransactionContextType.init(ffiContext:)` in `TransactionContext.swift:14-16` uses `TransactionContextType(rawValue: ffiContext.rawValue) ?? .mempool` — a survivable fallback. The new bridges should follow the same pattern: add an `.unknown` case (or pick a safe default per enum) and return that on the `default:` arm so wallet callbacks degrade gracefully instead of taking down the host app.
- [SUGGESTION] lines 50-61: Nil FFI input address silently becomes empty string
`InputDetail.init(ffi:)` collapses a missing FFI address pointer to `""`:
self.address = ffi.address != nil
? String(cString: ffi.address)
: ""
Downstream consumers cannot distinguish "FFI returned no address" (e.g. unparseable script, P2PK input, multisig redemption) from "address is the empty string". `TransactionRecord.label` already models the absent case correctly with `String?`, and `OutputDetail` sidesteps the issue by not exposing an address at all. Modeling `InputDetail.address` as `String?` and returning `nil` on a null pointer preserves the distinction for analyzers and UI without any FFI change.
- [SUGGESTION] lines 1-132: No tests cover the new FFI-to-Swift bridges
This file adds three enum bridges with hand-written FFI value mappings, two new structs with FFI-pointer initializers, and an `init(handle:)` that walks unsafe C arrays via `p.input_details[i]` / `p.output_details[i]`. The only Swift test file in `packages/swift-sdk/Tests/` is `WalletSerializationTests.swift`; no test exercises this code. The enum mapping is positional and easy to silently mis-align against the C header (note `FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_REGISTRAR` ↔ `.providerUpdateRegister`), and the `*_count` fields drive raw pointer indexing. A small unit test that builds an `FFITransactionRecord` in memory and asserts the resulting wrapper would catch both classes of regression. The PR checklist claims tests were added/updated; none accompany this change.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionContext.swift`:
- [BLOCKING] lines 19-44: New TransactionContext and BlockInfo wrappers expose no readable fields to SDK consumers
`NotOwnedTransactionRecord.context` is now `public let`, but the wrapper types it points at hide every payload field. In `TransactionContext.swift`:
- `BlockInfo` (line 19) is `public class`, yet `height`, `block_hash`, and `timestamp` (lines 20–22) have no access modifier and therefore default to `internal`.
- `TransactionContext` (line 31) is `public class`, yet `context_type`, `block_info`, and `islock_data` (lines 32–34) likewise default to `internal`.
Downstream apps receiving `record.context` through the public `SPVWalletEventsHandler.onTransactionReceived` callback can hold the object but cannot inspect whether the transaction is mempool vs in-block, read block metadata, or access the IS lock bytes — defeating the purpose of exposing the field. Because this file is new in this PR, it's part of the feature delta and the stored properties should be declared `public let` (and the initializers reconsidered for `public` if external construction is intended).
| init(ffi: FFITransactionType) { | ||
| switch ffi { | ||
| case FFI_TRANSACTION_TYPE_STANDARD: self = .standard | ||
| case FFI_TRANSACTION_TYPE_COIN_JOIN: self = .coinJoin | ||
| case FFI_TRANSACTION_TYPE_PROVIDER_REGISTRATION: self = .providerRegistration | ||
| case FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_REGISTRAR: self = .providerUpdateRegister | ||
| case FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_SERVICE: self = .providerUpdateService | ||
| case FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_REVOCATION: self = .providerUpdateRevocation | ||
| case FFI_TRANSACTION_TYPE_ASSET_LOCK: self = .assetLock | ||
| case FFI_TRANSACTION_TYPE_ASSET_UNLOCK: self = .assetUnlock | ||
| case FFI_TRANSACTION_TYPE_COINBASE: self = .coinbase | ||
| case FFI_TRANSACTION_TYPE_IGNORED: self = .ignored | ||
| default: fatalError("Unknown FFITransactionType value: \(ffi)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public enum TransactionDirection { | ||
| case incoming | ||
| case outgoing | ||
| case internalDir | ||
| case coinjoin | ||
|
|
||
| init(ffi: FFITransactionDirection) { | ||
| switch ffi { | ||
| case FFI_TRANSACTION_DIRECTION_INCOMING: self = .incoming | ||
| case FFI_TRANSACTION_DIRECTION_OUTGOING: self = .outgoing | ||
| case FFI_TRANSACTION_DIRECTION_INTERNAL: self = .internalDir | ||
| case FFI_TRANSACTION_DIRECTION_COIN_JOIN: self = .coinjoin | ||
| default: fatalError("Unknown FFITransactionDirection value: \(ffi)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public struct InputDetail { | ||
| public let index: UInt32 | ||
| public let value: UInt64 | ||
| public let address: String | ||
|
|
||
| public init(ffi: FFIInputDetail) { | ||
| self.index = ffi.index | ||
| self.value = ffi.value | ||
| self.address = ffi.address != nil | ||
| ? String(cString: ffi.address) | ||
| : "" | ||
| } | ||
| } | ||
|
|
||
| public enum OutputRole { | ||
| case received | ||
| case change | ||
| case sent | ||
| case unspendable | ||
|
|
||
| init(ffi: FFIOutputRole) { | ||
| switch ffi { | ||
| case FFI_OUTPUT_ROLE_RECEIVED: self = .received | ||
| case FFI_OUTPUT_ROLE_CHANGE: self = .change | ||
| case FFI_OUTPUT_ROLE_SENT: self = .sent | ||
| case FFI_OUTPUT_ROLE_UNSPENDABLE: self = .unspendable | ||
| default: fatalError("Unknown FFIOutputRole value: \(ffi)") | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Unknown FFI enum values crash the host app from the SPV transaction callback
All three new FFI-to-Swift enum bridges terminate the process via fatalError(...) on any unrecognized discriminant:
TransactionType.init(ffi:)line 28TransactionDirection.init(ffi:)line 45OutputRole.init(ffi:)line 76
These values originate in the externally-built DashSDKFFI.xcframework (a separately-compiled Rust target). They are consumed inside NotOwnedTransactionRecord.init(handle:) (line 117–118 and via OutputDetail.init(ffi:) at line 87), which is invoked synchronously from onSpvTransactionReceivedCallbackC at SPVEventHandler.swift:532 — the public SPVWalletEventsHandler.onTransactionReceived callback path. Any future Rust-side enum growth, version skew between this Swift wrapper and the bundled xcframework, or any unexpected sentinel value will hard-crash the embedding application while delivering a wallet event.
This diverges from the SDK's established convention for FFI enum bridges. TransactionContextType.init(ffiContext:) in TransactionContext.swift:14-16 uses TransactionContextType(rawValue: ffiContext.rawValue) ?? .mempool — a survivable fallback. The new bridges should follow the same pattern: add an .unknown case (or pick a safe default per enum) and return that on the default: arm so wallet callbacks degrade gracefully instead of taking down the host app.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TransactionRecord.swift`:
- [BLOCKING] lines 16-78: Unknown FFI enum values crash the host app from the SPV transaction callback
All three new FFI-to-Swift enum bridges terminate the process via `fatalError(...)` on any unrecognized discriminant:
- `TransactionType.init(ffi:)` line 28
- `TransactionDirection.init(ffi:)` line 45
- `OutputRole.init(ffi:)` line 76
These values originate in the externally-built `DashSDKFFI.xcframework` (a separately-compiled Rust target). They are consumed inside `NotOwnedTransactionRecord.init(handle:)` (line 117–118 and via `OutputDetail.init(ffi:)` at line 87), which is invoked synchronously from `onSpvTransactionReceivedCallbackC` at `SPVEventHandler.swift:532` — the public `SPVWalletEventsHandler.onTransactionReceived` callback path. Any future Rust-side enum growth, version skew between this Swift wrapper and the bundled xcframework, or any unexpected sentinel value will hard-crash the embedding application while delivering a wallet event.
This diverges from the SDK's established convention for FFI enum bridges. `TransactionContextType.init(ffiContext:)` in `TransactionContext.swift:14-16` uses `TransactionContextType(rawValue: ffiContext.rawValue) ?? .mempool` — a survivable fallback. The new bridges should follow the same pattern: add an `.unknown` case (or pick a safe default per enum) and return that on the `default:` arm so wallet callbacks degrade gracefully instead of taking down the host app.
| public struct InputDetail { | ||
| public let index: UInt32 | ||
| public let value: UInt64 | ||
| public let address: String | ||
|
|
||
| public init(ffi: FFIInputDetail) { | ||
| self.index = ffi.index | ||
| self.value = ffi.value | ||
| self.address = ffi.address != nil | ||
| ? String(cString: ffi.address) | ||
| : "" | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Nil FFI input address silently becomes empty string
InputDetail.init(ffi:) collapses a missing FFI address pointer to "":
self.address = ffi.address != nil
? String(cString: ffi.address)
: ""
Downstream consumers cannot distinguish "FFI returned no address" (e.g. unparseable script, P2PK input, multisig redemption) from "address is the empty string". TransactionRecord.label already models the absent case correctly with String?, and OutputDetail sidesteps the issue by not exposing an address at all. Modeling InputDetail.address as String? and returning nil on a null pointer preserves the distinction for analyzers and UI without any FFI change.
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/TransactionRecord.swift`:
- [SUGGESTION] lines 50-61: Nil FFI input address silently becomes empty string
`InputDetail.init(ffi:)` collapses a missing FFI address pointer to `""`:
self.address = ffi.address != nil
? String(cString: ffi.address)
: ""
Downstream consumers cannot distinguish "FFI returned no address" (e.g. unparseable script, P2PK input, multisig redemption) from "address is the empty string". `TransactionRecord.label` already models the absent case correctly with `String?`, and `OutputDetail` sidesteps the issue by not exposing an address at all. Modeling `InputDetail.address` as `String?` and returning `nil` on a null pointer preserves the distinction for analyzers and UI without any FFI change.
| import Foundation | ||
| import DashSDKFFI | ||
|
|
||
| // This struct is not mapping all fields of FFITransactionRecord | ||
| // for the lack of wrappers | ||
| public enum TransactionType { | ||
| case standard | ||
| case coinJoin | ||
| case providerRegistration | ||
| case providerUpdateRegister | ||
| case providerUpdateService | ||
| case providerUpdateRevocation | ||
| case assetLock | ||
| case assetUnlock | ||
| case coinbase | ||
| case ignored | ||
|
|
||
| init(ffi: FFITransactionType) { | ||
| switch ffi { | ||
| case FFI_TRANSACTION_TYPE_STANDARD: self = .standard | ||
| case FFI_TRANSACTION_TYPE_COIN_JOIN: self = .coinJoin | ||
| case FFI_TRANSACTION_TYPE_PROVIDER_REGISTRATION: self = .providerRegistration | ||
| case FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_REGISTRAR: self = .providerUpdateRegister | ||
| case FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_SERVICE: self = .providerUpdateService | ||
| case FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_REVOCATION: self = .providerUpdateRevocation | ||
| case FFI_TRANSACTION_TYPE_ASSET_LOCK: self = .assetLock | ||
| case FFI_TRANSACTION_TYPE_ASSET_UNLOCK: self = .assetUnlock | ||
| case FFI_TRANSACTION_TYPE_COINBASE: self = .coinbase | ||
| case FFI_TRANSACTION_TYPE_IGNORED: self = .ignored | ||
| default: fatalError("Unknown FFITransactionType value: \(ffi)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public enum TransactionDirection { | ||
| case incoming | ||
| case outgoing | ||
| case internalDir | ||
| case coinjoin | ||
|
|
||
| init(ffi: FFITransactionDirection) { | ||
| switch ffi { | ||
| case FFI_TRANSACTION_DIRECTION_INCOMING: self = .incoming | ||
| case FFI_TRANSACTION_DIRECTION_OUTGOING: self = .outgoing | ||
| case FFI_TRANSACTION_DIRECTION_INTERNAL: self = .internalDir | ||
| case FFI_TRANSACTION_DIRECTION_COIN_JOIN: self = .coinjoin | ||
| default: fatalError("Unknown FFITransactionDirection value: \(ffi)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public struct InputDetail { | ||
| public let index: UInt32 | ||
| public let value: UInt64 | ||
| public let address: String | ||
|
|
||
| public init(ffi: FFIInputDetail) { | ||
| self.index = ffi.index | ||
| self.value = ffi.value | ||
| self.address = ffi.address != nil | ||
| ? String(cString: ffi.address) | ||
| : "" | ||
| } | ||
| } | ||
|
|
||
| public enum OutputRole { | ||
| case received | ||
| case change | ||
| case sent | ||
| case unspendable | ||
|
|
||
| init(ffi: FFIOutputRole) { | ||
| switch ffi { | ||
| case FFI_OUTPUT_ROLE_RECEIVED: self = .received | ||
| case FFI_OUTPUT_ROLE_CHANGE: self = .change | ||
| case FFI_OUTPUT_ROLE_SENT: self = .sent | ||
| case FFI_OUTPUT_ROLE_UNSPENDABLE: self = .unspendable | ||
| default: fatalError("Unknown FFIOutputRole value: \(ffi)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public struct OutputDetail { | ||
| public let index: UInt32 | ||
| public let role: OutputRole | ||
|
|
||
| public init(ffi: FFIOutputDetail) { | ||
| self.index = ffi.index | ||
| self.role = OutputRole(ffi: ffi.role) | ||
| } | ||
| } | ||
|
|
||
| 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 transactionType: TransactionType | ||
| public let direction: TransactionDirection | ||
| public let fee: UInt64 | ||
| public let inputDetails: [InputDetail] | ||
| public let outputDetails: [OutputDetail] | ||
| 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) | ||
| self.transactionType = TransactionType(ffi: p.transaction_type) | ||
| self.direction = TransactionDirection(ffi: p.direction) | ||
|
|
||
| self.inputDetails = p.input_details != nil | ||
| ? (0..<p.input_details_count).map { i in | ||
| InputDetail(ffi: p.input_details[i]) | ||
| } | ||
| : [] | ||
|
|
||
| self.outputDetails = p.output_details != nil | ||
| ? (0..<p.output_details_count).map { i in | ||
| OutputDetail(ffi: p.output_details[i]) | ||
| } | ||
| : [] | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No tests cover the new FFI-to-Swift bridges
This file adds three enum bridges with hand-written FFI value mappings, two new structs with FFI-pointer initializers, and an init(handle:) that walks unsafe C arrays via p.input_details[i] / p.output_details[i]. The only Swift test file in packages/swift-sdk/Tests/ is WalletSerializationTests.swift; no test exercises this code. The enum mapping is positional and easy to silently mis-align against the C header (note FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_REGISTRAR ↔ .providerUpdateRegister), and the *_count fields drive raw pointer indexing. A small unit test that builds an FFITransactionRecord in memory and asserts the resulting wrapper would catch both classes of regression. The PR checklist claims tests were added/updated; none accompany this change.
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/TransactionRecord.swift`:
- [SUGGESTION] lines 1-132: No tests cover the new FFI-to-Swift bridges
This file adds three enum bridges with hand-written FFI value mappings, two new structs with FFI-pointer initializers, and an `init(handle:)` that walks unsafe C arrays via `p.input_details[i]` / `p.output_details[i]`. The only Swift test file in `packages/swift-sdk/Tests/` is `WalletSerializationTests.swift`; no test exercises this code. The enum mapping is positional and easy to silently mis-align against the C header (note `FFI_TRANSACTION_TYPE_PROVIDER_UPDATE_REGISTRAR` ↔ `.providerUpdateRegister`), and the `*_count` fields drive raw pointer indexing. A small unit test that builds an `FFITransactionRecord` in memory and asserts the resulting wrapper would catch both classes of regression. The PR checklist claims tests were added/updated; none accompany this change.
2 participants
I just added the missing fields in the TransactionRecord struct
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Breaking Changes