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

chuksys · 2025-08-11T18:17:26Z

This PR introduces a unified.rs module (which is a refactor of the unified_qr.rs module) - this refactor allows us to use this module as a single API for sending payments to BIP 21/321 URIs as well as BIP 353 HRNs, creating a simpler interface for users.

https://github.com/rust-bitcoin/bitcoin-payment-instructions is used to parse BIP 21/321 URIs as well as the BIP 353 HRNs.

Changes

The unified_qr.rs module has been renamed to unified.rs.
The UnifiedQrPayment struct has been renamed to UnifiedPayment.
The QRPaymentResult enum has been renamed to UnifiedPaymentResult.
The send method in unified.rs now supports sending to both BIP 21/321 URIs as well as BIP 353 HRNs.
The integration tests for the module have been updated accordingly.

I will follow-up with adding an end-to-end test that asserts that sending to HRNs using this API works.

This PR closes #521

ldk-reviews-bot · 2025-08-11T18:17:29Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull

Cool, thanks! Did a first ~high-level round. Feel free to undraft this.

tnull · 2025-08-14T08:28:16Z

src/payment/unified.rs

@@ -304,7 +304,7 @@ impl DeserializationError for Extras {
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use crate::payment::unified_qr::Extras;
+	use crate::payment::unified::Extras;


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, ...}.

src/lib.rs

tnull · 2025-08-14T08:30:00Z

src/payment/unified.rs

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


Maybe UnifiedPaymentResult then?

tnull · 2025-08-14T08:31:21Z

Cargo.toml

@@ -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" }


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.

Done! You should be able to reference bitcoin-payment-instructions 0.5 now.

tnull · 2025-08-14T08:34:20Z

src/builder.rs

@@ -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()));


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

tnull · 2025-08-14T08:38:29Z

src/payment/unified.rs

+	amount::Amount as BPIAmount, PaymentInstructions, PaymentMethod,
+};
+
+use crate::types::HRNResolver;


nit: Let's move this up to the other crate:: imports.

tnull · 2025-08-14T08:54:33Z

tests/integration_tests_rust.rs

Please include the test changes in the corresponding commit(s) that require them. Each commit should build and test individually, not just the whole PR.

src/payment/unified.rs

tnull · 2025-08-14T08:59:32Z

src/payment/unified.rs

+	pub async fn send(
+		&self, uri_str: &str, amount_msat: Option<u64>,
+	) -> Result<PaymentResult, Error> {
+		let instructions = match PaymentInstructions::parse(


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.

tnull · 2025-08-14T09:03:40Z

src/payment/unified.rs

+					return Err(Error::InvalidAmount);
+				}
+			},
+			PaymentInstructions::FixedAmount(ref instr) => instr.clone(),


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

tnull

This needs a rebase by now, also.

ldk-reviews-bot · 2025-08-25T00:00:29Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-08-27T00:01:13Z

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

enigbe

Thank you for the PR.
I have reviewed this and approve of the refactor. I just have a small note about per-commit compilation and testing where, because of a function signature mismatch between UnifiedPayment's send and the bindings-exposed definition, the corresponding commit (16fbedc) that introduced the change failed to compile. In the same vein, albeit not as important, in 7c1a2db, with warnings enabled, compilation fails because of the unused hrn_resolver.

Otherwise, this LGTM and I'd be happy to approve this when these are addressed.

enigbe · 2025-08-28T01:29:50Z

src/payment/unified.rs

