bip21 #110
bip21 #110
Conversation
|
Warning Rate limit exceeded@reez has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 0 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. WalkthroughThe recent updates to the LDKNodeMonday project enhance its capability to manage BIP21 payment requests, streamline project dependencies, and improve error handling. New Swift files for BIP21 functionality have been added, while redundant references have been removed and existing structures updated. These modifications refine the overall architecture for payment processing. Changes
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 as PR comments)
Additionally, you can add 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
Outside diff range, codebase verification and nitpick comments (1)
LDKNodeMonday.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
3-3: Removal ofldk-nodedependency.The
ldk-nodedependency has been removed, but it is still actively imported and used in multiple files. Ensure that all functionality relying on this dependency is updated or replaced accordingly before removing it.
LDKNodeMonday/View Model/Profile/Peer/PeersListViewModel.swiftLDKNodeMonday/View Model/Profile/Peer/DisconnectViewModel.swiftLDKNodeMonday/View Model/Home/StartViewModel.swiftLDKNodeMonday/View Model/Home/OnboardingViewModel.swiftLDKNodeMonday/View Model/Home/BitcoinViewModel.swiftLDKNodeMonday/View Model/Home/AmountViewModel.swiftLDKNodeMonday/View Model/Home/Payments/PaymentsViewModel.swiftLDKNodeMonday/View Model/Profile/NodeIDViewModel.swiftLDKNodeMonday/View Model/Profile/Peer/PeerViewModel.swiftLDKNodeMonday/View Model/Profile/Danger/SeedViewModel.swiftLDKNodeMonday/View Model/Profile/Channel/ChannelsListViewModel.swiftLDKNodeMonday/View/Profile/Channel/ChannelDetailView.swiftLDKNodeMonday/View Model/Profile/Channel/ChannelDetailViewModel.swiftLDKNodeMonday/View Model/Profile/Channel/ChannelAddViewModel.swiftLDKNodeMonday/Service/EventService/EventService.swiftLDKNodeMonday/Service/Lightning Service/LightningServiceError.swiftLDKNodeMonday/Service/KeyService/KeyService.swiftLDKNodeMonday/Service/Lightning Service/LightningNodeService.swiftLDKNodeMonday/Extensions/PaymentKind+Extensions.swiftLDKNodeMonday/View/Profile/Danger/SeedView.swiftLDKNodeMonday/View Model/Home/Receive/ZeroAmountViewModel.swiftLDKNodeMonday/View Model/Home/Receive/JITInvoiceViewModel.swiftLDKNodeMonday/View Model/Home/Receive/Bolt12ZeroInvoiceViewModel.swiftLDKNodeMonday/View Model/Home/Receive/Bolt12InvoiceViewModel.swiftLDKNodeMonday/View Model/Home/Receive/AmountInvoiceViewModel.swiftLDKNodeMonday/View Model/Home/Receive/AddressViewModel.swiftLDKNodeMonday/Extensions/Event+Extensions.swiftLDKNodeMonday/View Model/Home/Receive/BIP21ViewModel.swiftLDKNodeMonday/Extensions/Network+Extensions.swiftLDKNodeMonday/View/Home/Receive/BIP21View.swiftLDKNodeMonday/View/Home/Receive/ZeroInvoiceView.swiftLDKNodeMonday/View/Home/Receive/JITInvoiceView.swiftLDKNodeMonday/View/Home/Receive/Bolt12ZeroInvoiceView.swiftLDKNodeMonday/View/Home/Payments/PaymentsListView.swiftLDKNodeMonday/View/Home/Receive/Bolt12InvoiceView.swiftLDKNodeMonday/View/Home/Receive/AmountInvoiceView.swiftLDKNodeMonday/View/Home/OnboardingView.swiftLDKNodeMonday/Extensions/ChannelDetails+Extensions.swiftLDKNodeMonday/Model/LightningPaymentStatus.swiftLDKNodeMonday/View/Home/StartView.swiftAnalysis chain
Removal of
ldk-nodedependency.The
ldk-nodedependency has been removed. Ensure that all functionality relying on this dependency has been updated or replaced accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the removal of `ldk-node` dependency. # Test: Search for the usage of `ldk-node` in the codebase. Expect: No remaining usages. rg --type swift 'import LDKNode'Length of output: 2941
Script:
#!/bin/bash # Description: Verify the actual usage of `LDKNode` in the identified files. # List of files where `LDKNode` is imported files=( "LDKNodeMonday/View Model/Profile/Peer/PeersListViewModel.swift" "LDKNodeMonday/View Model/Profile/Peer/DisconnectViewModel.swift" "LDKNodeMonday/View Model/Home/StartViewModel.swift" "LDKNodeMonday/View Model/Home/OnboardingViewModel.swift" "LDKNodeMonday/View Model/Home/BitcoinViewModel.swift" "LDKNodeMonday/View Model/Home/AmountViewModel.swift" "LDKNodeMonday/View Model/Home/Payments/PaymentsViewModel.swift" "LDKNodeMonday/View Model/Profile/NodeIDViewModel.swift" "LDKNodeMonday/View Model/Profile/Peer/PeerViewModel.swift" "LDKNodeMonday/View Model/Profile/Danger/SeedViewModel.swift" "LDKNodeMonday/View Model/Profile/Channel/ChannelsListViewModel.swift" "LDKNodeMonday/View/Profile/Channel/ChannelDetailView.swift" "LDKNodeMonday/View Model/Profile/Channel/ChannelDetailViewModel.swift" "LDKNodeMonday/View Model/Profile/Channel/ChannelAddViewModel.swift" "LDKNodeMonday/Service/EventService/EventService.swift" "LDKNodeMonday/Service/Lightning Service/LightningServiceError.swift" "LDKNodeMonday/Service/KeyService/KeyService.swift" "LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift" "LDKNodeMonday/Extensions/PaymentKind+Extensions.swift" "LDKNodeMonday/View/Profile/Danger/SeedView.swift" "LDKNodeMonday/View Model/Home/Receive/ZeroAmountViewModel.swift" "LDKNodeMonday/View Model/Home/Receive/JITInvoiceViewModel.swift" "LDKNodeMonday/View Model/Home/Receive/Bolt12ZeroInvoiceViewModel.swift" "LDKNodeMonday/View Model/Home/Receive/Bolt12InvoiceViewModel.swift" "LDKNodeMonday/View Model/Home/Receive/AmountInvoiceViewModel.swift" "LDKNodeMonday/View Model/Home/Receive/AddressViewModel.swift" "LDKNodeMonday/Extensions/Event+Extensions.swift" "LDKNodeMonday/View Model/Home/Receive/BIP21ViewModel.swift" "LDKNodeMonday/Extensions/Network+Extensions.swift" "LDKNodeMonday/View/Home/Receive/BIP21View.swift" "LDKNodeMonday/View/Home/Receive/ZeroInvoiceView.swift" "LDKNodeMonday/View/Home/Receive/JITInvoiceView.swift" "LDKNodeMonday/View/Home/Receive/Bolt12ZeroInvoiceView.swift" "LDKNodeMonday/View/Home/Payments/PaymentsListView.swift" "LDKNodeMonday/View/Home/Receive/Bolt12InvoiceView.swift" "LDKNodeMonday/View/Home/Receive/AmountInvoiceView.swift" "LDKNodeMonday/View/Home/OnboardingView.swift" "LDKNodeMonday/Extensions/ChannelDetails+Extensions.swift" "LDKNodeMonday/Model/LightningPaymentStatus.swift" "LDKNodeMonday/View/Home/StartView.swift" ) # Search for usage of LDKNode in the identified files for file in "${files[@]}"; do echo "Checking usage in $file" rg 'LDKNode' "$file" doneLength of output: 14210
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- LDKNodeMonday.xcodeproj/project.pbxproj (15 hunks)
- LDKNodeMonday.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (2 hunks)
- LDKNodeMonday/Model/ReceiveOption.swift (1 hunks)
- LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (2 hunks)
- LDKNodeMonday/Service/Lightning Service/LightningServiceError.swift (1 hunks)
- LDKNodeMonday/View Model/Home/Receive/BIP21ViewModel.swift (1 hunks)
- LDKNodeMonday/View/Home/Receive/BIP21View.swift (1 hunks)
- LDKNodeMonday/View/Home/Receive/ReceiveView.swift (1 hunks)
Additional comments not posted (20)
LDKNodeMonday/Model/ReceiveOption.swift (1)
17-17: Addition of new casebip21toReceiveOptionenum.The new case
bip21is consistent with the existing cases and correctly expands the enum to support BIP21.LDKNodeMonday.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
2-2: Update oforiginHashvalue.The
originHashvalue has been updated, reflecting a new state of the package dependencies.LDKNodeMonday/View Model/Home/Receive/BIP21ViewModel.swift (4)
13-15: Good use of@Publishedproperties inBIP21ViewModel.The use of
@Publishedproperties ensures that the UI will be updated when these properties change.
19-42: Proper error handling inreceivePaymentmethod.The
receivePaymentmethod correctly handles errors and updates the UI accordingly. Ensure thatLightningNodeService.shared.receivehandles all necessary cases.
44-46: Clear invoice functionality.The
clearInvoicemethod correctly resets theunifiedproperty.
48-53: Efficient use ofDispatchQueue.main.asyncingetColormethod.The
getColormethod efficiently updates thenetworkColorproperty on the main thread.LDKNodeMonday/View/Home/Receive/ReceiveView.swift (1)
34-35: LGTM! The new case for handlingbip21is well integrated.The addition of the
bip21case enhances the functionality of theReceiveViewby supporting BIP21 invoices.LDKNodeMonday/Service/Lightning Service/LightningServiceError.swift (1)
158-163: LGTM! The new error cases are well handled.The addition of
UriParameterParsingFailedandInvalidUricases enhances the robustness of the error handling in thehandleNodeErrorfunction.LDKNodeMonday/View/Home/Receive/BIP21View.swift (3)
12-17: LGTM! The state variables are well-defined.The state variables
isCopied,showCheckmark,showingReceiveViewErrorAlert, andisKeyboardVisibleare appropriately defined for managing the view's state.
19-167: LGTM! The body property is well-structured.The body property of the
BIP21Viewstruct is well-structured and follows SwiftUI best practices. The view correctly handles different states and user interactions.
173-175: LGTM! The preview provider is correctly defined.The preview provider for the
BIP21Viewstruct is correctly defined, allowing for easy previewing of the view in Xcode.LDKNodeMonday.xcodeproj/project.pbxproj (9)
6-6: Verify compatibility with Xcode version.The
objectVersionhas been updated from 56 to 60. Ensure that this change is compatible with the current Xcode version used in the project.
72-72: Verify integration and inclusion ofBIP21View.swift.Ensure that the new file
BIP21View.swiftis correctly integrated into the project and included in the build process.
73-73: Verify integration and inclusion ofBIP21ViewModel.swift.Ensure that the new file
BIP21ViewModel.swiftis correctly integrated into the project and included in the build process.
74-74: Verify integration and replacement ofLDKNodereference.Ensure that the new reference for
LDKNodeis correctly integrated into the project and that it replaces the old reference.
599-599: Verify integration and replacement ofldk-nodepackage reference.Ensure that the new local package reference for
ldk-nodeis correctly integrated into the project and that it replaces the old remote package reference.
983-983: Verify integration and replacement ofLDKNodereference.Ensure that the new reference for
LDKNodeis correctly integrated into the project and that it replaces the old reference.
635-635: Verify integration and inclusion ofBIP21View.swift.Ensure that the new file
BIP21View.swiftis correctly integrated into the project and included in the build process.
695-695: Verify integration and inclusion ofBIP21ViewModel.swift.Ensure that the new file
BIP21ViewModel.swiftis correctly integrated into the project and included in the build process.
918-921: Verify integration and replacement ofldk-nodepackage reference.Ensure that the new local package reference for
ldk-nodeis correctly integrated into the project and that it replaces the old remote package reference.
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (1)
LDKNodeMonday/View/Home/Receive/BIP21View.swift (1)
78-78: Add a TODO comment or remove commented-out code.The commented-out QRCodeView should be either removed or annotated with a TODO comment explaining why it is commented out.
// TODO: Re-enable QRCodeView once BitcoinUI is updated to support BIP21 // QRCodeView(qrCodeType: .lightning(viewModel.invoice))
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- LDKNodeMonday/View/Home/Receive/BIP21View.swift (1 hunks)
Additional comments not posted (6)
LDKNodeMonday/View/Home/Receive/BIP21View.swift (6)
1-11: LGTM!The header comments and import statements are appropriate and necessary for the file.
226-232: LGTM!The clear invoice button implementation is well-structured and follows SwiftUI conventions.
239-261: LGTM!The
onAppearandonReceivemodifiers are well-implemented and follow SwiftUI conventions.
262-270: LGTM!The alert presentation is well-implemented and follows SwiftUI conventions.
276-280: LGTM!The
UnifiedQRComponentsstruct is well-implemented and follows Swift conventions.
316-318: LGTM!The SwiftUI preview is well-implemented and follows SwiftUI conventions.
| HStack(alignment: .center) { | ||
|
|
||
| VStack(alignment: .leading, spacing: 5.0) { | ||
| Text("On Chain") | ||
| .font(.caption) | ||
| .bold() | ||
| if let components = parseUnifiedQR(viewModel.unified) { | ||
| Text(components.onchain) | ||
| .font(.caption) | ||
| .truncationMode(.middle) | ||
| .lineLimit(1) | ||
| .foregroundColor(.secondary) | ||
| .redacted( | ||
| reason: viewModel.unified.isEmpty ? .placeholder : [] | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| Spacer() | ||
|
|
||
| if let components = parseUnifiedQR(viewModel.unified) { | ||
| Button { | ||
| UIPasteboard.general.string = components.onchain | ||
| onchainIsCopied = true | ||
| onchainShowCheckmark = true | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 2) { | ||
| onchainIsCopied = false | ||
| onchainShowCheckmark = false | ||
| } | ||
| } label: { | ||
| HStack { | ||
| withAnimation { | ||
| Image( | ||
| systemName: onchainShowCheckmark | ||
| ? "checkmark" : "doc.on.doc" | ||
| ) | ||
| .font(.title2) | ||
| .minimumScaleFactor(0.5) | ||
| } | ||
| } | ||
| .bold() | ||
| .foregroundColor(viewModel.networkColor) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider extracting on-chain address view into a separate function.
Extracting the on-chain address view into a separate function can improve readability and maintainability.
@ViewBuilder
private func onchainAddressView(components: UnifiedQRComponents) -> some View {
HStack(alignment: .center) {
VStack(alignment: .leading, spacing: 5.0) {
Text("On Chain")
.font(.caption)
.bold()
Text(components.onchain)
.font(.caption)
.truncationMode(.middle)
.lineLimit(1)
.foregroundColor(.secondary)
.redacted(
reason: viewModel.unified.isEmpty ? .placeholder : []
)
}
Spacer()
Button {
UIPasteboard.general.string = components.onchain
onchainCopyState.isCopied = true
onchainCopyState.showCheckmark = true
DispatchQueue.main.asyncAfter(deadline: .now() + 2) {
onchainCopyState.isCopied = false
onchainCopyState.showCheckmark = false
}
} label: {
HStack {
withAnimation {
Image(
systemName: onchainCopyState.showCheckmark
? "checkmark" : "doc.on.doc"
)
.font(.title2)
.minimumScaleFactor(0.5)
}
}
.bold()
.foregroundColor(viewModel.networkColor)
}
}
.padding()
}
var body: some View {
// Existing code
if let components = parseUnifiedQR(viewModel.unified) {
onchainAddressView(components: components)
}
// Existing code
}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
LDKNodeMonday/View/Home/Receive/ReceiveView.swift (1)
22-35: Verify the impact of commenting out cases in the switch statement.Several cases in the switch statement have been commented out. The commented-out cases for
.bolt12and.bitcoinare referenced in other parts of the codebase, which might lead to broken functionality. Please ensure that these cases are handled properly to avoid any issues.
.bolt12is referenced inLDKNodeMonday/Extensions/PaymentKind+Extensions.swift..bitcoinis referenced in multiple files including:
LDKNodeMonday/Utilities/Constants.swiftLDKNodeMonday/Service/Lightning Service/LightningNodeService.swiftLDKNodeMonday/View Model/Home/OnboardingViewModel.swiftLDKNodeMonday/Extensions/Network+Extensions.swiftAnalysis chain
Verify the impact of commenting out cases in the switch statement.
Several cases in the switch statement have been commented out. Ensure that these changes do not break any functionality in the UI.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of commenting out cases in the switch statement. # Test: Search for the usage of commented-out cases in the switch statement. Expect: No broken references. rg --type swift -A 5 $'case .bolt11Zero' rg --type swift -A 5 $'case .bolt11' rg --type swift -A 5 $'case .bolt12' rg --type swift -A 5 $'case .bitcoin'Length of output: 8023
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- LDKNodeMonday/Model/ReceiveOption.swift (1 hunks)
- LDKNodeMonday/View/Home/Receive/ReceiveView.swift (4 hunks)
Additional context used
SwiftLint
LDKNodeMonday/View/Home/Receive/ReceiveView.swift
[Warning] 57-57: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 59-59: Prefer at least one space after slashes for comments
(comment_spacing)
Additional comments not posted (5)
LDKNodeMonday/Model/ReceiveOption.swift (2)
25-28: LGTM!The changes in the
systemImageNameproperty align with the commented-out enum cases.
11-17: Verify the impact of commenting out enum cases.Several cases in the
ReceiveOptionenum have been commented out. Ensure that these changes do not break any functionality elsewhere in the codebase.Verification successful
No impact found from commenting out enum cases.
The commented-out enum cases in
ReceiveOptiondo not appear to be referenced elsewhere in the codebase, indicating that these changes should not break any functionality.
ReceiveOption.bolt11ZeroReceiveOption.bolt11ReceiveOption.bolt12ReceiveOption.bitcoinScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of commenting out enum cases in `ReceiveOption`. # Test: Search for the usage of commented-out enum cases. Expect: No broken references. rg --type swift -A 5 $'ReceiveOption.bolt11Zero' rg --type swift -A 5 $'ReceiveOption.bolt11' rg --type swift -A 5 $'ReceiveOption.bolt12' rg --type swift -A 5 $'ReceiveOption.bitcoin'Length of output: 180
Script:
#!/bin/bash # Description: Verify the impact of commenting out enum cases in `ReceiveOption`. # Test: Search for the usage of commented-out enum cases across all file types with more context lines. rg 'ReceiveOption.bolt11Zero' -A 10 rg 'ReceiveOption.bolt11' -A 10 rg 'ReceiveOption.bolt12' -A 10 rg 'ReceiveOption.bitcoin' -A 10Length of output: 132
LDKNodeMonday/View/Home/Receive/ReceiveView.swift (3)
79-79: LGTM!The changes in the
#Previewsection align with the shift towards prioritizing BIP21.
11-11: Verify the impact of changing the default value ofselectedOption.The default value of
selectedOptionhas been changed from.bolt11Zeroto.bip21. Ensure that this change does not break any functionality elsewhere in the codebase.
57-59: Verify the impact of commenting out font size settings.The font size settings for images and text have been commented out. Ensure that these changes do not affect the UI appearance negatively.
Tools
SwiftLint
[Warning] 57-57: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 59-59: Prefer at least one space after slashes for comments
(comment_spacing)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- LDKNodeMonday/Extensions/String+Extensions.swift (3 hunks)
- LDKNodeMonday/Model/ReceiveOption.swift (1 hunks)
- LDKNodeMonday/View/Home/Receive/ReceiveView.swift (4 hunks)
Additional comments not posted (6)
LDKNodeMonday/Model/ReceiveOption.swift (2)
11-17: Verify the rationale for commenting out enum cases.Several cases in the
ReceiveOptionenum are commented out. Ensure this aligns with the intended functionality and consider documenting the reason for future reference.
25-28: Verify the impact of default case change insystemImageName.The default case now returns "qrcode" for all unmatched cases. Ensure this change aligns with the intended visual representation of options in the UI.
LDKNodeMonday/View/Home/Receive/ReceiveView.swift (2)
11-11: Verify the impact of changing the defaultselectedOption.The default state for
selectedOptionis now.bip21. Ensure this change does not negatively impact other parts of the application that rely on the previous default.
79-79: Verify the accuracy ofCustomSegmentedPickerpreview.The preview now uses
selectedOption: .constant(.bip21). Ensure this accurately reflects the intended functionality and appearance.LDKNodeMonday/Extensions/String+Extensions.swift (2)
178-182: Verify the correctness of BIP21 amount validation.The
processBitcoinAddressfunction now validates the amount against the spendable balance. Ensure this logic is correct and robust to prevent incorrect transaction processing.
203-207: Verify the correctness of BIP21 URI handling.The
extractBitcoinAddressfunction now handles query parameters in BIP21 URIs. Ensure this logic correctly handles all possible BIP21 URI formats and edge cases.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- LDKNodeMonday.xcodeproj/project.pbxproj (15 hunks)
- LDKNodeMonday.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (2 hunks)
- LDKNodeMonday/View/Home/Receive/BIP21View.swift (1 hunks)
Additional comments not posted (17)
LDKNodeMonday.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (2)
2-2: Verify the impact of the updatedoriginHash.The
originHashhas been updated, which may affect the state of the package dependencies. Ensure that this change aligns with the intended updates and does not introduce any inconsistencies.
9-10: Ensure compatibility with the updatedbitcoinuibranch and revision.The
bitcoinuidependency has been updated to track themainbranch with a new revision. Verify that this change is compatible with the rest of the project and does not introduce breaking changes.LDKNodeMonday/View/Home/Receive/BIP21View.swift (7)
8-13: Imports and struct declaration look good.The necessary modules are imported, and the
BIP21Viewstruct is properly declared.
14-23: Consider consolidating state variables.The state variables for copying and showing checkmarks can be consolidated into a single struct to improve readability and maintainability.
struct CopyState { var isCopied = false var showCheckmark = false } @State private var onchainCopyState = CopyState() @State private var bolt11CopyState = CopyState() @State private var bolt12CopyState = CopyState()
24-72: Refactor nested conditionals and repetitive code.The nested conditionals and repetitive code for handling text fields and buttons can be refactored into separate functions or views to improve readability.
@ViewBuilder private func amountInputView() -> some View { VStack(alignment: .leading) { Text("Sats") .bold() .padding(.horizontal) ZStack { TextField( "0", text: $viewModel.amountSat ) .keyboardType(.numberPad) .submitLabel(.done) .padding(EdgeInsets(top: 0, leading: 0, bottom: 0, trailing: 32)) if !viewModel.amountSat.isEmpty { HStack { Spacer() Button { self.viewModel.amountSat = "" } label: { Image(systemName: "xmark.circle.fill") .foregroundColor(.secondary) } .padding(.trailing, 8) } } } .padding(.horizontal) } .padding() } var body: some View { VStack { if viewModel.unified == "" { amountInputView() Button { Task { let amountSat = (UInt64(viewModel.amountSat) ?? 0) await viewModel.receivePayment( amountSat: amountSat, message: "Monday Wallet", expirySecs: UInt32(3600) ) } } label: { Text("Create Amount Invoice") } } else { // Existing code for handling non-empty viewModel.unified } } }
82-125: Consider extracting on-chain address view into a separate function.Extracting the on-chain address view into a separate function can improve readability and maintainability.
@ViewBuilder private func onchainAddressView(components: UnifiedQRComponents) -> some View { HStack(alignment: .center) { VStack(alignment: .leading, spacing: 5.0) { Text("On Chain") .font(.caption) .bold() Text(components.onchain) .font(.caption) .truncationMode(.middle) .lineLimit(1) .foregroundColor(.secondary) .redacted( reason: viewModel.unified.isEmpty ? .placeholder : [] ) } Spacer() Button { UIPasteboard.general.string = components.onchain onchainCopyState.isCopied = true onchainCopyState.showCheckmark = true DispatchQueue.main.asyncAfter(deadline: .now() + 2) { onchainCopyState.isCopied = false onchainCopyState.showCheckmark = false } } label: { HStack { withAnimation { Image( systemName: onchainCopyState.showCheckmark ? "checkmark" : "doc.on.doc" ) .font(.title2) .minimumScaleFactor(0.5) } } .bold() .foregroundColor(viewModel.networkColor) } } .padding() } var body: some View { // Existing code if let components = parseUnifiedQR(viewModel.unified) { onchainAddressView(components: components) } // Existing code }
130-173: Consider extracting Bolt11 address view into a separate function.Extracting the Bolt11 address view into a separate function can improve readability and maintainability.
@ViewBuilder private func bolt11AddressView(components: UnifiedQRComponents) -> some View { HStack(alignment: .center) { VStack(alignment: .leading, spacing: 5.0) { Text("Bolt 11") .font(.caption) .bold() Text(components.bolt11) .font(.caption) .truncationMode(.middle) .lineLimit(1) .foregroundColor(.secondary) .redacted( reason: viewModel.unified.isEmpty ? .placeholder : [] ) } Spacer() Button { UIPasteboard.general.string = components.bolt11 bolt11CopyState.isCopied = true bolt11CopyState.showCheckmark = true DispatchQueue.main.asyncAfter(deadline: .now() + 2) { bolt11CopyState.isCopied = false bolt11CopyState.showCheckmark = false } } label: { HStack { withAnimation { Image( systemName: bolt11CopyState.showCheckmark ? "checkmark" : "doc.on.doc" ) .font(.title2) .minimumScaleFactor(0.5) } } .bold() .foregroundColor(viewModel.networkColor) } } .padding() } var body: some View { // Existing code if let components = parseUnifiedQR(viewModel.unified) { bolt11AddressView(components: components) } // Existing code }
178-221: Consider extracting Bolt12 address view into a separate function.Extracting the Bolt12 address view into a separate function can improve readability and maintainability.
@ViewBuilder private func bolt12AddressView(components: UnifiedQRComponents) -> some View { HStack(alignment: .center) { VStack(alignment: .leading, spacing: 5.0) { Text("Bolt 12") .font(.caption) .bold() Text(components.bolt12) .font(.caption) .truncationMode(.middle) .lineLimit(1) .foregroundColor(.secondary) .redacted( reason: viewModel.unified.isEmpty ? .placeholder : [] ) } Spacer() Button { UIPasteboard.general.string = components.bolt12 bolt12CopyState.isCopied = true bolt12CopyState.showCheckmark = true DispatchQueue.main.asyncAfter(deadline: .now() + 2) { bolt12CopyState.isCopied = false bolt12CopyState.showCheckmark = false } } label: { HStack { withAnimation { Image( systemName: bolt12CopyState.showCheckmark ? "checkmark" : "doc.on.doc" ) .font(.title2) .minimumScaleFactor(0.5) } } .bold() .foregroundColor(viewModel.networkColor) } } .padding() } var body: some View { // Existing code if let components = parseUnifiedQR(viewModel.unified) { bolt12AddressView(components: components) } // Existing code }
282-314: Improve readability and error handling inparseUnifiedQRfunction.The
parseUnifiedQRfunction can be refactored for better readability and error handling.func parseUnifiedQR(_ unifiedQR: String) -> UnifiedQRComponents? { // Split the string by '?' let components = unifiedQR.components(separatedBy: "?") guard components.count > 1 else { return nil } // Extract onchain (everything before the first '?') and remove the "BITCOIN:" prefix var onchain = components[0] if onchain.lowercased().hasPrefix("bitcoin:") { onchain = String(onchain.dropFirst(8)) // Remove "BITCOIN:" } // Join the rest of the components back together let remainingString = components.dropFirst().joined(separator: "?") // Split the remaining string by '&' let params = remainingString.components(separatedBy: "&") var bolt11: String? var bolt12: String? for param in params { if param.starts(with: "lightning=") { bolt11 = String(param.dropFirst("lightning=".count)) } else if param.starts(with: "lno=") { bolt12 = String(param.dropFirst("lno=".count)) } } guard let bolt11 = bolt11, let bolt12 = bolt12 else { return nil } return UnifiedQRComponents(onchain: onchain, bolt11: bolt11, bolt12: bolt12) }LDKNodeMonday.xcodeproj/project.pbxproj (8)
6-6: Verify compatibility with Xcode version.The
objectVersionhas been updated from 56 to 60. Ensure this change is compatible with the Xcode version used for this project.
72-73: Addition of BIP21 files is aligned with PR objectives.The new file references for
BIP21View.swiftandBIP21ViewModel.swiftare correctly added to the project. Ensure these files are implemented and tested.Also applies to: 159-160, 290-290, 386-386, 635-635, 695-695
74-74: Ensure correct integration ofLDKNode.The reference for
LDKNodehas been updated. Verify that the integration is correct and aligns with the project's requirements.Also applies to: 196-196, 564-564, 983-985
917-921: Verify local package path forldk-node/bindings/swift.The project now uses a local package reference for
ldk-node/bindings/swift. Ensure the path is correct and accessible during the build process.
599-599: Ensure no dependencies on removed remote package reference.The remote package reference for
ldk-nodehas been removed. Verify that all dependencies are correctly managed with the new local reference.
937-937: Verify stability ofBitcoinUImain branch.The branch for
BitcoinUIis specified asmain. Ensure this branch is stable and intended for use in the project.
983-985: Ensure consistency with local package reference forLDKNode.The
XCSwiftPackageProductDependencyforLDKNodehas been updated. Ensure consistency with the local package reference.
Line range hint
6-986: Overall project structure changes approved.The project structure has been updated with new files and dependencies. Ensure thorough testing of the build process to verify that no issues have been introduced.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
LDKNodeMonday/View Model/Home/AmountViewModel.swift (1)
107-107: Ensure consistent error handling across methods.While logging unexpected errors is good, consider standardizing error messages or handling mechanisms across different payment methods for consistency.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- LDKNodeMonday/Extensions/String+Extensions.swift (5 hunks)
- LDKNodeMonday/View Model/Home/AmountViewModel.swift (3 hunks)
- LDKNodeMonday/View/Home/Receive/BIP21View.swift (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- LDKNodeMonday/View/Home/Receive/BIP21View.swift
Additional comments not posted (6)
LDKNodeMonday/View Model/Home/AmountViewModel.swift (3)
93-94: Logging of successful payment ID is a good practice.Logging the payment ID upon successful transaction enhances traceability and debugging capabilities.
96-96: Improved error visibility with specific logging.Logging the localized description of
NodeErrordirectly helps in identifying node-related issues more quickly.
124-124: Case-insensitive check enhances flexibility.Modifying the condition to be case-insensitive by using
lowercased()allows for more flexible input handling which is beneficial in environments with variable input formats.LDKNodeMonday/Extensions/String+Extensions.swift (3)
13-74: Enhanced regex handling and detailed logging inbolt11amount.The updated regex pattern allows for more flexibility in parsing Bolt11 amounts, and the detailed logging at each step aids in debugging. Ensure that the regex is tested thoroughly to handle all expected formats.
Tools
SwiftLint
[Error] 16-16: Force tries should be avoided
(force_try)
286-288: Proper handling of query parameters in Bitcoin addresses.The update to handle query parameters when extracting Bitcoin addresses from BIP21 URIs is a necessary improvement for accurate address processing.
183-225: Comprehensive handling of BIP21 URIs inprocessBIP21.The function systematically processes BIP21 URIs, extracting relevant payment information. It prioritizes different payment types and handles query parameters effectively.
However, ensure that all edge cases and potential error scenarios are covered by unit tests, especially for malformed URIs and unexpected query parameters.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- LDKNodeMonday/Extensions/String+Extensions.swift (4 hunks)
- LDKNodeMonday/Model/ReceiveOption.swift (1 hunks)
- LDKNodeMonday/View Model/Home/AmountViewModel.swift (2 hunks)
- LDKNodeMonday/View/Home/Receive/BIP21View.swift (1 hunks)
- LDKNodeMonday/View/Home/Receive/ReceiveView.swift (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- LDKNodeMonday/Model/ReceiveOption.swift
- LDKNodeMonday/View Model/Home/AmountViewModel.swift
- LDKNodeMonday/View/Home/Receive/BIP21View.swift
- LDKNodeMonday/View/Home/Receive/ReceiveView.swift
Additional comments not posted (3)
LDKNodeMonday/Extensions/String+Extensions.swift (3)
37-53: Logic for amount conversion approved.The logic to convert the amount based on the multiplier is correctly implemented and aligns with Bitcoin denomination standards.
164-198: Logic for processing BIP21 URIs approved.The function correctly handles different query parameters and prioritizes Bolt 12 offers over Bolt 11 invoices. The use of
String(format:)for formatting the Bitcoin amount is appropriate and consistent with the overall handling of Bitcoin denominations.
245-247: Logic for extracting Bitcoin addresses approved.The function correctly handles the extraction of the Bitcoin address from a BIP21 URI, ensuring that only the address portion is returned. The use of
uppercased()to normalize the address is appropriate and aligns with common practices for address handling.
Description
Adding BIP21 support per lightningdevkit/ldk-node#302
Using locally generated bindings of ldk-node for now
[x] Receive
[x] Send
[x] Remove Bolt11
[x] Remove Bolt12
[x] Remove OnChain
[x] Add QRCodeView back once BitcoinUI updated for bip21 reez/BitcoinUI#101
Notes to the reviewers
Changelog notice
Checklists
All Submissions:
.swift-formatfileNew Features:
Bugfixes:
Summary by CodeRabbit
New Features
Bug Fixes
Chores