ux: send flow #171
ux: send flow #171
Conversation
WalkthroughThis pull request reorganizes and refines the project’s send/payment functionalities. Several source files are renamed or newly added to clarify their roles, particularly around sending payments. Changes include updates to event and address processing extensions, modifications in the Lightning service to configure liquidity and support Bolt11 transactions, and updates in view models and views for handling manual entry, QR scanning, and reviewing payments. Additionally, legacy files are removed, and UI components are updated to improve state management and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SendView
participant SendViewModel
participant LightningNodeService
participant EventService
User->>SendView: Initiates send payment flow
SendView->>SendViewModel: Updates state (scan, manual entry, review)
SendViewModel->>LightningNodeService: call sendBolt11Payment(invoice, params)
LightningNodeService-->>SendViewModel: Return PaymentId or error
SendViewModel->>EventService: Update lastEvent with transaction result
EventService-->>SendView: Trigger UI update (toast/alert)
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 7
🧹 Nitpick comments (30)
LDKNodeMonday/Model/PaymentType.swift (1)
10-15: Refined Enumeration Naming and CasesThe updated
PaymentTypeenum and its cases (lightning,lightningURL,bitcoin, andnone) are concise and improve clarity. Please ensure that any references to the oldPaymentenum are updated across the entire codebase. As a minor nitpick, consider updating the file header (line 2) to reflect the new file name ("PaymentType.swift") for consistency, if that change hasn’t been applied yet.LDKNodeMonday/View/Home/Payments/PaymentsListView.swift (1)
49-58: Consider extracting the filter logic to a separate function.The filter logic is complex and makes the ForEach declaration harder to read. Consider extracting it to a function with a descriptive name like
filteredAndSortedPayments().- ForEach( - payments - .filter { - $0.status != .pending || - ($0.status == .pending && Double($0.latestUpdateTimestamp) > Date().timeIntervalSince1970 - 1800 && ($0.amountMsat ?? 0) > 0) - } - .sorted { $0.latestUpdateTimestamp > $1.latestUpdateTimestamp }, - id: \.id - ) + ForEach(filteredAndSortedPayments(), id: \.id)Then add this function to the struct:
private func filteredAndSortedPayments() -> [PaymentDetails] { // Constants improve readability let pendingTimeoutSeconds: TimeInterval = 1800 // 30 minutes let currentTime = Date().timeIntervalSince1970 return payments .filter { $0.status != .pending || ($0.status == .pending && Double($0.latestUpdateTimestamp) > currentTime - pendingTimeoutSeconds && ($0.amountMsat ?? 0) > 0) } .sorted { $0.latestUpdateTimestamp > $1.latestUpdateTimestamp } }LDKNodeMonday/View/Home/Send/SendReviewView.swift (1)
39-60: Improve error handling for better UX.While the code checks for errors with
viewModel.sendError == nil, it doesn't display errors directly to the user in this view. Consider adding an alert or error message display when sending fails.Also, consider extracting the magic number
0.25to a named constant for better maintainability:- DispatchQueue.main.asyncAfter(deadline: .now() + 0.25) { + let animationDelay: Double = 0.25 + DispatchQueue.main.asyncAfter(deadline: .now() + animationDelay) {LDKNodeMonday/View/Home/Receive/AmountEntryView.swift (1)
85-118: Consider limiting maximum input length in NumpadButton.The NumpadButton implementation has good handling for different button types, but it doesn't limit the maximum number of digits that can be entered. This could potentially allow very large numbers that exceed UInt64 limits when converted.
Consider adding a check to prevent overflow:
} else { if numpadAmount == "0" { numpadAmount = character } else { + // Prevent entering too many digits (UInt64.max is 20 digits) + guard numpadAmount.count < 20 else { return } numpadAmount.append(character) } }Also, the space button handling could be more consistent with other conditions:
- } else if character == " " { - return + } else if character == " " { + // Do nothing for space characterLDKNodeMonday/View/Home/Send/SendView.swift (1)
90-98: Preview uses mock data with payment confirmation stateThe preview is currently set to the
.paymentSentstate, which only shows the final confirmation screen. Consider adding previews for other states or making the preview state configurable for more comprehensive preview testing.LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (3)
2-3: File header comment doesn't match class nameThe file header comment shows
AmountViewModel.swiftbut the actual class implemented isSendViewModel, indicating that the file was likely renamed without updating the header.-// AmountViewModel.swift +// SendViewModel.swift
53-58: Redundant main thread dispatch with @mainactorThe class is already annotated with
@MainActor, which ensures all code runs on the main thread, makingDispatchQueue.main.asyncredundant.- DispatchQueue.main.async { self.sendError = .init( title: errorString.title, detail: errorString.detail ) - }
61-66: Same redundant dispatch in the catch blockSimilar to the previous issue, this
DispatchQueue.main.asyncis redundant.- DispatchQueue.main.async { self.sendError = .init( title: "Unexpected error", detail: error.localizedDescription ) - }LDKNodeMonday/Service/EventService/EventService.swift (2)
12-12: Simplify optional initializationThe explicit initialization with
nilis redundant since optionals are nil by default.- @Published var lastEvent: Event? = nil + @Published var lastEvent: Event?🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 12-12: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
23-23: Fix commented-out code formattingThe code contains an inline comment with improper spacing.
- self?.lastEvent = nil //"\(notification.object.debugDescription)" + self?.lastEvent = nil // "\(notification.object.debugDescription)"🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 23-23: Prefer at least one space after slashes for comments
(comment_spacing)
LDKNodeMonday/View/Home/Send/SendManualEntry.swift (3)
62-75: Consider adding handling for invalid inputWhen extracting payment info, there should be error handling to provide user feedback when parsing fails but doesn't return nil.
let (extractedAmount, extractedPaymentAddress) = viewModel.address.extractPaymentInfo(spendableBalance: 0) if extractedPaymentAddress != nil && viewModel.paymentAddress == nil { viewModel.amountSat = extractedAmount viewModel.paymentAddress = extractedPaymentAddress if extractedAmount != 0 { withAnimation { viewModel.sendViewState = .reviewPayment } } +} else if viewModel.address.count > 0 && extractedPaymentAddress == nil { + // Provide feedback that the entered address couldn't be parsed + // This could be a status message or a subtle visual indicator }
78-105: Good conditional UI controls but consider confirmation before deletionThe toggle between scan button and delete button is well implemented. However, consider adding confirmation before deleting payment information, especially for larger amounts.
112-114: Consider combining onChange handlers for better organizationThere are two separate onChange handlers for the address field. Consider combining them for better code organization.
- .overlay( - RoundedRectangle(cornerRadius: 24) - .stroke(Color.accentColor, lineWidth: 2) - ).onChange(of: viewModel.address) { _, newValue in - viewModel.address = newValue.replacingOccurrences(of: " ", with: "") - } + .overlay( + RoundedRectangle(cornerRadius: 24) + .stroke(Color.accentColor, lineWidth: 2) + )And modify the earlier onChange handler:
.onChange(of: viewModel.address) { + // Remove spaces from input + let cleanedAddress = viewModel.address.replacingOccurrences(of: " ", with: "") + if cleanedAddress != viewModel.address { + viewModel.address = cleanedAddress + return + } + let (extractedAmount, extractedPaymentAddress) = - viewModel.address.extractPaymentInfo(spendableBalance: 0) + cleanedAddress.extractPaymentInfo(spendableBalance: 0)LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (3)
80-81: Peer configuration change should include documentationThe trusted peers configuration has been updated from
lqwdtomutinywithout explaining the rationale. Adding a comment would help future developers understand the change.
103-106: Document the reason for changing the liquidity sourceSimilar to the trusted peers change, the liquidity source has been updated from
lqwdtomutinywithout explanation. This change should be documented.
495-496: Mock implementation needs realistic dataThe mock implementation returns an empty string as the payment ID. Consider using a realistic mock value instead.
- sendBolt11Payment: { _, _ in PaymentId("") }, + sendBolt11Payment: { _, _ in PaymentId("mock_payment_12345") },LDKNodeMonday/Extensions/Event+Extensions.swift (4)
14-31: Good title mapping, but consider handling all event typesThe
titleproperty provides clear, user-friendly labels for events. However, the default case returns an empty string, which might not be ideal for UI elements.default: - return "" + return "Event Notification" // Or use a generic title derived from the event type
82-89: Consider expanding amount reporting to other payment typesThe
amountproperty only returns a value forpaymentReceivedevents. Consider extending this to include amounts for other payment-related events.public var amount: UInt64 { switch self { case .paymentReceived(_, _, let amountMsat): return amountMsat.mSatsAsSats + case .paymentSuccessful(_, _, let amountMsat): + return amountMsat?.mSatsAsSats ?? 0 + case .paymentFailed(_, _, _): + return 0 // Could extract amount if available in the event + case .paymentClaimable(_, _, let amountMsat, _): + return amountMsat.mSatsAsSats default: return 0 }
92-137: Good implementation of EventItemView, but needs timestamp handlingThe
EventItemViewis well-structured, but displays "Just now" for all events rather than showing the actual timestamp.Consider adding a timestamp property and formatter:
struct EventItemView: View { var event: Event? //var displayBalanceType: DisplayBalanceType var price: Double + var timestamp: Date? + + // Format relative time - "2 mins ago", "Just now", etc. + private var formattedTime: String { + guard let timestamp = timestamp else { return "Just now" } + // Calculate time difference and return appropriate string + let interval = Date().timeIntervalSince(timestamp) + if interval < 60 { + return "Just now" + } else if interval < 3600 { + return "\(Int(interval / 60)) mins ago" + } else if interval < 86400 { + return "\(Int(interval / 3600)) hours ago" + } else { + return "\(Int(interval / 86400)) days ago" + } + } var body: some View { // ... - Text("Just now") + Text(formattedTime) .font(.system(.subheadline, design: .rounded)) .foregroundColor(.secondary) // ... } }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 94-94: Prefer at least one space after slashes for comments
(comment_spacing)
94-94: Remove or implement commented codeThere's a commented property
displayBalanceTypethat should either be implemented or removed.- //var displayBalanceType: DisplayBalanceType🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 94-94: Prefer at least one space after slashes for comments
(comment_spacing)
LDKNodeMonday/View/Home/Send/SendScanAddressView.swift (3)
57-60: Provide more informative error messagesThe alert message "Unsupported scan format" doesn't give the user enough information about what went wrong or how to fix it.
- alertMessage = "Unsupported scan format" + alertMessage = "The scanned QR code format is not supported. Please scan a Bitcoin or Lightning payment address." isShowingAlert = true
87-90: Add more descriptive error message for paste actionSimilar to the scan error message, the paste error message could be more informative.
- alertMessage = "Unsupported paste format" + alertMessage = "The pasted text is not a valid Bitcoin or Lightning payment address." isShowingAlert = true
123-127: Consider adapting scanner UI to support dark modeThe scan rectangle uses specific colors that might not adapt well to dark mode. Consider using dynamic colors or checking the color scheme.
- RoundedRectangle(cornerRadius: 30) - .fill(.black.opacity(0.1)) - .stroke(.white, lineWidth: 4) - .frame(width: geometry.size.width * 0.8, height: geometry.size.width * 0.8) + RoundedRectangle(cornerRadius: 30) + .fill(Color(UIColor.systemBackground).opacity(0.1)) + .stroke(Color(UIColor.label), lineWidth: 4) + .frame(width: geometry.size.width * 0.8, height: geometry.size.width * 0.8)LDKNodeMonday/View/Home/BitcoinView.swift (5)
203-214: Consider re-enabling balance checks.You've commented out the
.disabled(viewModel.unifiedBalance == 0)modifier which previously prevented users from attempting to send when they have no balance.- }.buttonStyle( - BitcoinFilled( - width: 120, - tintColor: .accent, - isCapsule: true - ) - ) //.disabled(viewModel.unifiedBalance == 0) + }.buttonStyle( + BitcoinFilled( + width: 120, + tintColor: .accent, + isCapsule: true + ) + ).disabled(viewModel.unifiedBalance == 0)Also, as a style note, add a space after the comment slashes:
- ) //.disabled(viewModel.unifiedBalance == 0) + ) // .disabled(viewModel.unifiedBalance == 0)🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 213-213: Prefer at least one space after slashes for comments
(comment_spacing)
218-227: Consider re-enabling balance checks.Similar to the Send button, you've commented out the balance check for the Scan QR button. Consider re-enabling this check to prevent users from scanning QR codes when they have no balance to send.
- } //.disabled(viewModel.unifiedBalance == 0) + }.disabled(viewModel.unifiedBalance == 0)As a style note, add a space after the comment slashes:
- } //.disabled(viewModel.unifiedBalance == 0) + } // .disabled(viewModel.unifiedBalance == 0)🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 227-227: Prefer at least one space after slashes for comments
(comment_spacing)
203-214: Comment spacing needs to be fixedThere's an issue with the comment spacing at the end of this code block.
- ) //.disabled(viewModel.unifiedBalance == 0) + ) // .disabled(viewModel.unifiedBalance == 0)🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 213-213: Prefer at least one space after slashes for comments
(comment_spacing)
218-227: Comment spacing needs to be fixedThere's an issue with the comment spacing at the end of this code block.
- } //.disabled(viewModel.unifiedBalance == 0) + } // .disabled(viewModel.unifiedBalance == 0)🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 227-227: Prefer at least one space after slashes for comments
(comment_spacing)
213-227: Consider re-enabling balance checks for send buttonsBoth the Send and Scan QR buttons have commented-out code that would disable them when the balance is zero. Consider whether these checks should be re-enabled to prevent users from attempting to send when they have no funds.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 213-213: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 227-227: Prefer at least one space after slashes for comments
(comment_spacing)
LDKNodeMonday/Extensions/String+Extensions.swift (2)
167-167: Track LNURL support implementationThe TODO comment indicates LNURL support is not yet implemented. While this is expected per the PR objectives, it would be good to create a tracking issue for this future enhancement.
Would you like me to help create a tracking issue for the LNURL support implementation?
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 167-167: TODOs should be resolved (Implement support for lnurl)
(todo)
169-170: Standardize error return valuesFor consistency with other error cases in the method, consider returning
nilfor the payment address instead of.none.- return (0, .none) + return (0, nil)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
LDKNodeMonday.xcodeproj/project.pbxproj(18 hunks)LDKNodeMonday/Extensions/Event+Extensions.swift(2 hunks)LDKNodeMonday/Extensions/String+Extensions.swift(3 hunks)LDKNodeMonday/Model/PaymentType.swift(1 hunks)LDKNodeMonday/Model/SendReceiveOption.swift(1 hunks)LDKNodeMonday/Service/EventService/EventService.swift(1 hunks)LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift(7 hunks)LDKNodeMonday/View Model/Home/Send/AmountViewModel.swift(0 hunks)LDKNodeMonday/View Model/Home/Send/SendViewModel.swift(1 hunks)LDKNodeMonday/View/Home/BitcoinView.swift(5 hunks)LDKNodeMonday/View/Home/Payments/PaymentsListView.swift(1 hunks)LDKNodeMonday/View/Home/Receive/AmountEntryView.swift(2 hunks)LDKNodeMonday/View/Home/Send/AmountView.swift(0 hunks)LDKNodeMonday/View/Home/Send/SendManualEntry.swift(1 hunks)LDKNodeMonday/View/Home/Send/SendReviewView.swift(1 hunks)LDKNodeMonday/View/Home/Send/SendScanAddressView.swift(5 hunks)LDKNodeMonday/View/Home/Send/SendView.swift(1 hunks)LDKNodeMonday/View/Settings/Channel/ChannelsListView.swift(1 hunks)LDKNodeMonday/View/Settings/Peers/PeersListView.swift(1 hunks)
💤 Files with no reviewable changes (2)
- LDKNodeMonday/View Model/Home/Send/AmountViewModel.swift
- LDKNodeMonday/View/Home/Send/AmountView.swift
👮 Files not reviewed due to content moderation or server errors (2)
- LDKNodeMonday/View/Home/BitcoinView.swift
- LDKNodeMonday/Extensions/String+Extensions.swift
🧰 Additional context used
🧠 Learnings (1)
LDKNodeMonday/View/Settings/Peers/PeersListView.swift (1)
Learnt from: danielnordh
PR: reez/Monday#131
File: LDKNodeMonday/View/Home/OnboardingView.swift:15-15
Timestamp: 2025-03-20T20:18:14.803Z
Learning: In iOS 17, when using the new Observable pattern in SwiftUI, it's appropriate to use `@State` for view models instead of `@ObservedObject`.
🧬 Code Definitions (6)
LDKNodeMonday/View/Home/Send/SendReviewView.swift (1)
LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (1) (1)
send(29-69)
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (1)
LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (1) (1)
send(29-69)
LDKNodeMonday/View/Home/Send/SendManualEntry.swift (1)
LDKNodeMonday/Extensions/String+Extensions.swift (2) (2)
truncateMiddle(212-217)extractPaymentInfo(154-171)
LDKNodeMonday/Extensions/Event+Extensions.swift (2)
LDKNodeMonday/Extensions/String+Extensions.swift (1) (1)
truncated(83-90)LDKNodeMonday/Extensions/UInt64+Extensions.swift (1) (1)
formattedSatsAsUSD(51-55)
LDKNodeMonday/View/Home/BitcoinView.swift (1)
LDKNodeMonday/View Model/Home/BitcoinViewModel.swift (1) (1)
update(42-54)
LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (1)
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (2) (2)
send(244-247)sendBolt11Payment(250-259)
🪛 SwiftLint (0.57.0)
LDKNodeMonday/Service/EventService/EventService.swift
[Warning] 23-23: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 12-12: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
LDKNodeMonday/View/Home/Send/SendView.swift
[Warning] 68-68: Prefer at least one space after slashes for comments
(comment_spacing)
LDKNodeMonday/Extensions/Event+Extensions.swift
[Warning] 94-94: Prefer at least one space after slashes for comments
(comment_spacing)
LDKNodeMonday/View/Home/BitcoinView.swift
[Warning] 213-213: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 227-227: Prefer at least one space after slashes for comments
(comment_spacing)
LDKNodeMonday/Extensions/String+Extensions.swift
[Warning] 167-167: TODOs should be resolved (Implement support for lnurl)
(todo)
[Warning] 191-191: TODOs should be resolved (Need to extract amount from of...)
(todo)
🔇 Additional comments (59)
LDKNodeMonday/View/Settings/Channel/ChannelsListView.swift (1)
14-14: Correctly updated property wrapper for iOS 17 compatibilityThe change from
let viewModelto@State var viewModelaligns with modern SwiftUI practices in iOS 17, which recommends using the new Observable pattern. This modification ensures that the view will properly update when the view model's state changes.LDKNodeMonday/View/Settings/Peers/PeersListView.swift (1)
13-13: Correctly adopted iOS 17 state management patternThe change from
@ObservedObject var viewModelto@State var viewModelfollows the recommended practice for iOS 17's new Observable pattern. This modification is consistent with the approach used in other views like ChannelsListView and aligns with the learning that for the new Observable pattern in iOS 17, it's appropriate to use@Statefor view models instead of@ObservedObject.LDKNodeMonday/Model/SendReceiveOption.swift (1)
28-35: Well-structured SendViewState implementation.The SendViewState enum provides a clear flow structure for the send process with logical state progression from scanning to payment confirmation. Good choice of protocols (String, CaseIterable, Identifiable) consistent with the existing ReceiveOption enum pattern.
LDKNodeMonday/View/Home/Send/SendReviewView.swift (2)
11-13: LGTM: Good implementation of SendReviewView struct.Clean declaration with proper dependency on SendViewModel through ObservedObject.
14-32: LGTM: Well-structured review display.Clearly presents the amount in both sats and USD format along with the recipient address. Good use of spacing and appropriate font styling.
LDKNodeMonday/View/Home/Receive/AmountEntryView.swift (1)
70-70: LGTM: Clearer navigation title.Simplified navigation title improves user interface conciseness.
LDKNodeMonday/View/Home/Send/SendView.swift (3)
11-88: Well-structured send flow UI with appropriate state managementThe
SendViewprovides a clean implementation of the send flow with different states (scan, manual entry, review, confirmation) and proper error handling. The use of the navigation stack and state-based UI switching creates a good user experience.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 68-68: Prefer at least one space after slashes for comments
(comment_spacing)
23-24: Verify the hardcoded spendable balance valueThe spendable balance is hardcoded to 0, which might result in inaccurate information being displayed to the user.
Consider passing the actual available balance from the view model:
- spendableBalance: 0 + spendableBalance: viewModel.spendableBalance
67-69: Commented-out code should be addressedThere's commented out code that might indicate incomplete functionality.
Is
generateAddressesfunctionality still needed? If so, uncomment and implement it; if not, remove the commented line completely.Also, add a space after the slashes to fix the comment formatting:
- //await viewModel.generateAddresses() + // await viewModel.generateAddresses()🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 68-68: Prefer at least one space after slashes for comments
(comment_spacing)
LDKNodeMonday.xcodeproj/project.pbxproj (5)
13-15: New Send flow files properly integratedThe new Send flow files (
SendView.swift,SendReviewView.swift, andSendManualEntry.swift) have been properly added to the project structure and build phases.Also applies to: 99-101
32-32: Good refactoring of ReceiveOption to SendReceiveOptionRenaming
ReceiveOption.swifttoSendReceiveOption.swiftappropriately reflects its expanded functionality to support both send and receive operations.Also applies to: 644-644
47-47: Improved naming of address view to SendScanAddressViewRenaming
AddressView.swifttoSendScanAddressView.swiftprovides better context about the view's purpose and relationship to the send flow.Also applies to: 664-664
817-817: Development team ID has been changedThe development team ID has been changed to
8DAB7BVU34, which may affect signing and distribution if this differs from the previous configuration.Is this change intentional? Make sure it aligns with your team's distribution plans.
Also applies to: 850-850
834-834: Bundle identifier has been changedThe bundle identifier has been changed to
com.danielnordh.LDKNodeMonday. If this is different from what was used before, it could affect app distribution and user upgrades.If this is a public app, changing the bundle identifier will result in the App Store treating it as a completely new app. Users of the previous version won't receive updates.
Also applies to: 867-867
LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (2)
13-28: Well-structured view model with appropriate property declarationsThe
SendViewModelfollows good practices with@MainActorannotation, clear property definitions, and proper initialization.
29-46: Payment sending logic handles multiple address typesThe implementation correctly handles different payment types (BIP21, onchain, bolt11) with appropriate client method calls.
LDKNodeMonday/Service/EventService/EventService.swift (2)
20-24: Transition to structured event handlingGood refactoring to handle structured
Eventobjects instead of string messages, which provides richer context for event handling.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 23-23: Prefer at least one space after slashes for comments
(comment_spacing)
27-40: Verify error notification handlingThe error notification handling has been commented out with a "Don't use this" comment. Ensure this change doesn't impact error reporting in the app.
Is there an alternative error handling mechanism in place? If errors aren't being handled properly, users might not receive important feedback.
LDKNodeMonday/View/Home/Send/SendManualEntry.swift (1)
11-143: Well-structured manual entry view with clean UI elementsThe view layout is well-organized with clear sections for amount and recipient address entry. The implementation effectively leverages a view model for state management and provides good user feedback.
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (3)
249-259: Well-implemented Bolt11 payment methodThe new
sendBolt11Paymentmethod is well-structured and correctly handles the required parameters. This adds important functionality for the send flow.Consider adding comments about potential error states and how they should be handled by callers.
335-336: Good improvement to event handlingPosting the full event object instead of just its description provides more flexibility and data to listeners. Ensure all listeners are updated to handle the full object.
440-445: Good implementation of the Bolt11 payment client methodThe client implementation properly forwards to the shared service, maintaining consistency with other methods.
LDKNodeMonday/Extensions/Event+Extensions.swift (2)
59-61: Good cleanup of unused parametersThe simplified pattern matching for
paymentClaimableimproves code readability.
65-80: Well-implemented icon mapping for eventsThe
iconNameproperty provides appropriate system icons for different event types, enhancing the visual representation.LDKNodeMonday/View/Home/Send/SendScanAddressView.swift (1)
13-155: Well-structured scan view with good refactoring toward view model patternThe refactoring from
AddressViewtoSendScanAddressViewusing a view model improves code organization and state management. The scan and paste functionality is implemented clearly.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 40-40: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
LDKNodeMonday/View/Home/BitcoinView.swift (17)
66-69: LGTM! Updated event service interaction.The change from
lastMessagetolastEventaligns with the transition to a more structured event-based approach. This will provide better type safety and clearer event handling throughout the application.
76-78: LGTM! Consistent event subscription.Updating
onReceiveto use$lastEventensures the view model refreshes when events occur. This maintains synchronization between the UI and the underlying data.
96-97: LGTM! Enhanced toast notification.The
EventItemViewimplementation provides a more structured way to display event information with price context, improving the user experience with more informative notifications.
195-197: LGTM! State variables for modal presentation.Adding these state variables for managing the presentation of send sheets enables a more modular approach to handling different entry points for the send flow.
243-276: LGTM! Modal presentation implementation.The sheet modifiers are well-implemented, handling both manual entry and camera scanning entry points for the send flow. The use of
onDismissto update the view model ensures data stays fresh after a transaction.
289-298: LGTM! Sheet reset on events.This ensures all sheets are automatically dismissed when an event occurs (like a successful payment), providing a seamless user experience where the UI reacts appropriately to payment events.
66-69: Consistent event handling used for toast notificationsThe change from
lastMessagetolastEventaligns with the refactoring of the event system to use structured event objects rather than simple messages, providing richer information for toast notifications.
76-78: Event service correctly integrated with view model updatesThe view model is now properly updated whenever a new event is received, ensuring the UI reflects the latest state after operations like sending or receiving payments.
96-97: Enhanced event display with price informationThe new
EventItemViewprovides a richer presentation of events by including price information, which improves the user experience by showing contextually relevant data.
195-197: Well-structured state management for send flowsAdding separate state variables for the manual entry and camera presentation sheets creates a clear separation of concerns and makes the code more maintainable.
243-276: Well-implemented sheet presentations for the send flowThe implementation of sheet presentations for both manual entry and camera scanning provides a consistent user experience. The onDismiss handlers ensure that the view model is properly updated after a send operation.
289-298: Clean sheet dismissal on new eventsThis onChange handler ensures that all sheets are automatically dismissed when a new event occurs, such as when a payment is successfully sent. This provides a smooth user experience by preventing stale UI states.
66-69: Consistent change fromlastMessagetolastEventThe update from checking
lastMessagetolastEventaligns well with the enhanced send flow approach. This change reflects a more structured event-based pattern rather than simple message passing.
96-97: Improved user feedback with EventItemViewReplacing simple text messages with the
EventItemViewcomponent improves the user experience by providing more contextual information about transactions, including price data.
195-197: Good addition of state variables for send sheet managementThe addition of these state variables properly supports the two entry points for sending (manual entry and camera scanning) as mentioned in the PR objectives.
243-276: Well-structured sheet presentation with proper update handlingThe implementation of the send sheets with appropriate
onDismisshandlers ensures that the view model is updated after a send action. This maintains data consistency between the UI and the underlying model.
289-298: Effective sheet state managementThe
onChangehandler properly resets all sheets when an event occurs, which prevents UI state inconsistencies after transactions are completed.LDKNodeMonday/Extensions/String+Extensions.swift (17)
110-112: LGTM! Renamed and improved visibility.Renaming from
isBitcoinAddresstoisBip21Addressbetter reflects the property's purpose, and making it public allows for more flexible use throughout the codebase.
131-152: LGTM! Improved return types.The function now returns more appropriate types (
UInt64andPaymentAddress?), enhancing type safety and making the API more intuitive. The simplification of the logic also improves readability.
154-171: Address the TODOs in a future PR.The method has been updated to return appropriate types, but there's a TODO comment on line 167 for LNURL support that should be addressed in a follow-up PR.
Please confirm that LNURL support is planned in a future PR as mentioned in the PR objectives.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 167-167: TODOs should be resolved (Implement support for lnurl)
(todo)
173-183: LGTM! Improved return types.The function signature change to return
UInt64andPaymentAddressaligns with the overall refactoring to use stronger types, improving the API consistency.
185-199: Address the Bolt12 amount extraction TODO.There's a TODO comment on line 191 about extracting amounts from Bolt12 offers. This limitation should be documented and addressed in a future PR.
Please confirm that the Bolt12 amount extraction will be implemented in a future PR when LDK Node supports this functionality.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 191-191: TODOs should be resolved (Need to extract amount from of...)
(todo)
212-217: LGTM! Useful helper method.The new
truncateMiddlemethod is a clean implementation that will be helpful for displaying long addresses (like Bitcoin or Lightning addresses) in a user-friendly way.
110-112: Good renaming for Bitcoin address checkingRenaming
isBitcoinAddresstoisBip21Addressmore accurately reflects its functionality, as it checks for both BIP21 format addresses and standard Bitcoin addresses.
131-152: Improved BIP21 processing with structured return typesThe refactored
processBIP21method now returns more appropriate types - a UInt64 for the amount and an optional PaymentAddress object. This makes the interface more type-safe and clearer to use.
173-183: Improved return type consistency in address processingThe
processBitcoinAddressmethod now returns consistent types (UInt64, PaymentAddress) aligning with other methods, which improves code maintainability and reduces the chance of errors when using these methods.
212-217: Useful string truncation helper for displaying addressesThe new
truncateMiddlefunction is a good addition that will help improve the UI by displaying long Bitcoin or Lightning addresses in a more readable format, showing the beginning and end parts with an ellipsis in the middle.
154-171:❓ Verification inconclusive
TODO: Implement LNURL support
There's a TODO comment about implementing support for LNURL addresses. As the PR objectives mention supporting various payment formats, consider whether this should be addressed in this PR or tracked as a separate task.
🌐 Web query:
What are the current standards for LNURL support in Lightning wallet applications?💡 Result:
LNURL (Lightning Network URL) is a protocol standard enhancing Lightning Network usability by simplifying transactions and authentication. Its current implementation standards are outlined through LNURL Documents (LUDs), a suite of modular specifications adopted by wallets and services. Below are the key aspects of LNURL support in Lightning wallets:
Core LNURL Standards
LNURL-Pay (LUD-06)
Enables static, reusable payment links (QR codes or URLs) for receiving funds. Users no longer need to generate new invoices for each transaction[1][8][14].
- Supports optional comments (LUD-12) and success actions (LUD-09)[5][16].
- Used for Lightning Addresses (e.g.,
[email protected]), which map to LNURL-Pay endpoints[2][8].LNURL-Withdraw (LUD-03)
Allows senders to pre-generate vouchers for recipients to claim funds, streamlining payouts (e.g., payroll or withdrawals)[2][7][11].LNURL-Auth (LUD-04)
Facilitates passwordless authentication via cryptographic signatures from Lightning wallets, enabling secure logins without personal data[1][8][16].
- Example: Lightninglogin.live uses LNURL-Auth for anonymous user registration[2].
LNURL-Channel (LUD-02)
Automates channel creation by embedding node connection details in QR codes, simplifying inbound liquidity management[2][11].
Wallet Implementation
Wallets must support LUD-01 (base encoding/decoding) and implement relevant LUDs for specific features. Leading non-custodial wallets with robust LNURL support include:
Wallet Supported Features Notes Breez LNURL-Pay, Withdraw, Auth, Channel Integrates Lightning Addresses[6][12]. Phoenix LNURL-Pay, Auth Uses dynamic channels via splicing[6]. Wallet of Satoshi LNURL-Pay, Withdraw Static QR code support[1]. Zeus LNURL-Pay, Channel Custom node management[12]. Blixt LNURL-Pay, Auth Runs an embedded SPV/LND node[12]. Tools like BTCPayServer and LNbits also implement LNURL for merchant solutions[16].
Developer Tools & Compatibility
- npm’s
lnurlpackage: Provides Node.js servers/clients for building LNURL-compatible apps[5].- Backward Compatibility: Lightspark’s UMA standard extends LNURL for compliance (e.g., travel rules) while maintaining compatibility[3].
- GitHub Specifications: The lnurl/luds repo defines all LUDs, with services like BTCPayServer supporting subsets (e.g., LUD-01/06/16/17)[16].
Functional Benefits
- Simplified Payments: Eliminates manual invoice generation for merchants[1][8].
- Enhanced Security: Auth replaces traditional 2FA with wallet-based signatures[1][11].
- Liquidity Management: LNURL-Channel automates peer connections[2][11].
Adoption Challenges
- Server Dependency: LNURL-Pay/Auth require web servers, limiting standalone use[8][13].
- Privacy Trade-offs: HTTP-based LNURL can expose IP addresses unless Tor-enabled[8][13].
For developers, the modular LUD framework allows incremental implementation, with core wallets prioritizing LNURL-Pay and Auth. As of 2025, LNURL remains pivotal for Lightning’s UX growth, complemented by newer standards like Bolt 12 and UMA[3][13].
Citations:
- 1: https://blog.areabitcoin.co/lnurl-protocol/
- 2: https://coincharge.io/en/lnurl-for-lightning-wallets/
- 3: https://www.nobsbitcoin.com/lightspark-uma/
- 4: https://www.bacloud.com/en/knowledgebase/220/bitcoin-lightning-network-full-node-hardware-requirements-2025-updated.html
- 5: https://www.npmjs.com/package/lnurl
- 6: https://atlas21.com/guide-to-using-a-lightning-network-wallet/
- 7: https://support.exodus.com/support/en/articles/8598830-lightning-in-exodus-support-ends-may-30-2025
- 8: https://coinfinity.co/en/blog/lightning-deep-dive
- 9: https://blog.ishosting.com/en/lightning-network-wallets
- 10: https://cryptopotato.com/utxo-stack-unveils-whitepaper-2025-roadmap-and-new-website-pioneering-liquidity-incentives-for-the-lightning-network/
- 11: https://d-central.tech/what-is-lnurl-unlocking-the-power-of-the-lightning-network/
- 12: https://bringin.xyz/blog/learn/self-custodial-lightning-wallets/
- 13: https://river.com/learn/lightning-payments-2025/
- 14: https://docs.lightning.engineering/the-lightning-network/payment-lifecycle/understanding-lightning-invoices
- 15: https://osl.com/en/academy/article/what-is-bitcoins-lightning-network-and-how-does-it-improve-transaction-speed
- 16: https://github.com/lnurl/luds
- 17: https://www.fidelitydigitalassets.com/research-and-insights/lightning-network-expanding-bitcoin-use-cases
- 18: https://docs.zerohash.com/docs/lnurl-deposits-and-withdrawals
- 19: https://www.voltage.cloud/blog/how-does-lnurl-work-enhancing-lightnings-user-experience
- 20: https://www.debutinfotech.com/blog/what-is-bitcoin-lightning-network-wallet
Clarify LNURL Support Implementation Strategy
The
extractPaymentInfomethod currently returns a placeholder(0, nil)for LNURL addresses with a TODO note. Given that LNURL is now a core component in Lightning wallet applications—with standards such as LNURL-Pay (LUD-06) driving simplified payment flows—please confirm whether LNURL support (starting with LNURL-Pay) should be implemented in this PR or deferred to a separate task. If deferring, kindly add a tracking issue or documentation reference outlining the planned LNURL sub-protocol(s) to be integrated later.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 167-167: TODOs should be resolved (Implement support for lnurl)
(todo)
185-199:❓ Verification inconclusive
TODO: Extract amount from Bolt12 offers
There's a TODO comment about extracting amounts from Bolt12 offers. Consider whether this limitation should be documented for users or if this should be implemented before release.
🏁 Script executed:
#!/bin/bash # Search for any other references to Bolt12 offers in the codebase rg -A 3 "bolt12" --glob "*.swift"Length of output: 8756
Action Required: Bolt12 Offer Amount Extraction Decision
The implementation in
LDKNodeMonday/Extensions/String+Extensions.swift(lines 185–199) currently defaults to converting the providedamountto a UInt64 for Bolt12 offers—without properly extracting the offer amount—as noted by the TODO comment. Given that references to Bolt12 (e.g., in various view models and payment handling logic) are present throughout the codebase, please decide on one of the following approaches before release:
- Document the Limitation: Clearly inform users that extracting the actual amount from Bolt12 offers isn’t supported.
- Implement the Extraction: Enhance the logic to actually extract the amount from Bolt12 offers if feasible with ldkNode.
Please verify which option best aligns with the project requirements.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 191-191: TODOs should be resolved (Need to extract amount from of...)
(todo)
110-112: Improved naming withisBip21AddressRenaming from
isBitcoinAddresstoisBip21Addressprovides better semantics, as it accurately indicates what the property checks for. Making it public also supports the send flow enhancements.
131-152: Better type safety with UInt64 return typeChanging the return type from string-based tuples to
(UInt64, PaymentAddress?)improves type safety and makes the intent of the function clearer. The implementation properly handles BIP21 addresses according to the PR objectives.
154-171: Consistent address type handlingThe updated
extractPaymentInfomethod now consistently handles different address types, maintaining the type safety improvements. The TODOs for LNURL support align with the PR objectives that mention future features in separate PRs.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 167-167: TODOs should be resolved (Implement support for lnurl)
(todo)
173-183: Enhanced Bitcoin address processingThe conversion to using
PaymentAddresstypes makes the code more structured and easier to maintain. The handling of amount values asUInt64also improves consistency.
212-217: Useful string truncation utilityThe new
truncateMiddlefunction is a good addition that will improve the UI display of long addresses, enhancing the user experience when reviewing transactions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
LDKNodeMonday/View/Home/Send/SendView.swift (1)
65-67: Address SwiftLint comment spacing warning.SwiftLint warns about missing spaces after
//. Adding a space after the slashes will resolve the warning and align with standard Swift comment formatting.- //await viewModel.generateAddresses() + // await viewModel.generateAddresses()🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 67-67: Prefer at least one space after slashes for comments
(comment_spacing)
LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (1)
45-48: Consider validating the invoice before instantiatingBolt11Invoice.If the string is invalid, it will throw at initialization. For a smoother user experience, perform validation first and present a more descriptive error if the string is malformed.
- _ = try await lightningClient.sendBolt11Payment( - Bolt11Invoice(paymentAddress?.address ?? ""), - nil - ) + let invoiceString = paymentAddress?.address ?? "" + guard let invoice = Bolt11Invoice(invoiceString, validate: true) else { + DispatchQueue.main.async { + self.sendError = .init( + title: "Invalid Invoice", + detail: "Please enter a valid Bolt11 invoice." + ) + } + return + } + _ = try await lightningClient.sendBolt11Payment(invoice, nil)LDKNodeMonday/View/Home/Send/SendManualEntry.swift (1)
66-75: Enhance error handling for invalid addresses.Currently, the view transitions only if a valid address is extracted. Users who type an invalid address get no feedback. Consider showing an error or resetting the text field if parsing fails.
LDKNodeMonday/View/Home/BitcoinView.swift (4)
66-69: Avoid multiple watchers if unnecessary.
Currently, there is an.onChange(of: eventService.lastEvent)to toggleshowToastwhile there's also an.onReceive(eventService.$lastEvent)further down. If both watchers share overlapping responsibilities, consider combining them into a single, cohesive observer for better maintainability.
96-98: Enhance the toast’s interactive capabilities.
Currently, the toast displays anEventItemViewbut offers no user ability to dismiss or interact with the alert. Consider allowing a manual dismissal or more contextual user actions if the event is critical.
195-197: Check for duplication in eventService usage.
Having a separate@StateObjectforeventServiceinTransactionButtonsmight cause confusion if you already have one at a higher scope. Consider passing it down as a dependency or using an EnvironmentObject to avoid potential divergence in event states.
289-298: Consolidated dismissal logic is helpful.
Dismissing all sheets on event change ensures the UI is in sync with new events.Consider describing the event scenario in code comments or in naming, so future maintainers understand why these sheets must close when
lastEventchanges.LDKNodeMonday/View/Home/Send/SendScanAddressView.swift (2)
39-42: Remove unused geometry variable.
Since you are not usinggeometryinside the closure, rename it to_to avoid warnings and improve clarity.-.background( - GeometryReader { geometry in - Color.clear - } -) +.background( + GeometryReader { _ in + Color.clear + } +)🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 39-39: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
107-107: Remove unused dismiss environment variable.
@Environment(\.dismiss) private var dismissis not currently used. Consider removing it or leveraging it to dismiss the scanner if needed.LDKNodeMonday/Extensions/String+Extensions.swift (1)
183-195: TODO remains unimplemented for Bolt12.
A placeholder is present for extracting amounts from offers in LN addresses.Would you like assistance opening a tracking issue or adding an interim fallback for partial LNURL installations?
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 188-188: TODOs should be resolved (Need to extract amount from of...)
(todo)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
LDKNodeMonday/Extensions/String+Extensions.swift(3 hunks)LDKNodeMonday/View Model/Home/Send/SendViewModel.swift(1 hunks)LDKNodeMonday/View/Home/BitcoinView.swift(5 hunks)LDKNodeMonday/View/Home/Payments/PaymentsListView.swift(1 hunks)LDKNodeMonday/View/Home/Send/SendManualEntry.swift(1 hunks)LDKNodeMonday/View/Home/Send/SendScanAddressView.swift(1 hunks)LDKNodeMonday/View/Home/Send/SendView.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- LDKNodeMonday/View/Home/Payments/PaymentsListView.swift
🧰 Additional context used
🧬 Code Definitions (3)
LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (1)
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (2) (2)
send(244-247)sendBolt11Payment(250-259)
LDKNodeMonday/View/Home/Send/SendManualEntry.swift (1)
LDKNodeMonday/Extensions/String+Extensions.swift (2) (2)
truncateMiddle(209-214)extractPaymentInfo(154-171)
LDKNodeMonday/View/Home/Send/SendScanAddressView.swift (1)
LDKNodeMonday/Extensions/String+Extensions.swift (1) (1)
extractPaymentInfo(154-171)
🪛 SwiftLint (0.57.0)
LDKNodeMonday/View/Home/Send/SendScanAddressView.swift
[Warning] 39-39: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
LDKNodeMonday/Extensions/String+Extensions.swift
[Warning] 167-167: TODOs should be resolved (Implement support for lnurl)
(todo)
[Warning] 188-188: TODOs should be resolved (Need to extract amount from of...)
(todo)
LDKNodeMonday/View/Home/Send/SendView.swift
[Warning] 67-67: Prefer at least one space after slashes for comments
(comment_spacing)
🔇 Additional comments (15)
LDKNodeMonday/View/Home/Send/SendView.swift (1)
17-64: Good integration of the send flow states.This state-based layout within a
NavigationViewneatly segments the user journey for scanning, manual entry, review, and final confirmation of payment. No major issues are detected.LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (1)
13-27: Clean property definitions and initialization.The properties and initializer for
SendViewModelare well structured, making the code easy to read and maintain.LDKNodeMonday/View/Home/Send/SendManualEntry.swift (1)
130-130: Reinstate or add validation for large amounts.This is a repeated suggestion: if applicable, disable the "Review" button or show a warning when the entered amount exceeds the user's spendable balance, preventing accidental overspending.
LDKNodeMonday/View/Home/BitcoinView.swift (4)
76-78: No issues found here.
CallingviewModel.update()upon receiving a new event is sensible.
203-213: Button labeling and disabling logic looks good.
The.disabled(viewModel.unifiedBalance == 0)check prevents sending with zero balance, which improves UX and reduces error states.
243-259: Good approach for presenting manual entry.
The usage of.sheetwith anonDismissupdating theviewModelis a solid pattern for refreshing data after user actions.
260-276: Clean separation between manual entry and camera scan.
Using distinct sheets for manual entry vs. scanning clarifies the code path. No issues spotted here.LDKNodeMonday/View/Home/Send/SendScanAddressView.swift (2)
1-45: Overall structure is clear.
This file provides a dedicated scanning and pasting interface, displaying error alerts for invalid formats. The approach to handle scanning results is straightforward. No immediate issues spotted.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 39-39: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
47-78: Consistent logic between scanning and pasting.
When scanning (lines 64–71), if the amount is 0, you navigate to.manualEntry, otherwise.reviewPayment. When pasting (lines 93–95), you always navigate to.reviewPayment. Aligning these paths ensures consistent user flows.LDKNodeMonday/Extensions/String+Extensions.swift (6)
110-112: ExposingisBip21Addressis appropriate.
Making it public is beneficial, given it’s used throughout the codebase to detect BIP21 URIs.
114-124: Robust pattern matching for Bitcoin addresses.
This regex covers common P2PKH and Bech32 formats. No issues found here.
131-152: Properly returning(UInt64, PaymentAddress?).
Switching from string-based amounts to integer sats is more consistent with satoshi calculations and reduces parsing errors.
154-171: Clean addressing of multiple Bitcoin/Lightning scenarios.
Extraction logic is well-structured for BIP21, Lightning, or LNURL placeholders. The fallbackreturn (0, .none)approach is good default behavior.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 167-167: TODOs should be resolved (Implement support for lnurl)
(todo)
173-180: Efficient on-chain address extraction.
UsingqueryParams["amount"]for BIP21-based amounts is consistent.
209-214: Truncation logic is straightforward.
Ensures large strings are shortened. No concerns here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (1)
56-64: 🛠️ Refactor suggestionAdd error throwing to default case in payment type handling
The default case for unhandled payment address types sets an error message but doesn't throw an error, which means the method might complete successfully despite the error.
default: debugPrint("Unhandled paymentAddress type") DispatchQueue.main.async { self.sendError = .init( title: "Unsupported payment type", detail: "The payment address type is not supported." ) } + throw NSError( + domain: "com.ldknodemonday.error", + code: -1, + userInfo: [NSLocalizedDescriptionKey: "Unsupported payment address type"] + ) }LDKNodeMonday/View/Home/Send/SendManualEntry.swift (1)
120-132: 🛠️ Refactor suggestionConsider adding validation for amounts greater than available balance
The Review button is disabled when amount is zero, but there's no validation to ensure the amount doesn't exceed the user's available balance.
.buttonStyle( BitcoinFilled( tintColor: .accent, isCapsule: true ) -).disabled(viewModel.amountSat == 0) +).disabled(viewModel.amountSat == 0 || viewModel.disableSend()) .padding(.bottom, 40)
🧹 Nitpick comments (8)
LDKNodeMonday/View/Home/Send/SendView.swift (2)
66-69: Fix the comment spacing in the onAppear taskThe commented line of code has insufficient spacing after the slashes according to Swift style conventions.
.onAppear { Task { - //await viewModel.generateAddresses() + // await viewModel.generateAddresses() } }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 67-67: Prefer at least one space after slashes for comments
(comment_spacing)
66-69: Consider removing or implementing the commented codeThere's a commented-out task to generate addresses. Either implement this functionality if it's needed or remove it if it's no longer required.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 67-67: Prefer at least one space after slashes for comments
(comment_spacing)
LDKNodeMonday/View/Home/Send/SendReviewView.swift (1)
69-83: Refactor force unwrap in disableSend() methodThe force unwrap of
viewModel.paymentAddress!on line 73 could potentially crash if the payment address is nil, despite the prior nil check.func disableSend() -> Bool { if viewModel.paymentAddress.isNil { return true } else { - switch viewModel.paymentAddress!.type { + guard let paymentAddress = viewModel.paymentAddress else { return true } + switch paymentAddress.type { case .onchain: return viewModel.amountSat > viewModel.balances.spendableOnchainBalanceSats case .bip21: return viewModel.amountSat > viewModel.balances.spendableOnchainBalanceSats && viewModel.amountSat > viewModel.balances.totalLightningBalanceSats case .bolt11, .bolt11Jit, .bolt12: return viewModel.amountSat > viewModel.balances.totalLightningBalanceSats } } }LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (1)
1-6: Rename the file to match class nameThe file is named "AmountViewModel.swift" but contains a class called "SendViewModel". It's best practice to name the file after the primary class it contains.
Rename the file from "AmountViewModel.swift" to "SendViewModel.swift" to match the class name.
LDKNodeMonday/View/Home/Send/SendManualEntry.swift (1)
112-114: Add additional address validationYou're removing spaces from the address, but consider adding more comprehensive validation for cryptocurrency addresses.
.onChange(of: viewModel.address) { _, newValue in - viewModel.address = newValue.replacingOccurrences(of: " ", with: "") + let cleanedAddress = newValue.replacingOccurrences(of: " ", with: "") + // Optional: Add additional validation if needed + viewModel.address = cleanedAddress }LDKNodeMonday/View/Home/BitcoinView.swift (1)
76-78: Ensure intended synchronization with the parent view's eventService.
.onReceive(eventService.$lastEvent)callsviewModel.update()on each event. This is generally fine, but be aware of potential performance overhead if events arrive frequently. Also, consider whether the parentBitcoinViewis already updating the sameviewModelin its own.onReceiveor.onChangelogic.If you need help consolidating these updates to avoid redundant refreshes, let me know!
LDKNodeMonday/View/Home/Send/SendScanAddressView.swift (2)
49-78: Scan result logic seems solid.Handling different scan outcomes with
.successand.failureis properly structured. Notably, you reassignsendViewStateto.reviewPaymentthen override it ifamountSat == 0. Consider simplifying:withAnimation { viewModel.sendViewState = (viewModel.amountSat == 0) ? .manualEntry : .reviewPayment }
104-142: Potential closure parameter usage.At line 39, SwiftLint complains about the unused parameter
geometry. If it isn’t needed, replacegeometry inwith_ in. This avoids linter warnings and clarifies intent.Apply this diff to remove the unused parameter:
- GeometryReader { geometry in + GeometryReader { _ in Color.clear }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
LDKNodeMonday/View Model/Home/Send/SendViewModel.swift(1 hunks)LDKNodeMonday/View/Home/BitcoinView.swift(5 hunks)LDKNodeMonday/View/Home/Send/SendManualEntry.swift(1 hunks)LDKNodeMonday/View/Home/Send/SendReviewView.swift(1 hunks)LDKNodeMonday/View/Home/Send/SendScanAddressView.swift(1 hunks)LDKNodeMonday/View/Home/Send/SendView.swift(1 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
LDKNodeMonday/View/Home/Send/SendReviewView.swift (1)
LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (1) (1)
send(36-85)
LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (1)
LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (2) (2)
send(244-247)sendBolt11Payment(250-259)
LDKNodeMonday/View/Home/Send/SendManualEntry.swift (1)
LDKNodeMonday/Extensions/String+Extensions.swift (2) (2)
truncateMiddle(209-214)extractPaymentInfo(154-171)
LDKNodeMonday/View/Home/BitcoinView.swift (1)
LDKNodeMonday/View Model/Home/BitcoinViewModel.swift (1) (1)
update(42-54)
LDKNodeMonday/View/Home/Send/SendScanAddressView.swift (1)
LDKNodeMonday/Extensions/String+Extensions.swift (1) (1)
extractPaymentInfo(154-171)
🪛 SwiftLint (0.57.0)
LDKNodeMonday/View/Home/Send/SendView.swift
[Warning] 67-67: Prefer at least one space after slashes for comments
(comment_spacing)
LDKNodeMonday/View/Home/Send/SendScanAddressView.swift
[Warning] 39-39: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
🔇 Additional comments (14)
LDKNodeMonday/View/Home/Send/SendView.swift (1)
11-87: LGTM! Clean implementation of the sending flow UIThe SendView implementation follows good practices:
- Uses MVVM architecture with ObservedObject for state management
- Properly handles different view states with clear transitions
- Includes error handling with user-friendly alerts
- Has accessibility considerations with dynamic type size limits
- Uses environment variables appropriately for dismissal
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 67-67: Prefer at least one space after slashes for comments
(comment_spacing)
LDKNodeMonday/View/Home/Send/SendReviewView.swift (1)
11-84: LGTM! Good implementation of the payment review screenThe review screen is well-structured with:
- Clear display of payment amount in both satoshis and USD
- Recipient address display with truncation for long addresses
- Proper validation for insufficient funds
- Specific disabling logic based on payment type and available balances
LDKNodeMonday/View Model/Home/Send/SendViewModel.swift (1)
13-86: LGTM! Well-structured view model with proper error handlingThe SendViewModel implementation:
- Follows MVVM pattern with proper separation of concerns
- Uses @mainactor annotation to ensure UI updates happen on the main thread
- Handles different payment address types correctly
- Includes comprehensive error handling
- Properly throws errors after recording them for the UI
LDKNodeMonday/View/Home/Send/SendManualEntry.swift (1)
11-133: LGTM! Clean implementation of manual entry viewThe manual entry view is well-structured with:
- Clear separation between amount and recipient address fields
- Good use of animation for state transitions
- Proper handling of different payment address formats
- Ability to scan QR codes for convenience
- Address cleanup to remove spaces
LDKNodeMonday/View/Home/BitcoinView.swift (6)
196-198: Review multipleEventServiceinstances.Instantiation of
@StateObject private var eventService = EventService()withinTransactionButtonspotentially duplicates theEventServicedeclared at the parentBitcoinViewlevel (line 22). This could lead to inconsistent or out-of-sync events if these objects aren't meant to be separate sources of truth. Confirm whether you want distinct event services or could reuse the parent’s.Would you like to verify other references to
EventService()in the codebase? If so, I can generate a script to locate them.
203-213: Send button enablement logic looks good.Disabling the button when
viewModel.unifiedBalance == 0is a helpful guard against sending with zero balance and aligns well with prior review feedback.
218-219: Consistent QR button disabling logic.Great job ensuring the QR scan button is also disabled for zero balance. This provides consistency and prevents users from initiating a send flow without funds.
243-260: Sheets for manual and camera send flow are well-structured.Presenting two separate sheets for manual entry and camera scanning is a clean approach. The repetition of
onDismiss: { Task { viewModel.update() } }in both sheets is acceptable, though you might consider extracting common logic into a helper method if it grows more complex.Also applies to: 261-278
291-300: Sheet dismissal on event change.The
.onChange(of: eventService.lastEvent)block here automatically dismisses all sheets when an event occurs. This approach can be convenient for a quick UI reset, but it might also surprise users who are in the middle of manual entry. Verify this behavior aligns with the intended user experience.
66-69:❓ Verification inconclusive
Potential duplication of event observers.
You're simultaneously using
.onChange(of: eventService.lastEvent)here and.onReceive(eventService.$lastEvent)below (line 76) to trigger different side effects. While this may be intentional, it can also cause confusion or race conditions if both blocks fire in close succession. Consider consolidating or clearly documenting why two separate observers are needed.Run this script to search for
.onChange(and.onReceive(calls oneventService.lastEventto ensure there are no unintentional duplicates:
🏁 Script executed:
#!/bin/bash rg -A 3 --line-number --heading 'onChange\s*\(\s*of:\s*eventService\.lastEvent|onReceive\s*\(\s*eventService\.\$lastEvent'Length of output: 331
Action: Review and Document Dual Event Observers in BitcoinView.swift
After verifying the code, it appears that the use of two separate observers on
eventService.lastEventis intentional but could potentially lead to confusion or race conditions if not clearly justified:
- Location 1: Lines 66–69 use an
onChangeobserver that updatesshowToastbased on the state ofeventService.lastEvent.- Location 2: Lines 76–78 use an
onReceiveobserver that triggers aviewModel.update(), as confirmed by the grep output.If both observers are needed because they serve distinct purposes, please consider adding clear documentation to explain the separation of concerns. If consolidation is practical without altering intended behavior, that might simplify maintenance and reduce potential timing issues.
LDKNodeMonday/View/Home/Send/SendScanAddressView.swift (4)
1-19: Overall structure for scanning and pasting addresses is clear.The file-level setup with
@ObservedObject var viewModeland local state for alerts is straightforward. Handling the pasteboard withUIPasteboard.generalis appropriate. No major issues spotted.
19-45: Alerting mechanism is succinct.Presenting an
Alertwhen the scanned or pasted data is unsupported is a clean solution. The approach ensures user awareness of invalid formats without unnecessary complexity.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 39-39: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
80-102: Pasting address workflow is consistent with scanning.Reusing
extractPaymentInfo()ensures consistent parsing. The fallback to an alert for invalid formats is appropriate. Good job maintaining symmetry between scan and paste flows.
145-155: Preview code.The SwiftUI preview is straightforward and helps ensure the layout behaves as expected. No further suggestions here.
| if viewModel.sendError == nil { | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.25) { | ||
| withAnimation { | ||
| viewModel.sendViewState = .paymentSent | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for failed payments
Currently, you check if viewModel.sendError == nil after attempting to send, but there's no feedback to the user if the send operation fails.
Task {
try await viewModel.send()
}
if viewModel.sendError == nil {
DispatchQueue.main.asyncAfter(deadline: .now() + 0.25) {
withAnimation {
viewModel.sendViewState = .paymentSent
}
}
+} else {
+ // Optional: Add haptic feedback for error
+ let generator = UINotificationFeedbackGenerator()
+ generator.notificationOccurred(.error)
}📝 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.
| if viewModel.sendError == nil { | |
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.25) { | |
| withAnimation { | |
| viewModel.sendViewState = .paymentSent | |
| } | |
| } | |
| } | |
| Task { | |
| try await viewModel.send() | |
| } | |
| if viewModel.sendError == nil { | |
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.25) { | |
| withAnimation { | |
| viewModel.sendViewState = .paymentSent | |
| } | |
| } | |
| } else { | |
| // Optional: Add haptic feedback for error | |
| let generator = UINotificationFeedbackGenerator() | |
| generator.notificationOccurred(.error) | |
| } |
| func truncateMiddle(first: Int, last: Int) -> String { | ||
| guard self.count > first + last else { return self } | ||
| let start = self.prefix(first) | ||
| let end = self.suffix(last) | ||
| return "\(start)…\(end)" | ||
| } | ||
|
|
There was a problem hiding this comment.
There might be a reason this custom function is needed (if so leave it), i looked at the call site and couldn’t tell totally for sure, but if we can use https://developer.apple.com/documentation/swiftui/view/truncationmode(_:) instead that would be preferred I’m thinking.
There was a problem hiding this comment.
truncationmode() doesn't allow for specifying how many remaining numbers you want, so it ends up showing a very long string. So I ended up adding this.
| @@ -46,7 +46,19 @@ struct PaymentsListView: View { | |||
| .listRowSeparator(.hidden) | |||
| } else { | |||
| // List payments | |||
| ForEach(payments, id: \.id) { payment in | |||
| // Filter out: .pending that are older than 30 minutes or 0 amount | |||
There was a problem hiding this comment.
Nice filtering out older than 30 mins
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
LDKNodeMonday/Service/EventService/EventService.swift (1)
12-12: Remove redundant nil initialization.The initialization with
nilis redundant since Swift automatically initializes optional properties with nil.- @Published var lastEvent: Event? = nil + @Published var lastEvent: Event?🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 12-12: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
LDKNodeMonday/Extensions/Event+Extensions.swift (1)
92-136: Consider dark mode compatibility for EventItemView.The
EventItemViewis well-structured, but it uses hardcoded colors that might not adapt well to dark mode. Consider using Color assets or semantic colors that respect the user's appearance settings.- .background(Color.white) + .background(Color(.systemBackground))Also, the timestamp is hardcoded as "Just now" instead of showing the actual time of the event. Consider adding a timestamp property to the Event type or accepting a Date parameter to display a more accurate time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
LDKNodeMonday/Extensions/Event+Extensions.swift(2 hunks)LDKNodeMonday/Service/EventService/EventService.swift(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
LDKNodeMonday/Extensions/Event+Extensions.swift (2)
LDKNodeMonday/Extensions/String+Extensions.swift (1) (1)
truncated(83-90)LDKNodeMonday/Extensions/UInt64+Extensions.swift (1) (1)
formattedSatsAsUSD(51-55)
🪛 SwiftLint (0.57.0)
LDKNodeMonday/Service/EventService/EventService.swift
[Warning] 12-12: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🔇 Additional comments (6)
LDKNodeMonday/Service/EventService/EventService.swift (2)
12-25: Good refactoring from String messages to structured Event objects.The change from
lastMessage: String?tolastEvent: Event?is a good improvement that enables more structured data handling. This allows for better type safety and the ability to extract specific properties from events like shown in the Event+Extensions.swift file.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 12-12: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
14-25:❓ Verification inconclusive
Verify if error handling was intentionally removed.
There appears to be a removed observer for
.ldkErrorReceivednotifications. Was this intentional, or are errors now being handled differently in the application? Make sure this doesn't result in lost error information.
🏁 Script executed:
#!/bin/bash # Check if there are other places in the codebase where .ldkErrorReceived is being observed or if it's been replaced echo "Checking for .ldkErrorReceived notification references:" rg "ldkErrorReceived" --type swift echo "----" echo "Checking for error handling in BitcoinView.swift:" rg "lastEvent" "BitcoinView.swift" -A 5 -B 5Length of output: 648
Ensure ldkErrorReceived Errors Are Handled as Intended
The current implementation ofEventService.swift(lines 14–25) registers an observer only for.ldkEventReceivednotifications and omits an observer for.ldkErrorReceived. We found that.ldkErrorReceivedis defined inLDKNodeMonday/Extensions/Notification+Extensions.swiftand is posted fromLDKNodeMonday/View Model/Home/Send/SendViewModel.swift, but no corresponding observer is present inEventService.swift.Please verify that this change was intentional and that error handling now occurs elsewhere. Confirm that removing the observer in
EventService.swiftdoesn’t lead to dropped error notifications or lost error information.LDKNodeMonday/Extensions/Event+Extensions.swift (4)
14-31: Good addition of title property.The
titlecomputed property provides human-readable descriptions for each event type, which enhances the user experience. This is a good abstraction that separates the display logic from the view implementation.
59-61: Good simplification of paymentClaimable case.The simplification of the pattern matching for the
paymentClaimablecase makes the code cleaner while maintaining functionality.
65-89: Good addition of iconName and amount properties.These computed properties enhance the Event type with UI-specific information, making it easier to display events consistently throughout the app. The implementation is clean and follows good Swift practices.
117-126: Good conditional display of amount information.The conditional display of the amount information only when it's relevant (i.e., for received payments) is a good practice that avoids showing unnecessary zeros. The formatting of the amount in both satoshis and USD provides valuable context for the user.
This PR updates the send flow screens:
Does not include (to follow in separate PRs)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor