[RFC] event: expose BOLT12 invoice in PaymentSuccessful for proof of payment#733
[RFC] event: expose BOLT12 invoice in PaymentSuccessful for proof of payment#733vincenzopalazzo wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
@tnull IDK if this is a good design to have with the ffi, but I had to work around some unify ffi limitation with the enum type that is used inside the |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
There was a problem hiding this comment.
Exposing the BOLT12 invoice makes sense to me, though we should do it properly instead of just exposing the bytes.
4c6b18e to
a3a60b3
Compare
There was a problem hiding this comment.
The changes are getting closer, but please make sure to avoid unnecessary boilerplate and stick to the approach we took for the other types (like Offer, Refund, etc).
This also needs a rebase by now, sorry!
Btw, please re-request review when you made updates, otherwise I might not always see it immediately.
22abd3b to
a8d05cb
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
327a59a to
e6200d6
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
bindings/ldk_node.udl
Outdated
| sequence<u8> encode(); | ||
| }; | ||
|
|
||
| // TODO: Add StaticInvoice and PaidBolt12Invoice once we upgrade to UniFFI 0.29+. See #757. |
There was a problem hiding this comment.
nit: Let's drop this TODO, one above on Event is sufficient.
| pub use unified::{UnifiedPayment, UnifiedPaymentResult}; | ||
|
|
||
| #[cfg(not(feature = "uniffi"))] | ||
| pub use crate::types::PaidBolt12Invoice; |
There was a problem hiding this comment.
No need to re-export the type here, let's just drop it and use the LDK type directly.
There was a problem hiding this comment.
We should be good to drop these changes now?
|
@vincenzopalazzo We now landed the uniffi v0.29 upgrade, so this should be unblocked. Note that as part of that upgrade we switched to use proc macros where possible, in particular |
025fea4 to
991fa56
Compare
…ment Add the `bolt12_invoice` field to the `PaymentSuccessful` event, enabling users to obtain proof of payment for BOLT12 transactions. Problem: After a successful BOLT12 payment, users had no way to access the paid invoice data. This made it impossible to provide proof of payment to third parties, who need both the payment preimage and the original invoice to verify that sha256(preimage) matches the invoice's payment_hash. Solution: Add `bolt12_invoice: Option<PaidBolt12Invoice>` to `PaymentSuccessful`. With the UniFFI v0.29 upgrade now supporting objects in enum variants, we can expose the proper `PaidBolt12Invoice` type across both native Rust and FFI builds without cfg-gating the Event field. For non-UniFFI builds, LDK's `PaidBolt12Invoice` is re-exported directly. For UniFFI builds, a wrapper `PaidBolt12Invoice` enum is defined in ffi/types.rs with `From` conversions and delegating serialization. A minimal `StaticInvoice` FFI wrapper is also added to support the `PaidBolt12Invoice::StaticInvoice` variant. The FFI enum variants are named `Bolt12`/`Static` (rather than `Bolt12Invoice`/`StaticInvoice`) to avoid Kotlin sealed class name collisions where inner data classes would shadow top-level types. TLV tag 7 is used for serialization, maintaining backward compatibility: older readers silently skip the unknown odd tag, and newer readers deserialize `None` from events without it. Closes lightningdevkit#757. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
971aee2 to
cd7c42d
Compare
|
Thanks @tnull I used the help of claude to navigate a little bit the changes and now I pushed it (I have to change some name inside the enum for the kotlin naming convention hope this is not a big deal. Please let me know if this is good enough or if there is a better way to implement the functionality in the codebase, thanks |
tnull
left a comment
There was a problem hiding this comment.
Cool, mostly looks good, just some comments regarding cleanups we should now be able to do?
| pub use unified::{UnifiedPayment, UnifiedPaymentResult}; | ||
|
|
||
| #[cfg(not(feature = "uniffi"))] | ||
| pub use crate::types::PaidBolt12Invoice; |
There was a problem hiding this comment.
We should be good to drop these changes now?
| pub(crate) type PendingPaymentStore = DataStore<PendingPaymentDetails, Arc<Logger>>; | ||
|
|
||
| #[cfg(not(feature = "uniffi"))] | ||
| pub use lightning::events::PaidBolt12Invoice; |
| #[tokio::test(flavor = "multi_thread", worker_threads = 1)] | ||
| async fn bolt12_proof_of_payment() { | ||
| let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); | ||
| let chain_source = TestChainSource::Esplora(&electrsd); |
There was a problem hiding this comment.
Please use random_chain_source here so this test is run on all chain sources.
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 1)] | ||
| async fn bolt12_proof_of_payment() { |
There was a problem hiding this comment.
AFAICT this is essentially the same as simple_bolt12_send_receive? Should we just add a check for the proof-of-payment/invoice there?
| }, | ||
| } | ||
|
|
||
| // Verify serialization round-trip (tests TLV tag 7 backward compatibility) |
There was a problem hiding this comment.
Hmm, that seems very out-of-place here? If anything, a serialization roundtrip test should be a unit test in event.rs, but I don't think we need it.
This patch adds the
bolt12_invoicefield to thePaymentSuccessfulevent, enabling users to obtain proof of payment for BOLT12 transactions.Problem:
Previously, after a successful BOLT12 payment, users had no way to access the paid invoice data. This made it impossible to provide proof of payment to third parties, who need both the payment preimage and the original invoice to verify that sha256(preimage) matches the invoice's payment_hash.
Solution:
Add a
bolt12_invoice: Option<Vec<u8>>field toPaymentSuccessfulthat contains the serialized BOLT12 invoice bytes. The invoice is serialized using LDK's standard encoding, which can be parsed back usingBolt12Invoice::try_from(bytes)in native Rust, or by hex-encoding the bytes and usingBolt12Invoice.from_str()in FFI bindings.Design decisions:
Vec<u8>rather than the complexPaidBolt12Invoicetype to avoid UniFFI limitations with objects in enum variantsNoneforStaticInvoice(async payments) since proof of payment is not possible for those payment types anywayThis implementation follows the maintainer guidance from PR #563 to expose the invoice via the event rather than storing it in the payment store.