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

Add support for paying to BOLT12 #549

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Add support for paying to BOLT12 #549

wants to merge 29 commits into from

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented Nov 6, 2024

This PR adds support for paying to a BOLT12 offer.

The flow is as follows:

  • user specifies a BOLT12 offer destination when paying
  • SDK calls Boltz endpoint that fetches a BOLT12 invoice for given offer
  • SDK calls send_payment_via_swap with the BOLT12 invoice as argument, instead of the usual BOLT11

Open questions

  • Should we also support paying directly to BOLT12 invoices, in addition to BOLT12 offers?
  • Expand PaymentDetails::Lightning with a new field for bolt12_invoice? And / or an optional bolt12_offer?
    • Or alternatively rename the existing bolt11 field to invoice, so it means either BOLT11 or BOLT12 invoice?
  • Should we fetch the invoice during prepare? This may take a long time for slow endpoints (Phoenix), causing the user to wait a few seconds for prepare to return.
  • Update sdk_common::input_parser to parse BOLT12 offers / invoice? Or keep changes local to the Liquid SDK?
    • Use BIP-353 "DNS Payment Instructions" for BOLT12 resolution? (similar to LN Address)

Open TODOs

  • Validate returned invoice (check signature) before trying to pay it
  • Detect / handle MRH in BOLT12 invoices
  • Handle both BOLT11 and BOLT12 in persister
  • When paying to a bolt12 offer, list_payments shows payments as Pending
  • When paying to a bolt12 offer, list_payments shows the bolt12 invoice under bolt11 (should show offer instead)

lib/core/src/sdk.rs Outdated Show resolved Hide resolved
lib/core/src/sdk.rs Outdated Show resolved Hide resolved
lib/core/src/sdk.rs Outdated Show resolved Hide resolved
lib/core/src/persist/mod.rs Outdated Show resolved Hide resolved
lib/core/src/sdk.rs Outdated Show resolved Hide resolved
lib/core/src/sdk.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@dangeross dangeross left a comment

Choose a reason for hiding this comment

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

Looking good

cli/src/commands.rs Outdated Show resolved Hide resolved
Comment on lines +868 to +870
_ => Err(PaymentError::amount_missing(
"Expected PayAmount of type Receiver when processing a Bolt12 offer",
)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's fine to not handle the drain case now, there are issues to inverse calculate the Boltz fee #559.

lib/core/src/sdk.rs Outdated Show resolved Hide resolved
ok300 and others added 8 commits November 14, 2024 09:09
Co-authored-by: Ross Savage <551697+dangeross@users.noreply.github.com>
MRH with Bolt12 is not possible because receive is not yet supported.
Removed self-transfer check TODO, because self-transfers are not possible with Bolt12 when receive is not supported.
lib/core/src/sdk.rs Outdated Show resolved Hide resolved
@ok300 ok300 marked this pull request as ready for review November 18, 2024 11:26
Comment on lines +186 to +243
// Add bolt12_offer column for Send Swaps
"
ALTER TABLE send_swaps RENAME TO send_swaps_old;

CREATE TABLE send_swaps (
id TEXT NOT NULL PRIMARY KEY,
invoice TEXT NOT NULL UNIQUE,
bolt12_offer TEXT,
preimage TEXT,
payer_amount_sat INTEGER NOT NULL,
receiver_amount_sat INTEGER NOT NULL,
create_response_json TEXT NOT NULL,
refund_private_key TEXT NOT NULL,
lockup_tx_id TEXT,
refund_tx_id TEXT,
created_at INTEGER NOT NULL,
state INTEGER NOT NULL,
description TEXT,
id_hash TEXT,
payment_hash TEXT
) STRICT;

INSERT INTO send_swaps (
id,
invoice,
bolt12_offer,
preimage,
payer_amount_sat,
receiver_amount_sat,
create_response_json,
refund_private_key,
lockup_tx_id,
refund_tx_id,
created_at,
state,
description,
id_hash,
payment_hash
) SELECT
id,
invoice,
NULL,
preimage,
payer_amount_sat,
receiver_amount_sat,
create_response_json,
refund_private_key,
lockup_tx_id,
refund_tx_id,
created_at,
state,
description,
id_hash,
payment_hash
FROM send_swaps_old;

DROP TABLE send_swaps_old;
",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're adding a nullable column, do we need to recreate the table?

Comment on lines +316 to +320
bolt11: match maybe_send_swap_bolt12_offer.is_some() {
true => None, // We don't expose the Bolt12 invoice
false => maybe_send_swap_invoice,
},
bolt12_offer: maybe_send_swap_bolt12_offer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't we expose the bolt12 invoice? The bolt11 is used as the payment destination in the send swap case, so we need to use either the offer or invoice.

Lightning(string swap_id, string description, string? preimage, string? bolt11, string? payment_hash, string? refund_tx_id, u64? refund_tx_amount_sat);
Lightning(string swap_id, string description, string? preimage, string? bolt11, string? bolt12_offer, string? payment_hash, string? refund_tx_id, u64? refund_tx_amount_sat);
Copy link
Collaborator

Choose a reason for hiding this comment

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

RN needs regenerating

@@ -533,7 +533,7 @@ interface GetPaymentRequest {

[Enum]
interface PaymentDetails {
Lightning(string swap_id, string description, string? preimage, string? bolt11, string? payment_hash, string? refund_tx_id, u64? refund_tx_amount_sat);
Lightning(string swap_id, string description, string? preimage, string? bolt11, string? bolt12_offer, string? payment_hash, string? refund_tx_id, u64? refund_tx_amount_sat);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change I think requires the notification plugin to be updated

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Other than @dangeross comments I think it looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants