fix(swift-sdk): fixed ios app transaction display#3081
fix(swift-sdk): fixed ios app transaction display#3081QuantumExplorer merged 1 commit intov3.1-devfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughWallet transaction model and APIs were simplified (removed Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant Manager as CoreWalletManager
participant Engine as WalletEngine/FFI
participant Store as SwiftData/Storage
UI->>Manager: createWallet(params, isImport, network)
Manager->>Engine: compute/addWalletAndSerialize(..., birthHeight)
Engine-->>Manager: walletHandle
Manager->>Engine: generateSeedIfNeeded(walletHandle)
Engine-->>Manager: seedData
Manager->>Store: insert Wallet entity (walletHandle, birthHeight, seedData)
Store-->>Manager: persist ack
Manager-->>UI: created wallet metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: "4197a89b31fc701b8ff26cd306f22f33875c25cd4df5219cb5bc440ffcc9caf6"
)Xcode manual integration:
|
17ecb5b to
e3889aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/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
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.
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
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
eebbebf to
6c96a03
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift (1)
109-114:⚠️ Potential issue | 🔴 CriticalBug:
importWalletdoesn't passisImport: truetocreateWallet, causing incorrectbirthHeight.The
importWalletmethod callscreateWalletwithout passingisImport: true, sobirthHeightdefaults to0instead of730_000for mainnet imports. This means imported wallets will sync from an incorrect starting point and may miss historical transactions or experience unnecessary sync delays.Per the relevant context snippet (Context snippet 1),
birthHeightis passed directly towallet_manager_add_wallet_from_mnemonic_return_serialized_bytes()and sets the wallet's sync starting point.🐛 Proposed fix
func importWallet(label: String, network: AppNetwork, mnemonic: String, pin: String) async throws -> HDWallet { - let wallet = try await createWallet(label: label, mnemonic: mnemonic, pin: pin) - wallet.isImported = true - try modelContainer.mainContext.save() + let wallet = try await createWallet(label: label, mnemonic: mnemonic, pin: pin, isImport: true) return wallet }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift` around lines 109 - 114, The importWallet function currently calls createWallet without signaling an import, causing birthHeight to remain default; update importWallet to call createWallet(label:network:mnemonic:pin:isImport:) with isImport: true so the created HDWallet is given the correct birthHeight (e.g., 730_000 for mainnet) used when calling wallet_manager_add_wallet_from_mnemonic_return_serialized_bytes, then persist and return the wallet as before.
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift (1)
150-159: Force-unwrap on FFI call may crash if wallet/account is not found.
try!will crash the app ifgetManagedAccountfails (e.g., invalid wallet ID, account not found). Consider usingtry?with a fallback or propagating the error.♻️ Suggested refactor
- public func getTransactions(for wallet: HDWallet, accountIndex: UInt32 = 0) -> [WalletTransaction] { - // Get managed account - let managedAccount = try! sdkWalletManager.getManagedAccount( + public func getTransactions(for wallet: HDWallet, accountIndex: UInt32 = 0) -> [WalletTransaction] { + guard let managedAccount = try? sdkWalletManager.getManagedAccount( walletId: wallet.walletId, accountIndex: accountIndex, accountType: .standardBIP44 - ) - + ) else { + return [] + } return managedAccount.getTransactions() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift` around lines 150 - 159, The getTransactions(for:accountIndex:) function uses a force-try on the FFI call sdkWalletManager.getManagedAccount which can crash if the wallet/account is missing; change this to safely handle errors by using try? (or rethrow the error) and provide a fallback or propagate the error: replace the force-try in getTransactions(for:accountIndex:) when calling sdkWalletManager.getManagedAccount (and subsequently calling managedAccount.getTransactions()) with a safe optional binding or throw from getTransactions so callers can handle the failure, ensuring you reference getTransactions(for:accountIndex:) and getManagedAccount(walletId:accountIndex:accountType:) in the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- Around line 92-94: Fix the comment typo in CoreWalletManager by changing "ans"
to "and" in the inline comment that precedes the calls
modelContainer.mainContext.insert(wallet) and try
modelContainer.mainContext.save(); update the comment to read "// Insert wallet
into context and save it".
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift`:
- Around line 63-66: The call site in ManagedAccount.getTransactions uses
preconditionFailure(success, ...) which always crashes; replace this misuse by
checking success properly: either use precondition(success, "Invalid state:
managed_core_account_get_transactions can only fail if any pointer is nil") so
it only traps when condition is false, or (preferred) use a guard around the FFI
call (managed_core_account_get_transactions) to handle a false success value
gracefully (returning an empty result or throwing) and log or surface the
pointer-nil failure instead of unconditionally crashing; update the
managed_core_account_get_transactions handling in getTransactions to use the
chosen defensive pattern.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swift`:
- Line 52: Update the user-facing string in TransactionListView by changing the
Text view's literal from "Transaction will appear here once you send or receive
Dash" to use the plural "Transactions" so it reads "Transactions will appear
here once you send or receive Dash"; locate the Text(...) in
TransactionListView.swift (within the TransactionListView) and replace the
single-word noun to correct the grammar.
- Around line 41-73: The computed view properties emptyStateView and
transactionList are declared outside the TransactionListView struct; move both
properties inside the TransactionListView body so they become members of the
struct, ensuring they can access instance state like selectedTransaction and
sortedTransactions and use TransactionRowView; verify the closing brace for
TransactionListView is placed after these properties so the file compiles.
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- Around line 109-114: The importWallet function currently calls createWallet
without signaling an import, causing birthHeight to remain default; update
importWallet to call createWallet(label:network:mnemonic:pin:isImport:) with
isImport: true so the created HDWallet is given the correct birthHeight (e.g.,
730_000 for mainnet) used when calling
wallet_manager_add_wallet_from_mnemonic_return_serialized_bytes, then persist
and return the wallet as before.
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- Around line 150-159: The getTransactions(for:accountIndex:) function uses a
force-try on the FFI call sdkWalletManager.getManagedAccount which can crash if
the wallet/account is missing; change this to safely handle errors by using try?
(or rethrow the error) and provide a fallback or propagate the error: replace
the force-try in getTransactions(for:accountIndex:) when calling
sdkWalletManager.getManagedAccount (and subsequently calling
managedAccount.getTransactions()) with a safe optional binding or throw from
getTransactions so callers can handle the failure, ensuring you reference
getTransactions(for:accountIndex:) and
getManagedAccount(walletId:accountIndex:accountType:) in the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e5c5330-fecb-452e-85ac-12455b93a47d
📒 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
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swift
6c96a03 to
83b88ac
Compare
83b88ac to
b673473
Compare
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
Wallet
UI