fix(swift-sdk): fixed wallet balance calculation#3082
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAccount balance moved from a tuple to a concrete Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "SwiftUI View\n(AccountList/Detail)"
participant Core as "CoreWalletManager"
participant WM as "WalletManager"
participant MA as "ManagedAccount"
rect rgba(200,230,250,0.5)
UI->>Core: getAccounts(for: wallet) (synchronous)
Core->>WM: getManagedAccountCollection(walletId) (optional)
alt collection present
WM->>MA: getBalance() (synchronous)
MA-->>WM: Balance
WM-->>Core: [AccountInfo with Balance]
Core-->>UI: [AccountInfo...] (synchronous)
else collection missing
WM-->>Core: nil
Core-->>UI: [] (synchronous)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
ZocoLini
changed the title
ZocoLini
force-pushed
the
chore/swift-sdk-improvements-fixes
branch
3 times, most recently
from
f267961 to
251de42
Compare
![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: 2
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/WalletManager.swift (1)
480-486:⚠️ Potential issue | 🔴 CriticalAdd wallet ID length validation before FFI call.
The method passes
walletIddirectly tomanaged_wallet_get_account_collectionwithout validating the 32-byte requirement that all other wallet ID-based FFI methods in this class enforce. This creates an unsafe FFI boundary.Proposed fix
public func getManagedAccountCollection(walletId: Data) -> ManagedAccountCollection? { + guard walletId.count == 32 else { + return nil + } var error = FFIError() let collectionHandle = walletId.withUnsafeBytes { idBytes in let idPtr = idBytes.bindMemory(to: UInt8.self).baseAddress return managed_wallet_get_account_collection(handle, idPtr, &error) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift` around lines 480 - 486, The getManagedAccountCollection method lacks a 32-byte wallet ID length check before calling the FFI function managed_wallet_get_account_collection; add a guard that validates walletId.count == 32 (or returns nil/sets FFIError appropriately) at the start of getManagedAccountCollection in WalletManager to avoid passing improperly sized buffers into the FFI boundary, and only proceed to the withUnsafeBytes call when the length is correct.
🧹 Nitpick comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift (1)
15-20: Reintroduce explicit loading/error states in this view.This path now jumps directly to empty/list rendering, so transient fetch failures and first-load progress are not represented.
As per coding guidelines,
packages/swift-sdk/SwiftExampleApp/**/*View.swift: Implement proper loading and error states in SwiftUI views.Also applies to: 32-39
🤖 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/AccountListView.swift` around lines 15 - 20, AccountListView currently jumps straight to empty/list rendering based only on accounts.isEmpty; add explicit loading and error UI states by introducing a loading flag and an optional error state (e.g., `@State` var isLoading: Bool and `@State` var loadError: Error?) and use them to drive the view: show a ProgressView while isLoading, show a ContentUnavailableView with the error message and a Retry button when loadError != nil (calling the existing fetchAccounts() or the view's data-loading method), and only then fall back to the accounts.isEmpty / list rendering; apply the same pattern for the other block referenced (lines ~32-39) to ensure first-load progress and transient failures are represented.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift (1)
28-35: Keep a loading state for account-detail fetch.The current flow renders error/content only; there is no explicit loading phase during detail retrieval.
As per coding guidelines,
packages/swift-sdk/SwiftExampleApp/**/*View.swift: Implement proper loading and error states in SwiftUI views.Also applies to: 60-62, 621-625
🤖 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/AccountDetailView.swift` around lines 28 - 35, The view currently only shows error or content based on errorMessage and detailInfo; add an explicit loading state in AccountDetailView by introducing a Bool (e.g., isLoading) or a Result/enum state and render a ProgressView (or a dedicated LoadingView) when loading is true or when both errorMessage and detailInfo are nil and a fetch is in progress; update the fetch/account-detail logic (where detailInfo is populated) to set isLoading = true before the network call and set it false on success or failure so the UI transitions through Loading -> Content or Loading -> Error consistently.
🤖 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 196-201: Replace the unsafe guard-assert pattern in
getAccountDetails(for:accountInfo:) and getAccounts by deterministically
handling a nil collection from
sdkWalletManager.getManagedAccountCollection(walletId:), i.e., if the collection
is nil return a sensible default instead of asserting: in getAccountDetails
return a default AccountDetailInfo value (matching the struct's zero/empty
state) and in getAccounts return an empty [AccountDetailInfo] array; update both
functions to early-return these defaults when collection is nil so subsequent
calls like collection.getBIP44Account() and collection.getBIP44Indices() never
run on nil.
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift`:
- Around line 138-143: The getBalance() function uses assert(success, ...) after
calling managed_core_account_get_balance(handle, &ffiBalance), which is disabled
in Release builds; replace this with a runtime check that fails in production
(e.g., guard success else { preconditionFailure("...") }) so a missing/invalid
FFI result cannot silently proceed; update the check in getBalance() to mirror
the robust validation style used in getTransactions() but using
preconditionFailure for mandatory FFI validation and include a clear message
referencing managed_core_account_get_balance and the FFIBalance/Balance
conversion.
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- Around line 480-486: The getManagedAccountCollection method lacks a 32-byte
wallet ID length check before calling the FFI function
managed_wallet_get_account_collection; add a guard that validates walletId.count
== 32 (or returns nil/sets FFIError appropriately) at the start of
getManagedAccountCollection in WalletManager to avoid passing improperly sized
buffers into the FFI boundary, and only proceed to the withUnsafeBytes call when
the length is correct.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift`:
- Around line 28-35: The view currently only shows error or content based on
errorMessage and detailInfo; add an explicit loading state in AccountDetailView
by introducing a Bool (e.g., isLoading) or a Result/enum state and render a
ProgressView (or a dedicated LoadingView) when loading is true or when both
errorMessage and detailInfo are nil and a fetch is in progress; update the
fetch/account-detail logic (where detailInfo is populated) to set isLoading =
true before the network call and set it false on success or failure so the UI
transitions through Loading -> Content or Loading -> Error consistently.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift`:
- Around line 15-20: AccountListView currently jumps straight to empty/list
rendering based only on accounts.isEmpty; add explicit loading and error UI
states by introducing a loading flag and an optional error state (e.g., `@State`
var isLoading: Bool and `@State` var loadError: Error?) and use them to drive the
view: show a ProgressView while isLoading, show a ContentUnavailableView with
the error message and a Retry button when loadError != nil (calling the existing
fetchAccounts() or the view's data-loading method), and only then fall back to
the accounts.isEmpty / list rendering; apply the same pattern for the other
block referenced (lines ~32-39) to ensure first-load progress and transient
failures are represented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a90865fd-5260-403c-8ca9-09b83a469ee6
📒 Files selected for processing (8)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Models/HDWalletModels.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/HDWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
ZocoLini
force-pushed
the
chore/swift-sdk-improvements-fixes
branch
from
251de42 to
0449ad4
Compare
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: "5a6e810b82fa0eba8ee37cbd983fea676d625ad695667595c074ea6f3da0d958"
)Xcode manual integration:
|
ZocoLini
force-pushed
the
chore/swift-sdk-improvements-fixes
branch
from
0449ad4 to
e234771
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift (1)
138-145:⚠️ Potential issue | 🔴 CriticalUse
preconditionFailureorguardinstead ofassertfor FFI validation.This was flagged in a previous review:
assert()is disabled in Release builds (-Ooptimization), so ifmanaged_core_account_get_balancefails in production, the function silently continues with a potentially invalidffiBalance.Compare with
getTransactions()(lines 62-66) which correctly usesguard success else { throw ... }.Proposed fix
public func getBalance() -> Balance { var ffiBalance = FFIBalance() let success = managed_core_account_get_balance(handle, &ffiBalance) - - assert(success, "This can only fail if handle or ffiBalance are null") - + guard success else { + preconditionFailure("managed_core_account_get_balance failed: handle or ffiBalance is null") + } return Balance(ffiBalance: ffiBalance) }🤖 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 138 - 145, The assert in ManagedAccount.getBalance should be replaced with a runtime-safe check so failures in Release builds don’t proceed with an invalid FFIBalance; update the call to managed_core_account_get_balance to use either a guard that throws a descriptive error (matching the pattern used in getTransactions) or use preconditionFailure with a clear message on failure—e.g., check the Bool success from managed_core_account_get_balance and on false either throw a managed-core failure error or call preconditionFailure("managed_core_account_get_balance failed: handle or ffiBalance is null") before returning Balance(ffiBalance: ffiBalance).packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift (1)
196-201:⚠️ Potential issue | 🔴 CriticalReplace
guard-assertpattern with deterministic failure handling.This was flagged in a previous review:
assert(false, ...)is disabled in Release builds. IfgetManagedAccountCollectionreturnsnil, the guard block does not terminate execution, and subsequent code accessescollection(e.g.,collection.getBIP44Account()), causing undefined behavior or crashes.Both occurrences at lines 199-201 and 357 need deterministic exits.
Proposed fix for both locations
public func getAccountDetails(for wallet: HDWallet, accountInfo: AccountInfo) -> AccountDetailInfo { let collection = sdkWalletManager.getManagedAccountCollection(walletId: wallet.walletId) - guard let collection else { - assert(false, "The walletId is always valid") - } + guard let collection else { + // Return a minimal AccountDetailInfo when collection unavailable + return AccountDetailInfo( + account: accountInfo, + accountType: FFIAccountType(rawValue: 0), + xpub: nil, + derivationPath: "", + gapLimit: 20, + usedAddresses: 0, + unusedAddresses: 0, + externalAddresses: [], + internalAddresses: [] + ) + }public func getAccounts(for wallet: HDWallet) -> [AccountInfo] { let collection = sdkWalletManager.getManagedAccountCollection(walletId: wallet.walletId) - guard let collection else { assert(false, "The walletId is always valid") } + guard let collection else { + return [] // Return empty list when collection unavailable + } var list: [AccountInfo] = []Also applies to: 354-358
🤖 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 196 - 201, The guard-assert pattern in getAccountDetails(for:accountInfo:) (and the other occurrence around collection.getBIP44Account()) must be replaced with deterministic failure handling because assert is disabled in Release; change the guard to either throw a meaningful error or call fatalError with a clear message when sdkWalletManager.getManagedAccountCollection(walletId:) returns nil so execution cannot continue with a nil collection, and apply the same change at the other occurrence that accesses collection.getBIP44Account(); ensure the replacement provides a descriptive message referencing the walletId/context so failures are diagnosable.
🧹 Nitpick comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift (1)
16-16: Remove unusedisLoadingstate variable.The
isLoadingstate variable at line 16 is no longer used sinceloadAccountDetails()is now synchronous and the loading UI was removed. This is dead code.Remove unused state
`@State` private var detailInfo: AccountDetailInfo? -@State private var isLoading = true `@State` private var errorMessage: String?Also applies to: 620-626
🤖 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/AccountDetailView.swift` at line 16, Remove the dead state variable by deleting the `@State` private var isLoading declaration and any related unused references to isLoading in AccountDetailView (it was left over after loadAccountDetails() became synchronous and loading UI was removed); search for and remove any other unused isLoading occurrences in the same file (e.g., the other instance referenced in the range around lines 620-626) so the SwiftUI view no longer contains an unused state property.packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (1)
480-498: Removed walletId validation creates inconsistency with other methods.Other methods in this class (e.g.,
getWallet,getReceiveAddress,getChangeAddress,getManagedAccount) validate thatwalletId.count == 32before calling FFI. This method now skips that validation, allowing invalid wallet IDs to reach the FFI layer.Additionally, the FFI error information is discarded when returning
nil— callers cannot distinguish between "invalid walletId" and "collection unavailable for valid wallet."Consider either:
- Restoring the 32-byte validation for consistency, or
- Documenting that callers must validate walletId before calling this method.
Option 1: Restore validation for consistency
public func getManagedAccountCollection(walletId: Data) -> ManagedAccountCollection? { + guard walletId.count == 32 else { + return nil + } var error = FFIError()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift` around lines 480 - 498, The method getManagedAccountCollection removed the 32-byte walletId validation and can pass invalid IDs into managed_wallet_get_account_collection, causing inconsistent behavior and masking whether nil is due to invalid input; restore the same validation used by getWallet/getReceiveAddress/getChangeAddress/getManagedAccount by adding a guard walletId.count == 32 at the start of getManagedAccountCollection and return nil immediately when the length is wrong so the FFI is not called (leave the existing FFI error handling/defer as-is for the valid-path case).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- Around line 196-201: The guard-assert pattern in
getAccountDetails(for:accountInfo:) (and the other occurrence around
collection.getBIP44Account()) must be replaced with deterministic failure
handling because assert is disabled in Release; change the guard to either throw
a meaningful error or call fatalError with a clear message when
sdkWalletManager.getManagedAccountCollection(walletId:) returns nil so execution
cannot continue with a nil collection, and apply the same change at the other
occurrence that accesses collection.getBIP44Account(); ensure the replacement
provides a descriptive message referencing the walletId/context so failures are
diagnosable.
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift`:
- Around line 138-145: The assert in ManagedAccount.getBalance should be
replaced with a runtime-safe check so failures in Release builds don’t proceed
with an invalid FFIBalance; update the call to managed_core_account_get_balance
to use either a guard that throws a descriptive error (matching the pattern used
in getTransactions) or use preconditionFailure with a clear message on
failure—e.g., check the Bool success from managed_core_account_get_balance and
on false either throw a managed-core failure error or call
preconditionFailure("managed_core_account_get_balance failed: handle or
ffiBalance is null") before returning Balance(ffiBalance: ffiBalance).
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- Around line 480-498: The method getManagedAccountCollection removed the
32-byte walletId validation and can pass invalid IDs into
managed_wallet_get_account_collection, causing inconsistent behavior and masking
whether nil is due to invalid input; restore the same validation used by
getWallet/getReceiveAddress/getChangeAddress/getManagedAccount by adding a guard
walletId.count == 32 at the start of getManagedAccountCollection and return nil
immediately when the length is wrong so the FFI is not called (leave the
existing FFI error handling/defer as-is for the valid-path case).
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift`:
- Line 16: Remove the dead state variable by deleting the `@State` private var
isLoading declaration and any related unused references to isLoading in
AccountDetailView (it was left over after loadAccountDetails() became
synchronous and loading UI was removed); search for and remove any other unused
isLoading occurrences in the same file (e.g., the other instance referenced in
the range around lines 620-626) so the SwiftUI view no longer contains an unused
state property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 832c430e-3c39-4504-b5d8-0e1f4f802ede
📒 Files selected for processing (7)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Models/HDWalletModels.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
thepastaclaw
suggested changes
Mar 17, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR correctly changes wallet balance to sum all accounts instead of using only the main account. However, it introduces two critical bugs: guard let x else { assert(false, ...) } is invalid Swift in release builds (guard-else requires a Never-returning exit), and assert(success) in ManagedAccount.getBalance() is stripped in release builds, silently returning zeroed balances on FFI failure. Both patterns replace previously correct error handling with constructs that fail under -O optimization.
Reviewed commit: e234771
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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/Core/Wallet/CoreWalletManager.swift`:
- [BLOCKING] lines 196-201: `guard let ... else { assert(false) }` is invalid in release builds
Swift `guard` requires the else-clause to unconditionally exit scope (return, throw, break, continue, or a `Never`-returning call). `assert(false, ...)` returns `Void`, not `Never`. Under `-Onone` (debug), the runtime trap may mask this, but under `-O` (release) `assert` is stripped entirely, leaving a guard-else body that falls through — a compile error. This pattern appears at two call sites: `getAccountDetails` (line 199) and `getAccounts` (line 357).
- [SUGGESTION] lines 164-186: Wallet-level `Balance.total` formula differs from FFI per-account total
The wallet-level balance is constructed via `Balance(confirmed:unconfirmed:immature:locked:)` which computes `total = confirmed + unconfirmed + immature` (excludes locked). Per-account balances come from `Balance(ffiBalance:)` which sets `total = ffiBalance.total` (FFI-defined formula). If the FFI includes locked in its total, or uses a different formula, the wallet-level total will be inconsistent with the sum of per-account totals. Verify the FFI total formula matches, or use the same computation path.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift`:
- [BLOCKING] lines 138-144: `assert(success)` stripped in release builds — silent zero-balance on FFI failure
The previous code used `guard success else { throw KeyWalletError.invalidState(...) }` which properly surfaced FFI failures. The replacement `assert(success, ...)` is a no-op under `-O`. If the FFI call fails in a release build, `ffiBalance` stays default-initialized (all zeros) and `Balance(ffiBalance: ffiBalance)` returns a zero balance that flows into the wallet-level aggregation — silent data corruption. Use `precondition` (not stripped in release) or restore the throwing guard.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- [SUGGESTION] lines 480-498: Removed walletId length validation and FFI error propagation
The previous implementation validated `walletId.count == 32` before passing the raw pointer to the Rust FFI, and threw `KeyWalletError(ffiError: error)` on failure. Both are removed: the length check is gone (shorter Data passed via `withUnsafeBytes` could cause a buffer over-read on the Rust side if it assumes 32 bytes), and the FFI error message is freed in `defer` without being read or logged. Consider restoring the length precondition and at minimum logging the error before freeing it.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- [SUGGESTION] lines 386-401: loadAccountCounts uses single count for all network slots
The method calls `getAccounts(for: wallet).count` once and assigns the same value to whichever network is enabled. Since only one network is active at a time, this is functionally correct (the others get nil), but the code structure with three separate if-branches applying the same count is misleading. A simpler approach would set the count for the active network directly.
e234771 to
0e617f8
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift (1)
28-35:⚠️ Potential issue | 🟠 MajorHandle
nildetail responses explicitly to avoid blank screen.When
getAccountDetails(...)returnsnil, neithererrorMessagenordetailInfois set, so this view renders no content instead of a loading/failure state.Suggested fix
ScrollView { - if let error = errorMessage { + if isLoading { + ProgressView("Loading account details...") + .frame(maxWidth: .infinity, minHeight: 240) + } else if let error = errorMessage { ContentUnavailableView( "Failed to Load Details", systemImage: "exclamationmark.triangle", description: Text(error) ) @@ private func loadAccountDetails() { - self.detailInfo = walletService.walletManager.getAccountDetails( + isLoading = true + let details = walletService.walletManager.getAccountDetails( for: wallet, accountInfo: account ) + self.detailInfo = details + self.errorMessage = (details == nil) ? "Account details are unavailable." : nil + isLoading = false }As per coding guidelines, "Implement proper loading and error states in SwiftUI views".
Also applies to: 621-625
🤖 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/AccountDetailView.swift` around lines 28 - 35, AccountDetailView currently only shows ContentUnavailableView when errorMessage is set or detailInfo when present, so a nil response from getAccountDetails leaves the view blank; update the view body to add a final else branch (after the existing if let error = errorMessage / else if let info = detailInfo) that explicitly renders a loading or fallback state (e.g., ProgressView or a ContentUnavailableView with "No details available" and a retry hint) and ensure the retry action calls getAccountDetails or triggers the same loader logic used elsewhere; reference AccountDetailView, getAccountDetails(...), errorMessage and detailInfo when making this change.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift (1)
15-39:⚠️ Potential issue | 🟠 MajorReintroduce explicit loading/error states; empty-state-only flow masks failures.
This branch now maps both “still loading” and “account fetch failed” to
"No Accounts", so users can’t distinguish an actual empty wallet from a retrieval problem.Suggested fix
struct AccountListView: View { @@ `@State` private var accounts: [AccountInfo] = [] + `@State` private var isLoading = true + `@State` private var errorMessage: String? @@ - if accounts.isEmpty { + if isLoading { + ProgressView("Loading accounts...") + } else if let errorMessage { + ContentUnavailableView( + "Failed to Load Accounts", + systemImage: "exclamationmark.triangle", + description: Text(errorMessage) + ) + } else if accounts.isEmpty { ContentUnavailableView( "No Accounts", systemImage: "folder", @@ private func loadAccounts() { - self.accounts = walletService.walletManager.getAccounts(for: wallet) + isLoading = true + let loaded = walletService.walletManager.getAccounts(for: wallet) + self.accounts = loaded + // If manager cannot surface errors yet, at least keep a separate non-loading state. + self.errorMessage = nil + isLoading = false } }As per coding guidelines, "Implement proper loading and error states in SwiftUI views".
🤖 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/AccountListView.swift` around lines 15 - 39, The view currently treats loading and fetch errors the same as an empty account list; add explicit loading and error state handling in AccountListView by introducing `@State` vars (e.g., isLoading: Bool and loadError: Error?) alongside accounts, update loadAccounts to set isLoading = true, clear loadError, perform the fetch via walletService.walletManager.getAccounts(for:), catching and assigning any error to loadError and finally setting isLoading = false, and update the body to show a ProgressView when isLoading, an error view (or ContentUnavailableView with the error message and retry action) when loadError != nil, and only show the "No Accounts" ContentUnavailableView when accounts.isEmpty and loadError == nil and !isLoading; ensure NavigationLink/List remain unchanged for the success path.
♻️ Duplicate comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift (1)
196-233:⚠️ Potential issue | 🔴 CriticalFix invalid optional chaining and nil-path fallback in account details lookup.
Line 197 uses invalid Swift syntax (
...)?), and the function currently falls through to a syntheticAccountDetailInfowhen collection/account lookup fails instead of returningnil.#!/bin/bash # Verify the invalid optional-chaining token and inspect the surrounding control flow. rg -n 'getManagedAccountCollection\(walletId:.*\)\?' packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift sed -n '190,260p' packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftSuggested fix
public func getAccountDetails(for wallet: HDWallet, accountInfo: AccountInfo) -> AccountDetailInfo? { - let collection = sdkWalletManager.getManagedAccountCollection(walletId: wallet.walletId)? + guard let collection = sdkWalletManager.getManagedAccountCollection(walletId: wallet.walletId) else { + return nil + } // Resolve managed account from category and optional index var managed: ManagedAccount? @@ - var ffiType = FFIAccountType(rawValue: 0) - if let m = managed { - ffiType = FFIAccountType(rawValue: m.accountType?.rawValue ?? 0) + guard let m = managed else { + return nil + } + + var ffiType = FFIAccountType(rawValue: m.accountType?.rawValue ?? 0) + do { // Query all generated addresses (0 to 0 means "all addresses" in FFI) - if let pool = m.getExternalAddressPool(), let infos = try? pool.getAddresses(from: 0, to: 0) { + if let pool = m.getExternalAddressPool(), let infos = try? pool.getAddresses(from: 0, to: 0) { externalDetails = infos.map { info in AddressDetail(address: info.address, index: info.index, path: info.path, isUsed: info.used, publicKey: info.publicKey?.map { String(format: "%02x", $0) }.joined() ?? "") } } @@ - } + }🤖 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 196 - 233, The getAccountDetails function uses invalid optional-chaining syntax on sdkWalletManager.getManagedAccountCollection(...) and currently falls through to build a synthetic AccountDetailInfo when the collection or managed account is missing; change the line to safely unwrap the collection with optional binding (e.g., guard let collection = sdkWalletManager.getManagedAccountCollection(walletId: wallet.walletId) else { return nil }) and similarly guard or return nil if the resolved ManagedAccount (from getBIP44Account/getBIP32Account/getIdentityRegistrationAccount/etc.) is nil before continuing; ensure ffiType, externalDetails/internalDetails population and derivationPath logic only run when managed is present so the function returns nil for missing collection/account instead of fabricating an AccountDetailInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift`:
- Around line 28-35: AccountDetailView currently only shows
ContentUnavailableView when errorMessage is set or detailInfo when present, so a
nil response from getAccountDetails leaves the view blank; update the view body
to add a final else branch (after the existing if let error = errorMessage /
else if let info = detailInfo) that explicitly renders a loading or fallback
state (e.g., ProgressView or a ContentUnavailableView with "No details
available" and a retry hint) and ensure the retry action calls getAccountDetails
or triggers the same loader logic used elsewhere; reference AccountDetailView,
getAccountDetails(...), errorMessage and detailInfo when making this change.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift`:
- Around line 15-39: The view currently treats loading and fetch errors the same
as an empty account list; add explicit loading and error state handling in
AccountListView by introducing `@State` vars (e.g., isLoading: Bool and loadError:
Error?) alongside accounts, update loadAccounts to set isLoading = true, clear
loadError, perform the fetch via walletService.walletManager.getAccounts(for:),
catching and assigning any error to loadError and finally setting isLoading =
false, and update the body to show a ProgressView when isLoading, an error view
(or ContentUnavailableView with the error message and retry action) when
loadError != nil, and only show the "No Accounts" ContentUnavailableView when
accounts.isEmpty and loadError == nil and !isLoading; ensure NavigationLink/List
remain unchanged for the success path.
---
Duplicate comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- Around line 196-233: The getAccountDetails function uses invalid
optional-chaining syntax on sdkWalletManager.getManagedAccountCollection(...)
and currently falls through to build a synthetic AccountDetailInfo when the
collection or managed account is missing; change the line to safely unwrap the
collection with optional binding (e.g., guard let collection =
sdkWalletManager.getManagedAccountCollection(walletId: wallet.walletId) else {
return nil }) and similarly guard or return nil if the resolved ManagedAccount
(from getBIP44Account/getBIP32Account/getIdentityRegistrationAccount/etc.) is
nil before continuing; ensure ffiType, externalDetails/internalDetails
population and derivationPath logic only run when managed is present so the
function returns nil for missing collection/account instead of fabricating an
AccountDetailInfo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3243c84-57e6-46e5-b65a-27a437080cf7
📒 Files selected for processing (7)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Models/HDWalletModels.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Models/HDWalletModels.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swift
- packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
ZocoLini
force-pushed
the
chore/swift-sdk-improvements-fixes
branch
from
0e617f8 to
17c9932
Compare
Contributor
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/Core/Wallet/CoreWalletManager.swift (1)
150-162:⚠️ Potential issue | 🟠 MajorForce-unwrap (
try!) can crash the app on FFI failure.Lines 152 and 161 use
try!which will crash the application if the FFI calls fail. This contradicts the PR's pattern of converting to non-throwing, graceful-failure APIs elsewhere.Consider returning an empty array on failure to maintain consistency:
Proposed fix
public func getTransactions(for wallet: HDWallet, accountIndex: UInt32 = 0) -> [WalletTransaction] { - // Get managed account - let managedAccount = try! sdkWalletManager.getManagedAccount( - walletId: wallet.walletId, - accountIndex: accountIndex, - accountType: .standardBIP44 - ) - - // Get current height (TODO: get from SPV client when available) - let currentHeight: UInt32 = 0 - - return try! managedAccount.getTransactions(currentHeight: currentHeight) + do { + let managedAccount = try sdkWalletManager.getManagedAccount( + walletId: wallet.walletId, + accountIndex: accountIndex, + accountType: .standardBIP44 + ) + let currentHeight: UInt32 = 0 + return try managedAccount.getTransactions(currentHeight: currentHeight) + } catch { + print("Failed to get transactions: \(error)") + return [] + } }🤖 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 - 162, getTransactions currently uses force-throws (try!) on sdkWalletManager.getManagedAccount and managedAccount.getTransactions which can crash on FFI failure; change getTransactions to catch errors instead: perform the getManagedAccount and getTransactions calls inside a do-catch (or use optional try with guard/if let) and on any error return an empty [WalletTransaction] to match the non-throwing API pattern, ensuring you reference getTransactions(for:accountIndex:), sdkWalletManager.getManagedAccount(...), and managedAccount.getTransactions(currentHeight:) when editing.
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift (1)
16-16: Dead code:isLoadingstate is declared but never used.The
isLoadingstate variable is no longer read or modified after the refactor to synchronous loading. Remove it to clean up the code.Proposed fix
`@State` private var detailInfo: AccountDetailInfo? - `@State` private var isLoading = true `@State` private var errorMessage: String?🤖 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/AccountDetailView.swift` at line 16, Remove the unused `@State` variable isLoading from AccountDetailView: locate the declaration "@State private var isLoading = true" in the AccountDetailView struct and delete it, and then run a quick compile to ensure no references remain; if any residual checks or bindings reference isLoading (e.g., in view modifiers or conditional logic), remove or replace those references so the view uses the current synchronous loading flow.
🤖 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 187-189: The getReceiveAddress method currently uses try! on
sdkWalletManager.getReceiveAddress which can crash; replace the force-try with
proper error handling by either (a) changing the signature of
getReceiveAddress(for:accountIndex:) to throw and rethrow the underlying error
from sdkWalletManager.getReceiveAddress, or (b) change the return type to
String? (or return an empty string) and use do/catch to call
sdkWalletManager.getReceiveAddress(walletId:accountIndex:) and return the
successful value or nil/"" on failure; update any callers accordingly (refer to
getReceiveAddress and sdkWalletManager.getReceiveAddress).
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- Around line 150-162: getTransactions currently uses force-throws (try!) on
sdkWalletManager.getManagedAccount and managedAccount.getTransactions which can
crash on FFI failure; change getTransactions to catch errors instead: perform
the getManagedAccount and getTransactions calls inside a do-catch (or use
optional try with guard/if let) and on any error return an empty
[WalletTransaction] to match the non-throwing API pattern, ensuring you
reference getTransactions(for:accountIndex:),
sdkWalletManager.getManagedAccount(...), and
managedAccount.getTransactions(currentHeight:) when editing.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift`:
- Line 16: Remove the unused `@State` variable isLoading from AccountDetailView:
locate the declaration "@State private var isLoading = true" in the
AccountDetailView struct and delete it, and then run a quick compile to ensure
no references remain; if any residual checks or bindings reference isLoading
(e.g., in view modifiers or conditional logic), remove or replace those
references so the view uses the current synchronous loading flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6668d9a-8f7d-45cd-bb4d-98cc51249ae3
📒 Files selected for processing (7)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Models/HDWalletModels.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
ZocoLini
force-pushed
the
chore/swift-sdk-improvements-fixes
branch
from
17c9932 to
9b8da2c
Compare
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This push fully resolves both blocking findings from the prior review. The assert(false) in guard-else bodies (which is invalid Swift in release builds) has been replaced with proper returns (return nil / return []). The assert(success) in ManagedAccount.getBalance() (which was stripped under -O, silently returning zero balances) has been replaced with preconditionFailure which correctly traps in all build modes. The walletId.count == 32 length check in getManagedAccountCollection has also been restored. The wallet-level Balance.total formula inconsistency (prior suggestion) remains unaddressed but is low-risk.
Reviewed commit: 17c9932
Contributor
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/Core/Wallet/CoreWalletManager.swift (1)
150-162:⚠️ Potential issue | 🟠 MajorSame force-unwrap pattern in
getTransactions.Both
getManagedAccountandgetTransactionsusetry!which will crash on FFI failures. Consider usingtry?with appropriate fallbacks.🤖 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 - 162, The getTransactions function currently uses force-try (try!) on sdkWalletManager.getManagedAccount and managedAccount.getTransactions which will crash on FFI failures; change these to safe error handling (either use try? with a sensible fallback such as returning an empty [WalletTransaction] when obtaining managedAccount or fetching transactions fails, or wrap both calls in a do-catch and propagate or log the error instead), replacing the forced unwraps around getManagedAccount and managedAccount.getTransactions and ensuring currentHeight handling remains the same.
🧹 Nitpick comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift (1)
16-16: Dead state variable:isLoadingis never updated.The
isLoadingstate is initialized totruebut never set tofalseafterloadAccountDetails()completes. With the switch to synchronous loading, this variable is now unused. Consider removing it to avoid confusion.- `@State` private var isLoading = true🤖 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/AccountDetailView.swift` at line 16, The isLoading `@State` variable in AccountDetailView is dead (initialized true but never updated after loadAccountDetails()), so remove the unused property to avoid confusion: delete the declaration "@State private var isLoading = true" and any references to isLoading; verify loadAccountDetails() and the view's synchronous loading logic remain correct without relying on isLoading.packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (1)
480-503: FFI error information silently discarded.The error from
managed_wallet_get_account_collectionis freed without being logged or propagated. This makes debugging failures difficult. Consider logging the error before returningnil.Suggested improvement
guard let collection = collectionHandle else { + if let msg = error.message { + print("getManagedAccountCollection failed: \(String(cString: msg))") + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift` around lines 480 - 503, getManagedAccountCollection currently discards FFIError from managed_wallet_get_account_collection by freeing error.message without logging or propagating it; capture the error details (error.message and/or error.code) after the FFI call and before calling error_message_free, and emit them via the module's logging facility (or a debug print if no logger exists) so failures are visible, then free the message and return nil as before; reference symbols: getManagedAccountCollection, managed_wallet_get_account_collection, FFIError, and error_message_free.
🤖 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/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift`:
- Around line 621-626: The loadAccountDetails function currently assigns
detailInfo from walletService.walletManager.getAccountDetails without handling a
nil result; update loadAccountDetails to check the returned value from
getAccountDetails(for:account:) and if it’s nil set an appropriate errorMessage
(and clear or set detailInfo to nil) so the SwiftUI view can show an error
state; reference the loadAccountDetails method,
walletService.walletManager.getAccountDetails, the detailInfo property, and the
errorMessage property when implementing this conditional assignment and UI state
update.
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- Around line 150-162: The getTransactions function currently uses force-try
(try!) on sdkWalletManager.getManagedAccount and managedAccount.getTransactions
which will crash on FFI failures; change these to safe error handling (either
use try? with a sensible fallback such as returning an empty [WalletTransaction]
when obtaining managedAccount or fetching transactions fails, or wrap both calls
in a do-catch and propagate or log the error instead), replacing the forced
unwraps around getManagedAccount and managedAccount.getTransactions and ensuring
currentHeight handling remains the same.
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift`:
- Around line 480-503: getManagedAccountCollection currently discards FFIError
from managed_wallet_get_account_collection by freeing error.message without
logging or propagating it; capture the error details (error.message and/or
error.code) after the FFI call and before calling error_message_free, and emit
them via the module's logging facility (or a debug print if no logger exists) so
failures are visible, then free the message and return nil as before; reference
symbols: getManagedAccountCollection, managed_wallet_get_account_collection,
FFIError, and error_message_free.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift`:
- Line 16: The isLoading `@State` variable in AccountDetailView is dead
(initialized true but never updated after loadAccountDetails()), so remove the
unused property to avoid confusion: delete the declaration "@State private var
isLoading = true" and any references to isLoading; verify loadAccountDetails()
and the view's synchronous loading logic remain correct without relying on
isLoading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9af4700-17c2-41cf-93e3-dad70e9dae93
📒 Files selected for processing (7)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Models/HDWalletModels.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/ManagedAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This push is a rebase onto the updated base branch (v3.1-dev) with no changes to the PR's own code. The prior review's approval stands — all previously identified blocking issues (assert(false) in guard-else, assert(success) stripped in release builds) remain resolved.
Reviewed commit: 9b8da2c
ZocoLini
force-pushed
the
chore/swift-sdk-improvements-fixes
branch
from
9b8da2c to
ac1541c
Compare
ZocoLini
force-pushed
the
chore/swift-sdk-improvements-fixes
branch
from
ac1541c to
897b021
Compare
QuantumExplorer
approved these changes
Mar 17, 2026
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
3 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.
Wallet balance was implemented by retrieving Main account balance, this PR fixes that and replaces that logic with the sum of all account balances inside a wallet
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Bug Fixes
Chores
Refactor