Skip to content

Add new store for inbound payment metadata #425

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

Open
tnull opened this issue Jan 3, 2025 · 4 comments
Open

Add new store for inbound payment metadata #425

tnull opened this issue Jan 3, 2025 · 4 comments
Milestone

Comments

@tnull
Copy link
Collaborator

tnull commented Jan 3, 2025

Prior to v0.1 LDK only offered PaymentIds for outbound payments, which led us to track inbound payments by an ID derived for the payment hash. This was always messy as the latter is only available for BOLT11 payments, resulting in BOLT11 payments being added to the payment store upon invoice creation, and BOLT12 payments only being added upon receiving the invoice request.

With lightningdevkit/rust-lightning#3303 LDK added PaymentIds for inbound payments, so we should switch to use them in a backwards-compatible manner (i.e., probably still checking whether the old-style ID is available in the store for now). However, as this will mean we that we can't add the payment store entry upon invoice creation also for BOLT11 payments, we finally need a different solution to store additional metadata (invoices, but also JIT-channel fee limits) before receiving the payment. That is we need to add a PaymentMetatdataStore or similar.

@TheBlueMatt
Copy link

Re #528 (comment) just figured I'd mention here so that it doesn't get lost - we definitely need to store the HRN as well as the DNSSEC proof (and BOLT 12 Invoice itself, once it's in the payment event from LDK in 0.2) because those are the proof of payment.

@moisesPompilio
Copy link
Contributor

moisesPompilio commented May 2, 2025

I'm working on this issue and have some questions regarding the PaymentMetadata structure. I'm considering the following structure and I’d like to know if this includes all the necessary information, or if I should add, remove, or change anything:

pub enum PaymentMetadataDetail {
	Bolt11 {
		invoice: String,
		preimage: Option<PaymentPreimage>,
	},
	Bolt12Offer {
		offer_id: OfferId,
		preimage: Option<PaymentPreimage>,
		dnssec_proof: Option<DNSSECProofWrapper>,
		hrn: Option<HumanReadableName>,
		quantity: Option<u64>,
		hash: Option<PaymentHash>,
	},
	Bolt12Refund {
		refund: Refund,
		hrn: Option<HumanReadableName>,
		dnssec_proof: Option<DNSSECProofWrapper>,
		invoice: String,
		preimage: Option<PaymentPreimage>,
	},
}

pub struct JitChannelFeeLimits {
	pub max_total_opening_fee_msat: Option<u64>,
	pub max_proportional_opening_fee_ppm_msat: Option<u64>,
	pub counterparty_skimmed_fee_msat: Option<u64>,
}

pub struct PaymentMetadata {
	pub id: PaymentId,
	pub latest_update_timestamp: u64,
	pub direction: PaymentDirection,
	pub payment_metadata_detail: PaymentMetadataDetail,
	pub jit_channel_fee_limit: Option<JitChannelFeeLimits>,
	pub status: PaymentStatus,
}

moisesPompilio added a commit to moisesPompilio/ldk-node that referenced this issue May 6, 2025
…allback

- Add PaymentMetadata to represent metadata for inbound payments
- Add PaymentMetadataStore for persisting PaymentMetadata
- Add PaymentDataWithFallback to handle backward compatibility during the transition from legacy bolt11/bolt12 pending payments to the new PaymentMetadata format

Issue lightningdevkit#425
moisesPompilio added a commit to moisesPompilio/ldk-node that referenced this issue May 6, 2025
moisesPompilio added a commit to moisesPompilio/ldk-node that referenced this issue May 6, 2025
Introduces the optional `legacy_payment_store` feature to enable testing of legacy payment flows during the transition to `PaymentMetadata` for inbound payments.

Issue lightningdevkit#425
moisesPompilio added a commit to moisesPompilio/ldk-node that referenced this issue May 6, 2025
…g paymentStore

Inbound Bolt11 and Bolt12 payments that are pending should now be stored in `PaymentMetadataStore` rather than directly in `PaymentStore`.
Once the payment is completed (successful or failed), it is then moved to `PaymentStore`, and its status is updated in `PaymentMetadata`.
This change maintains backward compatibility with the previous behavior of storing all payment information directly in `PaymentStore`.

issue lightningdevkit#425
moisesPompilio added a commit to moisesPompilio/ldk-node that referenced this issue May 6, 2025
…cy behavior

The test was modified to be compatible with the new `PaymentMetadata` logic, while still verifying compatibility with the legacy rule that stores payments directly in `PaymentStore`.

Issue lightningdevkit#425
moisesPompilio added a commit to moisesPompilio/ldk-node that referenced this issue May 6, 2025
moisesPompilio added a commit to moisesPompilio/ldk-node that referenced this issue May 6, 2025
Introduces the optional `legacy_payment_store` feature to enable testing of legacy payment flows during the transition to `PaymentMetadata` for inbound payments.

Issue lightningdevkit#425
moisesPompilio added a commit to moisesPompilio/ldk-node that referenced this issue May 6, 2025
…g paymentStore

Inbound Bolt11 and Bolt12 payments that are pending should now be stored in `PaymentMetadataStore` rather than directly in `PaymentStore`.
Once the payment is completed (successful or failed), it is then moved to `PaymentStore`, and its status is updated in `PaymentMetadata`.
This change maintains backward compatibility with the previous behavior of storing all payment information directly in `PaymentStore`.

issue lightningdevkit#425
moisesPompilio added a commit to moisesPompilio/ldk-node that referenced this issue May 6, 2025
…cy behavior

The test was modified to be compatible with the new `PaymentMetadata` logic, while still verifying compatibility with the legacy rule that stores payments directly in `PaymentStore`.

Issue lightningdevkit#425
moisesPompilio added a commit to moisesPompilio/ldk-node that referenced this issue May 6, 2025
@moisesPompilio
Copy link
Contributor

Opened the PR as a draft. It includes the implementation of PaymentMetadata and the plan for backward compatibility with the old payment persistence logic. It’s complete and works, but I’m not sure if the metadata structure is ideal — maybe some fields need to be added, removed, or the flow adjusted.

@tnull
Copy link
Collaborator Author

tnull commented May 8, 2025

I'm working on this issue and have some questions regarding the PaymentMetadata structure. I'm considering the following structure and I’d like to know if this includes all the necessary information, or if I should add, remove, or change anything:

Excuse the delay here, I was traveling for the last week or so.

I think there are some misconceptions about what the payment metadata store is meant to be: it's not meant to replace the payment store, but would rather be used to hold additional metadata (like offers and invoices, also some fields related to JIT payments) that need to be associated with the payment ids (although not super clear yet which way that link would go).

Unfortunately, I think the approach over at #537 is headed in the wrong direction (note we def. don't want to introduce a legacy_payment_store feature, for example). If you want on this issue, I suggest we first sync via Discord or a call to outline a rough direction before continuing (I'll reach out shortly).

Please also note that it's usually best practice to indicate that you'll be working on something before you start. This for one avoids potential redundant work (e.g., if someone else were already working on an issue), and would have allowed us to provide additional context, and find alignment on the approach before spending time on doing the actual work.

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

No branches or pull requests

3 participants