Add payment hash to `lnurl_pay()` return type #550

dleutenegger · 2023-10-24T08:49:09Z

Resolves #540

erdemyerebasmaz · 2023-10-24T16:07:18Z

libs/sdk-react-native/ios/BreezSDKMapper.swift


        throw SdkError.Generic(message: "Invalid enum variant \(type) for enum LnUrlPayResult")
    }

    static func dictionaryOf(lnUrlPayResult: LnUrlPayResult) -> [String: Any?] {
        switch lnUrlPayResult {
        case let .endpointSuccess(
-            data
+            datapaymentHash


erdemyerebasmaz · 2023-10-24T16:07:23Z

libs/sdk-react-native/ios/BreezSDKMapper.swift

@@ -3018,6 +3023,15 @@ class BreezSDKMapper {
                "type": "endpointError",
                "data": dictionaryOf(lnUrlErrorData: data),
            ]
+
+        case let .payError(
+            paymentHashreason


Seemed to be an issue with the rn codegen. I moved the LnUrlPayResult data into wrapper structs, which resolved the issue.

I'll investigate the RN codegen issue

I think your change is the preferred (common) way we handle interface variants anyway. But it seems variants with multiple tuple/struct like data attributes are not being rendered correctly in swift.

dangeross

Looks good otherwise. Just a comment around naming

dangeross · 2023-10-25T10:25:34Z

libs/sdk-core/src/breez_services.rs

-                    data: maybe_sa_processed,
+                    data: LnUrlSuccessData {
+                        payment_hash: details.payment_hash.clone(),
+                        data: maybe_sa_processed,


Can I propose we change data to success_action to avoid data.data?

Would make sense IMO.

ok300

Left a few comments, otherwise looks good!

ok300 · 2023-10-25T11:03:37Z

libs/sdk-core/src/lnurl/pay.rs

-        },
+        EndpointSuccess { data: LnUrlSuccessData },
+        EndpointError { data: LnUrlErrorData },
+        PayError { data: LnUrlPayErrorData },


@dleutenegger Useful new error type! Can you please add a line about it in the method's rustdoc?

This makes me think that perhaps the right way here is to return the errors as rust errors as right now we are mixing both success and error in the success case (We don't use rust error here).
I do understand the change as the mixture has been existed before this change so I am OK to leave it as is but perhaps after we merge the error code PR @dangeross we can think of changing here to use rust errors if that make sense of course?

I don't think the error code PR would give a clean way to return the payment hash (and the reason) of the failed payment, which was the intent of the original issue.

ok300 · 2023-10-25T11:04:45Z

libs/sdk-bindings/src/breez_sdk.udl

@@ -444,10 +444,20 @@ dictionary BitcoinAddressData {
    string? message;
 };

+dictionary LnUrlSuccessData {


This only applies to LNURL-pay, so a more accurate name is LnUrlPaySuccessData IMO.

ok300 · 2023-10-25T11:11:12Z

libs/sdk-core/src/breez_services.rs

+                    Err(SdkError::SendPaymentFailed { err }) => {
+                        let invoice = parse_invoice(cb.pr.as_str())?;
+
+                        return Ok(LnUrlPayResult::PayError {


Two points here:

for consistency, I would remove the return ...; and just place the Ok(..) on the last line of the block, just like the other two branches of match

is this really an Ok(..) and not an Err(..)?

I think this needs to stay as is.

The return is explicitly required as we want to exit the function and return the PayError to the caller.

It must be an Ok(..) instead of an Err(..) as this is the current way the method handles errors, as pointed out by @roeierez Add payment hash to lnurl_pay() return type #550 (comment).

The return is explicitly required as we want to exit the function and return the PayError to the caller.

We unwrap the result (and throw this error as well, if it would be wrapped as Err) just a few lines below in L317.

It must be an Ok(..) instead of an Err(..) as this is the current way the method handles errors, as pointed out by @roeierez Add payment hash to lnurl_pay() return type #550 (comment).

The current way (before the PR): we are not mixing success and error in the success case

The current way (in this PR): we are mixing them with the Ok(..) I mentioned above

So changing the Ok(..) into an Err(..) would address Roei's point around mixing result types. It would also make future migrations to SdkError simpler IMO.

Nevermind, my bad.

You want to return LnUrlPayResult in the success case, so Ok(..) is correct.

The generic anyhow::Error thrown now only takes a string as arg, so cannot be used to wrap anything more meaningful. The send_payment step already throws an SdkError, but that's converted back to anyhow::Error when unwrapping on L317.

All this can be handled better with SdkError in the future.

ok300 · 2023-10-25T11:12:44Z

libs/sdk-core/src/breez_services.rs

-                    data: maybe_sa_processed,
+                    data: LnUrlSuccessData {
+                        payment_hash: details.payment_hash.clone(),
+                        data: maybe_sa_processed,


Would make sense IMO.

dangeross

LGTM!

dangeross · 2023-10-26T07:55:40Z

Seems there is a conflict @dleutenegger. Can you take a look?

dangeross · 2023-10-26T13:12:33Z

Still some issues, can you rerun the flutter_rust_bridge?

erdemyerebasmaz · 2023-10-26T13:31:51Z

Seems there is a conflict @dleutenegger. Can you take a look?
Still some issues, can you rerun the flutter_rust_bridge?

@dangeross This is likely due to latest Flutter SDK & flutter_rust_bridge_codegen upgrade.

@dleutenegger Could you rebase your branch on top of main first then run

make clean
make init
make dev

from sdk-flutter directory.

dangeross · 2023-10-29T20:21:26Z

Hi @dleutenegger, would you mind rebasing and rerunning the RN codegen? I've just merged the error code PR #469 and there is a small conflict.

erdemyerebasmaz reviewed Oct 24, 2023

View reviewed changes

roeierez requested review from dangeross and ok300 October 24, 2023 16:21

erdemyerebasmaz mentioned this pull request Oct 24, 2023

Integrate latest breez-sdk changes breez/c-breez#697

Closed

4 tasks

dleutenegger force-pushed the lnurlp-expose-payment-hash branch from 27b2847 to 6d5e659 Compare October 25, 2023 08:28

dangeross requested changes Oct 25, 2023

View reviewed changes

ok300 requested changes Oct 25, 2023

View reviewed changes

dleutenegger requested review from dangeross and ok300 October 25, 2023 13:05

dangeross mentioned this pull request Oct 25, 2023

Fix generation of variants with multiple variables breez/breez-sdk-rn-generator#11

Merged

ok300 approved these changes Oct 25, 2023

View reviewed changes

dangeross approved these changes Oct 25, 2023

View reviewed changes

dleutenegger force-pushed the lnurlp-expose-payment-hash branch from 7b6b7f0 to bb1b3b4 Compare October 25, 2023 14:49

danielgranhao mentioned this pull request Oct 25, 2023

Implement LNURL-pay getlipa/lipa-lightning-lib#711

Merged

dleutenegger force-pushed the lnurlp-expose-payment-hash branch from bb1b3b4 to 82044db Compare October 26, 2023 10:33

dleutenegger force-pushed the lnurlp-expose-payment-hash branch from 82044db to 8bd80b5 Compare October 27, 2023 19:48

ubbabeck mentioned this pull request Nov 2, 2023

add PaymentHash to lnurlp breez/c-breez#707

Closed

dleutenegger force-pushed the lnurlp-expose-payment-hash branch from 8bd80b5 to 5ed953a Compare November 6, 2023 08:32

dleutenegger added 6 commits November 7, 2023 15:23

Add payment hash to lnurl_pay() return type

c0c9748

Move LnUrlPayResult data into wrapper structs

6ae8237

Rename LnUrlSuccessData data field to success_action

dd561c0

Add rustdocs for LnUrlPayResult::PayError

4b088f3

Rename LnUrlSuccessData to LnUrlPaySuccessData

dcf296f

Adjust lnurl_pay to new error model

097f53a

dleutenegger force-pushed the lnurlp-expose-payment-hash branch from 5ed953a to 097f53a Compare November 7, 2023 14:24

dangeross merged commit 6b1b0d5 into breez:main Nov 7, 2023
7 checks passed

dleutenegger deleted the lnurlp-expose-payment-hash branch November 8, 2023 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add payment hash to `lnurl_pay()` return type #550

Add payment hash to `lnurl_pay()` return type #550

dleutenegger commented Oct 24, 2023

erdemyerebasmaz Oct 24, 2023

erdemyerebasmaz Oct 24, 2023

dleutenegger Oct 25, 2023

dangeross Oct 25, 2023

dangeross Oct 25, 2023

dangeross left a comment

dangeross Oct 25, 2023

ok300 Oct 25, 2023

ok300 left a comment

ok300 Oct 25, 2023

roeierez Oct 25, 2023

dangeross Oct 25, 2023

ok300 Oct 25, 2023

ok300 Oct 25, 2023

dleutenegger Oct 25, 2023

ok300 Oct 25, 2023

ok300 Oct 25, 2023 •

edited

Loading

ok300 Oct 25, 2023

dangeross left a comment

dangeross commented Oct 26, 2023

dangeross commented Oct 26, 2023

erdemyerebasmaz commented Oct 26, 2023

dangeross commented Oct 29, 2023

Add payment hash to lnurl_pay() return type #550

Add payment hash to lnurl_pay() return type #550

Conversation

dleutenegger commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangeross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ok300 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ok300 Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangeross left a comment

Choose a reason for hiding this comment

dangeross commented Oct 26, 2023

dangeross commented Oct 26, 2023

erdemyerebasmaz commented Oct 26, 2023

dangeross commented Oct 29, 2023

Add payment hash to `lnurl_pay()` return type #550

Add payment hash to `lnurl_pay()` return type #550

ok300 Oct 25, 2023 •

edited

Loading