-
Notifications
You must be signed in to change notification settings - Fork 1
Test/lnurl #266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test/lnurl #266
Conversation
f111656 to
7c9b838
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds comprehensive UI testing support for LNURL functionality (pay and withdraw flows) and refactors navigation patterns in LNURL withdraw components. The changes focus on improving testability through accessibility identifiers, enhancing debugging capabilities with sheet lifecycle logging, and standardizing navigation patterns using closure callbacks instead of binding propagation.
Key changes include:
- Refactored LNURL withdraw views to use closure-based navigation instead of @binding navigationPath
- Added extensive accessibility identifiers for UI testing across LNURL flows
- Enhanced SheetViewModel with detailed logging for debugging sheet lifecycle
- Normalized LNURL pay data from millisatoshis to satoshis for consistency
- Added automatic peer connection for LNURL channels
- Improved UX by pre-filling minimum amounts in LNURL amount entry screens
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Bitkit/Views/Wallets/Send/SendSheet.swift | Added LNURL withdraw routes with closure-based navigation |
| Bitkit/Views/Wallets/Send/SendAmountView.swift | Added test identifier to NumberPadTextField |
| Bitkit/Views/Wallets/Send/LnurlPayConfirm.swift | Added test identifiers, keyboard handling, and improved comment field validation |
| Bitkit/Views/Wallets/Send/LnurlPayAmount.swift | Added test identifiers and auto-populate minimum amount on view appear |
| Bitkit/Views/Wallets/LnurlWithdraw/LnurlWithdrawSheet.swift | Refactored to use closure callbacks instead of navigationPath binding |
| Bitkit/Views/Wallets/LnurlWithdraw/LnurlWithdrawFailure.swift | Removed navigationPath binding parameter |
| Bitkit/Views/Wallets/LnurlWithdraw/LnurlWithdrawConfirm.swift | Replaced navigationPath binding with onFailure closure, added test identifiers |
| Bitkit/Views/Wallets/LnurlWithdraw/LnurlWithdrawAmount.swift | Replaced navigationPath binding with onContinue closure, added test identifiers and auto-populate |
| Bitkit/Views/Transfer/LnurlChannel.swift | Added automatic peer connection attempt when fetching channel info |
| Bitkit/Views/Transfer/FundManualSuccessView.swift | Added accessibility identifiers for testing |
| Bitkit/Views/Sheets/LnurlAuth/LnurlAuthSheet.swift | Added accessibility identifiers to buttons |
| Bitkit/ViewModels/SheetViewModel.swift | Enhanced with detailed logging including caller context, added hideSheetIfActive method |
| Bitkit/ViewModels/AppViewModel.swift | Normalized LNURL pay data to satoshis, added accessibility identifiers to toasts |
| Bitkit/Utilities/PaymentNavigationHelper.swift | Added routing logic for LNURL withdraw flows |
| Bitkit/Utilities/Lnurl.swift | Refactored channel request to use createChannelRequestUrl function |
| Bitkit/MainNavView.swift | Updated to use hideSheetIfActive for scanner sheet dismissal |
| Bitkit/Components/TextField.swift | Added submitLabel parameter with default value |
| Bitkit/Components/NumberPadTextField.swift | Added optional testIdentifier parameter |
Comments suppressed due to low confidence (1)
Bitkit/ViewModels/AppViewModel.swift:299
- LNURL withdraw data should be normalized to satoshis (like LNURL pay data) for consistency. The
lnurlWithdrawDatais stored with millisatoshi values, requiring division by 1000 in every view that uses it (seeLnurlWithdrawAmount.swiftlines 12, 16). Consider normalizingminWithdrawableandmaxWithdrawableby dividing by 1000 before storing, similar to howlnurlPayDatais normalized at lines 252-254.
private func handleLnurlWithdraw(_ data: LnurlWithdrawData) {
// Check if lightning service is running
guard lightningService.status?.isRunning == true else {
toast(type: .error, title: "Lightning not running", description: "Please try again later.")
return
}
// Check if minWithdrawable > maxWithdrawable
if (data.minWithdrawable ?? 1000) > data.maxWithdrawable {
toast(
type: .warning,
title: t("other__lnurl_withdr_error"),
description: t("other__lnurl_withdr_error_minmax")
)
return
}
// Check if we have enough receiving capacity
let lightningBalance = lightningService.balances?.totalLightningBalanceSats ?? 0
if lightningBalance < (data.minWithdrawable ?? 1000) / 1000 {
toast(
type: .warning,
title: t("other__lnurl_withdr_error"),
description: t("other__lnurl_withdr_error_no_capacity")
)
return
}
lnurlWithdrawData = data
2e330fc to
7f2dbaa
Compare
7f2dbaa to
a0bf1f6
Compare
jvsena42
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, haven't tested yet
| case lnurlWithdrawAmount | ||
| case lnurlWithdrawConfirm | ||
| case lnurlWithdrawFailure(amount: UInt64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the reasoning of having LNURL Withdraw screens here and the bug that necessitates it, but conceptually they don't belong in the send flow. We might want to revisit this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tAck
Tested:
- 🟢 LNURL P1ay without amounts
- 🟢 LNURL Pay with
minSendable=maxSendableand quickpay ON - 🟢 LNURL Channel
- 🟢 LNURL Widthdraw
- 🟢 LNURL Widthdraw with
{min,max}Withdrawable- updated bitkit-docker with guide and support for these params
Otherwise LGTM
Description
Changes to enable
@lnurle2e test.Linked Issues/Tasks
Screenshot / Video
Insert relevant screenshot / recording