Skip to content

Refactor unified_qr.rs to use bitcoin-payment-instructions #607

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ log = { version = "0.4.22", default-features = false, features = ["std"]}

vss-client = "0.3"
prost = { version = "0.11.6", default-features = false}
bitcoin-payment-instructions = { git = "https://github.com/rust-bitcoin/bitcoin-payment-instructions" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, @TheBlueMatt, can we get a bitcoin-payment-instructions release that includes rust-bitcoin/bitcoin-payment-instructions#6 ?

@chuksys In the meantime, please import a specific commit via rev = "..", as otherwise our builds might break as soon as bitcoin-payment-instructions's main changes.


[target.'cfg(windows)'.dependencies]
winapi = { version = "0.3", features = ["winbase"] }
Expand Down
10 changes: 5 additions & 5 deletions bindings/ldk_node.udl
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ interface Node {
Bolt12Payment bolt12_payment();
SpontaneousPayment spontaneous_payment();
OnchainPayment onchain_payment();
UnifiedQrPayment unified_qr_payment();
UnifiedPayment unified_payment();
LSPS1Liquidity lsps1_liquidity();
[Throws=NodeError]
void connect(PublicKey node_id, SocketAddress address, boolean persist);
Expand Down Expand Up @@ -235,11 +235,11 @@ interface FeeRate {
u64 to_sat_per_vb_ceil();
};

interface UnifiedQrPayment {
interface UnifiedPayment {
[Throws=NodeError]
string receive(u64 amount_sats, [ByRef]string message, u32 expiry_sec);
[Throws=NodeError]
QrPaymentResult send([ByRef]string uri_str);
[Throws=NodeError, Async]
PaymentResult send([ByRef]string uri_str, u64? amount_msat);
};

interface LSPS1Liquidity {
Expand Down Expand Up @@ -409,7 +409,7 @@ interface PaymentKind {
};

[Enum]
interface QrPaymentResult {
interface PaymentResult {
Onchain(Txid txid);
Bolt11(PaymentId payment_id);
Bolt12(PaymentId payment_id);
Expand Down
11 changes: 11 additions & 0 deletions src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ use std::sync::{Arc, Mutex, RwLock};
use std::time::SystemTime;
use vss_client::headers::{FixedHeaders, LnurlAuthToJwtProvider, VssHeaderProvider};

use bitcoin_payment_instructions::onion_message_resolver::LDKOnionMessageDNSSECHrnResolver;

const VSS_HARDENED_CHILD_INDEX: u32 = 877;
const VSS_LNURL_AUTH_HARDENED_CHILD_INDEX: u32 = 138;
const LSPS_HARDENED_CHILD_INDEX: u32 = 577;
Expand Down Expand Up @@ -1633,6 +1635,14 @@ fn build_with_store_internal(
let (stop_sender, _) = tokio::sync::watch::channel(());
let background_processor_task = Mutex::new(None);

let hrn_resolver = Arc::new(LDKOnionMessageDNSSECHrnResolver::new(network_graph.clone()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Please use Arc::clone(&..) here and everywhere for Arcs. This helps distinguishing that we in fact do not clone the actual value here.


let peer_manager_clone = peer_manager.clone();

hrn_resolver.register_post_queue_action(Box::new(move || {
peer_manager_clone.process_events();
}));

Ok(Node {
runtime,
stop_sender,
Expand Down Expand Up @@ -1660,6 +1670,7 @@ fn build_with_store_internal(
payment_store,
is_listening,
node_metrics,
hrn_resolver,
})
}

Expand Down
2 changes: 1 addition & 1 deletion src/ffi/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ pub use crate::logger::{LogLevel, LogRecord, LogWriter};
pub use crate::payment::store::{
ConfirmationStatus, LSPFeeLimits, PaymentDirection, PaymentKind, PaymentStatus,
};
pub use crate::payment::{MaxTotalRoutingFeeLimit, QrPaymentResult, SendingParameters};
pub use crate::payment::{MaxTotalRoutingFeeLimit, PaymentResult, SendingParameters};

pub use lightning::chain::channelmonitor::BalanceSource;
pub use lightning::events::{ClosureReason, PaymentFailureReason};
Expand Down
16 changes: 10 additions & 6 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,13 @@ use io::utils::write_node_metrics;
use liquidity::{LSPS1Liquidity, LiquiditySource};
use payment::{
Bolt11Payment, Bolt12Payment, OnchainPayment, PaymentDetails, SpontaneousPayment,
UnifiedQrPayment,
UnifiedPayment,
};
use peer_store::{PeerInfo, PeerStore};
use types::{
Broadcaster, BumpTransactionEventHandler, ChainMonitor, ChannelManager, DynStore, Graph,
KeysManager, OnionMessenger, PaymentStore, PeerManager, Router, Scorer, Sweeper, Wallet,
HRNResolver, KeysManager, OnionMessenger, PaymentStore, PeerManager, Router, Scorer, Sweeper,
Wallet,
};
pub use types::{ChannelDetails, CustomTlvRecord, PeerDetails, UserChannelId};

Expand Down Expand Up @@ -202,6 +203,7 @@ pub struct Node {
payment_store: Arc<PaymentStore>,
is_listening: Arc<AtomicBool>,
node_metrics: Arc<RwLock<NodeMetrics>>,
hrn_resolver: Arc<HRNResolver>,
}

impl Node {
Expand Down Expand Up @@ -935,13 +937,14 @@ impl Node {
/// [BOLT 12]: https://github.com/lightning/bolts/blob/master/12-offer-encoding.md
/// [BIP 21]: https://github.com/bitcoin/bips/blob/master/bip-0021.mediawiki
#[cfg(not(feature = "uniffi"))]
pub fn unified_qr_payment(&self) -> UnifiedQrPayment {
UnifiedQrPayment::new(
pub fn unified_payment(&self) -> UnifiedPayment {
UnifiedPayment::new(
self.onchain_payment().into(),
self.bolt11_payment().into(),
self.bolt12_payment().into(),
Arc::clone(&self.config),
Arc::clone(&self.logger),
Arc::clone(&self.hrn_resolver),
)
}

Expand All @@ -952,13 +955,14 @@ impl Node {
/// [BOLT 12]: https://github.com/lightning/bolts/blob/master/12-offer-encoding.md
/// [BIP 21]: https://github.com/bitcoin/bips/blob/master/bip-0021.mediawiki
#[cfg(feature = "uniffi")]
pub fn unified_qr_payment(&self) -> Arc<UnifiedQrPayment> {
Copy link
Collaborator

@tnull tnull Aug 14, 2025

Choose a reason for hiding this comment

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

Mind updating the docs here and above that this will now also be used for HRNs?

Arc::new(UnifiedQrPayment::new(
pub fn unified_payment(&self) -> Arc<UnifiedPayment> {
Arc::new(UnifiedPayment::new(
self.onchain_payment(),
self.bolt11_payment(),
self.bolt12_payment(),
Arc::clone(&self.config),
Arc::clone(&self.logger),
Arc::clone(&self.hrn_resolver),
))
}

Expand Down
4 changes: 2 additions & 2 deletions src/payment/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ mod bolt12;
mod onchain;
mod spontaneous;
pub(crate) mod store;
mod unified_qr;
mod unified;

pub use bolt11::Bolt11Payment;
pub use bolt12::Bolt12Payment;
Expand All @@ -21,7 +21,7 @@ pub use spontaneous::SpontaneousPayment;
pub use store::{
ConfirmationStatus, LSPFeeLimits, PaymentDetails, PaymentDirection, PaymentKind, PaymentStatus,
};
pub use unified_qr::{QrPaymentResult, UnifiedQrPayment};
pub use unified::{PaymentResult, UnifiedPayment};

/// Represents information used to send a payment.
#[derive(Clone, Debug, PartialEq)]
Expand Down
148 changes: 106 additions & 42 deletions src/payment/unified_qr.rs → src/payment/unified.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@
// http://opensource.org/licenses/MIT>, at your option. You may not use this file except in
// accordance with one or both of these licenses.

//! Holds a payment handler allowing to create [BIP 21] URIs with an on-chain, [BOLT 11], and [BOLT 12] payment
//! Holds a payment handler allowing to create [BIP 21] URIs with on-chain, [BOLT 11], and [BOLT 12] payment
//! options.
//!
//! Also allows to send payments using these URIs as well as [BIP 353] HRNs.
//!
//! [BIP 21]: https://github.com/bitcoin/bips/blob/master/bip-0021.mediawiki
//! [BIP 353]: https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki
//! [BOLT 11]: https://github.com/lightning/bolts/blob/master/11-payment-encoding.md
//! [BOLT 12]: https://github.com/lightning/bolts/blob/master/12-offer-encoding.md
use crate::error::Error;
Expand All @@ -23,12 +26,18 @@ use lightning_invoice::{Bolt11Invoice, Bolt11InvoiceDescription, Description};

use bip21::de::ParamKind;
use bip21::{DeserializationError, DeserializeParams, Param, SerializeParams};
use bitcoin::address::{NetworkChecked, NetworkUnchecked};
use bitcoin::address::NetworkChecked;
use bitcoin::{Amount, Txid};

use std::sync::Arc;
use std::vec::IntoIter;

use bitcoin_payment_instructions::{
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 this group up to underneath the bitcoin group.

amount::Amount as BPIAmount, PaymentInstructions, PaymentMethod,
};

use crate::types::HRNResolver;
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 this up to the other crate:: imports.


type Uri<'a> = bip21::Uri<'a, NetworkChecked, Extras>;

#[derive(Debug, Clone)]
Expand All @@ -40,26 +49,31 @@ struct Extras {
/// A payment handler allowing to create [BIP 21] URIs with an on-chain, [BOLT 11], and [BOLT 12] payment
/// option.
///
/// Should be retrieved by calling [`Node::unified_qr_payment`]
/// Should be retrieved by calling [`Node::unified_payment`]
///
/// This handler allows you to send payments to these URIs as well as [BIP 353] HRNs.
///
/// [BIP 21]: https://github.com/bitcoin/bips/blob/master/bip-0021.mediawiki
/// [BIP 353]: https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki
/// [BOLT 11]: https://github.com/lightning/bolts/blob/master/11-payment-encoding.md
/// [BOLT 12]: https://github.com/lightning/bolts/blob/master/12-offer-encoding.md
/// [`Node::unified_qr_payment`]: crate::Node::unified_qr_payment
pub struct UnifiedQrPayment {
/// [`Node::unified_payment`]: crate::Node::unified_payment
pub struct UnifiedPayment {
onchain_payment: Arc<OnchainPayment>,
bolt11_invoice: Arc<Bolt11Payment>,
bolt12_payment: Arc<Bolt12Payment>,
config: Arc<Config>,
logger: Arc<Logger>,
hrn_resolver: Arc<HRNResolver>,
}

impl UnifiedQrPayment {
impl UnifiedPayment {
pub(crate) fn new(
onchain_payment: Arc<OnchainPayment>, bolt11_invoice: Arc<Bolt11Payment>,
bolt12_payment: Arc<Bolt12Payment>, config: Arc<Config>, logger: Arc<Logger>,
hrn_resolver: Arc<HRNResolver>,
) -> Self {
Self { onchain_payment, bolt11_invoice, bolt12_payment, config, logger }
Self { onchain_payment, bolt11_invoice, bolt12_payment, config, logger, hrn_resolver }
}

/// Generates a URI with an on-chain address, [BOLT 11] invoice and [BOLT 12] offer.
Expand Down Expand Up @@ -129,54 +143,104 @@ impl UnifiedQrPayment {
Ok(format_uri(uri))
}

/// Sends a payment given a [BIP 21] URI.
/// Sends a payment given a [BIP 21] URI or [BIP 353] HRN.
///
/// This method parses the provided URI string and attempts to send the payment. If the URI
/// has an offer and or invoice, it will try to pay the offer first followed by the invoice.
/// If they both fail, the on-chain payment will be paid.
///
/// Returns a `QrPaymentResult` indicating the outcome of the payment. If an error
/// Returns a `PaymentResult` indicating the outcome of the payment. If an error
/// occurs, an `Error` is returned detailing the issue encountered.
///
/// [BIP 21]: https://github.com/bitcoin/bips/blob/master/bip-0021.mediawiki
pub fn send(&self, uri_str: &str) -> Result<QrPaymentResult, Error> {
let uri: bip21::Uri<NetworkUnchecked, Extras> =
uri_str.parse().map_err(|_| Error::InvalidUri)?;

let uri_network_checked =
uri.clone().require_network(self.config.network).map_err(|_| Error::InvalidNetwork)?;

if let Some(offer) = uri_network_checked.extras.bolt12_offer {
let offer = maybe_wrap(offer);
match self.bolt12_payment.send(&offer, None, None) {
Ok(payment_id) => return Ok(QrPaymentResult::Bolt12 { payment_id }),
Err(e) => log_error!(self.logger, "Failed to send BOLT12 offer: {:?}. This is part of a unified QR code payment. Falling back to the BOLT11 invoice.", e),
/// [BIP 353]: https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki
pub async fn send(
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this is the first async method (mod Node::next_event_async) we do expose in the API, which makes it a bit incongruent with the rest of the API.

That said, I'm not super opposed to it, let's use this as a first experimental venture into async-API world. If any users complain, we could reuse our runtime to drive this and expose a blocking API, which is what we did so far.

&self, uri_str: &str, amount_msat: Option<u64>,
) -> Result<PaymentResult, Error> {
let instructions = match PaymentInstructions::parse(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than matching, let's just use .map_err(|e| .. )?, which should make this more readable and less vertical.

Also possible for ~most other matches below.

uri_str,
self.config.network,
self.hrn_resolver.as_ref(),
true,
)
.await
{
Ok(instr) => instr,
Err(e) => {
log_error!(self.logger, "Failed to parse payment instructions: {:?}", e);
return Err(Error::UriParameterParsingFailed);
},
};

let resolved = match instructions {
PaymentInstructions::ConfigurableAmount(ref instr) => {
if let Some(amount) = amount_msat {
let amt = match BPIAmount::from_sats(amount) {
Ok(amt) => amt,
Err(e) => {
log_error!(self.logger, "Error while coverting amount : {:?}", e);
return Err(Error::InvalidAmount);
},
};
match instr.clone().set_amount(amt, self.hrn_resolver.as_ref()).await {
Ok(resolved) => resolved,
Err(e) => {
log_error!(self.logger, "Failed to set amount: {:?}", e);
return Err(Error::InvalidAmount);
},
}
} else {
log_error!(self.logger, "No amount specified. Aborting the payment.");
return Err(Error::InvalidAmount);
}
},
PaymentInstructions::FixedAmount(ref instr) => instr.clone(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we at least check that the amount_msat (if provided) is greater or equal to the fixed amount?

};

if let Some(PaymentMethod::LightningBolt12(offer)) =
resolved.methods().iter().find(|m| matches!(m, PaymentMethod::LightningBolt12(_)))
{
let offer = maybe_wrap(offer.clone());
if let Some(amount_msat) = amount_msat {
match self.bolt12_payment.send_using_amount(&offer, amount_msat, None, None) {
Ok(payment_id) => return Ok(PaymentResult::Bolt12 { payment_id }),
Err(e) => log_error!(self.logger, "Failed to send BOLT12 offer: {:?}. This is part of a unified payment. Falling back to the BOLT11 invoice.", e),
}
} else {
match self.bolt12_payment.send(&offer, None, None) {
Ok(payment_id) => return Ok(PaymentResult::Bolt12 { payment_id }),
Err(e) => log_error!(self.logger, "Failed to send BOLT12 offer: {:?}. This is part of a unified payment. Falling back to the BOLT11 invoice.", e),
}
}
}

if let Some(invoice) = uri_network_checked.extras.bolt11_invoice {
let invoice = maybe_wrap(invoice);
if let Some(PaymentMethod::LightningBolt11(invoice)) =
resolved.methods().iter().find(|m| matches!(m, PaymentMethod::LightningBolt11(_)))
{
let invoice = maybe_wrap(invoice.clone());
match self.bolt11_invoice.send(&invoice, None) {
Ok(payment_id) => return Ok(QrPaymentResult::Bolt11 { payment_id }),
Err(e) => log_error!(self.logger, "Failed to send BOLT11 invoice: {:?}. This is part of a unified QR code payment. Falling back to the on-chain transaction.", e),
Ok(payment_id) => return Ok(PaymentResult::Bolt11 { payment_id }),
Err(e) => log_error!(self.logger, "Failed to send BOLT11 invoice: {:?}. This is part of a unified payment. Falling back to the on-chain transaction.", e),
}
}

let amount = match uri_network_checked.amount {
Some(amount) => amount,
None => {
log_error!(self.logger, "No amount specified in the URI. Aborting the payment.");
return Err(Error::InvalidAmount);
},
};

let txid = self.onchain_payment.send_to_address(
&uri_network_checked.address,
amount.to_sat(),
None,
)?;
if let Some(PaymentMethod::OnChain(address)) =
resolved.methods().iter().find(|m| matches!(m, PaymentMethod::OnChain(_)))
{
let amount = match resolved.onchain_payment_amount() {
Some(amount) => amount,
None => {
log_error!(self.logger, "No amount specified. Aborting the payment.");
return Err(Error::InvalidAmount);
},
};

Ok(QrPaymentResult::Onchain { txid })
let txid =
self.onchain_payment.send_to_address(&address, amount.sats().unwrap(), None)?;
return Ok(PaymentResult::Onchain { txid });
}
log_error!(self.logger, "Payable methods not found in URI");
Err(Error::PaymentSendingFailed)
}
}

Expand All @@ -189,7 +253,7 @@ impl UnifiedQrPayment {
/// [`PaymentId`]: lightning::ln::channelmanager::PaymentId
/// [`Txid`]: bitcoin::hash_types::Txid
#[derive(Debug)]
pub enum QrPaymentResult {
pub enum PaymentResult {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe UnifiedPaymentResult then?

/// An on-chain payment.
Onchain {
/// The transaction ID (txid) of the on-chain payment.
Expand Down Expand Up @@ -304,8 +368,8 @@ impl DeserializationError for Extras {
#[cfg(test)]
mod tests {
use super::*;
use crate::payment::unified_qr::Extras;
use bitcoin::{Address, Network};
use crate::payment::unified::Extras;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think we can just drop this line as the super::* import above covers Extras. Although, it would actually be preferable to switch to explicit import here, i.e., use super::{Extras, ...}.

use bitcoin::{address::NetworkUnchecked, Address, Network};
use std::str::FromStr;

#[test]
Expand Down
Loading
Loading