chore(swift-sdk): add wrappers for the missing TransactionRecord fields in the swift-sdk#3488
Open
chore(swift-sdk): add wrappers for the missing TransactionRecord fields in the swift-sdk#3488
Conversation
Contributor
|
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 |
Collaborator
|
⏳ Review in progress (commit 0d1b44c) |
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "caa57cefe58d85138d0c3bd633448166e7a724a95468bc52502e568607f9af40"
)Xcode manual integration:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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
suggested changes
Apr 14, 2026
Collaborator
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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