Skip to content

Support passing preimage for spontaneous payments #549

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.

Sign up for GitHub

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
wants to merge 1 commit into
base: main
Choose a base branch
from
+281 −95
Open

Support passing preimage for spontaneous payments #549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions bindings/ldk_node.udl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,10 @@ interface SpontaneousPayment {
[Throws=NodeError]
PaymentId send_with_custom_tlvs(u64 amount_msat, PublicKey node_id, SendingParameters? sending_parameters, sequence<CustomTlvRecord> custom_tlvs);
[Throws=NodeError]
PaymentId send_with_preimage(u64 amount_msat, PublicKey node_id, PaymentPreimage preimage, SendingParameters? sending_parameters);
[Throws=NodeError]
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Let's move these new entries to the correct location w.r.t. API, i.e. below send_with_custom_tlvs.

PaymentId send_with_preimage_and_custom_tlvs(u64 amount_msat, PublicKey node_id, sequence<CustomTlvRecord> custom_tlvs, PaymentPreimage preimage, SendingParameters? sending_parameters);
[Throws=NodeError]
void send_probes(u64 amount_msat, PublicKey node_id);
};

Expand Down Expand Up @@ -446,6 +450,10 @@ dictionary PaymentDetails {
u64 latest_update_timestamp;
};

dictionary PaymentPreimage {
bytes inner;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about what would be the most appropriate description for [u8;N]: bytes vs sequence<u8>?

Copy link
Author

Choose a reason for hiding this comment

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

bytes represents fixed-size binary data like preimage better

};

