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#3488
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.
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