Return invoice on successful lnurl-withdraw #436

ok300 · 2023-09-07T10:44:59Z

Include the LNInvoice as inner data in the Ok result of BreezServices::lnurl_withdraw.

Subtasks:

uniffi
flutter bridge
React Native bindings
Store optional field in list_payments
Add c-breez PR: Apply mandatory payment status and lnurlWithdraw changes c-breez#630

Fixes #427

ok300 · 2023-09-07T10:50:35Z

@roeierez it's ready for review.

I will add the RN bindings as last step, once we agree on the general structure.

dangeross · 2023-09-07T11:04:32Z

I will add the RN bindings as last step, once we agree on the general structure.

It could be the first use of the RN codegen 😎

ok300 · 2023-09-07T11:07:23Z

The PR simply adds the invoice in the withdraw Ok response.

However, if the goal is to be able to link a payment with a LNURL-withdraw request, an alternative way to do this is to extend LnPaymentDetails, which is a field of Payment

breez-sdk/libs/sdk-core/src/models.rs

Lines 661 to 678 in 41b3a01

    
           pub struct LnPaymentDetails { 
        
               pub payment_hash: String, 
        
               pub label: String, 
        
               pub destination_pubkey: String, 
        
               pub payment_preimage: String, 
        
               pub keysend: bool, 
        
               pub bolt11: String, 
        
               /// Only set for [PaymentType::Sent] payments that are part of a LNURL-pay workflow where 
        
               /// the endpoint returns a success action 
        
               pub lnurl_success_action: Option<SuccessActionProcessed>, 
        
               /// Only set for [PaymentType::Sent] payments that are sent to a Lightning Address 
        
               pub ln_address: Option<String>, 
        
               /// Only set for [PaymentType::Sent] payments where the receiver endpoint returned LNURL metadata 
        
               pub lnurl_metadata: Option<String>, 
        
           }

with an lnurl_withdraw_endpoint: Optional<String>, where we can store the LNURL-w endpoint associated with this Payment.

That may be preferable to the approach in this PR, because with the PR, our LNURL handling becomes inconsistent:

lnurl_pay service call result has no reference to the invoice, but LnPaymentDetails has a few pay-related optional fields:

breez-sdk/libs/sdk-core/src/models.rs

Lines 669 to 677 in 41b3a01

    
           /// Only set for [PaymentType::Sent] payments that are part of a LNURL-pay workflow where 
        
           /// the endpoint returns a success action 
        
           pub lnurl_success_action: Option<SuccessActionProcessed>, 
        
           /// Only set for [PaymentType::Sent] payments that are sent to a Lightning Address 
        
           pub ln_address: Option<String>, 
        
           /// Only set for [PaymentType::Sent] payments where the receiver endpoint returned LNURL metadata 
        
           pub lnurl_metadata: Option<String>,

lnurl_withdraw has the reverse: invoice in the service call result, but no reference in the Payment

What do you think? Is the PR approach ok? Or better switch to the above? Or do both?

danielgranhao · 2023-09-08T08:56:01Z

If possible, as a user, I would like to have both :) that way, I can avoid always searching for the correct payment after making an LNURL withdrawal.

Otherwise, the withdrawal flow is:

Start withdrawal
Search for the created payment associated with the withdrawal
Wait for BreezEvent::InvoicePaid that includes the hash of the withdrawal payment

If we have what the current version of this PR proposes, the middle step can be discarded.

ok300 · 2023-09-15T13:51:03Z

@dangeross I took your RN generator for a test-drive and ran it on this PR.

Can you please doublecheck if all looks good? fe8884d

dangeross

Looks good in general. There seems to be lots of linting changes with the kotlin files though. Can you give me your ktlint and kotlin versions?

ktlint --version
kotlin -version

ok300 · 2023-09-15T14:34:42Z

ktlint --version

1.0.0

kotlin -version

Kotlin version 1.9.10-release-459 (JRE 20.0.2)

dangeross · 2023-09-15T14:43:28Z

ktlint --version

1.0.0

Ok, seems to be a version difference. I've upgraded to 1.0.0

roeierez · 2023-09-17T05:54:55Z

@ok300 the PR looks good to me. As for your comment above do you want to add the step of associated the withdraw endpoint with the payment in the storage or not? Another option is in a separate PR.

roeierez

LGTM! We need a corresponding c-breez PR as I believe this breaks it.

ok300 · 2023-09-18T08:53:00Z

If possible, as a user, I would like to have both :)

Then I'll add both in this PR.

roeierez

Looks good! Dropped a NIT comment.

roeierez · 2023-09-20T20:01:23Z

libs/sdk-core/src/models.rs

