Return success on failure to decrypt AES success action

[andrei-21]

If there is an AES success action in payRequest (LUD-10) then failure to decrypt ciphertext should not fail the payment, since the funds were actually sent and the lightning payment has succeeded.

Solves #632.

[andrei-21]

Is it a good idea to use Reslut for that? If yes, how do I convert it into dart?

[ok300]

Result is a good direction, try to use aResult-like enum with an inner field. These play better with bindings and uniffi AFAIK.

Something like

    pub enum AesSuccessActionDataResult {
        Decrypted { data: AesSuccessActionDataDecrypted },
        Error { data: String }
    }

To propagate the changes to the bindings for other languages, you'd have to

• update breez_sdk.udl with the new fields
• run cargo build in sdk-bindings
• run make init / make dev in sdk-flutter
• run the command listed in the README of https://github.com/breez/breez-sdk-rn-generator

[andrei-21]

The PR is ready for review, I went with ErrorStatus not Error because it is a keyword in UDL:

    pub enum AesSuccessActionDataResult {
        Decrypted { data: AesSuccessActionDataDecrypted },
        ErrorStatus { data: String },
    }

Bindings and other stuff were generated with some help from @dleutenegger.

[dangeross]

Overall looks good I think, just some comments around the error variant variables

[dangeross]

I think it's more consistent to call this reason instead of data with a string

Suggested change

	ErrorStatus { data: String },
	ErrorStatus { reason: String },

or use LnUrlErrorData like in LnUrlWithdrawResult and LnUrlCallbackStatus

Suggested change

	ErrorStatus { data: String },
	ErrorStatus { data: LnUrlErrorData },

[andrei-21]

I renamed data to reason. There is no need to use LnUrlErrorData here, because the payment hash will be returned in:

                Ok(LnUrlPayResult::EndpointSuccess {
                    data: LnUrlPaySuccessData {
                        payment_hash: details.payment_hash.clone(),
                        success_action: maybe_sa_processed,
                    },
                })

and maybe_sa_processed will have AesSuccessActionDataResult::ErrorStatus.

[dangeross]

LGTM!

[ok300]

Looks good!

Could you please also add a short test case for when decryption fails?

There is test_lnurl_pay_aes_success_action that covers the success scenario, I imagine a similar one can check the new result type in case of failed decryption.

[andrei-21]

Looks good!

Could you please also add a short test case for when decryption fails?

There is test_lnurl_pay_aes_success_action that covers the success scenario, I imagine a similar one can check the new result type in case of failed decryption.

I added test_lnurl_pay_aes_success_action_fail_to_decrypt for that.

[ok300]

My bad, I missed it.

Then it's all good!

[andrei-21]

You haven't missed it, I added it after you asked. Do not trust git commit time ;)

[roeierez]

LGTM