@@ -139,7 +139,7 @@ impl UnifiedQrPayment {
 	/// 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> {
+	pub fn send(&self, uri_str: &str) -> Result<UnifiedPaymentResult, Error> {


In 16fbedcc, there's a function signature mismatch between send and its bindings-exposed definition. The latter is async and has an optional amount_msat parameter. The commit does not compile because of this.

Thanks @enigbe for the review - I have fixed this.

ldk-reviews-bot · 2025-08-30T00:01:04Z

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-09-01T00:01:47Z

🔔 4th Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Excuse the delay here!

Some comments, but generally looks pretty good as a first step (can't wait for end-to-end tests for the resolution though).

This is also still dependant on a release of bitcoin-payment-instructions, which, as I hear, should happen really soon though.

tnull · 2025-09-02T12:09:18Z

src/payment/unified.rs

+			uri_str,
+			self.config.network,
+			self.hrn_resolver.as_ref(),
+			true,


We set supports_proof_of_payment_callbacks to true here, but never actually handle the callback. This seems like an omission?

I'll change this to false for now.

tnull · 2025-09-02T12:12:27Z

src/payment/unified.rs

+					Error::InvalidAmount
+				})?;
+
+				let amt = BPIAmount::from_sats(amount).map_err(|e| {


Shouldn't we keep the msat resolution here?

Yes! We should use BPIAmount::from_milli_sats instead - will fix. Thanks for catching that!

tnull · 2025-09-02T12:22:49Z

src/payment/unified.rs

+					Error::InvalidAmount
+				})?;
+
+				instr.clone().set_amount(amt, self.hrn_resolver.as_ref()).await.map_err(|e| {


You can easily avoid these clones by not taking a ref:

diff --git a/src/payment/unified.rs b/src/payment/unified.rs index 68eb55e5..c9f26ee9 100644 --- a/src/payment/unified.rs +++ b/src/payment/unified.rs @@ -168,7 +168,7 @@ impl UnifiedPayment { })?; let resolved = match instructions { - PaymentInstructions::ConfigurableAmount(ref instr) => { + PaymentInstructions::ConfigurableAmount(instr) => { let amount = amount_msat.ok_or_else(|| { log_error!(self.logger, "No amount specified. Aborting the payment."); Error::InvalidAmount @@ -179,13 +179,12 @@ impl UnifiedPayment { Error::InvalidAmount })?; - instr.clone().set_amount(amt, self.hrn_resolver.as_ref()).await.map_err(|e| { + instr.set_amount(amt, self.hrn_resolver.as_ref()).await.map_err(|e| { log_error!(self.logger, "Failed to set amount: {:?}", e); Error::InvalidAmount })? }, - PaymentInstructions::FixedAmount(ref instr) => { - let instr = instr.clone(); + PaymentInstructions::FixedAmount(instr) => { if let Some(user_amount) = amount_msat { if instr.ln_payment_amount().map_or(false, |amt| user_amount < amt.milli_sats()) {

tnull · 2025-09-02T12:28:08Z

src/payment/unified.rs

+			PaymentInstructions::FixedAmount(ref instr) => {
+				let instr = instr.clone();
+				if let Some(user_amount) = amount_msat {
+					if instr.ln_payment_amount().map_or(false, |amt| user_amount < amt.milli_sats())


We likely want to compare with max_amount rather than the LN-specific one, no?

Yes, comparing with max_amount makes more sense! Thanks!

tnull · 2025-09-02T12:33:27Z

src/payment/unified.rs

+		{
+			let offer = maybe_wrap(offer.clone());
+			let payment_result = if let Some(amount_msat) = amount_msat {
+            	self.bolt12_payment.send_using_amount(&offer, amount_msat, None, None)


Please indent by tabs, not spaces. (Not sure how this got past rustfmt tbh.)

tnull · 2025-09-02T12:35:42Z

src/payment/unified.rs

+			})?;
+
+			let txid =
+				self.onchain_payment.send_to_address(&address, amount.sats().unwrap(), None)?;


It would be a bug if this ever failed, but please still simply error out if amount.sats() returns an error.

chuksys · 2025-09-02T15:14:37Z

Some comments, but generally looks pretty good as a first step (can't wait for end-to-end tests for the resolution though).

Thank you! Currently working on the end-to-end test - will open the PR for that asap.

This rename reflects that this module is a unified payment interface for both QR code payments and HRN payments passed in as a string without scanning a QR code

…UnifiedPaymentResult These renamings are necessary to reflect the expanded responsibilities for this module.

This commit adds a HRN Resolver to the Node struct which will be useful for resolving HRNs when making BIP 353 payments. It also passes the HRN Resolver into UnifiedPayment.

…353 HRNs

chuksys mentioned this pull request Aug 11, 2025

Add support for sending to human-readable names (BIP 353) #528

Closed

chuksys marked this pull request as draft August 11, 2025 18:25

tnull self-requested a review August 13, 2025 07:57

tnull reviewed Aug 14, 2025

View reviewed changes

chuksys marked this pull request as ready for review August 17, 2025 18:00

ldk-reviews-bot requested a review from joostjager August 17, 2025 18:01

tnull removed the request for review from joostjager August 18, 2025 08:38

chuksys force-pushed the refactor/adopt-bitcoin-payment-instructions branch 2 times, most recently from 05a948e to b4a71f6 Compare August 22, 2025 15:25

chuksys requested a review from tnull August 22, 2025 16:11

enigbe reviewed Aug 28, 2025

View reviewed changes

tnull reviewed Sep 2, 2025

View reviewed changes

chuksys added 7 commits September 4, 2025 16:48

Rename unified_qr to unified

9ea92d5

This rename reflects that this module is a unified payment interface for both QR code payments and HRN payments passed in as a string without scanning a QR code

rename UnifiedQRPayment to UnifiedPayment, rename QRPaymentResult to …

b11fead

…UnifiedPaymentResult These renamings are necessary to reflect the expanded responsibilities for this module.

add bitcoin-payment-instructions to cargo.toml

998bdab

Add hrn_resolver to Node and pass into UnifiedPayment

55522b9

This commit adds a HRN Resolver to the Node struct which will be useful for resolving HRNs when making BIP 353 payments. It also passes the HRN Resolver into UnifiedPayment.

Refactor unified.rs to support sending to BIP 21 URIs as well as BIP …

f1d1fa1

…353 HRNs

Fix typo in unified_payment send test and improve test name.

869839a

Switch to explicit imports in unified.rs mod tests

38e17d1

chuksys force-pushed the refactor/adopt-bitcoin-payment-instructions branch from b4a71f6 to 38e17d1 Compare September 5, 2025 11:07

chuksys mentioned this pull request Sep 5, 2025

Add support for resolving BIP 353 Human-Readable Names #630

Draft

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

chuksys commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

ldk-reviews-bot commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Aug 25, 2025

Uh oh!

ldk-reviews-bot commented Aug 27, 2025

Uh oh!

enigbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Aug 30, 2025

Uh oh!

ldk-reviews-bot commented Sep 1, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chuksys commented Sep 2, 2025

Uh oh!

Uh oh!

chuksys commented Aug 11, 2025 •

edited

Loading

ldk-reviews-bot commented Aug 11, 2025 •

edited

Loading