@@ -1114,6 +1117,18 @@ pub enum LnUrlCallbackStatus {
    },
 }

+/// [LnUrlCallbackStatus] specific to LNURL-withdraw, where the success case contains the invoice.
+#[derive(Serialize)]
+pub enum LnUrlWithdrawCallbackStatus {


Perhaps if we are introducing new types here we can use better naming?
LnUrlWithdrawCallbackStatus => LnUrlWithdrawResult
LnUrlWithdrawOkData => LnurlWithdrawSuccess
LnUrlErrorData => LnurlError

Persist failed payments breez/breez-sdk#463 Return invoice on successful lnurl-withdraw breez/breez-sdk#436

erdemyerebasmaz · 2023-09-21T12:51:49Z

@ok300 @roeierez
Shouldn't we apply these naming changes to LnUrlCallbackStatus used on lnurl_auth as well?

ok300 · 2023-09-21T12:57:41Z

@erdemyerebasmaz how about LnUrlCallbackStatus => LnUrlCallbackResult ?

It's used not only in LNURL-auth, but also as intermediary step in LNURL-withdraw.

erdemyerebasmaz · 2023-09-21T13:03:17Z

@erdemyerebasmaz how about LnUrlCallbackStatus => LnUrlCallbackResult ?

It's used not only in LNURL-auth, but also as intermediary step in LNURL-withdraw.

@ok300 I see, isn't it supposed to be LnUrlResult according to:

Perhaps if we are introducing new types here we can use better naming?
LnUrlWithdrawCallbackStatus => LnUrlWithdrawResult
LnUrlWithdrawOkData => LnurlWithdrawSuccess
LnUrlErrorData => LnurlError

The naming was fine and consistent with other parts of the app before these changes imo.

ok300 · 2023-09-21T13:07:20Z

Well, there are 2 naming conventions which don't overlap:

one is for the LNURL-withdraw specific case. Calling the struct LnUrlWithdrawResult makes sense.
one is for a generic case that affects several callback types. LnUrlResult gives the wrong impression that its what a LNURL-workflow is supposed to end in, but it's only the case for auth. It's an intermediary result in withdraw. IMO LnUrlCallbackResult better captures the nature if it.

erdemyerebasmaz · 2023-09-21T13:12:08Z

Well, there are 2 naming conventions which don't overlap:

one is for the LNURL-withdraw specific case. Calling the struct LnUrlWithdrawResult makes sense.

one is for a generic case that affects several callback types. LnUrlResult gives the wrong impression that its what a LNURL-workflow is supposed to end in, but it's only the case for auth. It's an intermediary result in withdraw. IMO LnUrlCallbackResult better captures the nature if it.

Thanks for the clarification, second item was what made it confusing for me. If that's the case, I see no issue with keeping LnUrlCallbackStatus as it is.

ok300 · 2023-09-21T13:17:48Z

Then @roeierez this PR is ready for final review.

I also linked the c-breez PR by @erdemyerebasmaz in the description above.

Persist failed payments breez/breez-sdk#463 Return invoice on successful lnurl-withdraw breez/breez-sdk#436

dangeross

Looks good!

roeierez

LGTM

Persist failed payments breez/breez-sdk#463 Return invoice on successful lnurl-withdraw breez/breez-sdk#436

ok300 requested a review from roeierez September 7, 2023 10:45

ok300 force-pushed the ok300-lnurl-withdraw-extend-ok branch from 085f429 to 145fa91 Compare September 7, 2023 10:46

ok300 force-pushed the ok300-lnurl-withdraw-extend-ok branch 2 times, most recently from 08aaabf to 335e61f Compare September 15, 2023 13:45

ok300 requested a review from dangeross September 15, 2023 13:51

dangeross approved these changes Sep 15, 2023

View reviewed changes

roeierez approved these changes Sep 17, 2023

View reviewed changes

ok300 force-pushed the ok300-lnurl-withdraw-extend-ok branch 2 times, most recently from e779168 to fd8c3d7 Compare September 20, 2023 16:20

ok300 requested review from roeierez and dangeross September 20, 2023 16:29

roeierez requested changes Sep 20, 2023

View reviewed changes

ok300 added the c-breez label Sep 21, 2023

erdemyerebasmaz force-pushed the ok300-lnurl-withdraw-extend-ok branch from 35d0171 to cdd81b8 Compare September 21, 2023 11:55

erdemyerebasmaz added a commit to breez/c-breez that referenced this pull request Sep 21, 2023

Apply payment status and lnurlWithdraw changes

a13aca7

Persist failed payments breez/breez-sdk#463 Return invoice on successful lnurl-withdraw breez/breez-sdk#436

erdemyerebasmaz mentioned this pull request Sep 21, 2023

Apply mandatory payment status and lnurlWithdraw changes breez/c-breez#630

Merged

ok300 requested a review from roeierez September 21, 2023 13:18

ok300 and others added 8 commits September 21, 2023 15:20

Return invoice on successful lnurl-withdraw

4304053

Add React Native bindings

d5c1f02

Store LNURL-w endpoint in payment metadata

3c0e3c3

Update RN bindings

0171926

Update return type of lnurlWithdraw on sdk-flutter

412dede

Rename LnUrlWithdrawCallbackStatus -> LnUrlWithdrawResult

65d4caa

Rename LnUrlWithdrawOkData -> LnUrlWithdrawSuccessData

7981df6

Update RN bindings

d8ccf23

ok300 force-pushed the ok300-lnurl-withdraw-extend-ok branch from 8e1096e to d8ccf23 Compare September 21, 2023 13:21

erdemyerebasmaz added a commit to breez/c-breez that referenced this pull request Sep 21, 2023

Apply payment status and lnurlWithdraw changes

836754a

Persist failed payments breez/breez-sdk#463 Return invoice on successful lnurl-withdraw breez/breez-sdk#436

erdemyerebasmaz mentioned this pull request Sep 21, 2023

Add Android example breez/breez-sdk-examples#1

Merged

dangeross approved these changes Sep 21, 2023

View reviewed changes

roeierez approved these changes Sep 22, 2023

View reviewed changes

ok300 merged commit 9420ed5 into main Sep 23, 2023
5 checks passed

ok300 deleted the ok300-lnurl-withdraw-extend-ok branch September 23, 2023 07:55

erdemyerebasmaz added a commit to breez/c-breez that referenced this pull request Sep 25, 2023

Apply payment status and lnurlWithdraw changes

8b07abf

Persist failed payments breez/breez-sdk#463 Return invoice on successful lnurl-withdraw breez/breez-sdk#436

erdemyerebasmaz added a commit to breez/c-breez that referenced this pull request Sep 25, 2023

Apply payment status and lnurlWithdraw changes

a1247ef

Persist failed payments breez/breez-sdk#463 Return invoice on successful lnurl-withdraw breez/breez-sdk#436

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return invoice on successful lnurl-withdraw #436

Return invoice on successful lnurl-withdraw #436

ok300 commented Sep 7, 2023 •

edited

ok300 commented Sep 7, 2023

dangeross commented Sep 7, 2023

ok300 commented Sep 7, 2023 •

edited

danielgranhao commented Sep 8, 2023

ok300 commented Sep 15, 2023

dangeross left a comment

ok300 commented Sep 15, 2023

dangeross commented Sep 15, 2023

roeierez commented Sep 17, 2023

roeierez left a comment

ok300 commented Sep 18, 2023

roeierez left a comment

roeierez Sep 20, 2023

erdemyerebasmaz commented Sep 21, 2023

ok300 commented Sep 21, 2023

erdemyerebasmaz commented Sep 21, 2023 •

edited

ok300 commented Sep 21, 2023

erdemyerebasmaz commented Sep 21, 2023

ok300 commented Sep 21, 2023

dangeross left a comment

roeierez left a comment

Return invoice on successful lnurl-withdraw #436

Return invoice on successful lnurl-withdraw #436

Conversation

ok300 commented Sep 7, 2023 • edited

ok300 commented Sep 7, 2023

dangeross commented Sep 7, 2023

ok300 commented Sep 7, 2023 • edited

danielgranhao commented Sep 8, 2023

ok300 commented Sep 15, 2023

dangeross left a comment

Choose a reason for hiding this comment

ok300 commented Sep 15, 2023

dangeross commented Sep 15, 2023

roeierez commented Sep 17, 2023

roeierez left a comment

Choose a reason for hiding this comment

ok300 commented Sep 18, 2023

roeierez left a comment

Choose a reason for hiding this comment

roeierez Sep 20, 2023

Choose a reason for hiding this comment

erdemyerebasmaz commented Sep 21, 2023

ok300 commented Sep 21, 2023

erdemyerebasmaz commented Sep 21, 2023 • edited

ok300 commented Sep 21, 2023

erdemyerebasmaz commented Sep 21, 2023

ok300 commented Sep 21, 2023

dangeross left a comment

Choose a reason for hiding this comment

roeierez left a comment

Choose a reason for hiding this comment

ok300 commented Sep 7, 2023 •

edited

ok300 commented Sep 7, 2023 •

edited

erdemyerebasmaz commented Sep 21, 2023 •

edited