dictionary SendingParameters {
MaxTotalRoutingFeeLimit? max_total_routing_fee_msat;
u32? max_total_cltv_expiry_delta;
Expand Down Expand Up @@ -829,9 +837,6 @@ typedef string PaymentId;
[Custom]
typedef string PaymentHash;

[Custom]
typedef string PaymentPreimage;

[Custom]
typedef string PaymentSecret;

Expand Down
6 changes: 4 additions & 2 deletions src/balance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,16 @@
// http://opensource.org/licenses/MIT>, at your option. You may not use this file except in
// accordance with one or both of these licenses.

use crate::ffi::maybe_wrap;
use crate::payment::PaymentPreimage;
use crate::sweep::value_from_descriptor;

use lightning::chain::channelmonitor::Balance as LdkBalance;
use lightning::chain::channelmonitor::BalanceSource;
use lightning::ln::types::ChannelId;
use lightning::util::sweep::{OutputSpendStatus, TrackedSpendableOutput};

use lightning_types::payment::{PaymentHash, PaymentPreimage};
use lightning_types::payment::PaymentHash;

use bitcoin::secp256k1::PublicKey;
use bitcoin::{BlockHash, Txid};
Expand Down Expand Up @@ -263,7 +265,7 @@ impl LightningBalance {
amount_satoshis,
timeout_height,
payment_hash,
payment_preimage,
payment_preimage: maybe_wrap(payment_preimage),
},
LdkBalance::MaybeTimeoutClaimableHTLC {
amount_satoshis,
Expand Down
20 changes: 11 additions & 9 deletions src/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// http://opensource.org/licenses/MIT>, at your option. You may not use this file except in
// accordance with one or both of these licenses.

use crate::ffi::maybe_wrap;
use crate::types::{CustomTlvRecord, DynStore, PaymentStore, Sweeper, Wallet};

use crate::{
Expand All @@ -22,6 +23,7 @@ use crate::logger::Logger;
use crate::payment::store::{
PaymentDetails, PaymentDetailsUpdate, PaymentDirection, PaymentKind, PaymentStatus,
};
use crate::payment::PaymentPreimage;

use crate::io::{
EVENT_QUEUE_PERSISTENCE_KEY, EVENT_QUEUE_PERSISTENCE_PRIMARY_NAMESPACE,
Expand All @@ -39,7 +41,7 @@ use lightning::routing::gossip::NodeId;
use lightning::util::errors::APIError;
use lightning::util::ser::{Readable, ReadableArgs, Writeable, Writer};

use lightning_types::payment::{PaymentHash, PaymentPreimage};
use lightning_types::payment::PaymentHash;

use lightning_liquidity::lsps2::utils::compute_opening_fee;

Expand Down Expand Up @@ -740,7 +742,7 @@ where
let quantity = payment_context.invoice_request.quantity;
let kind = PaymentKind::Bolt12Offer {
hash: Some(payment_hash),
preimage: payment_preimage,
preimage: payment_preimage.map(|preimage| maybe_wrap(preimage)),
secret: Some(payment_secret),
offer_id,
payer_note,
Expand Down Expand Up @@ -785,7 +787,7 @@ where
// Since it's spontaneous, we insert it now into our store.
let kind = PaymentKind::Spontaneous {
hash: payment_hash,
preimage: Some(preimage),
preimage: Some(maybe_wrap(preimage)),
};

let payment = PaymentDetails::new(
Expand Down Expand Up @@ -871,7 +873,7 @@ where
payment_secret,
..
} => PaymentDetailsUpdate {
preimage: Some(payment_preimage),
preimage: Some(payment_preimage.map(|preimage| maybe_wrap(preimage))),
secret: Some(Some(payment_secret)),
amount_msat: Some(Some(amount_msat)),
status: Some(PaymentStatus::Succeeded),
Expand All @@ -880,7 +882,7 @@ where
PaymentPurpose::Bolt12OfferPayment {
payment_preimage, payment_secret, ..
} => PaymentDetailsUpdate {
preimage: Some(payment_preimage),
preimage: Some(payment_preimage.map(|preimage| maybe_wrap(preimage))),
secret: Some(Some(payment_secret)),
amount_msat: Some(Some(amount_msat)),
status: Some(PaymentStatus::Succeeded),
Expand All @@ -891,14 +893,14 @@ where
payment_secret,
..
} => PaymentDetailsUpdate {
preimage: Some(payment_preimage),
preimage: Some(payment_preimage.map(|preimage| maybe_wrap(preimage))),
secret: Some(Some(payment_secret)),
amount_msat: Some(Some(amount_msat)),
status: Some(PaymentStatus::Succeeded),
..PaymentDetailsUpdate::new(payment_id)
},
PaymentPurpose::SpontaneousPayment(preimage) => PaymentDetailsUpdate {
preimage: Some(Some(preimage)),
preimage: Some(Some(maybe_wrap(preimage))),
amount_msat: Some(Some(amount_msat)),
status: Some(PaymentStatus::Succeeded),
..PaymentDetailsUpdate::new(payment_id)
Expand Down Expand Up @@ -960,7 +962,7 @@ where

let update = PaymentDetailsUpdate {
hash: Some(Some(payment_hash)),
preimage: Some(Some(payment_preimage)),
preimage: Some(Some(maybe_wrap(payment_preimage))),
fee_paid_msat: Some(fee_paid_msat),
status: Some(PaymentStatus::Succeeded),
..PaymentDetailsUpdate::new(payment_id)
Expand Down Expand Up @@ -992,7 +994,7 @@ where
let event = Event::PaymentSuccessful {
payment_id: Some(payment_id),
payment_hash,
payment_preimage: Some(payment_preimage),
payment_preimage: Some(maybe_wrap(payment_preimage)),
fee_paid_msat,
};

Expand Down
27 changes: 26 additions & 1 deletion src/ffi/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ where
wrapped_type.as_ref().as_ref()
}

#[cfg(feature = "uniffi")]
pub fn maybe_extract<T, R>(wrapped_type: T) -> Result<R, crate::error::Error>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this, as we could just reuse maybe_try_convert_enum (which could be renamed to maybe_try_from, maybe).

In general, I'd prefer it if we wouldn't add any more wrapper helpers in this PR, I think (?) we should be able to make it work with what's there, no?

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I will make use of the wrapper helper we currently have.

where
R: TryFrom<T, Error = crate::error::Error>,
{
R::try_from(wrapped_type)
}

#[cfg(feature = "uniffi")]
pub fn maybe_try_convert_enum<T, R>(wrapped_type: &T) -> Result<R, crate::error::Error>
where
Expand All @@ -27,10 +35,15 @@ where
}

#[cfg(feature = "uniffi")]
pub fn maybe_wrap<T>(ldk_type: impl Into<T>) -> std::sync::Arc<T> {
pub fn maybe_wrap_arc<T>(ldk_type: impl Into<T>) -> std::sync::Arc<T> {
std::sync::Arc::new(ldk_type.into())
}

#[cfg(feature = "uniffi")]
pub fn maybe_wrap<T>(ldk_type: impl Into<T>) -> T {
ldk_type.into()
}

Comment on lines +38 to +46
Copy link

Choose a reason for hiding this comment

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

I'm probably missing something here but I'm wondering why we need to "split" maybe_wrap into these 2 functions.

Copy link
Author

Choose a reason for hiding this comment

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

maybe_wrap and maybe_wrap_arc is to handle different type wrapping requirements depending on whether the uniffi feature is enabled. maybe_wrap_arc wraps the input type in a std::sync::Arc<T>. maybe_wrap performs a simple into() conversion without additional wrapping, used in cases where the type doesn’t need to be wrapped in an Arc.

Copy link

Choose a reason for hiding this comment

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

From my understanding, I thought maybe_wrap (as it was before) already handled wrapping depending on whether the uniFFI feature flag is enabled - if enabled wrap in an Arc, if not return same value as passed in?

I noticed maybe_wrap (this new version) is used to conditionally wrap the preimage - does this mean the preimage never needs to be wrapped in an Arc even when the uniFFI feature flag is enabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

In #542, the wrapper types introduced were for structs with a bit more complexity and state than PaymentPreimage. From my understanding, it makes sense to allocate them on the heap because they are also expensive to clone. PaymentPreimage is simpler, wraps 32 bytes, and should be cheaper to clone and doesn't need to be Arc'ed.

Also, when compiled with uniffi, there was a bit of conflict about converting maybe_wrap into a macro so it can handle the different cases (i.e. types needing heap allocation vs types that do not), however I think @tnull may not like the idea for reasons stated in #526.

This change is easily revertible though if Arcing is preferred.

Copy link

Choose a reason for hiding this comment

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

In #542, the wrapper types introduced were for structs with a bit more complexity and state than PaymentPreimage. From my understanding, it makes sense to allocate them on the heap because they are also expensive to clone. PaymentPreimage is simpler, wraps 32 bytes, and should be cheaper to clone and doesn't need to be Arc'ed.

Thanks for the clarification! This makes sense.

#[cfg(not(feature = "uniffi"))]
pub fn maybe_deref<T>(value: &T) -> &T {
value
Expand All @@ -41,7 +54,19 @@ pub fn maybe_try_convert_enum<T>(value: &T) -> Result<&T, crate::error::Error> {
Ok(value)
}

#[cfg(not(feature = "uniffi"))]
pub fn maybe_wrap_arc<T>(value: T) -> T {
value
}

#[cfg(not(feature = "uniffi"))]
pub fn maybe_wrap<T>(value: T) -> T {
value
}
Comment on lines +57 to 65
Copy link

Choose a reason for hiding this comment

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

These 2 fns are basically the same, please is there a reason why we need both of them?

Copy link
Author

@aagbotemi aagbotemi Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

both maybe_wrap_arc and maybe_wrap are identical, simply returning the input value. The reason for keeping them as separate functions is to maintain a consistent API that aligns with their distinct behaviors when the uniffi feature is enabled


#[cfg(not(feature = "uniffi"))]
pub fn maybe_extract<T>(wrapped_type: T) -> Result<T, crate::error::Error>
where
{
Ok(wrapped_type)
}
71 changes: 59 additions & 12 deletions src/ffi/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,18 @@ pub use crate::payment::store::{
};
pub use crate::payment::{MaxTotalRoutingFeeLimit, QrPaymentResult, SendingParameters};

use bitcoin::io::Read;
pub use lightning::chain::channelmonitor::BalanceSource;
pub use lightning::events::{ClosureReason, PaymentFailureReason};
use lightning::ln::msgs::DecodeError;
pub use lightning::ln::types::ChannelId;
pub use lightning::offers::offer::OfferId;
pub use lightning::routing::gossip::{NodeAlias, NodeId, RoutingFees};
pub use lightning::util::string::UntrustedString;

pub use lightning_types::payment::{PaymentHash, PaymentPreimage, PaymentSecret};
pub use lightning_types::payment::{
PaymentHash, PaymentPreimage as LdkPaymentPreimage, PaymentSecret,
};

pub use lightning_invoice::{Description, SignedRawBolt11Invoice};

Expand Down Expand Up @@ -58,7 +62,7 @@ use lightning::ln::channelmanager::PaymentId;
use lightning::offers::invoice::Bolt12Invoice as LdkBolt12Invoice;
use lightning::offers::offer::{Amount as LdkAmount, Offer as LdkOffer};
use lightning::offers::refund::Refund as LdkRefund;
use lightning::util::ser::Writeable;
use lightning::util::ser::{Readable, Writeable};
use lightning_invoice::{Bolt11Invoice as LdkBolt11Invoice, Bolt11InvoiceDescriptionRef};

use std::convert::TryInto;
Expand Down Expand Up @@ -665,21 +669,64 @@ impl UniffiCustomTypeConverter for PaymentHash {
}
}

impl UniffiCustomTypeConverter for PaymentPreimage {
type Builtin = String;
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct PaymentPreimage {
pub(crate) inner: Vec<u8>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency reasons, please just store an LdkPaymentPreimage as an inner.

Copy link
Author

Choose a reason for hiding this comment

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

Alright. Thank you

}

fn into_custom(val: Self::Builtin) -> uniffi::Result<Self> {
if let Some(bytes_vec) = hex_utils::to_vec(&val) {
let bytes_res = bytes_vec.try_into();
if let Ok(bytes) = bytes_res {
return Ok(PaymentPreimage(bytes));
impl From<LdkPaymentPreimage> for PaymentPreimage {
fn from(ldk_value: LdkPaymentPreimage) -> Self {
PaymentPreimage { inner: ldk_value.0.to_vec() }
}
}

impl TryFrom<PaymentPreimage> for LdkPaymentPreimage {
type Error = Error;

fn try_from(preimage: PaymentPreimage) -> Result<Self, Self::Error> {
if preimage.inner.len() != 32 {
return Err(Error::InvalidPaymentPreimage);
}

let mut array = [0u8; 32];
array.copy_from_slice(&preimage.inner);
Ok(LdkPaymentPreimage(array))
}
}

impl FromStr for PaymentPreimage {
type Err = Error;

fn from_str(s: &str) -> Result<Self, Self::Err> {
if let Some(bytes) = hex_utils::to_vec(s) {
if let Ok(array) = bytes.try_into() {
return Ok(Self::from(LdkPaymentPreimage(array)));
}
}
Err(Error::InvalidPaymentPreimage.into())

Err(Error::InvalidPaymentPreimage)
}
}

fn from_custom(obj: Self) -> Self::Builtin {
hex_utils::to_string(&obj.0)
impl Writeable for PaymentPreimage {
fn write<W: lightning::util::ser::Writer>(
&self, writer: &mut W,
) -> Result<(), lightning::io::Error> {
let ldk_preimage = LdkPaymentPreimage::try_from(self.clone()).map_err(|_| {
lightning::io::Error::new(
lightning::io::ErrorKind::InvalidData,
"Invalid payment preimage",
)
})?;

ldk_preimage.0.write(writer)
}
}

impl Readable for PaymentPreimage {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, we really shouldn't roll our own custom serialization PaymentPreimage here, we just use the wrapper type in the API, for nothing else.

Copy link
Author

Choose a reason for hiding this comment

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

Alright. Noted

fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
let buf: [u8; 32] = Readable::read(r)?;
Ok(PaymentPreimage { inner: buf.to_vec() })
}
}

Expand Down
Loading
Loading