Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cln: allow amountless invoices in node tx list #2138

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

kallerosenbaum
Copy link
Contributor

@kallerosenbaum kallerosenbaum commented Nov 30, 2023

Bolt11 allows for invoices to have no amount, allowing the sender to select the amount to send. At least core lightning and LND support this.

When looking through my invoices on the node page, I get an error when it tries to fetch an unpaid invoice with no amount:

Nov 30 19:59:56 wilfrid poetry[2788777]:   File "/mnt/bitcoin/git/lnbits-legend/lnbits/nodes/cln.py", line 46, in wrapper
Nov 30 19:59:56 wilfrid poetry[2788777]:     return await f(*args, **kwargs)
Nov 30 19:59:56 wilfrid poetry[2788777]:   File "/mnt/bitcoin/git/lnbits-legend/lnbits/nodes/cln.py", line 311, in get_invoices
Nov 30 19:59:56 wilfrid poetry[2788777]:     data=[
Nov 30 19:59:56 wilfrid poetry[2788777]:   File "/mnt/bitcoin/git/lnbits-legend/lnbits/nodes/cln.py", line 312, in <listcomp>
Nov 30 19:59:56 wilfrid poetry[2788777]:     NodeInvoice(
Nov 30 19:59:56 wilfrid poetry[2788777]:   File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
Nov 30 19:59:56 wilfrid poetry[2788777]: pydantic.error_wrappers.ValidationError: 1 validation error for NodeInvoice
Nov 30 19:59:56 wilfrid poetry[2788777]: amount
Nov 30 19:59:56 wilfrid poetry[2788777]:   none is not an allowed value (type=type_error.none.not_allowed)
Nov 30 19:59:56 wilfrid poetry[2788777]: 2023-11-30 19:59:56.65 | ERROR | Exception: 1 validation error for NodeInvoice
Nov 30 19:59:56 wilfrid poetry[2788777]: amount
Nov 30 19:59:56 wilfrid poetry[2788777]:   none is not an allowed value (type=type_error.none.not_allowed)
Nov 30 19:59:56 wilfrid poetry[2788777]: 2023-11-30 19:59:56.65 | INFO | 92.60.40.221:0 - "GET /node/api/v1/invoices?limit=50&offset=0&usr=XXXXXXXXXXXXXXXXXXXXXXXXXX HTTP/1.1" 500

This PR fixes that issue by making NodeInvoice.amount optional. This causes the invoice to show amount 0 in the UI. I think this is acceptable. Ideally it should say "any" or something similar.

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e46b12d) 54.45% compared to head (fa02436) 60.35%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2138      +/-   ##
==========================================
+ Coverage   54.45%   60.35%   +5.89%     
==========================================
  Files          56       56              
  Lines        8454     8454              
==========================================
+ Hits         4604     5102     +498     
+ Misses       3850     3352     -498     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kallerosenbaum kallerosenbaum changed the title Allow amountless invoices Allow amountless invoices in node tx list Nov 30, 2023
@kallerosenbaum
Copy link
Contributor Author

Noting that this worked fine for LND, because they list value_msat: 0 on amountless invoices and thus sets NodeInvoice.amount = 0. So maybe I should make cln.py also set 0 (instead of none) in the case of an amountless invoice?

@prusnak
Copy link
Collaborator

prusnak commented Nov 30, 2023

Yes, I think the fix belongs to the node implementation, not to the base model.

@kallerosenbaum
Copy link
Contributor Author

There are a few cases that I can think of:

  • Normal invoice, amount_msat is set to whatever amount is required.
  • Keysend: amount_msat unset, and amount_recieved_msat is set. I think there is no state where keysends have neither set, but not sure. We fixed keysends in CLN: Handle optional invoice fields #2056
  • Unpaid amountless invoice. amount_msat is unset, and amount_received_msat is unset.
  • Paid amountless invoice. amount_msat is unset, and amount_received_msat is set.

So in the case of keysend I think it's ok to use amount_received_msat, as PR #2056 does. In case of a paid amountless invoice I think it's ok to display amount_received_msat as the amount (this is effectively what happens "by accident" due to PR #2056). And finally, in the case of an unpaid amountless invoice, the amount should simply be set to 0. I'll fix this.

@kallerosenbaum
Copy link
Contributor Author

@prusnak fixed.

@prusnak prusnak changed the title Allow amountless invoices in node tx list cln: allow amountless invoices in node tx list Nov 30, 2023
@prusnak
Copy link
Collaborator

prusnak commented Nov 30, 2023

@prusnak fixed.

I fixed the black/ruff formatter warnings and force pushed to the PR branch to fix the CI failures.

@prusnak prusnak added the node Lightning node backend related label Nov 30, 2023
@dni dni requested review from dni and motorina0 December 19, 2023 11:00
@dni dni requested review from callebtc and a team December 19, 2023 11:00
Copy link
Collaborator

@motorina0 motorina0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@prusnak prusnak merged commit abbcfbe into lnbits:dev Dec 19, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node Lightning node backend related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants