Refactor: Save network and server into BackupInfo #154
Conversation
|
Warning Rate limit exceeded@danielnordh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces significant updates to the backup and configuration management in the LDK Node application. Enhancements to the Changes
Sequence DiagramsequenceDiagram
participant User
participant OnboardingViewModel
participant KeyClient
participant BackupInfo
participant LightningNodeService
User->>OnboardingViewModel: Select Network & Server
OnboardingViewModel->>KeyClient: saveNetwork(networkString)
OnboardingViewModel->>KeyClient: saveServerURL(serverURL)
OnboardingViewModel->>BackupInfo: Create with mnemonic, network, server
BackupInfo->>LightningNodeService: Initialize with backup info
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
LDKNodeMonday/Model/BackupInfo.swift (2)
Line range hint
22-24: Review Equatable implementation for completeness.The current implementation only compares the
mnemonicproperty. Consider includingnetworkStringandserverURLin the comparison for a complete equality check.- return lhs.mnemonic == rhs.mnemonic + return lhs.mnemonic == rhs.mnemonic && + lhs.networkString == rhs.networkString && + lhs.serverURL == rhs.serverURL🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 16-16: This memberwise initializer would be synthesized automatically - you do not need to define it
(unneeded_synthesized_initializer)
16-20: Consider removing explicit initializer.The memberwise initializer would be synthesized automatically with the same implementation.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 16-16: This memberwise initializer would be synthesized automatically - you do not need to define it
(unneeded_synthesized_initializer)
LDKNodeMonday/Service/KeyService/KeyService.swift (1)
44-47: Consider atomic updates for BackupInfo modifications.The current implementation reads and then writes BackupInfo, which could lead to race conditions if multiple updates occur simultaneously. Consider implementing atomic updates or adding synchronization.
Also applies to: 55-58
LDKNodeMonday/Utilities/Constants.swift (1)
136-151: Improve URL handling in EsploraServer initializer.While the implementation works, consider these improvements:
- Use static property URLs instead of hardcoding to prevent maintenance issues
- Add URL normalization to handle variations (e.g., trailing slashes)
extension EsploraServer { init?(URLString: String) { + // Normalize URL by removing trailing slashes + let normalizedURL = URLString.trimmingCharacters(in: CharacterSet(charactersIn: "/")) + switch URLString { - case "https://blockstream.info/api": self = .blockstream_bitcoin - case "https://mempool.space/api": self = .mempoolspace_bitcoin - case "https://mutinynet.com/api": self = .mutiny_signet - case "http://signet.bitcoindevkit.net": self = .bdk_signet - case "https://mutinynet.ltbl.io/api": self = .lqwd_signet - case "http://127.0.0.1:3002": self = .local_regtest - case "http://blockstream.info/testnet/api": self = .blockstream_testnet - case "https://esplora.testnet.kuutamo.cloud": self = .kuutamo_testnet - case "https://mempool.space/testnet/api": self = .mempoolspace_testnet + case EsploraServer.blockstream_bitcoin.url: self = .blockstream_bitcoin + case EsploraServer.mempoolspace_bitcoin.url: self = .mempoolspace_bitcoin + case EsploraServer.mutiny_signet.url: self = .mutiny_signet + case EsploraServer.bdk_signet.url: self = .bdk_signet + case EsploraServer.lqwd_signet.url: self = .lqwd_signet + case EsploraServer.local_regtest.url: self = .local_regtest + case EsploraServer.blockstream_testnet.url: self = .blockstream_testnet + case EsploraServer.kuutamo_testnet.url: self = .kuutamo_testnet + case EsploraServer.mempoolspace_testnet.url: self = .mempoolspace_testnet default: return nil } } }LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (2)
19-19: Improve error handling in initialization.The current implementation silently handles errors with
try?and performs multiple separate error-prone operations. Consider consolidating the error handling and providing meaningful feedback.var network: Network var server: EsploraServer init( keyService: KeyClient = .live - ) { - let backupInfo = try? KeyClient.live.getBackupInfo() - self.network = Network(stringValue: backupInfo?.networkString ?? Network.signet.description) ?? .signet - self.server = EsploraServer(URLString: backupInfo?.serverURL ?? EsploraServer.mutiny_signet.url) ?? .mutiny_signet + ) throws { + do { + if let backupInfo = try keyService.getBackupInfo() { + self.network = Network(stringValue: backupInfo.networkString) ?? .signet + self.server = EsploraServer(URLString: backupInfo.serverURL) ?? .mutiny_signet + } else { + self.network = .signet + self.server = .mutiny_signet + } + } catch { + logger.error("Failed to initialize from backup: \(error.localizedDescription)") + self.network = .signet + self.server = .mutiny_signet + }Also applies to: 25-27
84-85: Deduplicate backup info creation logic.The BackupInfo creation logic is duplicated. Consider extracting it into a helper method.
+ private func createBackupInfo(mnemonic: String) -> BackupInfo { + return BackupInfo( + mnemonic: mnemonic, + networkString: self.network.description, + serverURL: self.server.url + ) + } + // In the init method: - let backupInfo = BackupInfo(mnemonic: newMnemonic, networkString: self.network.description, serverURL: self.server.url) + let backupInfo = createBackupInfo(mnemonic: newMnemonic)Also applies to: 92-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
LDKNodeMonday/Model/BackupInfo.swift(2 hunks)LDKNodeMonday/Service/KeyService/KeyService.swift(4 hunks)LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift(5 hunks)LDKNodeMonday/Utilities/Constants.swift(1 hunks)LDKNodeMonday/View Model/Home/OnboardingViewModel.swift(4 hunks)LDKNodeMonday/View Model/Profile/Danger/SeedViewModel.swift(1 hunks)LDKNodeMonday/View Model/Profile/SettingsViewModel.swift(2 hunks)LDKNodeMonday/View/Settings/SettingsView.swift(1 hunks)
🧰 Additional context used
📓 Learnings (1)
LDKNodeMonday/View Model/Profile/Danger/SeedViewModel.swift (1)
Learnt from: danielnordh
PR: reez/Monday#143
File: LDKNodeMonday/View/Settings/Danger/SeedView.swift:44-49
Timestamp: 2024-12-18T16:56:17.814Z
Learning: In SwiftUI for the LDKNodeMonday project, additional BIP39 validation for the seed mnemonic isn't strictly necessary because the node won't start unless the seed is valid.
🪛 SwiftLint (0.57.0)
LDKNodeMonday/Model/BackupInfo.swift
[Warning] 16-16: This memberwise initializer would be synthesized automatically - you do not need to define it
(unneeded_synthesized_initializer)
🔇 Additional comments (9)
LDKNodeMonday/View Model/Profile/Danger/SeedViewModel.swift (1)
13-13: LGTM!The initialization of
seedwith mock values for all required properties is correct.LDKNodeMonday/View Model/Profile/SettingsViewModel.swift (1)
Line range hint
71-84: Verify complete data cleanup in delete method.The method deletes documents and wallet but should also clean up the BackupInfo to ensure complete data removal.
✅ Verification successful
BackupInfo cleanup is properly implemented
The code already handles BackupInfo cleanup during wallet deletion. The
deleteWallet()implementation in LightningNodeService callskeyService.deleteBackupInfo(), which removes the BackupInfo from the keychain storage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if BackupInfo is properly cleaned up during deletion rg -A 5 "func.*delete.*BackupInfo"Length of output: 447
Script:
#!/bin/bash # Check deleteWallet implementation and BackupInfo usages echo "=== Searching for deleteWallet implementation ===" ast-grep --pattern 'func deleteWallet() { $$$ }' echo -e "\n=== Searching for BackupInfo references ===" rg "BackupInfo" -A 3 echo -e "\n=== Searching for KeyService usage in wallet deletion context ===" rg -B 3 -A 3 "deleteWallet.*keyService|keyService.*deleteWallet"Length of output: 17252
LDKNodeMonday/Service/KeyService/KeyService.swift (2)
74-92: LGTM! Clean API design.The KeyClient interface is well-designed with clear separation of concerns and consistent method signatures.
113-119: LGTM! Comprehensive mock implementation.The mock implementation correctly handles all new properties and methods.
LDKNodeMonday/View Model/Home/OnboardingViewModel.swift (2)
43-43: LGTM!The method name change from
saveEsploraURLtosaveServerURLaligns with the PR objective of consolidating server information.
111-111: LGTM!The updated BackupInfo constructor properly captures both network and server information, aligning with the PR objective.
LDKNodeMonday/View/Settings/SettingsView.swift (1)
42-42: LGTM!The method name change is consistent with the PR's objective of consolidating server information.
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (2)
52-52: LGTM!Properly uses the server URL from the new server property.
414-414: LGTM!The mock implementation correctly includes the new BackupInfo parameters.
| availableEsploraServers.first! // all networks have at least one server option | ||
| try keyClient.saveNetwork(selectedNetwork.description) |
There was a problem hiding this comment.
Replace force unwrap with safe default value.
While the comment indicates that all networks should have at least one server option, it's safer to provide a default value instead of force unwrapping to prevent potential crashes.
- availableEsploraServers.first! // all networks have at least one server option
+ availableEsploraServers.first ?? EsploraServer.mutiny_signet // Default to mutiny_signet if no servers availableCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
In this case it is questionable, as providing an EsploraServer for the wrong network will cause the application to not work.
There was a problem hiding this comment.
@danielnordh You make an excellent point. Using a default server from a different network would indeed be worse than failing fast with a force unwrap, as it could lead to silent failures or incorrect behavior. Since the available servers are determined by the network type at compile-time, the force unwrap is actually a reasonable choice here.
✏️ Learnings added
Learnt from: danielnordh
PR: reez/Monday#154
File: LDKNodeMonday/View Model/Home/OnboardingViewModel.swift:27-28
Timestamp: 2025-01-20T13:26:25.637Z
Learning: In the Monday app, Esplora servers must match their respective networks (bitcoin, testnet, signet, regtest). Providing a server from a different network as a fallback would cause the application to malfunction, making it preferable to fail fast rather than using an incompatible server.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
I see what you're saying here Daniel but I'd probably still like to avoid the force unwrap, maybe we can even just do something as simple as
| availableEsploraServers.first! // all networks have at least one server option | |
| try keyClient.saveNetwork(selectedNetwork.description) | |
| guard let server = availableEsploraServers.first else { | |
| // This should never happen, but if it does: | |
| fatalError("Configuration error: No Esplora servers available for \(selectedNetwork)") | |
| } | |
| self.selectedEsploraServer = server |
because that way we will still get the same crash a force unwrap gives us but maybe it will also give us a little extra information and better debugging along with it that we wouldnt get with the force unwrap (which would probably just give us a generic ""unexpectedly found nil" )
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Addressed as suggested.
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift
Outdated
Show resolved
Hide resolved
| func saveNetwork(networkString: String) throws { | ||
| let currentBackupInfo = try self.getBackupInfo() | ||
| let newBackupInfo = BackupInfo(mnemonic: currentBackupInfo.mnemonic, networkString: networkString, serverURL: currentBackupInfo.serverURL) | ||
| try self.saveBackupInfo(backupInfo: newBackupInfo) | ||
| } | ||
|
|
||
| func getNetwork() throws -> String? { | ||
| return keychain[string: "SelectedNetwork"] | ||
| let backupInfo = try self.getBackupInfo() | ||
| return backupInfo.networkString | ||
| } | ||
|
|
||
| func deleteNetwork() throws { | ||
| try keychain.remove("SelectedNetwork") | ||
| func saveServerURL(url: String) throws { | ||
| let currentBackupInfo = try self.getBackupInfo() | ||
| let newBackupInfo = BackupInfo(mnemonic: currentBackupInfo.mnemonic, networkString: currentBackupInfo.networkString, serverURL: url) | ||
| try self.saveBackupInfo(backupInfo: newBackupInfo) |
There was a problem hiding this comment.
for saveNetwork and saveServerURL does it make sense for us to just have one saveBackupInfo method instead now since all of those methods under the hood are saving the whole BackupInfo (and not just specific parts like network alone anymore)?
Not a blocker to this PR, but something I'd at least like your thoughts on to update in this PR or follow up with a PR about if it avoids the redundant loads/saves (even though the tradeoff is very slightly more verbose call sites) potentially. Thoughts?
There was a problem hiding this comment.
saveServer and saveNetwork are handy for the situations where we are (and will be, in the network switch PR) have UI for selecting the server and network independent of the mnemonic. There are moments where only one of them is changed, or there some time before setting both.
It saves bringing the mnemonic out in code places where it is only retrieved to be used to save one of the other values. I felt it made sense to keep the saveNetwork() and saveServer() functions that existed rather than complicate things further.
But if you have specific suggestions for optimisation that would be good.
| let deleteNetwork: () throws -> Void | ||
| let deleteEsplora: () throws -> Void |
There was a problem hiding this comment.
Did we move the deleteNetwork and deleteEsplora functionality to somewhere else or are they just not needed at all anymore?
There was a problem hiding this comment.
I removed them as they are not used anywhere anymore after removing the 'Full reset' option.
If we want to bring back the option to 'reset the node', I believe it is better to do that by deleting the documents folder containing the node on the network in question. 'Deleting' a network doesn't make that much sense when it is a required parameter of BackupInfo.
saveNetwork, or the default on node start should cover the use cases.
| availableEsploraServers.first! // all networks have at least one server option | ||
| try keyClient.saveNetwork(selectedNetwork.description) |
There was a problem hiding this comment.
I see what you're saying here Daniel but I'd probably still like to avoid the force unwrap, maybe we can even just do something as simple as
| availableEsploraServers.first! // all networks have at least one server option | |
| try keyClient.saveNetwork(selectedNetwork.description) | |
| guard let server = availableEsploraServers.first else { | |
| // This should never happen, but if it does: | |
| fatalError("Configuration error: No Esplora servers available for \(selectedNetwork)") | |
| } | |
| self.selectedEsploraServer = server |
because that way we will still get the same crash a force unwrap gives us but maybe it will also give us a little extra information and better debugging along with it that we wouldnt get with the force unwrap (which would probably just give us a generic ""unexpectedly found nil" )
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
LDKNodeMonday/Model/BackupInfo.swift (1)
Line range hint
22-24: Update Equatable implementation to compare all properties.The current implementation only compares
mnemonic, ignoringnetworkStringandserverURL. This could lead to bugs where BackupInfo instances with different networks or servers are considered equal.static func == (lhs: BackupInfo, rhs: BackupInfo) -> Bool { - return lhs.mnemonic == rhs.mnemonic + return lhs.mnemonic == rhs.mnemonic && + lhs.networkString == rhs.networkString && + lhs.serverURL == rhs.serverURL }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 16-16: This memberwise initializer would be synthesized automatically - you do not need to define it
(unneeded_synthesized_initializer)
🧹 Nitpick comments (2)
LDKNodeMonday/Model/BackupInfo.swift (1)
16-20: Remove redundant initializer.The memberwise initializer would be synthesized automatically by Swift - you don't need to define it explicitly.
- init(mnemonic: String, networkString: String, serverURL: String) { - self.mnemonic = mnemonic - self.networkString = networkString - self.serverURL = serverURL - }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 16-16: This memberwise initializer would be synthesized automatically - you do not need to define it
(unneeded_synthesized_initializer)
LDKNodeMonday/Service/KeyService/KeyService.swift (1)
45-46: Consider caching BackupInfo to reduce keychain reads.Multiple methods read from the keychain to get the current
BackupInfo. Consider caching theBackupInfoin memory to reduce keychain access overhead.private struct KeyService { private let keychain: Keychain + private var cachedBackupInfo: BackupInfo? // ... existing code ... func getBackupInfo() throws -> BackupInfo { + if let cached = cachedBackupInfo { + return cached + } guard let encryptedJsonData = try keychain.getData("BackupInfo") else { throw KeyServiceError.readError } let decoder = JSONDecoder() let backupInfo = try decoder.decode(BackupInfo.self, from: encryptedJsonData) + cachedBackupInfo = backupInfo return backupInfo } func saveBackupInfo(backupInfo: BackupInfo) throws { let encoder = JSONEncoder() let data = try encoder.encode(backupInfo) keychain[data: "BackupInfo"] = data + cachedBackupInfo = backupInfo } func deleteBackupInfo() throws { try keychain.remove("BackupInfo") + cachedBackupInfo = nil }Also applies to: 60-61
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
LDKNodeMonday/Model/BackupInfo.swift(2 hunks)LDKNodeMonday/Service/KeyService/KeyService.swift(4 hunks)LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift(5 hunks)LDKNodeMonday/View Model/Home/OnboardingViewModel.swift(4 hunks)LDKNodeMonday/View Model/Profile/Danger/SeedViewModel.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- LDKNodeMonday/View Model/Profile/Danger/SeedViewModel.swift
- LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift
- LDKNodeMonday/View Model/Home/OnboardingViewModel.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
LDKNodeMonday/Model/BackupInfo.swift
[Warning] 16-16: This memberwise initializer would be synthesized automatically - you do not need to define it
(unneeded_synthesized_initializer)
🔇 Additional comments (1)
LDKNodeMonday/Service/KeyService/KeyService.swift (1)
44-51: Consider consolidating save methods to reduce redundancy.The
saveNetworkandsaveServerURLmethods follow the same pattern and both save the entireBackupInfo. Consider consolidating them into a singleupdateBackupInfomethod to avoid redundant loads/saves.- func saveNetwork(networkString: String) throws { - let currentBackupInfo = try self.getBackupInfo() - let newBackupInfo = BackupInfo( - mnemonic: currentBackupInfo.mnemonic, - networkString: networkString, - serverURL: currentBackupInfo.serverURL - ) - try self.saveBackupInfo(backupInfo: newBackupInfo) - } - - func saveServerURL(url: String) throws { - let currentBackupInfo = try self.getBackupInfo() - let newBackupInfo = BackupInfo( - mnemonic: currentBackupInfo.mnemonic, - networkString: currentBackupInfo.networkString, - serverURL: url - ) - try self.saveBackupInfo(backupInfo: newBackupInfo) - } + func updateBackupInfo( + networkString: String? = nil, + serverURL: String? = nil + ) throws { + let currentBackupInfo = try self.getBackupInfo() + let newBackupInfo = BackupInfo( + mnemonic: currentBackupInfo.mnemonic, + networkString: networkString ?? currentBackupInfo.networkString, + serverURL: serverURL ?? currentBackupInfo.serverURL + ) + try self.saveBackupInfo(backupInfo: newBackupInfo) + }Also applies to: 59-66
| func getNetwork() throws -> String? { | ||
| return keychain[string: "SelectedNetwork"] | ||
| } | ||
|
|
||
| func deleteNetwork() throws { | ||
| try keychain.remove("SelectedNetwork") | ||
| let backupInfo = try self.getBackupInfo() | ||
| return backupInfo.networkString | ||
| } |
There was a problem hiding this comment.
Add error handling for missing backup info.
The get methods should handle the case where getBackupInfo() fails by propagating appropriate errors rather than potentially crashing.
func getNetwork() throws -> String? {
- let backupInfo = try self.getBackupInfo()
- return backupInfo.networkString
+ do {
+ let backupInfo = try self.getBackupInfo()
+ return backupInfo.networkString
+ } catch {
+ throw KeyServiceError.readError
+ }
}
func getServerURL() throws -> String? {
- let backupInfo = try self.getBackupInfo()
- return backupInfo.serverURL
+ do {
+ let backupInfo = try self.getBackupInfo()
+ return backupInfo.serverURL
+ } catch {
+ throw KeyServiceError.readError
+ }
}Also applies to: 69-71
Refactor Co-authored-by: Matthew Ramsden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
LDKNodeMonday/Utilities/Constants.swift (1)
Update Blockstream testnet URL to use HTTPS protocol
The Blockstream testnet API enforces HTTPS through redirects. Update the URL to use HTTPS directly:
static let blockstream_testnet = EsploraServer( name: "Blockstream", url: "https://blockstream.info/testnet/api" )Additionally:
- The BDK signet endpoint (
signet.bitcoindevkit.net) appears to be unreachable - you may want to verify its current status- HTTP is acceptable for the local development endpoint (
127.0.0.1)🔗 Analysis chain
Line range hint
104-135: Consider enforcing HTTPS for production endpoints.Several URLs are using insecure HTTP protocol:
http://signet.bitcoindevkit.nethttp://127.0.0.1:3002http://blockstream.info/testnet/apiWhile HTTP is acceptable for local development (127.0.0.1), production endpoints should use HTTPS to ensure secure communication.
Run this script to verify the security of these endpoints:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if HTTP endpoints support HTTPS endpoints=( "signet.bitcoindevkit.net" "blockstream.info/testnet/api" ) for endpoint in "${endpoints[@]}"; do echo "Checking HTTPS support for: $endpoint" curl -sI "https://$endpoint" | head -n1 doneLength of output: 277
Script:
#!/bin/bash # Description: Enhanced check for HTTP/HTTPS support with detailed diagnostics endpoints=( "signet.bitcoindevkit.net" "blockstream.info/testnet/api" ) for endpoint in "${endpoints[@]}"; do echo "=== Testing $endpoint ===" echo "HTTP test:" curl -v --max-time 5 "http://$endpoint" 2>&1 | grep "< HTTP" echo -e "\nHTTPS test:" curl -v --max-time 5 "https://$endpoint" 2>&1 | grep "< HTTP" echo -e "\n" doneLength of output: 876
♻️ Duplicate comments (1)
LDKNodeMonday/Utilities/Constants.swift (1)
150-163: 🛠️ Refactor suggestionRefactor URL mapping to reduce duplication.
The current implementation duplicates URLs in both static properties and switch cases, which could lead to maintenance issues. Consider using the dictionary-based approach from
urlToServerinstead of the switch statement.init?(URLString: String) { - switch URLString { - case "https://blockstream.info/api": self = .blockstream_bitcoin - case "https://mempool.space/api": self = .mempoolspace_bitcoin - case "https://mutinynet.com/api": self = .mutiny_signet - case "http://signet.bitcoindevkit.net": self = .bdk_signet - case "https://mutinynet.ltbl.io/api": self = .lqwd_signet - case "http://127.0.0.1:3002": self = .local_regtest - case "http://blockstream.info/testnet/api": self = .blockstream_testnet - case "https://esplora.testnet.kuutamo.cloud": self = .kuutamo_testnet - case "https://mempool.space/testnet/api": self = .mempoolspace_testnet - default: return nil - } + guard let server = Self.urlToServer[URLString] else { return nil } + self = server }
🧹 Nitpick comments (1)
LDKNodeMonday/Utilities/Constants.swift (1)
166-177: Add documentation and consider type safety improvements.The function would benefit from documentation explaining its purpose and the relationship between networks and servers. Also consider using a non-empty array type to indicate that each network always has at least one server.
+/// Returns the list of available Esplora servers for the specified network. +/// Each network is guaranteed to have at least one server available. +/// +/// - Parameter network: The Bitcoin network for which to retrieve available servers +/// - Returns: An array of available Esplora servers for the specified network public func availableServers(network: Network) -> [EsploraServer] {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift(5 hunks)LDKNodeMonday/Utilities/Constants.swift(3 hunks)LDKNodeMonday/View Model/Home/OnboardingViewModel.swift(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift
- LDKNodeMonday/View Model/Home/OnboardingViewModel.swift
This PR saves network and server in BackupInfo instead of separately in the Keychain.
This should make it easier to implement restarts and network switching.
Replaces #153
Summary by CodeRabbit
New Features
Improvements
Changes