feat(swift-sdk): add ZK sync, local Docker support, and account management#3393
feat(swift-sdk): add ZK sync, local Docker support, and account management#3393QuantumExplorer wants to merge 11 commits intov3.1-devfrom
Conversation
…d nullifier sync - Create ZKSyncService with periodic sync loop (30s interval) - Display shielded balance, orchard address, note/nullifier stats in CoreContentView - Persist balance and address across app launches via UserDefaults - Wire into UnifiedAppState lifecycle and pull-to-refresh Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request consolidates the Changes
Sequence Diagram(s)sequenceDiagram
participant UI as SwiftUI View
participant ZKSync as ZKSyncService
participant UnifiedState as UnifiedAppState
participant SDK as Dash SDK
participant ShieldedService as ShieldedService
UI->>UnifiedState: performZKSync() (manual trigger or periodic)
UnifiedState->>ZKSync: performSync(sdk, shieldedService)
alt Check Preconditions
ZKSync-->>UnifiedState: Return if isSyncing or poolClient nil
else Proceed
ZKSync->>ZKSync: Set isSyncing = true
ZKSync->>SDK: syncNotes()
SDK->>ShieldedService: Sync notes from pool
ShieldedService-->>SDK: Return synced notes count
ZKSync->>SDK: syncNullifiers()
SDK->>ShieldedService: Sync nullifiers from pool
ShieldedService-->>SDK: Return spent nullifiers count
ZKSync->>ZKSync: Update shieldedBalance, orchardAddress
ZKSync->>ZKSync: Update counters and lastSyncTime
ZKSync->>ZKSync: Persist to UserDefaults
alt Success
ZKSync->>ZKSync: Clear lastError, Log success
else Error
ZKSync->>ZKSync: Set lastError, Log error
end
ZKSync->>ZKSync: Set isSyncing = false
end
ZKSync-->>UnifiedState: Complete
UnifiedState-->>UI: Update published properties
UI->>UI: Re-render sync status display
sequenceDiagram
participant User as User
participant UI as AddAccountView
participant WalletManager as CoreWalletManager
participant SDK as Dash SDK
participant Wallet as HDWallet
User->>UI: Select account type, index, keyClass
User->>UI: Tap "Create Account"
UI->>UI: Validate inputs
UI->>UI: Set isCreating = true
UI->>WalletManager: addAccount(wallet, type, index, keyClass)
alt type == .platformPayment
WalletManager->>SDK: addPlatformPaymentAccount(accountIndex, keyClass)
SDK->>Wallet: Create platform payment account
Wallet-->>SDK: Account created
else Other types
WalletManager->>SDK: addAccount(type, index)
SDK->>Wallet: Create standard account
Wallet-->>SDK: Account created
end
SDK-->>WalletManager: Success or Error
alt Success
WalletManager-->>UI: Return
UI->>UI: Set isCreating = false
UI->>UI: Dismiss sheet
UI->>UI: Call loadAccounts() to refresh
UI-->>User: Account list updated
else Error
WalletManager-->>UI: Throw WalletError
UI->>UI: Set isCreating = false, isErrorShown = true
UI->>UI: Display error alert
UI-->>User: Error message shown
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use TrustedHttpContextProvider::new_with_url for local/regtest networks, connecting to quorum sidecar at localhost:22444 (dashmate Docker default) - Replace separate "Use Local DAPI" and "Use Local Core" toggles with unified "Use Docker Setup" toggle - Default DAPI address changed to https://127.0.0.1:2443 (envoy gateway) - Persist selected tab across app launches - SDK.init uses dash_sdk_create_trusted for all networks (Rust auto-detects local) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… account types - Restore AddAccountView from feat/platformSync branch for adding accounts to wallets - Add platformPayment, dashPayReceivingFunds, dashPayExternalAccount to AccountCategory and AccountType - Use wallet_add_platform_payment_account() for Platform Payment accounts (requires key_class) - Add CoreWalletManager.addAccount() public method - Add regtest (case 2) to SPV client network config instead of falling through to testnet - Default local Core P2P peer to 127.0.0.1:20001 (Docker dashmate port) - Add Local/Regtest option to wallet creation network picker Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ounts - Change default local DAPI from https to http (Docker envoy self-signed cert rejected) - Enumerate platform payment accounts in getAccounts() for wallet detail view Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add "Get 10 DASH from Faucet" button on Receive screen (Core tab, Docker mode only) - Calls seed node Core RPC sendtoaddress at 127.0.0.1:20302 - Add Faucet RPC Password field in Settings when Docker Setup is enabled - Password persisted in UserDefaults for subsequent faucet requests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove key-wallet-manager workspace dependency (merged into key-wallet) - Update rs-dpp and rs-platform-wallet to use key-wallet directly - Remove dashcore "std" feature (removed upstream) - Update WalletTransactionChecker for new check_core_transaction signature (added &mut Wallet and is_from_mempool params) - Add mark_instant_send_utxos to WalletInfoInterface impl Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e on switch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/SPV/SPVClient.swift (1)
44-133:⚠️ Potential issue | 🟠 MajorEnsure FFI pointers are always released on initializer failure.
If initialization throws after
configPtris allocated (at line 95 during dataDir configuration, line 117 during user agent setup, or line 128 if client creation fails), the FFI config and client pointers leak because cleanup only happens indestroy(), which is only called fromdeinitwhen the instance fully initializes. Since the initializer throws before assignment toself, the instance is never created anddeinitis never invoked.Add a
deferblock at the start ofinitto ensure failure-path cleanup:Proposed fix
init(network: Network = DashSDKNetwork(rawValue: 1), dataDir: String?, startHeight: UInt32) throws { + var createdConfig: UnsafeMutablePointer<FFIClientConfig>? + var createdClient: UnsafeMutablePointer<FFIDashSpvClient>? + var didInitialize = false + defer { + if !didInitialize { + if let createdClient { dash_spv_ffi_client_destroy(createdClient) } + if let createdConfig { dash_spv_ffi_config_destroy(createdConfig) } + } + } + if swiftLoggingEnabled { let level = (ProcessInfo.processInfo.environment["SPV_LOG"] ?? "off") print("[SPV][Log] Initialized SPV logging level=\(level)") } let configPtr: UnsafeMutablePointer<FFIClientConfig>? = { switch network.rawValue {After configPtr guard (line 63):
guard let configPtr = configPtr else { throw SPVError.configurationFailed } + createdConfig = configPtrAfter client guard (line 129):
guard let client = client else { print("[SPV][Init] Failed to create client: \(SPVClient.getLastDashFFIError())") throw SPVError.initializationFailed } + createdClient = clientAt end of init (line 133):
self.client = client config = configPtr + didInitialize = true }Per coding guidelines: "Swift code must properly wrap FFI functions with correct memory management across Swift/Rust boundaries".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift` around lines 44 - 133, The initializer for SPVClient must free FFI resources on all failure paths; add a defer immediately after unwrapping configPtr in init to free the config pointer if the initializer throws (call dash_spv_ffi_config_free(configPtr) from the defer), and when creating the client handle, if dash_spv_ffi_client_new returns nil or any subsequent throw occurs ensure any allocated client is freed (dash_spv_ffi_client_free(client)) and the config is freed as well; do this instead of relying on instance.destroy()/deinit since self is not assigned on init failure. Ensure you reference SPVClient.init, the local configPtr and client variables, and use the FFI free functions (dash_spv_ffi_config_free / dash_spv_ffi_client_free) inside the defer/cleanup paths.
🧹 Nitpick comments (7)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ReceiveAddressView.swift (2)
243-317: Consider storing the status-clearing Task to cancel on view dismissal.The nested Task at lines 313-316 that clears
faucetStatusafter 5 seconds will continue running even if the view is dismissed. While SwiftUI handles this gracefully (no crash), it's unnecessary work. Consider storing the Task in a property and canceling it in.onDisappearor using a different pattern.Additionally, the
defer { isFaucetLoading = false }at line 248 correctly ensures loading state is cleared, which is good error handling.💡 Optional: Cancel pending status clear on view dismissal
// Add property `@State` private var statusClearTask: Task<Void, Never>? // In requestFromFaucet, replace lines 313-316: statusClearTask?.cancel() statusClearTask = Task { try? await Task.sleep(nanoseconds: 5_000_000_000) if !Task.isCancelled { faucetStatus = nil } } // Add to view body: .onDisappear { statusClearTask?.cancel() }🤖 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/ReceiveAddressView.swift` around lines 243 - 317, The nested Task in requestFromFaucet continues running after the view is dismissed; add a cancellable Task property (e.g. `@State` private var statusClearTask: Task<Void, Never>?) and replace the inline Task at the end of requestFromFaucet with: cancel any existing statusClearTask, assign a new Task that sleeps 5s and checks Task.isCancelled before clearing faucetStatus; also cancel statusClearTask in the view's .onDisappear to avoid doing unnecessary work. Ensure you reference requestFromFaucet, faucetStatus, and the new statusClearTask property when making these changes.
303-303: Consider directing users to Settings for faucet password configuration.The error message "Auth failed — set faucetRPCPassword in UserDefaults" references UserDefaults directly, but users configure this in Settings. Consider updating to "Auth failed — check Faucet RPC Password in Settings" for better UX.
🤖 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/ReceiveAddressView.swift` at line 303, The faucetStatus message in ReceiveAddressView is misleading; update the assignment that sets faucetStatus (in ReceiveAddressView) from "Auth failed — set faucetRPCPassword in UserDefaults" to a user-facing string like "Auth failed — check Faucet RPC Password in Settings" so users are directed to the Settings UI rather than UserDefaults.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift (1)
294-299: UnusedselectedNetworksarray beyondprimaryNetwork.The code builds a
selectedNetworksarray but only uses the first element. Based on line 305-311 and the comment "SDK enforces unique wallet per mnemonic", only one wallet is created. Consider simplifying to just determine the primary network directly, or add a comment explaining why the array is built (e.g., future multi-network wallet support).🤖 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/CreateWalletView.swift` around lines 294 - 299, The code builds a selectedNetworks array but only uses its first element (primaryNetwork), which is unnecessary; either simplify by computing primaryNetwork directly from the boolean flags (createForMainnet, createForTestnet, createForDevnet with shouldShowDevnet, createForRegtest with shouldShowRegtest) or add a brief comment above selectedNetworks explaining that the array is intentionally built for potential future multi-network support while only primaryNetwork is used today by the wallet creation logic (see selectedNetworks and primaryNetwork usage in CreateWalletView.swift), and remove compactMap if you simplify.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ZKSyncService.swift (2)
98-144: Consider usingdeferto ensureisSyncingis reset.While the current placement of
isSyncing = falseat line 144 works correctly, usingdeferimmediately after settingisSyncing = truewould be more idiomatic Swift and defensive against future code changes that might add early returns.Proposed refactor using defer
isSyncing = true + defer { isSyncing = false } lastError = nil do { // ... sync logic ... } catch { lastError = error.localizedDescription SDKLogger.log( "ZK sync error: \(error.localizedDescription)", minimumLevel: .medium ) } - - isSyncing = false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ZKSyncService.swift` around lines 98 - 144, Move the isSyncing reset into a defer block immediately after setting isSyncing = true so it always runs even if the function returns early; i.e., after setting isSyncing = true add defer { isSyncing = false } and remove the trailing standalone isSyncing = false at the end, keeping the rest of the logic (calls to poolClient.syncNotes(sdk: sdk), poolClient.syncNullifiers(sdk: sdk), updates to notesSynced/totalNotesSynced/nullifiersSpent/totalNullifiersSpent/shieldedBalance/orchardAddress/persistedBalance/persistedOrchardAddress/lastSyncTime/syncCountSinceLaunch and the catch that sets lastError and logs) unchanged.
52-55: Potential overflow when persisting largeUInt64balance values.
UserDefaults.integer(forKey:)returnsInt, andInton 32-bit platforms is only 32 bits. While iOS devices are now 64-bit, storingUInt64asIntcan still lose the high bit (values ≥ 2^63 become negative). Consider usingUserDefaults.object(forKey:) as? Int64or storing the value as a string.Proposed fix using Double for safer storage
/// Persisted shielded balance (credits). private var persistedBalance: UInt64 { - get { UInt64(UserDefaults.standard.integer(forKey: "\(keyPrefix)_balance")) } - set { UserDefaults.standard.set(Int(newValue), forKey: "\(keyPrefix)_balance") } + get { UInt64(UserDefaults.standard.double(forKey: "\(keyPrefix)_balance")) } + set { UserDefaults.standard.set(Double(newValue), forKey: "\(keyPrefix)_balance") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ZKSyncService.swift` around lines 52 - 55, The current persistedBalance getter/setter uses UserDefaults.integer(...) and casts to UInt64, which can overflow on large balances; change it to store as an NSNumber (or string) via UserDefaults.set(NSNumber(value: ...), forKey: ...) and read via UserDefaults.object(forKey:) as? NSNumber then access .uint64Value (or parse the string) to round-trip full UInt64 safely; update the persistedBalance computed property and any use of keyPrefix to use these object-based reads/writes instead of integer(forKey:).packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift (1)
291-301: Inconsistent disabled state for "Clear" buttons.The Platform sync "Clear" button (lines 291-301) is not disabled during sync, while the ZK sync "Clear" button (line 451) is disabled when
zkSyncService.isSyncing. Consider making this behavior consistent across both sections.Option: Disable Platform Clear button during sync
Button { platformBalanceSyncService.reset() } label: { Text("Clear") .font(.caption) .fontWeight(.medium) } .buttonStyle(.borderedProminent) .tint(.red) .controlSize(.mini) + .disabled(platformBalanceSyncService.isSyncing) }Also applies to: 441-452
🤖 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/CoreContentView.swift` around lines 291 - 301, The Platform "Clear" button currently calls platformBalanceSyncService.reset() but does not disable during sync, causing inconsistent behavior compared to the ZK clear button which uses zkSyncService.isSyncing; update the Platform clear Button(s) to mirror the ZK behavior by adding the .disabled(platformBalanceSyncService.isSyncing) modifier to the Button that invokes platformBalanceSyncService.reset(), and similarly ensure the other Platform clear instance (the one matching lines 441-452) also uses .disabled with platformBalanceSyncService.isSyncing so both sections behave consistently with the ZK clear button that uses zkSyncService.isSyncing.packages/rs-dpp/Cargo.toml (1)
95-95: Feature name is now semantically misleading.The
core_key_wallet_managerfeature now enablesdep:key-walletinstead of the formerkey-wallet-managercrate. While this maintains backwards compatibility for feature flags, consumers enabling this feature expectingkey_wallet_managersymbols (per the relevant snippet inrs-sdk/Cargo.toml) will need to update their imports to usekey_wallet::manager::...paths instead.Consider documenting this migration or renaming the feature to
core_key_wallet_with_managerin a follow-up to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/Cargo.toml` at line 95, The feature flag core_key_wallet_manager now enables dep:key-wallet (providing key_wallet::manager::... symbols) but the name still implies the old key_wallet_manager crate; update the crate to either (A) rename the feature to core_key_wallet_with_manager and update all feature references (e.g., in any Cargo.toml that used core_key_wallet_manager) or (B) add explicit documentation/migration notes explaining that core_key_wallet_manager maps to dep:key-wallet and that consumers must change imports from key_wallet_manager::... to key_wallet::manager::...; ensure the chosen change updates the feature list, README or migration docs, and any references to the old symbol paths or feature name so consumers are not misled.
🤖 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/rs-dpp/src/lib.rs`:
- Around line 12-16: Add a backwards-compatible re-export for the removed
key_wallet_manager symbol so downstream crates using
dpp::key_wallet_manager::... keep compiling: restore a conditional alias by
adding a cfg-guarded re-export for the feature core_key_wallet_manager (e.g.,
under the same cfg style as the existing core_key_wallet feature) that maps
key_wallet to the old name (key_wallet_manager); alternatively, if you prefer
removal, delete the core_key_wallet_manager feature from Cargo.toml and update
documentation and downstream usages to import key_wallet::manager::... directly.
In `@packages/rs-platform-wallet/src/lib.rs`:
- Around line 24-25: The `pub use key_wallet;` re-export guarded by the
"manager" feature is unused; either remove the re-export from lib.rs (delete the
line `pub use key_wallet;` under #[cfg(feature = "manager")]) or, if intended
for external consumers, add a clear public doc comment above it and ensure the
"manager" feature actually exposes meaningful functionality in Cargo.toml (not
an empty `manager = []`) so external users can rely on
`platform_wallet::key_wallet`; references to `PlatformWalletInfo` and internal
imports should keep using direct `key_wallet` imports if you remove the
re-export.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift`:
- Around line 470-481: The Platform Payment loop in CoreWalletManager is using a
hardcoded Balance(confirmed: 0, unconfirmed: 0) which causes zero balances to be
shown; replace that with the actual balance from the account model by calling
m.getBalance() (or the equivalent method used for other account types), extract
confirmed/unconfirmed values (guarding for errors/optionals the same way as
other branches), and pass those values into AccountInfo(category:
.platformPayment, index: accountIdx, label: ..., balance: <actual balance>,
addressCount: ...); ensure you use
collection.getPlatformPaymentAccount(accountIndex:) and handle the optional
result and any thrown errors consistently with existing patterns in this file.
- Around line 378-384: The derivation path for the platformPayment case in
CoreWalletManager.swift is using purpose 15' but should be 17' to match DIP-17
and the other references; update the string returned in the platformPayment
branch (the platformPayment enum/case and its return
"m/9'/\(coinType)/15'/\(index ?? 0)'/x") to use "m/9'/\(coinType)/17'/\(index ??
0)'/x". While editing, review the dashPayReceivingFunds and
dashPayExternalAccount cases (they currently both return
"m/9'/\(coinType)/5'/0'/x") and confirm whether they are intentionally identical
or need different paths, adjusting them if necessary to reflect the intended
design.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AddAccountView.swift`:
- Around line 95-109: The derivationPath(index:keyClass:isTestnet:)
implementation is using purpose 17' for platformPayment but
CoreWalletManager.derivationPath(for:index:network:) uses 15'; update the
platformPayment return to use "15'" (i.e.
"m/9'/\(coinType)/15'/\(index)'/\(keyClass)'/..." or equivalent) so both
derivationPath implementations are consistent; verify the same purpose value is
used in both CoreWalletManager and this AddAccountView method.
---
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift`:
- Around line 44-133: The initializer for SPVClient must free FFI resources on
all failure paths; add a defer immediately after unwrapping configPtr in init to
free the config pointer if the initializer throws (call
dash_spv_ffi_config_free(configPtr) from the defer), and when creating the
client handle, if dash_spv_ffi_client_new returns nil or any subsequent throw
occurs ensure any allocated client is freed (dash_spv_ffi_client_free(client))
and the config is freed as well; do this instead of relying on
instance.destroy()/deinit since self is not assigned on init failure. Ensure you
reference SPVClient.init, the local configPtr and client variables, and use the
FFI free functions (dash_spv_ffi_config_free / dash_spv_ffi_client_free) inside
the defer/cleanup paths.
---
Nitpick comments:
In `@packages/rs-dpp/Cargo.toml`:
- Line 95: The feature flag core_key_wallet_manager now enables dep:key-wallet
(providing key_wallet::manager::... symbols) but the name still implies the old
key_wallet_manager crate; update the crate to either (A) rename the feature to
core_key_wallet_with_manager and update all feature references (e.g., in any
Cargo.toml that used core_key_wallet_manager) or (B) add explicit
documentation/migration notes explaining that core_key_wallet_manager maps to
dep:key-wallet and that consumers must change imports from
key_wallet_manager::... to key_wallet::manager::...; ensure the chosen change
updates the feature list, README or migration docs, and any references to the
old symbol paths or feature name so consumers are not misled.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ZKSyncService.swift`:
- Around line 98-144: Move the isSyncing reset into a defer block immediately
after setting isSyncing = true so it always runs even if the function returns
early; i.e., after setting isSyncing = true add defer { isSyncing = false } and
remove the trailing standalone isSyncing = false at the end, keeping the rest of
the logic (calls to poolClient.syncNotes(sdk: sdk),
poolClient.syncNullifiers(sdk: sdk), updates to
notesSynced/totalNotesSynced/nullifiersSpent/totalNullifiersSpent/shieldedBalance/orchardAddress/persistedBalance/persistedOrchardAddress/lastSyncTime/syncCountSinceLaunch
and the catch that sets lastError and logs) unchanged.
- Around line 52-55: The current persistedBalance getter/setter uses
UserDefaults.integer(...) and casts to UInt64, which can overflow on large
balances; change it to store as an NSNumber (or string) via
UserDefaults.set(NSNumber(value: ...), forKey: ...) and read via
UserDefaults.object(forKey:) as? NSNumber then access .uint64Value (or parse the
string) to round-trip full UInt64 safely; update the persistedBalance computed
property and any use of keyPrefix to use these object-based reads/writes instead
of integer(forKey:).
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift`:
- Around line 291-301: The Platform "Clear" button currently calls
platformBalanceSyncService.reset() but does not disable during sync, causing
inconsistent behavior compared to the ZK clear button which uses
zkSyncService.isSyncing; update the Platform clear Button(s) to mirror the ZK
behavior by adding the .disabled(platformBalanceSyncService.isSyncing) modifier
to the Button that invokes platformBalanceSyncService.reset(), and similarly
ensure the other Platform clear instance (the one matching lines 441-452) also
uses .disabled with platformBalanceSyncService.isSyncing so both sections behave
consistently with the ZK clear button that uses zkSyncService.isSyncing.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift`:
- Around line 294-299: The code builds a selectedNetworks array but only uses
its first element (primaryNetwork), which is unnecessary; either simplify by
computing primaryNetwork directly from the boolean flags (createForMainnet,
createForTestnet, createForDevnet with shouldShowDevnet, createForRegtest with
shouldShowRegtest) or add a brief comment above selectedNetworks explaining that
the array is intentionally built for potential future multi-network support
while only primaryNetwork is used today by the wallet creation logic (see
selectedNetworks and primaryNetwork usage in CreateWalletView.swift), and remove
compactMap if you simplify.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ReceiveAddressView.swift`:
- Around line 243-317: The nested Task in requestFromFaucet continues running
after the view is dismissed; add a cancellable Task property (e.g. `@State`
private var statusClearTask: Task<Void, Never>?) and replace the inline Task at
the end of requestFromFaucet with: cancel any existing statusClearTask, assign a
new Task that sleeps 5s and checks Task.isCancelled before clearing
faucetStatus; also cancel statusClearTask in the view's .onDisappear to avoid
doing unnecessary work. Ensure you reference requestFromFaucet, faucetStatus,
and the new statusClearTask property when making these changes.
- Line 303: The faucetStatus message in ReceiveAddressView is misleading; update
the assignment that sets faucetStatus (in ReceiveAddressView) from "Auth failed
— set faucetRPCPassword in UserDefaults" to a user-facing string like "Auth
failed — check Faucet RPC Password in Settings" so users are directed to the
Settings UI rather than UserDefaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb38884d-eab8-4b91-8933-312bd3579744
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
Cargo.tomlpackages/rs-dpp/Cargo.tomlpackages/rs-dpp/src/lib.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/examples/basic_usage.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rspackages/rs-platform-wallet/src/platform_wallet_info/wallet_transaction_checker.rspackages/rs-sdk-ffi/src/sdk.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/Models/HDWalletModels.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/KeyWalletTypes.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/AppState.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ZKSyncService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AddAccountView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ReceiveAddressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletsContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/UnifiedAppState.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift
💤 Files with no reviewable changes (1)
- Cargo.toml
| #[cfg(feature = "core_key_wallet")] | ||
| pub use key_wallet; | ||
|
|
||
| #[cfg(feature = "core_key_wallet_manager")] | ||
| pub use key_wallet_manager; | ||
| // key_wallet_manager was merged into key_wallet crate | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for usages of dpp::key_wallet_manager in the codebase
rg -n "dpp::key_wallet_manager" --type rustRepository: dashpay/platform
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if core_key_wallet_manager feature exists in rs-dpp Cargo.toml
cat packages/rs-dpp/Cargo.toml | grep -A 20 "\[features\]"Repository: dashpay/platform
Length of output: 800
🏁 Script executed:
#!/bin/bash
# Search for all references to key_wallet_manager in Cargo.toml files
rg "key_wallet_manager" --type toml -nRepository: dashpay/platform
Length of output: 370
🏁 Script executed:
#!/bin/bash
# Look for references to core_key_wallet_manager feature in the codebase
rg "core_key_wallet_manager" --type toml -nRepository: dashpay/platform
Length of output: 370
🏁 Script executed:
#!/bin/bash
# Check if key_wallet_manager module/directory still exists
fd "key_wallet_manager" --type f --type d packages/rs-dpp/Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the structure of key_wallet module to understand what was merged
ls -la packages/rs-dpp/src/ | grep -i walletRepository: dashpay/platform
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for any re-export patterns with key_wallet_manager to understand the merge better
rg "key_wallet_manager" packages/rs-dpp/src/ -nRepository: dashpay/platform
Length of output: 144
🏁 Script executed:
#!/bin/bash
# Check the current state of packages/rs-dpp/src/lib.rs around the removed export
sed -n '1,30p' packages/rs-dpp/src/lib.rsRepository: dashpay/platform
Length of output: 668
🏁 Script executed:
#!/bin/bash
# Check how rs-sdk handles core_key_wallet_manager feature
grep -A 5 -B 5 "core_key_wallet_manager" packages/rs-sdk/src/lib.rsRepository: dashpay/platform
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if there's any re-export of key_wallet or key_wallet_manager in rs-sdk
rg "pub use.*key_wallet" packages/rs-sdk/src/ -nRepository: dashpay/platform
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the git history to understand what was removed
git log --oneline -n 30 -- packages/rs-dpp/src/lib.rsRepository: dashpay/platform
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Check if key_wallet crate has a manager module or submodule
rg "mod.*manager|pub.*manager" packages/rs-dpp/Cargo.lock -A 2 | head -50Repository: dashpay/platform
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Search for any imports of key_wallet in test files or examples
rg "use key_wallet" packages/ -n --type rust | head -20Repository: dashpay/platform
Length of output: 2541
core_key_wallet_manager feature no longer provides re-export for backwards compatibility.
The core_key_wallet_manager feature still exists in Cargo.toml and is referenced by downstream crates (e.g., rs-sdk marks it as a default feature), but the corresponding pub use key_wallet_manager; re-export has been removed. While the comment indicates an intentional merge of key_wallet_manager into the key_wallet crate, external consumers that relied on dpp::key_wallet_manager::... paths will now encounter compilation errors.
Consider either:
- Adding a re-export alias for backwards compatibility:
#[cfg(feature = "core_key_wallet_manager")] pub use key_wallet as key_wallet_manager; - Removing the
core_key_wallet_managerfeature entirely if it is no longer needed - Documenting the migration path clearly (consumers should use
key_wallet::manager::...directly)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-dpp/src/lib.rs` around lines 12 - 16, Add a backwards-compatible
re-export for the removed key_wallet_manager symbol so downstream crates using
dpp::key_wallet_manager::... keep compiling: restore a conditional alias by
adding a cfg-guarded re-export for the feature core_key_wallet_manager (e.g.,
under the same cfg style as the existing core_key_wallet feature) that maps
key_wallet to the old name (key_wallet_manager); alternatively, if you prefer
removal, delete the core_key_wallet_manager feature from Cargo.toml and update
documentation and downstream usages to import key_wallet::manager::... directly.
| #[cfg(feature = "manager")] | ||
| pub use key_wallet_manager; | ||
| pub use key_wallet; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if key-wallet is used elsewhere in the crate without the manager feature gate
rg -n "use key_wallet" packages/rs-platform-wallet/src --type rustRepository: dashpay/platform
Length of output: 3546
🏁 Script executed:
# Check Cargo.toml for key-wallet dependency configuration
cat -n packages/rs-platform-wallet/Cargo.toml | head -40Repository: dashpay/platform
Length of output: 1120
🏁 Script executed:
# Search for "manager" feature definition in Cargo.toml
rg -n "manager" packages/rs-platform-wallet/Cargo.tomlRepository: dashpay/platform
Length of output: 116
🏁 Script executed:
# Check if platform_wallet_info is gated by manager feature
rg -n "#\[cfg.*manager" packages/rs-platform-wallet/src --type rustRepository: dashpay/platform
Length of output: 129
🏁 Script executed:
# Check what platform_wallet_info module exposes publicly
rg -n "^pub " packages/rs-platform-wallet/src/platform_wallet_info/mod.rs | head -20Repository: dashpay/platform
Length of output: 94
🏁 Script executed:
# Check if any of the crate's public API depends on the re-export being available
rg -n "platform_wallet::key_wallet" . --type rustRepository: dashpay/platform
Length of output: 42
Remove the unused re-export or clarify its purpose.
The pub use key_wallet; re-export at line 25 is feature-gated under manager, but the manager feature is empty (defined as manager = [] in Cargo.toml) and included in default features. More critically, this re-export is never used—no code in the codebase references platform_wallet::key_wallet. Internal modules import directly from key_wallet, and the public API (PlatformWalletInfo) does not depend on this re-export. Either remove it or document its intended purpose if it's meant for external consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-platform-wallet/src/lib.rs` around lines 24 - 25, The `pub use
key_wallet;` re-export guarded by the "manager" feature is unused; either remove
the re-export from lib.rs (delete the line `pub use key_wallet;` under
#[cfg(feature = "manager")]) or, if intended for external consumers, add a clear
public doc comment above it and ensure the "manager" feature actually exposes
meaningful functionality in Cargo.toml (not an empty `manager = []`) so external
users can rely on `platform_wallet::key_wallet`; references to
`PlatformWalletInfo` and internal imports should keep using direct `key_wallet`
imports if you remove the re-export.
| case .dashPayReceivingFunds: | ||
| return "m/9'/\(coinType)/5'/0'/x" | ||
| case .dashPayExternalAccount: | ||
| return "m/9'/\(coinType)/5'/0'/x" | ||
| case .platformPayment: | ||
| return "m/9'/\(coinType)/15'/\(index ?? 0)'/x" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for DIP-17 derivation path specifications in the codebase
rg -n "DIP.?17|purpose.?17|purpose.?15" --type rust --type swift -C2
# Search for platform payment derivation path constants
rg -n "m/9.*17|m/9.*15" -C2Repository: dashpay/platform
Length of output: 14400
Fix derivation path for platformPayment account type.
The path here uses purpose 15' but should use 17' to match the DIP-17 standard and be consistent with AddAccountView.derivationPath() and Wallet.addPlatformPaymentAccount() documentation, which both specify m/9'/coinType/17'/account'/key_class'/....
Additionally, verify whether .dashPayReceivingFunds and .dashPayExternalAccount should return identical paths or if they were meant to differ.
Proposed fix
case .platformPayment:
- return "m/9'/\(coinType)/15'/\(index ?? 0)'/x"
+ return "m/9'/\(coinType)/17'/\(index ?? 0)'/x"🤖 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 378 - 384, The derivation path for the platformPayment case in
CoreWalletManager.swift is using purpose 15' but should be 17' to match DIP-17
and the other references; update the string returned in the platformPayment
branch (the platformPayment enum/case and its return
"m/9'/\(coinType)/15'/\(index ?? 0)'/x") to use "m/9'/\(coinType)/17'/\(index ??
0)'/x". While editing, review the dashPayReceivingFunds and
dashPayExternalAccount cases (they currently both return
"m/9'/\(coinType)/5'/0'/x") and confirm whether they are intentionally identical
or need different paths, adjusting them if necessary to reflect the intended
design.
| // Platform Payment (DIP-17) | ||
| if collection.hasPlatformPaymentAccounts { | ||
| for accountIdx in 0..<collection.platformPaymentCount { | ||
| if let m = collection.getPlatformPaymentAccount(accountIndex: accountIdx, keyClass: 0) { | ||
| var addrCount = 0 | ||
| if let pool = m.getAddressPool(), let infos = try? pool.getAddresses(from: 0, to: 1000) { | ||
| addrCount = infos.count | ||
| } | ||
| list.append(AccountInfo(category: .platformPayment, index: accountIdx, label: "Platform Payment \(accountIdx)", balance: Balance(confirmed: 0, unconfirmed: 0), addressCount: (addrCount, 0))) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Platform Payment accounts always show zero balance.
The balance is hardcoded to Balance(confirmed: 0, unconfirmed: 0) instead of calling m.getBalance() like other account types in this method. This will cause Platform Payment accounts to always display a zero balance in the UI regardless of their actual balance.
Proposed fix
if collection.hasPlatformPaymentAccounts {
for accountIdx in 0..<collection.platformPaymentCount {
if let m = collection.getPlatformPaymentAccount(accountIndex: accountIdx, keyClass: 0) {
+ let b = m.getBalance()
var addrCount = 0
if let pool = m.getAddressPool(), let infos = try? pool.getAddresses(from: 0, to: 1000) {
addrCount = infos.count
}
- list.append(AccountInfo(category: .platformPayment, index: accountIdx, label: "Platform Payment \(accountIdx)", balance: Balance(confirmed: 0, unconfirmed: 0), addressCount: (addrCount, 0)))
+ list.append(AccountInfo(category: .platformPayment, index: accountIdx, label: "Platform Payment \(accountIdx)", balance: b, addressCount: (addrCount, 0)))
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Platform Payment (DIP-17) | |
| if collection.hasPlatformPaymentAccounts { | |
| for accountIdx in 0..<collection.platformPaymentCount { | |
| if let m = collection.getPlatformPaymentAccount(accountIndex: accountIdx, keyClass: 0) { | |
| var addrCount = 0 | |
| if let pool = m.getAddressPool(), let infos = try? pool.getAddresses(from: 0, to: 1000) { | |
| addrCount = infos.count | |
| } | |
| list.append(AccountInfo(category: .platformPayment, index: accountIdx, label: "Platform Payment \(accountIdx)", balance: Balance(confirmed: 0, unconfirmed: 0), addressCount: (addrCount, 0))) | |
| } | |
| } | |
| } | |
| // Platform Payment (DIP-17) | |
| if collection.hasPlatformPaymentAccounts { | |
| for accountIdx in 0..<collection.platformPaymentCount { | |
| if let m = collection.getPlatformPaymentAccount(accountIndex: accountIdx, keyClass: 0) { | |
| let b = m.getBalance() | |
| var addrCount = 0 | |
| if let pool = m.getAddressPool(), let infos = try? pool.getAddresses(from: 0, to: 1000) { | |
| addrCount = infos.count | |
| } | |
| list.append(AccountInfo(category: .platformPayment, index: accountIdx, label: "Platform Payment \(accountIdx)", balance: b, addressCount: (addrCount, 0))) | |
| } | |
| } | |
| } |
🤖 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 470 - 481, The Platform Payment loop in CoreWalletManager is using
a hardcoded Balance(confirmed: 0, unconfirmed: 0) which causes zero balances to
be shown; replace that with the actual balance from the account model by calling
m.getBalance() (or the equivalent method used for other account types), extract
confirmed/unconfirmed values (guarding for errors/optionals the same way as
other branches), and pass those values into AccountInfo(category:
.platformPayment, index: accountIdx, label: ..., balance: <actual balance>,
addressCount: ...); ensure you use
collection.getPlatformPaymentAccount(accountIndex:) and handle the optional
result and any thrown errors consistently with existing patterns in this file.
| func derivationPath(index: UInt32, keyClass: UInt32, isTestnet: Bool) -> String { | ||
| let coinType = isTestnet ? "1'" : "5'" | ||
| switch self { | ||
| case .bip44: | ||
| return "m/44'/\(coinType)/\(index)'" | ||
| case .bip32: | ||
| return "m/\(index)'" | ||
| case .coinjoin: | ||
| return "m/9'/\(coinType)/4'/\(index)'" | ||
| case .platformPayment: | ||
| return "m/9'/\(coinType)/17'/\(index)'/\(keyClass)'/..." | ||
| case .identityTopup: | ||
| return "m/9'/\(coinType)/5'/2'/\(index)'/..." | ||
| } | ||
| } |
There was a problem hiding this comment.
Derivation path uses purpose 17 here but 15 in CoreWalletManager.
This derivationPath method uses 17' for platformPayment (line 105), which aligns with "DIP-17" naming, but CoreWalletManager.derivationPath(for:index:network:) uses 15'. These should be consistent—see the related comment in CoreWalletManager.swift.
🤖 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/AddAccountView.swift`
around lines 95 - 109, The derivationPath(index:keyClass:isTestnet:)
implementation is using purpose 17' for platformPayment but
CoreWalletManager.derivationPath(for:index:network:) uses 15'; update the
platformPayment return to use "15'" (i.e.
"m/9'/\(coinType)/15'/\(index)'/\(keyClass)'/..." or equivalent) so both
derivationPath implementations are consistent; verify the same purpose value is
used in both CoreWalletManager and this AddAccountView method.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified 7 findings against the PR source. Five are functional correctness issues that should block approval; two are low-severity localhost/dev-tooling security concerns that are real but should be treated as comments, not release blockers for this example app.
Reviewed commit: f1ef279
🔴 3 blocking | 💬 4 nitpick(s)
2 additional findings
🔴 blocking: Creating a wallet does not initialize the new ZK sync path
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift (lines 306-316)
The wallet creation flow only calls walletService.walletManager.createWallet(...) and dismisses the sheet. The only places that call initializeShieldedService() are app startup and network switches in UnifiedAppState (UnifiedAppState.swift:96-103, 133-145, 245-256). performZKSync() also returns early until shieldedService.poolClient exists (UnifiedAppState.swift:233-240). On a clean launch with no wallets, creating the first wallet in-app leaves the shielded service uninitialized, so ZK sync never starts until the app is restarted or the network is switched.
🔴 blocking: Platform payment accounts are navigable even though their detail lookup is unimplemented
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift (lines 23-26)
The account list wraps every account in a NavigationLink to AccountDetailView. But CoreWalletManager.getAccountDetails(...) explicitly sets .platformPayment to managed = nil (CoreWalletManager.swift:259-260) and still returns an AccountDetailInfo with default FFIAccountType(rawValue: 0) and no real address/account metadata (CoreWalletManager.swift:265-302). Tapping a platform payment account therefore opens a detail screen backed by fabricated/default account-type information instead of actual DIP-17 data.
🤖 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/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift`:
- [BLOCKING] lines 306-316: Creating a wallet does not initialize the new ZK sync path
The wallet creation flow only calls `walletService.walletManager.createWallet(...)` and dismisses the sheet. The only places that call `initializeShieldedService()` are app startup and network switches in `UnifiedAppState` (`UnifiedAppState.swift:96-103`, `133-145`, `245-256`). `performZKSync()` also returns early until `shieldedService.poolClient` exists (`UnifiedAppState.swift:233-240`). On a clean launch with no wallets, creating the first wallet in-app leaves the shielded service uninitialized, so ZK sync never starts until the app is restarted or the network is switched.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift`:
- [BLOCKING] lines 23-26: Platform payment accounts are navigable even though their detail lookup is unimplemented
The account list wraps every account in a `NavigationLink` to `AccountDetailView`. But `CoreWalletManager.getAccountDetails(...)` explicitly sets `.platformPayment` to `managed = nil` (`CoreWalletManager.swift:259-260`) and still returns an `AccountDetailInfo` with default `FFIAccountType(rawValue: 0)` and no real address/account metadata (`CoreWalletManager.swift:265-302`). Tapping a platform payment account therefore opens a detail screen backed by fabricated/default account-type information instead of actual DIP-17 data.
In `packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift`:
- [BLOCKING] lines 167-176: Regtest/local SDK initialization fails unless Docker mode injects DAPI addresses
`SDK.init(network:)` only supplies `config.dapi_addresses` when `useDockerSetup` is true. Otherwise it calls `dash_sdk_create_trusted(&config)` with a null address list. On the Rust side, null addresses only have defaults for mainnet and testnet; regtest/local returns `InvalidParameter` (`packages/rs-sdk-ffi/src/sdk.rs:379-447`). That makes non-Docker regtest/local initialization fail outright.
| func startPeriodicSync(network: AppNetwork) { | ||
| networkName = network.rawValue | ||
|
|
||
| // Restore persisted state from previous session | ||
| let savedBalance = persistedBalance | ||
| if savedBalance > 0 { | ||
| shieldedBalance = savedBalance | ||
| } | ||
|
|
||
| let savedAddress = persistedOrchardAddress | ||
| if let addr = savedAddress, !addr.isEmpty { | ||
| orchardAddress = addr | ||
| } |
There was a problem hiding this comment.
💬 Medium: Network switches can leave stale shielded balance and address visible
startPeriodicSync(network:) changes the key prefix and restores persisted state only when the new network has a non-zero saved balance or a non-empty saved address. If the target network has no saved ZK state yet, shieldedBalance and orchardAddress are left unchanged, so the previous network's values remain visible. UnifiedAppState.handleNetworkSwitch() restarts ZK sync but does not clear ZKSyncService first (UnifiedAppState.swift:133-145), so the stale values persist until a later successful sync overwrites them.
source: ['codex']
| // Platform Payment (DIP-17) | ||
| if collection.hasPlatformPaymentAccounts { | ||
| for accountIdx in 0..<collection.platformPaymentCount { | ||
| if let m = collection.getPlatformPaymentAccount(accountIndex: accountIdx, keyClass: 0) { | ||
| var addrCount = 0 | ||
| if let pool = m.getAddressPool(), let infos = try? pool.getAddresses(from: 0, to: 1000) { | ||
| addrCount = infos.count | ||
| } | ||
| list.append(AccountInfo(category: .platformPayment, index: accountIdx, label: "Platform Payment \(accountIdx)", balance: Balance(confirmed: 0, unconfirmed: 0), addressCount: (addrCount, 0))) | ||
| } |
There was a problem hiding this comment.
💬 Medium: Platform payment account enumeration drops valid non-default account keys
The new listing logic synthesizes platform payment accounts as 0..<platformPaymentCount and always fetches keyClass: 0. The same collection type exposes getPlatformPaymentKeys() with the real (account, keyClass) pairs (ManagedAccountCollection.swift:263-281), and the add-account UI allows users to create platform payment accounts with arbitrary key classes. As written, any account whose key class is not 0 will never appear, and sparse account indices can also be skipped if platformPaymentCount is smaller than the highest account index.
source: ['codex']
| let forceLocal = UserDefaults.standard.bool(forKey: "useDockerSetup") | ||
| if forceLocal { | ||
| let localAddresses = Self.platformDAPIAddresses | ||
| print("🔵 SDK.init: Using local DAPI addresses: \(localAddresses)") | ||
| result = localAddresses.withCString { addressesCStr -> DashSDKResult in | ||
| var mutableConfig = config | ||
| mutableConfig.dapi_addresses = addressesCStr | ||
| print("🔵 SDK.init: Calling dash_sdk_create_trusted...") | ||
| return dash_sdk_create_trusted(&mutableConfig) | ||
| } | ||
| } else { | ||
| print("🔵 SDK.init: Using default network addresses") | ||
| result = dash_sdk_create_trusted(&config) |
There was a problem hiding this comment.
🔴 Blocking: Regtest/local SDK initialization fails unless Docker mode injects DAPI addresses
SDK.init(network:) only supplies config.dapi_addresses when useDockerSetup is true. Otherwise it calls dash_sdk_create_trusted(&config) with a null address list. On the Rust side, null addresses only have defaults for mainnet and testnet; regtest/local returns InvalidParameter (packages/rs-sdk-ffi/src/sdk.rs:379-447). That makes non-Docker regtest/local initialization fail outright.
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift`:
- [BLOCKING] lines 167-176: Regtest/local SDK initialization fails unless Docker mode injects DAPI addresses
`SDK.init(network:)` only supplies `config.dapi_addresses` when `useDockerSetup` is true. Otherwise it calls `dash_sdk_create_trusted(&config)` with a null address list. On the Rust side, null addresses only have defaults for mainnet and testnet; regtest/local returns `InvalidParameter` (`packages/rs-sdk-ffi/src/sdk.rs:379-447`). That makes non-Docker regtest/local initialization fail outright.
| let trusted_provider = if is_local { | ||
| info!("dash_sdk_create_trusted: using local quorum sidecar for regtest"); | ||
| match rs_sdk_trusted_context_provider::TrustedHttpContextProvider::new_with_url( | ||
| network, | ||
| "http://127.0.0.1:22444".to_string(), | ||
| std::num::NonZeroUsize::new(100).unwrap(), |
There was a problem hiding this comment.
💬 Low: Local trusted mode accepts quorum context over unauthenticated localhost HTTP
For SDKLocal/SDKRegtest, trusted mode switches to TrustedHttpContextProvider::new_with_url(..., "http://127.0.0.1:22444", ...). In trusted mode the app is intentionally relying on that sidecar instead of proof verification, so any local process that can bind or intercept that port can influence the returned quorum/context data. This is limited to localhost developer setups, so it is a real trust-boundary concern but not a production-severity issue for this example app.
source: ['codex']
| // Read RPC port and password from UserDefaults, with dashmate defaults | ||
| let rpcPort = UserDefaults.standard.string(forKey: "faucetRPCPort") ?? "20302" | ||
| let rpcUser = UserDefaults.standard.string(forKey: "faucetRPCUser") ?? "dashmate" | ||
| let rpcPassword = UserDefaults.standard.string(forKey: "faucetRPCPassword") ?? "dashmate" | ||
|
|
||
| guard let url = URL(string: "http://127.0.0.1:\(rpcPort)/") else { | ||
| faucetStatus = "Invalid RPC URL" | ||
| return | ||
| } | ||
|
|
||
| let body: [String: Any] = [ | ||
| "jsonrpc": "1.0", | ||
| "id": "faucet", | ||
| "method": "sendtoaddress", | ||
| "params": [address, 10] | ||
| ] | ||
|
|
||
| guard let jsonData = try? JSONSerialization.data(withJSONObject: body) else { | ||
| faucetStatus = "Failed to encode request" | ||
| return | ||
| } | ||
|
|
||
| var request = URLRequest(url: url) | ||
| request.httpMethod = "POST" | ||
| request.httpBody = jsonData | ||
| request.setValue("text/plain", forHTTPHeaderField: "Content-Type") | ||
|
|
||
| let credentials = "\(rpcUser):\(rpcPassword)" | ||
| if let credData = credentials.data(using: .utf8) { | ||
| request.setValue("Basic \(credData.base64EncodedString())", forHTTPHeaderField: "Authorization") |
There was a problem hiding this comment.
💬 Low: The local faucet flow uses persistent plaintext RPC credentials and sends them to any localhost listener on the configured port
The faucet feature reads faucetRPCUser/faucetRPCPassword from UserDefaults, falls back to dashmate:dashmate, and sends them as HTTP Basic auth to 127.0.0.1 (ReceiveAddressView.swift:256-285). The paired options UI stores faucetRPCPassword directly in UserDefaults through a plain TextField (Views/OptionsView.swift:58-62). Because this is only exposed for regtest Docker usage, the severity should stay low, but the finding itself is correct.
source: ['codex']
2 participants
Issue being fixed or feature implemented
Adds ZK shielded sync, local Docker network support, account management UI, and several UX improvements to the iOS SwiftExampleApp.
What was done?
ZK Shielded Sync
ZKSyncServicewith periodic sync loop (notes + nullifiers every 30s)Local Docker Network Support
dash_sdk_create_trustednow auto-detects local/regtest and uses quorum sidecar atlocalhost:22444http://127.0.0.1:2443sendtoaddress)Account Management
AddAccountViewfor adding accounts to wallets (BIP44, BIP32, CoinJoin, Platform Payment, Identity Top-up)platformPayment,dashPayReceivingFunds,dashPayExternalAccounttoAccountCategoryandAccountTypeCoreWalletManager.addAccount()with properwallet_add_platform_payment_account()for DIP-17 accountsUX Improvements
127.0.0.1:20001Dependency Updates
key-wallet-managermerge intokey-walletcratedashcorestdfeature (removed upstream)WalletTransactionCheckerfor new API signatureHow Has This Been Tested?
Breaking Changes
None
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements