LN-Zap / zap-android

Zap Wallet - Native android lightning wallet for node operators focused on user experience and ease of use ⚡️
MIT License
165 stars 49 forks source link

Invalid payment information error when scanning invoices from mobile wallets #324

Closed MrHash closed 3 years ago

MrHash commented 3 years ago

Trying to send to invoices in both Breez and Muun results in a "Invalid payment information" error. Invoice decodes correctly but Zap does not read them correctly

Expected Behavior

Invoice should be read correctly

Actual Behavior

Error message is presented saying "Invalid payment information"

Steps to Reproduce

  1. Generate an invoice in Muun or Breez
  2. Scan QR in Zap

Your Environment

michaelWuensch commented 3 years ago

@MrHash Thanks for the report, I will see if I can reproduce this.

x1ddos commented 3 years ago

I suspect it might be due to this lline: https://github.com/LN-Zap/zap-android/blob/7751f315/app/src/main/java/zapsolutions/zap/lnurl/pay/LnUrlPayBSDFragment.java#L412. Setting a non-zero amount works, for me at least. I didn't look at the logs though, just a hunch.

Also, unclear why there's a payment amount enforcement at https://github.com/LN-Zap/zap-android/blob/7751f315/app/src/main/java/zapsolutions/zap/lnurl/pay/LnUrlPayBSDFragment.java#L413-L415. It is sometimes useful to amend the amount from the sending party.

Unfortunately there's no justification in the code comments and https://github.com/LN-Zap/zap-android/pull/218/commits/77985bdf2b8fdb93a27aa841d4d7ab42f9a01680 commit message doesn't explain it either.

MrHash commented 3 years ago

Let's call @michaelWuensch for feedback

michaelWuensch commented 3 years ago

@x1ddos Thanks for your input! The first question I have is for @MrHash: Did you talk about lnurl payments as well or did you talk about normal ln invoices being payed?

In regards to invoices without a defined amount, thats true, Zap Android does not support them because of security concerns associated with them. I actually merged a PR (#364 ) a few days ago which now shows an explicit error message in that case. But as I can see now I forgot that we need a specific message for the lnurl flow as well. I will add that.

@x1ddos In regards to payment amount enforcement, please have a look at the lnurl spec, I am just following the spec (Step 8.) https://github.com/fiatjaf/lnurl-rfc/blob/luds/06.md

michaelWuensch commented 3 years ago

Actually when I think about it in the Lnurl pay flow there should not occur any 0 sat invoices. The wallet user is communicating an amount to the LNService which then sends him a ln invoice requesting that amount. Do I miss something?

x1ddos commented 3 years ago

Oh, right. I thought we were taling about the BOLT 11 spec, not LNURL. This is what I tried in https://github.com/LN-Zap/zap-android/issues/324#issuecomment-929189671. In it, the amount is optional.

So, I would've expected the app to follow a spec which the payment request / invoice was encoded in. What do you guys think?

michaelWuensch commented 3 years ago

@x1ddos Bolt11 is used in lnurl pay as well. I guess you most likely just quoted the wrong code lines. Maybe you wanted to quote this? https://github.com/LN-Zap/zap-android/blob/7751f31543828c0957d521ea757105550dacf1b9/app/src/main/java/zapsolutions/zap/util/InvoiceUtil.java#L207 That is where the 0 sat invoice check happens on the normal (non lnurl pay) flow.

x1ddos commented 3 years ago

Absolutely. My links were completely off. Thanks for clarifying :+1:

michaelWuensch commented 3 years ago

Ok, @MrHash can you confirm that it is working with invoices form Breez or Muun that specify a value?

MrHash commented 3 years ago

Yeh can confirm, a 0 sat invoice from Phoenix wallet fails, but a 1sat invoice is recognized.

michaelWuensch commented 3 years ago

@MrHash Ok, then this issue is already fixed by the 0.5.2-beta release. It should roll out as soon as google store approves it. Please note, that 0 sat invoices will still not work, but you will at least get a decent message.