Add liquidity checks and improve payment error handling

[slanesuke]

This PR introduces a few improvements including liquidity checks in the pay_for_offer and create_refund_builder, updated error handling, and support for fiat-denominated offers.

Changes:

• Liquidity Checks:
  • Added early aborts in pay_for_offer and create_refund_builder if channel liquidity is insufficient.
  • Added tests to validate the new liquidity checks.
• Error Handling:
  • Introduced Bolt12RequestError for errors related to BOLT12 payment/refund requests.
  • Introduced Bolt12CreationError for errors occurring during the creation of BOLT12 offers/refunds.
• Fiat-Denominated Offers:
  • Improved support for offers specifying an amount in fiat currencies.
  • Added tests for InvoiceRequest creation with currency-based amounts.
• Docs updates:
  • Updated the docs for InvoiceRequestBuilder::amount_msats to clarify behavior when handling currency-based offers.
  • Updated the docs for Bolt12SemanticError::InvalidAmount to include cases where the invoice amount does not match the expected amount.

Fixes #3174

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 88.41463% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.59%. Comparing base (43e28fe) to head (35e3daa).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	82.43%	9 Missing and 4 partials :warning:
lightning/src/ln/offers_tests.rs	96.49%	2 Missing :warning:
lightning/src/offers/invoice.rs	77.77%	2 Missing :warning:
lightning/src/offers/invoice_request.rs	91.30%	1 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3175      +/-   ##
==========================================
- Coverage   89.62%   89.59%   -0.04%     
==========================================
  Files         127      127              
  Lines      103531   103664     +133     
  Branches   103531   103664     +133     
==========================================
+ Hits        92787    92874      +87     
- Misses       8050     8084      +34     
- Partials     2694     2706      +12     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[TheBlueMatt]

Looks like all CI is failing here :(

[slanesuke]

@jkczyz I added a test to check the Insufficient Liquidity error in offers_tests. Moved the currency check to the parsing code and modified the amount check to trust the user-provided msats amount (I think I did this correctly).

And regarding the InsufficientLiquidity error type, I left it as is. I just saw the new comments so I'll add the Bolt12CreationError now!

[slanesuke]

@jkczyz I added a test to check the Insufficient Liquidity error in offers_tests. Moved the currency check to the parsing code and modified the amount check to trust the user-provided msats amount (I think I did this correctly).

And regarding the InsufficientLiquidity error type, I left it as is. I just saw the new comments so I'll add the Bolt12CreationError now!

Hmmm... looks like a few different test are failing now.

[tnull]

Hmmm... looks like a few different test are failing now.

Two of the failures are expected as they test the failure case you just removed 🙂

The ones failing with InsufficientAmount fail because you changed it to check check the offer amount (i.e., the amount for a single item), not the full amount to be sent (which is expected to be offer amount times quantity). 

[slanesuke]

The variant wrapping Bolt12SemanticError could use a better name and the docs for Bolt12CreationError may need some fine-tuning. But this should be close to finished.

Also, let me know if I need to move any other error variants from Bolt12SemanticErrors!

[slanesuke]

@jkczyz ready for review.

[slanesuke]

I’ve made the changes to add both Bolt12CreationError and Bolt12RequestError as separate error types. Since both share a couple common variants (InvalidSemantics and BlindedPathCreationFailed), I’m wondering about redundancy. Would you guys recommend keeping the current approach with these duplicated variants, or do you think there’s a better way to handle this to avoid redundancy?

Also, I’ve updated the approach to calculate the total channel liquidity by (hopefully) iterating directly over the channels themselves. Would this be correct/efficient, or are there better ways to accomplish this!?

(cc @jkczyz @TheBlueMatt)

[jkczyz]

I’ve made the changes to add both Bolt12CreationError and Bolt12RequestError as separate error types. Since both share a couple common variants (InvalidSemantics and BlindedPathCreationFailed), I’m wondering about redundancy. Would you guys recommend keeping the current approach with these duplicated variants, or do you think there’s a better way to handle this to avoid redundancy?

Nah, the redundancy is fine here, IMO.

Also, I’ve updated the approach to calculate the total channel liquidity by (hopefully) iterating directly over the channels themselves. Would this be correct/efficient, or are there better ways to accomplish this!?

(cc @jkczyz @TheBlueMatt)

Yeah, I think this is what @TheBlueMatt had in mind.

[slanesuke]

I’ve made the changes to add both Bolt12CreationError and Bolt12RequestError as separate error types. Since both share a couple common variants (InvalidSemantics and BlindedPathCreationFailed), I’m wondering about redundancy. Would you guys recommend keeping the current approach with these duplicated variants, or do you think there’s a better way to handle this to avoid redundancy?

Nah, the redundancy is fine here, IMO.

Also, I’ve updated the approach to calculate the total channel liquidity by (hopefully) iterating directly over the channels themselves. Would this be correct/efficient, or are there better ways to accomplish this!? (cc @jkczyz @TheBlueMatt)

Yeah, I think this is what @TheBlueMatt had in mind.

Sweet, thanks for the feedback! Just pushed the updates.

[slanesuke]

Regarding commit messages, please state not only the what but also the why (but not the how). Also, use the imperative instead mood rather than first person (e.g., "Moved" vs "I moved"). See the following guidelines: https://cbea.ms/git-commit/.

Okay, gotcha. Atomic commits and commit messages have been a bit of a weak spot for me, but I’ll make sure to get it right! Thanks again

[slanesuke]

rebased

[slanesuke]

rebased