Add a payment_metadata map in blinded * path contexts#4584
Add a payment_metadata map in blinded * path contexts#4584TheBlueMatt wants to merge 6 commits intolightningdevkit:mainfrom
payment_metadata map in blinded * path contexts#4584Conversation
We almost certainly don't want to be moving `option` TLVs during serialization, and while we had logic elsewhere to work around this previously its nice not to have to in the future.
|
👋 Thanks for assigning @jkczyz as a reviewer! |
7c4e653 to
ee6ba89
Compare
|
I've thoroughly reviewed the entire PR diff, cross-referencing serialization macros, flow propagation, pattern match exhaustiveness, backward compatibility, and deserialization safety. All areas I previously flagged have been verified as resolved (typos fixed, doc references corrected). After this comprehensive re-review, I found no new issues beyond those already covered by my prior inline comments. No new issues found. Prior inline comments status:
Verification summary of key areas:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4584 +/- ##
==========================================
- Coverage 87.15% 86.23% -0.92%
==========================================
Files 161 159 -2
Lines 109251 109248 -3
Branches 109251 109248 -3
==========================================
- Hits 95215 94209 -1006
- Misses 11560 12423 +863
- Partials 2476 2616 +140
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ee6ba89 to
792846c
Compare
Similar to how BOLT 11 payments can use a `payment_metadata` to provide arbitrary bytes in the invoice to be communicated back to them when receiving, its useful to be able to provide some bytes which are communicated back upon receiving a payment. Here we do so in the BOLT 12 blinded path contexts, offering a `BTreeMap<u64, Vec<u8>>` instead to enable more easily including multiple sets of data. Also note that a `Router` building a blinded path is allowed to modify the `payment_metadata` without breaking the payment. Tests by claude
792846c to
98cff64
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
98cff64 to
b8375a7
Compare
There was a problem hiding this comment.
Looks good, but it would be nice to provide a bit more ergonomic/self-descriptive types here. At the very least we should document what the semantics of the u64s are (or rather, that there are ~none and the user is free to set them).
Tagging @jkczyz as second reviewer on this.
| /// | ||
| /// [`RecipientOnionFields::payment_metadata`]: crate::ln::outbound_payment::RecipientOnionFields::payment_metadata | ||
| /// [`Bolt11Invoice::payment_metadata`]: lightning_invoice::Bolt11Invoice::payment_metadata | ||
| pub payment_metadata: Option<BTreeMap<u64, Vec<u8>>>, |
There was a problem hiding this comment.
See above: what are we expecting users to set the u64 to?
Shouldn't we rather use a dedicated PaymentMetadata type for this, or even an Option<Box<dyn Writeable>> or similar?
There was a problem hiding this comment.
Answered at #4584 (comment) but I do really want to force users into the K-V-map box here. Can do a type if we want but its a bit awkward that we have a separate BOLT 11 and BOLT 12 PaymentMetadata type...
There was a problem hiding this comment.
The Box<dyn Writeable> would get a bit ugly at read time. You'd need to parameterize the onion message parsing with your types.
| /// | ||
| /// [`RecipientOnionFields::payment_metadata`]: crate::ln::outbound_payment::RecipientOnionFields::payment_metadata | ||
| /// [`Bolt11Invoice::payment_metadata`]: lightning_invoice::Bolt11Invoice::payment_metadata | ||
| pub payment_metadata: Option<BTreeMap<u64, Vec<u8>>>, |
There was a problem hiding this comment.
Why use a map? Wouldn't a user's wrapper struct be just as efficient encoding -- or could be even more efficient if each part is of a fixed length? Deserialization would be simpler, too, if they didn't need to iterate a map.
There was a problem hiding this comment.
The intention is both to force users to think about forward compatibility from day one (in the sense that its already a K-V) but also because we want to reserve some keys in LDK (eg in #4463).
There was a problem hiding this comment.
The intention is both to force users to think about forward compatibility from day one (in the sense that its already a K-V)
Isn't that what using our serialization macros provide? Blinded paths are also not expected to be useable indefinitely, so the compatibility argument isn't as strong, IMO.
FWIW, the rationale given in the commit message only mentions composability.
but also because we want to reserve some keys in LDK (eg in #4463).
Wouldn't it be simpler to add a dedicated field for anything LDK-specific? Then we wouldn't need custom parsing / serialization logic (for each variant of PaymentContext) or need to worry about checking if users took a reserved type. This would need to be added somewhere in the code vs a simple declaration in the impl_writeable_tlv_based uses.
Similar to how BOLT 11 payments can use a `payment_metadata` to provide arbitrary bytes in the invoice to be communicated back to them when receiving, its useful to be able to provide some bytes which are communicated back upon receiving a payment. Here we do so in the BOLT 12 blinded message path contexts, offering a `BTreeMap<u64, Vec<u8>>` instead to enable more easily including multiple sets of data. We don't yet wire it up to the public `ChannelManager` API, but do allow selecting values for those using the manual `OffersMessageFlow`. Tests by claude
b8375a7 to
6bfdc0d
Compare
We do so both in the blinded message and payment paths, supporting async payments when data is injected in the blinded payment paths (eg via the
Router). We don't expose building offers with metadata yet.