Re: [PATCH damus] Refactor wallet invoice URL handling

[William Casarin]

On Wed, May 07, 2025 at 12:13:33PM -0700, Daniel D’Aquino wrote:
>This is a minor refactor on the way wallet invoice URLs are handled, in
>order to better fit the interface, enforce the design pattern, and avoid
>side-effects in a particular function that handles opening URLs.
>
>This design pattern was introduced to prevent issues on the previous
>pattern, where URL handling was done with side-effects inside multiple
>levels of nested logic and separate function calls, which would make
>debugging very difficult, and cause the app to fail silently.
>
>Closes: https://github.com/damus-io/damus/issues/3023
>Signed-off-by: Daniel D’Aquino <dan...@daquino.me>
>
>Closes: https://github.com/damus-io/damus/pull/3024
>---

LGTM!

Reviewed-by: William Casarin <jb...@jb55.com>

> damus/Components/InvoiceView.swift | 16 ++++++++++------
> damus/ContentView.swift | 4 ++++
> damus/Models/URLHandler.swift | 7 ++-----
> 3 files changed, 16 insertions(+), 11 deletions(-)
>
>diff --git a/damus/Components/InvoiceView.swift b/damus/Components/InvoiceView.swift
>index 36172a67a..b82191523 100644
>--- a/damus/Components/InvoiceView.swift
>+++ b/damus/Components/InvoiceView.swift
>@@ -94,26 +94,30 @@ enum OpenWalletError: Error {
> }
>
> func open_with_wallet(wallet: Wallet.Model, invoice: String) throws {
>+ let url = try getUrlToOpen(invoice: invoice, with: wallet)
>+ this_app.open(url)
>+}
>+
>+func getUrlToOpen(invoice: String, with wallet: Wallet.Model) throws(OpenWalletError) -> URL {
> if let url = URL(string: "\(wallet.link)\(invoice)"), this_app.canOpenURL(url) {
>- this_app.open(url)
>+ return url
> } else {
> guard let store_link = wallet.appStoreLink else {
>- throw OpenWalletError.no_wallet_to_open
>+ throw .no_wallet_to_open
> }
>
> guard let url = URL(string: store_link) else {
>- throw OpenWalletError.store_link_invalid
>+ throw .store_link_invalid
> }
>
> guard this_app.canOpenURL(url) else {
>- throw OpenWalletError.system_cannot_open_store_link
>+ throw .system_cannot_open_store_link
> }
>
>- this_app.open(url)
>+ return url
> }
> }
>
>-
> let test_invoice = Invoice(description: .description("this is a description"), amount: .specific(10000), string: "lnbc100n1p357sl0sp5t9n56wdztun39lgdqlr30xqwksg3k69q4q2rkr52aplujw0esn0qpp5mrqgljk62z20q4nvgr6lzcyn6fhylzccwdvu4k77apg3zmrkujjqdpzw35xjueqd9ejqcfqv3jhxcmjd9c8g6t0dcxqyjw5qcqpjrzjqt56h4gvp5yx36u2uzqa6qwcsk3e2duunfxppzj9vhypc3wfe2wswz607uqq3xqqqsqqqqqqqqqqqlqqyg9qyysgqagx5h20aeulj3gdwx3kxs8u9f4mcakdkwuakasamm9562ffyr9en8yg20lg0ygnr9zpwp68524kmda0t5xp2wytex35pu8hapyjajxqpsql29r", expiry: 604800, payment_hash: Data(), created_at: 1666139119)
>
> struct InvoiceView_Previews: PreviewProvider {
>diff --git a/damus/ContentView.swift b/damus/ContentView.swift
>index d69ea659f..83f98a097 100644
>--- a/damus/ContentView.swift
>+++ b/damus/ContentView.swift
>@@ -742,6 +742,8 @@ struct ContentView: View {
> case route(Route)
> /// Open a sheet
> case sheet(Sheets)
>+ /// Open an external URL
>+ case external_url(URL)
> /// Do nothing.
> ///
> /// ## Implementation notes
>@@ -758,6 +760,8 @@ struct ContentView: View {
> navigationCoordinator.push(route: route)
> case .sheet(let sheet):
> self.active_sheet = sheet
>+ case .external_url(let url):
>+ this_app.open(url)
> case .no_action:
> return
> }
>diff --git a/damus/Models/URLHandler.swift b/damus/Models/URLHandler.swift
>index 4feec53d9..90d8ca62c 100644
>--- a/damus/Models/URLHandler.swift
>+++ b/damus/Models/URLHandler.swift
>@@ -47,13 +47,10 @@ struct DamusURLHandler {
> if damus_state.settings.show_wallet_selector {
> return .sheet(.select_wallet(invoice: invoice.string))
> } else {
>- do {
>- try open_with_wallet(wallet: damus_state.settings.default_wallet.model, invoice: invoice.string)
>- return .no_action
>- }
>- catch {
>+ guard let url = try? getUrlToOpen(invoice: invoice.string, with: damus_state.settings.default_wallet.model) else {
> return .sheet(.select_wallet(invoice: invoice.string))
> }
>+ return .external_url(url)
> }
> case nil:
> break