fix(swift-sdk): fixed ios app transaction display#3081
fix(swift-sdk): fixed ios app transaction display#3081
Conversation
📝 WalkthroughWalkthroughSimplifies the transaction model and API: removes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ 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: "dd9438197021b4f1c87086a77a8c62e89555d089ab064b8fc1d8141a59f91842"
)Xcode manual integration:
|
ZocoLini
force-pushed
the
fix/ios-app-transaction-display
branch
3 times, most recently
from
17ecb5b to
e3889aa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/ManagedAccount.swift (1)
62-69:⚠️ Potential issue | 🟠 Major
assertis stripped in release builds - FFI failure would be silent.
assert(success, ...)only triggers in debug builds. In release builds with optimization (-O), ifmanaged_core_account_get_transactionsreturnsfalse, the code continues silently with potentially invalid state. Consider usingprecondition(which is kept in release builds) or returning an empty array with a logged warning.🔒 Recommended fix using precondition
let success = managed_core_account_get_transactions(handle, &transactionsPtr, &count) - assert(success, "This can only fail if any pointer is nil") + guard success else { + // FFI contract: this can only fail if any pointer is nil, which shouldn't happen + // Return empty rather than crash, but log for debugging + assertionFailure("managed_core_account_get_transactions failed unexpectedly") + return [] + }Or if the failure truly indicates a programming error that should never occur:
- assert(success, "This can only fail if any pointer is nil") + precondition(success, "managed_core_account_get_transactions failed - invalid handle state")🤖 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 62 - 69, Replace the debug-only assert on the FFI call so failures aren’t ignored in release: in ManagedAccount.swift where you call managed_core_account_get_transactions (and currently do assert(success, ...)), either use precondition(success, ...) to ensure the program traps in release builds or handle the false case by logging a warning and returning an empty array; update the code around managed_core_account_get_transactions/transactionsPtr/count to explicitly check success and act accordingly instead of relying on assert.
🧹 Nitpick comments (3)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swift (2)
54-74: Consider extracting shared type logic toWalletTransaction.The
typeIconandtypeColorcomputed properties duplicate logic fromTransactionDetailView. Moving these to aWalletTransactionextension would centralize the logic.♻️ Optional refactor: Add to WalletTransaction
// In WalletTransaction extension or struct public var typeIcon: String { switch netAmount { case let amount where amount > 0: return "arrow.down.circle.fill" case let amount where amount < 0: return "arrow.up.circle.fill" default: return "arrow.triangle.2.circlepath" } } public var typeColor: Color { switch netAmount { case let amount where amount > 0: return .green case let amount where amount < 0: return .red default: return .blue } }🤖 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/TransactionListView.swift` around lines 54 - 74, Move the duplicated computed-property logic for icons and colors out of TransactionListView and TransactionDetailView into WalletTransaction so it's centralized: create public computed properties (e.g., typeIcon and typeColor) on WalletTransaction that use transaction.netAmount to decide the icon and Color, then replace the local computed properties in TransactionListView and any similar logic in TransactionDetailView to use the new WalletTransaction.typeIcon and WalletTransaction.typeColor. Ensure you keep the existing symbol names (typeIcon, typeColor) so callers need only switch to accessing them on the WalletTransaction instance.
44-46: Synchronous FFI call on main thread.
loadTransactions()performs a synchronous FFI call directly on the main thread. For an example app with quick operations this is acceptable, but for production usage consider wrapping in aTask {@mainactorin }with the actual work dispatched to a background thread to prevent UI blocking during larger transaction sets.🤖 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/TransactionListView.swift` around lines 44 - 46, loadTransactions currently calls walletService.walletManager.getTransactions(for: wallet) synchronously on the main thread; change it to perform the FFI call off the main thread and only update self.transactions on the main actor — for example, wrap the operation in Task { } or Task.detached to call getTransactions on a background thread, await the result, then assign to self.transactions inside Task { `@MainActor` in ... } (refer to loadTransactions, walletService, and walletManager.getTransactions) so the heavy FFI work does not block the UI.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swift (1)
64-67: Consider simplifying the conditional for readability.The double negation
!transaction.isConfirmed ? "Pending" : "Confirmed"is correct but slightly harder to parse. Flipping the order improves clarity.♻️ Suggested improvement
TransactionDetailRow( label: "Status", - value: !transaction.isConfirmed ? "Pending" : "Confirmed" + value: transaction.isConfirmed ? "Confirmed" : "Pending" )🤖 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/TransactionDetailView.swift` around lines 64 - 67, The ternary in TransactionDetailRow currently uses a double negation (!transaction.isConfirmed ? "Pending" : "Confirmed"); replace it with a direct check on transaction.isConfirmed (transaction.isConfirmed ? "Confirmed" : "Pending") in the TransactionDetailView so the intent is clearer and easier to read.
🤖 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/ManagedAccount.swift`:
- Around line 55-57: The docstring for the transactions getter in
ManagedAccount.swift mentions a removed parameter `currentHeight`; update the
comment for the method (the "Get all transactions for this account" docblock) to
remove or replace the `- Parameter currentHeight:` line so it matches the
current function signature (e.g., the method named getAllTransactions /
transactions retrieval function in ManagedAccount). Ensure the Returns
description remains accurate and no references to `currentHeight` remain in the
docblock.
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift`:
- Around line 62-69: Replace the debug-only assert on the FFI call so failures
aren’t ignored in release: in ManagedAccount.swift where you call
managed_core_account_get_transactions (and currently do assert(success, ...)),
either use precondition(success, ...) to ensure the program traps in release
builds or handle the false case by logging a warning and returning an empty
array; update the code around
managed_core_account_get_transactions/transactionsPtr/count to explicitly check
success and act accordingly instead of relying on assert.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swift`:
- Around line 64-67: The ternary in TransactionDetailRow currently uses a double
negation (!transaction.isConfirmed ? "Pending" : "Confirmed"); replace it with a
direct check on transaction.isConfirmed (transaction.isConfirmed ? "Confirmed" :
"Pending") in the TransactionDetailView so the intent is clearer and easier to
read.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swift`:
- Around line 54-74: Move the duplicated computed-property logic for icons and
colors out of TransactionListView and TransactionDetailView into
WalletTransaction so it's centralized: create public computed properties (e.g.,
typeIcon and typeColor) on WalletTransaction that use transaction.netAmount to
decide the icon and Color, then replace the local computed properties in
TransactionListView and any similar logic in TransactionDetailView to use the
new WalletTransaction.typeIcon and WalletTransaction.typeColor. Ensure you keep
the existing symbol names (typeIcon, typeColor) so callers need only switch to
accessing them on the WalletTransaction instance.
- Around line 44-46: loadTransactions currently calls
walletService.walletManager.getTransactions(for: wallet) synchronously on the
main thread; change it to perform the FFI call off the main thread and only
update self.transactions on the main actor — for example, wrap the operation in
Task { } or Task.detached to call getTransactions on a background thread, await
the result, then assign to self.transactions inside Task { `@MainActor` in ... }
(refer to loadTransactions, walletService, and walletManager.getTransactions) so
the heavy FFI work does not block the UI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0aa01f8b-3e09-44de-aee1-5fcf7fe132db
📒 Files selected for processing (4)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swift
| /// Get all transactions for this account | ||
| /// - Parameter currentHeight: Current blockchain height for calculating confirmations | ||
| /// - Returns: Array of transactions |
There was a problem hiding this comment.
Outdated docstring references removed parameter.
The docstring still mentions currentHeight parameter which no longer exists in the function signature.
📝 Update docstring
// MARK: - Transactions
/// Get all transactions for this account
- /// - Parameter currentHeight: Current blockchain height for calculating confirmations
/// - Returns: Array of transactions
public func getTransactions() -> [WalletTransaction] {🤖 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 55 - 57, The docstring for the transactions getter in
ManagedAccount.swift mentions a removed parameter `currentHeight`; update the
comment for the method (the "Get all transactions for this account" docblock) to
remove or replace the `- Parameter currentHeight:` line so it matches the
current function signature (e.g., the method named getAllTransactions /
transactions retrieval function in ManagedAccount). Ensure the Returns
description remains accurate and no references to `currentHeight` remain in the
docblock.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR correctly simplifies the transaction model to align with the FFI spec, removing client-side confirmation counting and derived type fields. The only real concern is replacing a guard/throw with an assert that is stripped in release builds, silently converting FFI failures into empty transaction lists. The codex-general agent hallucinated three blocking findings by flagging pre-existing code not changed in this PR.
Reviewed commit: e3889aa
🟡 2 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift`:
- [SUGGESTION] lines 58-64: assert replaces guard/throw — FFI failures silently return empty array in release builds
The old code used `guard success else { throw KeyWalletError.invalidState(...) }` which propagated FFI failures to callers. The new code uses `assert(success, ...)` which is stripped in optimized (release) builds, and the function signature changed from `throws -> [WalletTransaction]` to `-> [WalletTransaction]`, so callers can no longer handle the error at all.
In practice, because `transactionsPtr` is initialized to `nil` and `count` to `0`, the subsequent `guard count > 0, let ptr = transactionsPtr` will catch the failure and return `[]`. So there's no crash risk — but FFI failures are silently swallowed as empty results rather than surfaced as errors.
The developer comment suggests this path is believed to be impossible, but defensive coding would still handle it explicitly.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swift`:
- [SUGGESTION] lines 17-30: Empty-state view was removed — new wallets see a blank list
The previous implementation had an `emptyStateView` with an icon ('doc.text.magnifyingglass'), heading ('No Transactions Yet'), and helper text. This was removed entirely; a new wallet with no transactions now shows an empty List with no user guidance. The PR description says the UI was made 'more compact and elegant', so this may be intentional — but the empty state provides important UX for first-time users.
| public func getTransactions() -> [WalletTransaction] { | ||
| var transactionsPtr: UnsafeMutablePointer<FFITransactionRecord>? | ||
| var count: size_t = 0 | ||
|
|
||
| let success = managed_core_account_get_transactions(handle, &transactionsPtr, &count) | ||
|
|
||
| guard success else { | ||
| throw KeyWalletError.invalidState("Failed to get transactions from managed account") | ||
| } | ||
| assert(success, "This can only fail if any pointer is nil") |
There was a problem hiding this comment.
🟡 Suggestion: assert replaces guard/throw — FFI failures silently return empty array in release builds
The old code used guard success else { throw KeyWalletError.invalidState(...) } which propagated FFI failures to callers. The new code uses assert(success, ...) which is stripped in optimized (release) builds, and the function signature changed from throws -> [WalletTransaction] to -> [WalletTransaction], so callers can no longer handle the error at all.
In practice, because transactionsPtr is initialized to nil and count to 0, the subsequent guard count > 0, let ptr = transactionsPtr will catch the failure and return []. So there's no crash risk — but FFI failures are silently swallowed as empty results rather than surfaced as errors.
The developer comment suggests this path is believed to be impossible, but defensive coding would still handle it explicitly.
| public func getTransactions() -> [WalletTransaction] { | |
| var transactionsPtr: UnsafeMutablePointer<FFITransactionRecord>? | |
| var count: size_t = 0 | |
| let success = managed_core_account_get_transactions(handle, &transactionsPtr, &count) | |
| guard success else { | |
| throw KeyWalletError.invalidState("Failed to get transactions from managed account") | |
| } | |
| assert(success, "This can only fail if any pointer is nil") | |
| public func getTransactions() -> [WalletTransaction] { | |
| var transactionsPtr: UnsafeMutablePointer<FFITransactionRecord>? | |
| var count: size_t = 0 | |
| let success = managed_core_account_get_transactions(handle, &transactionsPtr, &count) | |
| guard success else { | |
| // FFI docs say this can only fail if a pointer arg is nil, which | |
| // cannot happen here. Return empty rather than crash. | |
| return [] | |
| } |
source: ['claude-general', 'claude-ffi-engineer', 'codex-ffi-engineer']
🤖 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/ManagedAccount.swift`:
- [SUGGESTION] lines 58-64: assert replaces guard/throw — FFI failures silently return empty array in release builds
The old code used `guard success else { throw KeyWalletError.invalidState(...) }` which propagated FFI failures to callers. The new code uses `assert(success, ...)` which is stripped in optimized (release) builds, and the function signature changed from `throws -> [WalletTransaction]` to `-> [WalletTransaction]`, so callers can no longer handle the error at all.
In practice, because `transactionsPtr` is initialized to `nil` and `count` to `0`, the subsequent `guard count > 0, let ptr = transactionsPtr` will catch the failure and return `[]`. So there's no crash risk — but FFI failures are silently swallowed as empty results rather than surfaced as errors.
The developer comment suggests this path is believed to be impossible, but defensive coding would still handle it explicitly.
| /// Get all transactions for this account | ||
| /// - Parameter currentHeight: Current blockchain height for calculating confirmations | ||
| /// - Returns: Array of transactions |
There was a problem hiding this comment.
💬 Nitpick: Stale doc comment still references removed currentHeight parameter
The doc comment on line 56 says - Parameter currentHeight: Current blockchain height for calculating confirmations but the currentHeight parameter was removed from the function signature. This should be cleaned up.
| /// Get all transactions for this account | |
| /// - Parameter currentHeight: Current blockchain height for calculating confirmations | |
| /// - Returns: Array of transactions | |
| /// Get all transactions for this account | |
| /// - Returns: Array of transactions |
source: ['claude-general']
| var body: some View { | ||
| ZStack { | ||
| if isLoading { | ||
| ProgressView("Loading transactions...") | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity) | ||
| } else if transactions.isEmpty { | ||
| emptyStateView | ||
| } else { | ||
| transactionsList | ||
| List { | ||
| ForEach(sortedTransactions, id: \.txid) { transaction in | ||
| Button { | ||
| selectedTransaction = transaction | ||
| } label: { | ||
| TransactionRowView(transaction: transaction) | ||
| } | ||
| .buttonStyle(.plain) | ||
| } | ||
| } | ||
| .listStyle(.insetGrouped) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Empty-state view was removed — new wallets see a blank list
The previous implementation had an emptyStateView with an icon ('doc.text.magnifyingglass'), heading ('No Transactions Yet'), and helper text. This was removed entirely; a new wallet with no transactions now shows an empty List with no user guidance. The PR description says the UI was made 'more compact and elegant', so this may be intentional — but the empty state provides important UX for first-time users.
source: ['claude-general']
🤖 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/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swift`:
- [SUGGESTION] lines 17-30: Empty-state view was removed — new wallets see a blank list
The previous implementation had an `emptyStateView` with an icon ('doc.text.magnifyingglass'), heading ('No Transactions Yet'), and helper text. This was removed entirely; a new wallet with no transactions now shows an empty List with no user guidance. The PR description says the UI was made 'more compact and elegant', so this may be intentional — but the empty state provides important UX for first-time users.
| private func loadTransactions() { | ||
| self.transactions = walletService.walletManager.getTransactions(for: wallet) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Inconsistent indentation in loadTransactions()
The body of loadTransactions() uses 2-space indentation while the rest of the file uses 4-space indentation.
| private func loadTransactions() { | |
| self.transactions = walletService.walletManager.getTransactions(for: wallet) | |
| } | |
| private func loadTransactions() { | |
| self.transactions = walletService.walletManager.getTransactions(for: wallet) | |
| } |
source: ['claude-general']
ZocoLini
force-pushed
the
fix/ios-app-transaction-display
branch
from
e3889aa to
9b05176
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swift (1)
36-41: Consider async wrapper for FFI-backed transaction loading.
loadTransactions()synchronously callswalletManager.getTransactions(for:)which performs FFI calls (seeCoreWalletManager.swift:152-158). When invoked from.taskor.refreshable, this runs on the main thread and may cause brief UI jank if the wallet has many transactions.For the example app this is likely acceptable, but for production use you may want to wrap the call in a
Task.detachedor move it off the main thread.♻️ Optional async wrapper
- private func loadTransactions() { - self.transactions = walletService.walletManager.getTransactions(for: wallet) + private func loadTransactions() async { + let txs = await Task.detached { [walletService, wallet] in + walletService.walletManager.getTransactions(for: wallet) + }.value + self.transactions = txs }Note: This requires marking the call sites as
await loadTransactions().Also applies to: 44-46
🤖 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/TransactionListView.swift` around lines 36 - 41, loadTransactions() currently calls walletManager.getTransactions(for:) synchronously (FFI-backed) from UI entry points (.task and .refreshable), which can block the main thread; change the call sites to invoke loadTransactions from a background task (e.g. Task.detached { await loadTransactions() } or Task { await loadTransactions() } depending on desired priority) and make loadTransactions async so it awaits walletManager.getTransactions(for:) off the main thread. Update the two call sites shown (the .task and .refreshable blocks) to use await and ensure loadTransactions(), and any callers, are marked async to propagate the change.
🤖 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/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swift`:
- Around line 36-41: loadTransactions() currently calls
walletManager.getTransactions(for:) synchronously (FFI-backed) from UI entry
points (.task and .refreshable), which can block the main thread; change the
call sites to invoke loadTransactions from a background task (e.g. Task.detached
{ await loadTransactions() } or Task { await loadTransactions() } depending on
desired priority) and make loadTransactions async so it awaits
walletManager.getTransactions(for:) off the main thread. Update the two call
sites shown (the .task and .refreshable blocks) to use await and ensure
loadTransactions(), and any callers, are marked async to propagate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6762219-371f-4e41-be84-c85bbf29441d
📒 Files selected for processing (4)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental push adds only .codecov.yml coverage exclusion patterns for generated gRPC bindings, data contract crates, version tables, and simple-signer. No application logic, FFI, or Swift changes. Nothing to flag.
Reviewed commit: 9b05176
2 participants
The transactions list view was messy and trying to display information without following the FFI specification:
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Refactor
UI