Enable switching of networks and server #158
Conversation
Refactor Co-authored-by: Matthew Ramsden <6657488+reez@users.noreply.github.com>
LDKNodeMonday/View Model/Profile/NetworkSettingsViewModel.swift
Outdated
Show resolved
Hide resolved
|
added some questions before I kick off a deeper code review Everything worked testing on my end!
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
LDKNodeMonday/View/Home/OnboardingView.swift (2)
74-84: Consider adding loading state during wallet creation.The wallet creation process is asynchronous but there's no visual feedback to the user during this operation.
Button("Create wallet") { + @State private var isCreatingWallet = false Task { + isCreatingWallet = true await viewModel.saveSeed() + isCreatingWallet = false } } .buttonStyle( BitcoinFilled( tintColor: .accent, - isCapsule: true + isCapsule: true, + isLoading: isCreatingWallet ) )
74-84: Consider adding loading state during wallet creation.While the wallet creation logic is correct, the button remains interactive during the async operation.
Button("Create wallet") { + @State private var isCreatingWallet = false Task { + isCreatingWallet = true await viewModel.saveSeed() + isCreatingWallet = false } } .buttonStyle( BitcoinFilled( tintColor: .accent, isCapsule: true ) ) +.disabled(isCreatingWallet)LDKNodeMonday/View/Home/NetworkSettingsView.swift (2)
13-16: Consider using an enum for temporary settings state.The current implementation uses separate optional properties for temporary network and server. Consider using an enum to better represent the state.
+enum NetworkSettingsChange { + case network(Network) + case server(EsploraServer) + case both(network: Network, server: EsploraServer) +} + struct NetworkSettingsView: View { @Environment(\.dismiss) private var dismiss @Binding var walletClient: WalletClient @State private var showRestartAlert = false - @State private var tempNetwork: Network? - @State private var tempServer: EsploraServer? + @State private var pendingChange: NetworkSettingsChange?
89-98: Consider moving settings persistence to WalletClient.The settings persistence logic could be encapsulated in the WalletClient for better separation of concerns.
-do { - try KeyClient.live.saveNetwork(newNetwork.description) - try KeyClient.live.saveServerURL(newServer.url) -} catch let error { - await MainActor.run { - debugPrint(error.localizedDescription) - walletClient.appError = error - walletClient.appState = .error - } -} +await walletClient.saveNetworkSettings(network: newNetwork, server: newServer)LDKNodeMonday/App/LDKNodeMondayApp.swift (2)
Line range hint
26-46: Consider handling all app states explicitly.The
defaultcase in the switch statement might hide state handling issues. Consider explicitly handling all possible states ofwalletClient.appStateto make the state transitions more obvious and maintainable.- switch walletClient.appState { - case .onboarding: - OnboardingView(...) - case .wallet: - BitcoinView(...) - case .error: - ErrorView(error: self.appError) - default: - LoadingView() - } + switch walletClient.appState { + case .onboarding: + OnboardingView(...) + case .wallet: + BitcoinView(...) + case .error: + ErrorView(error: self.appError) + case .loading: + LoadingView() + }
59-64: Consider handling errors in applicationWillTerminate.The
stop()call should include error handling to ensure graceful shutdown even if the operation fails.func applicationWillTerminate(_ application: UIApplication) { - walletClient?.stop() + do { + try walletClient?.stop() + } catch { + print("Failed to stop wallet client: \(error)") + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
LDKNodeMonday.xcodeproj/project.pbxproj(5 hunks)LDKNodeMonday/App/LDKNodeMondayApp.swift(2 hunks)LDKNodeMonday/View Model/Home/OnboardingViewModel.swift(1 hunks)LDKNodeMonday/View/Home/NetworkSettingsView.swift(1 hunks)LDKNodeMonday/View/Home/OnboardingView.swift(3 hunks)LDKNodeMonday/View/Settings/SettingsView.swift(2 hunks)
🧰 Additional context used
📓 Learnings (3)
LDKNodeMonday/App/LDKNodeMondayApp.swift (2)
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/View/Settings/SettingsView.swift:118-123
Timestamp: 2025-01-28T15:46:14.253Z
Learning: In the LDKNodeMonday app's `WalletClient.delete()` function, errors during wallet deletion are handled through a do-catch block that updates `appError` with the caught error and sets `appState` to `.error`, with all state updates properly dispatched to the main actor.
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/View/Settings/SettingsView.swift:118-123
Timestamp: 2025-01-28T15:46:14.253Z
Learning: In the LDKNodeMonday app, errors during wallet deletion are handled directly within the `WalletClient.delete()` function by updating `appError` and `appState`.
LDKNodeMonday/View Model/Home/OnboardingViewModel.swift (3)
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/View Model/Home/OnboardingViewModel.swift:35-40
Timestamp: 2025-01-28T15:47:01.983Z
Learning: In the Monday app, errors during wallet creation are handled directly in the WalletClient.createWallet() function by updating appError and appState, rather than requiring error handling at the call site.
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/View/Home/ImportWalletView.swift:57-59
Timestamp: 2025-01-28T15:50:23.744Z
Learning: In the Monday app, error handling for wallet import is implemented in the WalletClient's createWallet() function, which updates appError and appState when an error occurs. The OnboardingViewModel's saveSeed() function delegates error handling to createWallet().
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/View/Settings/SettingsView.swift:118-123
Timestamp: 2025-01-28T15:46:14.253Z
Learning: In the LDKNodeMonday app, errors during wallet deletion are handled directly within the `WalletClient.delete()` function by updating `appError` and `appState`.
LDKNodeMonday/View/Settings/SettingsView.swift (1)
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/View/Settings/SettingsView.swift:118-123
Timestamp: 2025-01-28T15:46:14.253Z
Learning: In the LDKNodeMonday app's `WalletClient.delete()` function, errors during wallet deletion are handled through a do-catch block that updates `appError` with the caught error and sets `appState` to `.error`, with all state updates properly dispatched to the main actor.
🔇 Additional comments (18)
LDKNodeMonday.xcodeproj/project.pbxproj (1)
916-916: Verify the package version downgrade.The
ldk-nodepackage version has been downgraded from 0.4.3 to 0.4.2. This change needs to be reviewed to ensure it doesn't introduce compatibility issues or security vulnerabilities.Let's verify the package versions and any associated security advisories:
#!/bin/bash # Description: Check for security advisories and latest versions of the ldk-node package # Check GitHub releases for latest versions gh api \ -H "Accept: application/vnd.github+json" \ -H "X-GitHub-Api-Version: 2022-11-28" \ /repos/lightningdevkit/ldk-node/releases | jq -r '.[].tag_name' | head -n 5 # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: RUST, package: "ldk-node") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'[skip_cloning]
LDKNodeMonday/View Model/Home/OnboardingViewModel.swift (6)
12-13: LGTM! Property declarations look good.The transition from direct state management to using
WalletClientimproves encapsulation and separation of concerns.
22-24: LGTM! Clean initialization.The initialization is properly handling the binding unwrapping and setting up the required properties.
27-32: Verify error propagation in createWallet.Based on the retrieved learnings, errors during wallet creation are handled in
WalletClient.createWallet(). However, let's verify the implementation to ensure proper error handling.✅ Verification successful
Error handling in createWallet is properly implemented ✅
The method has comprehensive error handling with errors being caught and properly propagated through WalletClient's state management. The OnboardingViewModel doesn't need additional error handling as it can observe WalletClient's state changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find the implementation of createWallet in WalletClient ast-grep --pattern 'func createWallet(seedPhrase: String, network: Network, server: EsploraServer) async { $$$ }'Length of output: 1344
12-13: LGTM! Clean property declarations.The transition from appState to walletClient binding improves encapsulation by centralizing wallet management.
22-24: LGTM! Constructor properly initializes dependencies.The initialization is clean and correctly sets up the lightningClient from the walletClient.
27-32: LGTM! Wallet creation is properly delegated.Based on the retrieved learnings, error handling is correctly implemented in the WalletClient's createWallet() function.
LDKNodeMonday/View/Home/OnboardingView.swift (4)
31-48: LGTM! Clean network settings implementation.The network settings button and sheet presentation are well-implemented with proper state management through WalletClient.
112-116: LGTM! Preview configuration is correct.The preview is properly configured with mock dependencies.
31-44: LGTM! Network settings button implementation.Clean implementation with proper sheet presentation and network display.
112-116: LGTM! Preview uses appropriate mock client.The preview correctly initializes the view model with a mock WalletClient.
LDKNodeMonday/View/Home/NetworkSettingsView.swift (3)
89-98: LGTM! Proper error handling for network settings.The error handling for saving network settings is well-implemented with proper main actor dispatching.
13-16: LGTM! State management for network changes.Temporary state variables effectively handle the transition between selection and confirmation.
25-40: LGTM! Network selection with proper state handling.The implementation correctly differentiates between onboarding and runtime behavior.
LDKNodeMonday/App/LDKNodeMondayApp.swift (2)
19-21: LGTM! Clean initialization of WalletClient.The initialization properly sets up the wallet client in the shared AppDelegate instance.
29-38: Verify view model bindings across the app.The view models are initialized with
walletClientbinding. Let's verify that all view models consistently handle the wallet client binding.✅ Verification successful
View model bindings are consistently implemented ✅
All view models (
BitcoinViewModel,SettingsViewModel,OnboardingViewModel) consistently use@Binding var walletClient: WalletClientwith proper initialization patterns. Preview/mock implementations also follow the same pattern using.constant(WalletClient(...)).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for view model initializations with walletClient ast-grep --pattern 'init(walletClient: $_)'Length of output: 445
Script:
#!/bin/bash # Find all view model files echo "=== View Model Files ===" fd "ViewModel.swift|View.swift" echo -e "\n=== WalletClient Usage ===" rg "WalletClient" -A 2 echo -e "\n=== Binding Usage in View Models ===" rg "Binding<" -A 2Length of output: 9588
LDKNodeMonday/View/Settings/SettingsView.swift (2)
33-39: LGTM! Network settings navigation with current network display.Clean implementation showing the current network state in the badge.
117-122: LGTM! Wallet deletion with proper error handling.Based on the retrieved learning, error handling is correctly implemented in WalletClient.delete().
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
LDKNodeMonday/View/Home/NetworkSettingsView.swift (1)
30-33:⚠️ Potential issueReplace fatalError with proper error handling.
Using
fatalErrorin production code can lead to app crashes. This should be handled gracefully.- guard let server = availableServers(network: newNetwork).first else { - // This should never happen, but if it does: - fatalError("No servers available for \(newNetwork)") - } + guard let server = availableServers(network: newNetwork).first else { + walletClient.appError = MondayError.custom(message: "No servers available for \(newNetwork)") + walletClient.appState = .error + return + }
🧹 Nitpick comments (3)
LDKNodeMonday/View/Home/NetworkSettingsView.swift (3)
13-16: Consider using a dedicated type for managing temporary network settings.The current implementation uses separate state variables for temporary values. Consider encapsulating these in a dedicated type for better state management.
- @State private var tempNetwork: Network? - @State private var tempServer: EsploraServer? + @State private var pendingSettings: NetworkSettings? + + struct NetworkSettings { + let network: Network + let server: EsploraServer + }
89-98: Improve error handling specificity.The error handling could be more specific about what went wrong during the save operation.
do { try KeyClient.live.saveNetwork(newNetwork.description) try KeyClient.live.saveServerURL(newServer.url) - } catch let error { + } catch let error as KeyClientError { await MainActor.run { - debugPrint(error.localizedDescription) + let errorMessage = "Failed to save network settings: \(error.localizedDescription)" + debugPrint(errorMessage) + walletClient.appError = MondayError.custom(message: errorMessage) walletClient.appState = .error } + } catch { + await MainActor.run { + let errorMessage = "Unexpected error while saving network settings" + debugPrint(error.localizedDescription) + walletClient.appError = MondayError.custom(message: errorMessage) + walletClient.appState = .error + } }
80-105: Consider using structured concurrency for better task management.The current implementation could benefit from structured concurrency patterns for better error handling and task cancellation.
- if tempNetwork != nil || tempServer != nil { - Task { + if let newNetwork = tempNetwork ?? walletClient.network { + Task { + try? await withThrowingTaskGroup(of: Void.self) { group in + group.addTask { + try await saveNetworkSettings( + network: newNetwork, + server: tempServer ?? availableServers(network: newNetwork).first + ) + } + + group.addTask { + await walletClient.restart( + newNetwork: newNetwork, + newServer: tempServer ?? availableServers(network: newNetwork).first + ) + } + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
LDKNodeMonday/View/Home/NetworkSettingsView.swift(1 hunks)
🔇 Additional comments (2)
LDKNodeMonday/View/Home/NetworkSettingsView.swift (2)
50-66: Well-structured server picker implementation!The server picker implementation is clean and follows SwiftUI best practices:
- Proper binding usage
- Correct state management for both onboarding and settings changes
- Dynamic server list generation based on the current network
83-86:⚠️ Potential issueReplace fatalError with proper error handling in the restart flow.
Similar to the previous instance, using
fatalErrorshould be avoided.- guard let server = availableServers(network: newNetwork).first else { - // This should never happen, but if it does: - fatalError("No servers available for \(newNetwork)") - } + guard let server = availableServers(network: newNetwork).first else { + await MainActor.run { + walletClient.appError = MondayError.custom(message: "No servers available for \(newNetwork)") + walletClient.appState = .error + } + return + }Likely invalid or redundant comment.
|
@danielnordh code looks good so no hold up there. 1 last thing is seeing if you can reproduce what I think is a bug? Reproduce:
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-01-29.at.19.46.06.mp4 |
Very good catch. Fixed in 635a7dd |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
LDKNodeMonday/App/LDKNodeMondayApp.swift (2)
25-42: Consider encapsulatinglightningClientaccess.While the view hierarchy is well-structured, directly accessing
walletClient.lightningClientmight expose internal implementation details. Consider providing a dedicated method or property wrapper inWalletClientfor lightning-related operations.Example approach:
-lightningClient: walletClient.lightningClient +lightningClient: walletClient.getLightningService()
59-62: Add nil check for safer cleanup.While the cleanup is handled correctly, adding a nil check would make the code more robust.
- var walletClient: WalletClient? + weak var walletClient: WalletClient? func applicationWillTerminate(_ application: UIApplication) { - walletClient?.stop() + if let client = walletClient { + client.stop() + } }Also consider marking the
walletClientproperty asweakto avoid potential retain cycles.LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (2)
14-30: Consider improving thread safety and nil handling.The current singleton implementation could be enhanced:
- Replace force unwrapping with optional binding
- Use
OSAllocatedUnfairLock(iOS 16+) or dispatch_once pattern for better performance- Add memory barrier between check and initialization
- private static let lock = NSLock() - private static var _shared: LightningNodeService? - static var shared: LightningNodeService { - get { - lock.lock() - defer { lock.unlock() } - if _shared == nil { - _shared = LightningNodeService() - } - return _shared! - } - set { - lock.lock() - _shared = newValue - lock.unlock() - } - } + private static let lock = OSAllocatedUnfairLock() + private static var _shared: LightningNodeService? + static var shared: LightningNodeService { + get { + lock.withLock { + if _shared == nil { + _shared = LightningNodeService() + } + guard let instance = _shared else { + fatalError("Failed to initialize LightningNodeService") + } + return instance + } + } + set { + lock.withLock { + _shared = newValue + } + } + }
433-433: Enhance mock implementation for better testing.The mock implementation of
listenForEventsis empty. Consider simulating events for better test coverage.- listenForEvents: {} + listenForEvents: { + NotificationCenter.default.post( + name: .ldkEventReceived, + object: "MOCK_EVENT: Channel opened" + ) + }Also applies to: 489-492
LDKNodeMonday/View/Settings/SettingsView.swift (3)
24-42: LGTM! Consider enhancing accessibility for network badge.The Wallet section implementation looks good with proper integration of
WalletClientfor network management.Consider adding an accessibility label for the network badge to improve VoiceOver experience:
Label("Network", systemImage: "network") .badge((viewModel.network ?? "No network").capitalized) + .accessibilityLabel("Network: \((viewModel.network ?? "No network").capitalized)")
54-67: Enhance copy functionality with better feedback and error handling.The copy button implementation could be improved for better user experience and reliability.
Consider these enhancements:
Button { + do { UIPasteboard.general.string = viewModel.nodeID + UINotificationFeedbackGenerator().notificationOccurred(.success) showCheckmark = true - DispatchQueue.main.asyncAfter(deadline: .now() + 2) { + DispatchQueue.main.asyncAfter(deadline: .now() + 3) { showCheckmark = false } + } catch { + UINotificationFeedbackGenerator().notificationOccurred(.error) + } } label: {
167-173: Enhance preview with documentation and multiple states.Consider adding multiple preview states to help visualize different scenarios.
Here's a suggested enhancement:
#if DEBUG + /// Preview states for SettingsView #Preview { - SettingsView( - viewModel: .init( - walletClient: .constant(WalletClient(keyClient: KeyClient.mock)), - lightningClient: .mock - ) - ) + Group { + // Normal state + SettingsView( + viewModel: .init( + walletClient: .constant(WalletClient(keyClient: KeyClient.mock)), + lightningClient: .mock + ) + ) + .previewDisplayName("Normal State") + + // Loading state + SettingsView( + viewModel: .init( + walletClient: .constant(WalletClient(keyClient: KeyClient.mock)), + lightningClient: .mock, + isStoppingNode: true + ) + ) + .previewDisplayName("Loading State") + } } #endifLDKNodeMonday/View/Home/NetworkSettingsView.swift (3)
13-16: Consider renaming temporary state variables for clarity.While the temporary state pattern was previously approved, consider renaming these variables to better reflect their purpose:
- @State private var tempNetwork: Network? - @State private var tempServer: EsploraServer? + @State private var pendingNetwork: Network? + @State private var pendingServer: EsploraServer?
25-41: Extract network selection logic for better readability.The binding logic is complex and could be moved to a separate method for better maintainability.
+ private func handleNetworkSelection(_ newNetwork: Network) { + if walletClient.appState == .onboarding { + walletClient.network = newNetwork + guard let server = availableServers(network: newNetwork).first else { + fatalError("No servers available for \(newNetwork)") + } + walletClient.server = server + } else { + tempNetwork = newNetwork + showRestartAlert = true + } + } + Picker( "Network", - selection: Binding( - get: { walletClient.network }, - set: { newNetwork in - if walletClient.appState == .onboarding { - walletClient.network = newNetwork - guard let server = availableServers(network: newNetwork).first else { - fatalError("No servers available for \(newNetwork)") - } - walletClient.server = server - } else { - tempNetwork = newNetwork - showRestartAlert = true - } - } - ) + selection: Binding( + get: { walletClient.network }, + set: handleNetworkSelection + )
51-61: Extract server selection logic for consistency.For consistency with the network picker refactor, the server selection logic should also be extracted.
+ private func handleServerSelection(_ newServer: EsploraServer) { + if walletClient.appState == .onboarding { + walletClient.server = newServer + } else { + tempServer = newServer + showRestartAlert = true + } + } + Picker( "Server", - selection: Binding( - get: { walletClient.server }, - set: { newServer in - if walletClient.appState == .onboarding { - walletClient.server = newServer - } else { - tempServer = newServer - showRestartAlert = true - } - } - ) + selection: Binding( + get: { walletClient.server }, + set: handleServerSelection + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
LDKNodeMonday.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)LDKNodeMonday/App/LDKNodeMondayApp.swift(1 hunks)LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift(9 hunks)LDKNodeMonday/View/Home/NetworkSettingsView.swift(1 hunks)LDKNodeMonday/View/Settings/SettingsView.swift(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- LDKNodeMonday.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
🧰 Additional context used
📓 Learnings (3)
LDKNodeMonday/View/Settings/SettingsView.swift (1)
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/View/Settings/SettingsView.swift:118-123
Timestamp: 2025-01-28T15:46:14.253Z
Learning: In the LDKNodeMonday app's `WalletClient.delete()` function, errors during wallet deletion are handled through a do-catch block that updates `appError` with the caught error and sets `appState` to `.error`, with all state updates properly dispatched to the main actor.
LDKNodeMonday/App/LDKNodeMondayApp.swift (3)
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/App/LDKNodeMondayApp.swift:47-52
Timestamp: 2025-01-29T19:03:13.205Z
Learning: Error handling for wallet operations (start, create, restart, delete) is managed within the WalletClient class, which updates both appError and appState properties when errors occur. Additional error handling in LDKNodeMondayApp is not needed.
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/View/Settings/SettingsView.swift:118-123
Timestamp: 2025-01-28T15:46:14.253Z
Learning: In the LDKNodeMonday app, errors during wallet deletion are handled directly within the `WalletClient.delete()` function by updating `appError` and `appState`.
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/View/Settings/SettingsView.swift:118-123
Timestamp: 2025-01-28T15:46:14.253Z
Learning: In the LDKNodeMonday app's `WalletClient.delete()` function, errors during wallet deletion are handled through a do-catch block that updates `appError` with the caught error and sets `appState` to `.error`, with all state updates properly dispatched to the main actor.
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (1)
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift:150-155
Timestamp: 2025-01-28T15:52:09.651Z
Learning: In LightningNodeService.swift, cleanup operations like deleteDocuments() and deleteWallet() are intentionally kept as separate functions rather than being coupled with reset() to allow more granular control over when cleanup happens.
🔇 Additional comments (4)
LDKNodeMonday/App/LDKNodeMondayApp.swift (2)
15-19: LGTM! Clean state management setup.The initialization of
WalletClientand its integration withAppDelegatefollows SwiftUI best practices for state management and dependency injection.
47-51: LGTM! Clean state change handling.The implementation properly handles navigation path resets on state changes and wallet initialization follows SwiftUI's task pattern. Error handling is correctly delegated to
WalletClientas per the established pattern.LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (1)
149-162: Implementation looks good!The restart and reset methods are well-implemented with proper error handling and clear separation of concerns. The decision to keep cleanup operations separate (as mentioned in the learnings) provides good flexibility.
LDKNodeMonday/View/Home/NetworkSettingsView.swift (1)
128-128: LGTM! Preview implementation follows best practices.The preview correctly uses a mock KeyClient for testing.
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
LDKNodeMonday/View/Home/NetworkSettingsView.swift (1)
25-41:⚠️ Potential issueReplace fatalError with proper error handling.
Based on the established guidelines for the Monday app, fatal errors should be avoided in production code.
- guard let server = availableServers(network: newNetwork).first - else { - // This should never happen, but if it does: - fatalError("No servers available for \(newNetwork)") - } + guard let server = availableServers(network: newNetwork).first + else { + walletClient.appError = MondayError.custom(message: "No servers available for \(newNetwork)") + walletClient.appState = .error + return + }
🧹 Nitpick comments (2)
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (2)
41-52: Consider using Result type for error handling.The current implementation uses optional try and force unwrapping which could lead to runtime crashes.
Consider using a
Resulttype for more robust error handling:- if let backupInfo = try? KeyClient.live.getBackupInfo() { - guard let network = Network(stringValue: backupInfo.networkString) else { - fatalError("Configuration error: No Network found in BackupInfo") - } + let result = Result { try KeyClient.live.getBackupInfo() } + switch result { + case .success(let backupInfo): + guard let network = Network(stringValue: backupInfo.networkString) else { + logger.error("Invalid network string: \(backupInfo.networkString)") + self.network = .signet + self.server = .mutiny_signet + return + }
333-337: Consider documenting the LightningNodeClient interface.The public interface looks good, but could benefit from documentation explaining the purpose of each method, especially the new ones.
Add documentation for the new methods:
public struct LightningNodeClient { + /// Starts the Lightning node service let start: () async throws -> Void + /// Stops the Lightning node service let stop: () throws -> Void + /// Restarts the Lightning node service by stopping it, resetting the shared instance, and starting it again let restart: () async throws -> Void + /// Resets the Lightning node service by stopping it and clearing the shared instance let reset: () throws -> VoidAlso applies to: 361-361, 370-371, 433-435, 444-445, 491-493
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift(9 hunks)LDKNodeMonday/View/Home/NetworkSettingsView.swift(1 hunks)
🧰 Additional context used
📓 Learnings (2)
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (1)
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift:150-155
Timestamp: 2025-01-28T15:52:09.651Z
Learning: In LightningNodeService.swift, cleanup operations like deleteDocuments() and deleteWallet() are intentionally kept as separate functions rather than being coupled with reset() to allow more granular control over when cleanup happens.
LDKNodeMonday/View/Home/NetworkSettingsView.swift (1)
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/View/Home/NetworkSettingsView.swift:30-33
Timestamp: 2025-01-29T19:07:21.325Z
Learning: In the Monday app, fatal errors should be avoided in production code. Instead, errors should be presented to users by setting walletClient.appError and transitioning to walletClient.appState = .error.
🔇 Additional comments (7)
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (3)
14-30: Thread-safe singleton implementation looks good.The implementation correctly uses
NSLockto ensure thread safety when accessing the shared instance.
159-164: LGTM: reset() implementation aligns with design.The
reset()method is intentionally kept separate from cleanup operations as learned from previous feedback.
151-157: Verify restart() implementation.The
restart()method looks good but let's verify its usage in the codebase to ensure all callers handle the async/throws correctly.✅ Verification successful
restart() implementation and usage verified successfully
All callers properly handle the async/throws nature of the method with correct error propagation and state management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for restart() calls to verify error handling rg -A 5 'restart\(\)' --type swiftLength of output: 1949
LDKNodeMonday/View/Home/NetworkSettingsView.swift (4)
13-16: LGTM! Property declarations are well-structured.The binding to walletClient and state variables for managing temporary network settings are appropriate for this use case.
51-61: LGTM! Server picker binding is well-implemented.The conditional logic properly handles both onboarding and post-onboarding scenarios.
77-86: LGTM! Alert implementation is clear and user-friendly.The alert properly informs users about the restart requirement and provides clear actions.
63-66: Verify handling of empty server list.Please ensure that the UI gracefully handles scenarios where
availableServersreturns an empty list.
This PR enables network and server switching after onboarding.
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-01-28.at.14.31.15.mp4
Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the updated release notes:
New Features
WalletClientto manage wallet operations.Improvements
AppStatewithWalletClient.Changes
WalletClientfor state management.Bug Fixes
Dependency Updates
ldk-nodedependency from version0.4.2to0.4.3.