fix: resolve a few issues in iOS example app #2843
Conversation
WalkthroughThe changes modify wallet creation logic to distinguish between imported and new wallets, with conditional initialization of birth heights and sync-from heights. Additionally, the SPV mempool strategy is switched from BloomFilter to FetchAll. The Changes
Sequence DiagramsequenceDiagram
participant WalletService
participant WalletManager
participant HDWallet as HDWallet<br/>(Instantiation)
WalletService->>WalletManager: createWallet(isImport: Bool)
alt isImport = true
Note over WalletManager: birthHeight = 730000 (mainnet)<br/>or 1 (other networks)
Note over WalletManager: sync-from heights = fixed baselines
else isImport = false
Note over WalletManager: birthHeight = 0
Note over WalletManager: sync-from heights = derived from<br/>SPV checkpoint heights
end
WalletManager->>HDWallet: HDWallet(isImported: isImport)
HDWallet-->>WalletManager: wallet created
WalletManager-->>WalletService: return HDWallet
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/Services/WalletService.swift (1)
322-350: Remove duplicate sync-from height logic.The per-network sync-from heights are already set by
WalletManager.createWalletat lines 164-191 in WalletManager.swift. This duplicate logic creates unnecessary code duplication and risks inconsistency if the two implementations diverge.Remove lines 322-350 from WalletService since WalletManager handles this:
// Load the newly created wallet await loadWallet(wallet) - // Set per-network sync-from heights - // Imported wallets: mainnet=730000, testnet=0, devnet=0 - // New wallets: use current known tip for the selected network (fallback to latestHeaderHeight/checkpoint) - let isImported = isImport - if isImported { - // Imported wallet: use fixed per-network baselines - wallet.syncFromMainnet = 730_000 - wallet.syncFromTestnet = 0 - wallet.syncFromDevnet = 0 - } else { - // New wallet: per selected network, use the latest checkpoint height of that chain - let nets = networks ?? [walletNetwork] - for n in nets { - switch n { - case .mainnet: - let cp = SPVClient.latestCheckpointHeight(forNetwork: .init(rawValue: 0)) ?? 0 - print("[WalletService] New wallet baseline mainnet checkpoint=\(cp)") - wallet.syncFromMainnet = Int(cp) - case .testnet: - let cp = SPVClient.latestCheckpointHeight(forNetwork: .init(rawValue: 1)) ?? 0 - print("[WalletService] New wallet baseline testnet checkpoint=\(cp)") - wallet.syncFromTestnet = Int(cp) - case .devnet: - let cp = SPVClient.latestCheckpointHeight(forNetwork: .init(rawValue: 2)) ?? 0 - print("[WalletService] New wallet baseline devnet checkpoint=\(cp)") - wallet.syncFromDevnet = Int(cp) - } - } - } - - // Persist sync-from changes - try modelContainer?.mainContext.save() - print("=== WalletService.createWallet SUCCESS ===") return walletpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Wallet/WalletManager.swift (1)
202-206: Update importWallet to use the isImport parameter.The
importWalletmethod doesn't passisImport: truetocreateWallet, then manually setswallet.isImported = trueafterwards. This bypasses the new birthHeight and sync-from heights logic designed for imported wallets.Apply this diff to properly use the isImport parameter:
func importWallet(label: String, network: Network, mnemonic: String, pin: String) async throws -> HDWallet { - let wallet = try await createWallet(label: label, network: network, mnemonic: mnemonic, pin: pin) - wallet.isImported = true - try modelContainer.mainContext.save() - return wallet + return try await createWallet(label: label, network: network, mnemonic: mnemonic, pin: pin, isImport: true) }
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/SPV/SPVClient.swift (1)
350-350: Document the mempool strategy change.The mempool strategy was changed from BloomFilter (rawValue: 1) to FetchAll (rawValue: 0). While this aligns with the PR objectives ("Use mempool fetch all"), the rationale and implications should be documented.
Add a comment explaining the change:
// Enable mempool tracking and ensure detailed events are available dash_spv_ffi_config_set_mempool_tracking(configPtr, true) - dash_spv_ffi_config_set_mempool_strategy(configPtr, FFIMempoolStrategy(rawValue: 0)) // FetchAll + // Use FetchAll strategy to retrieve all mempool transactions (vs BloomFilter which only fetches matching transactions) + // This ensures we don't miss any relevant mempool activity across multiple wallets + dash_spv_ffi_config_set_mempool_strategy(configPtr, FFIMempoolStrategy(rawValue: 0)) // FetchAll _ = dash_spv_ffi_config_set_fetch_mempool_transactions(configPtr, true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/swift-sdk/Sources/SwiftDashSDK/SPV/SPVClient.swift(1 hunks)packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/WalletService.swift(1 hunks)packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Wallet/WalletManager.swift(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
packages/swift-sdk/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Make DPP types public in Swift where needed for visibility
Files:
packages/swift-sdk/Sources/SwiftDashSDK/SPV/SPVClient.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Wallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/WalletService.swift
packages/swift-sdk/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep the Swift app and SDK code in packages/swift-sdk
Files:
packages/swift-sdk/Sources/SwiftDashSDK/SPV/SPVClient.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Wallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/WalletService.swift
packages/swift-sdk/SwiftExampleApp/**/*.swift
📄 CodeRabbit inference engine (packages/swift-sdk/SwiftExampleApp/CLAUDE.md)
packages/swift-sdk/SwiftExampleApp/**/*.swift: Use Core SDK functions with the dash_core_sdk_* prefix
Use Platform SDK functions with the dash_sdk_* prefix
Use Unified SDK functions with the dash_unified_sdk_* prefix
Prefer using PersistentToken predicate helpers (e.g., mintableTokensPredicate, tokensWithControlRulePredicate) instead of manual filtering
Files:
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Wallet/WalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/WalletService.swift
🧠 Learnings (1)
📚 Learning: 2025-09-07T22:19:59.217Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/swift-sdk/SwiftExampleApp/CLAUDE.md:0-0
Timestamp: 2025-09-07T22:19:59.217Z
Learning: Applies to packages/swift-sdk/SwiftExampleApp/**/{PersistentPublicKey,PersistentIdentity}*.swift : Link private keys to PersistentPublicKey, not to PersistentIdentity
Applied to files:
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Wallet/WalletManager.swift
🧬 Code graph analysis (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Wallet/WalletManager.swift (3)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/WalletService.swift (2)
createWallet(289-363)error(78-80)packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift (2)
addWalletAndSerialize(603-674)addWalletAndSerialize(686-757)packages/swift-sdk/Sources/SwiftDashSDK/SPV/SPVClient.swift (1)
latestCheckpointHeight(975-991)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Wallet/WalletManager.swift (2)
92-105: LGTM! Clear birthHeight logic for imported vs. new wallets.The birthHeight calculation correctly distinguishes between imported wallets (which need to sync from a historical point) and new wallets (which can use the latest checkpoint). The use of
1instead of0for non-mainnet imported wallets is well-documented to avoid FFI misinterpretation.
164-191: LGTM! Proper per-network sync baseline initialization.The per-network sync-from heights are correctly set based on wallet type:
- Imported wallets use fixed historical baselines
- New wallets query the latest checkpoint for each enabled network
- Graceful handling of nil checkpoint values with optional binding
|
✅ 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: "1c58d8634be55469c2b3fe1753e2525cceaf7c1099c3ef07221c1f728ded9c75"
)Xcode manual integration:
|
Issue being fixed or feature implemented
Use mempool fetch all, and properly set sync height when we have multiple wallets
What was done?
How Has This Been Tested?
Tested locally
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Improvements