Refactor unified_qr.rs to use bitcoin-payment-instructions by chuksys · Pull Request #607 · lightningdevkit/ldk-node

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

 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

 /// [`Txid`]: bitcoin::hash_types::Txid
 #[derive(Debug)]
-pub enum QrPaymentResult {
+pub enum PaymentResult {


Maybe UnifiedPaymentResult then?

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

Cargo.toml


 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

 	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

 	///
 	/// [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.

tnull

Now that lightningdevkit/rust-lightning#3903 was merged, please point this PR to develop and include a fixup bumping the LDK dependencies to the respective commit hash on LDK main.

We then want to use pay_for_offer_from_hrn if the parsed payment instructions indicate that the offer was retrieved from an HRN.

chuksys · 2025-09-18T14:43:58Z

Now that lightningdevkit/rust-lightning#3903 was merged, please point this PR to develop and include a fixup bumping the LDK dependencies to the respective commit hash on LDK main.

We then want to use pay_for_offer_from_hrn if the parsed payment instructions indicate that the offer was retrieved from an HRN.

Sure, will do.

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

This commit ensures that when using the unified API to send to a HRN, we use pay_for_offer_from_hrn

This fixup points bitcoin-payment-instructions to my fork allowing me to bump some lightning dependencies in order to resolve dep conflicts here.

…ult to UnifiedPaymentResult

Just fixing some formatting issues.

This fixup commit simply fixes issues with the bolt12 send_using_amount method signature in the tests.

Just fixing a simple formatting issue.

chuksys · 2025-09-23T13:12:50Z

Cargo.toml

 vss-client = "0.3"
 prost = { version = "0.11.6", default-features = false}
+#bitcoin-payment-instructions = { version = "0.5" }
+bitcoin-payment-instructions = { git = "https://github.com/chuksys/bitcoin-payment-instructions", branch = "bump-lightning" }


Had to point to my fork of bitcoin-payment-instructions because I had to bump the LDK dependencies there to resolve dependency conflicts here in the develop branch since we bumped the LDK deps here to commit hash 50391d3a3efa7a8f32d119d126a633e4b1981ee6.

Perhaps we could have a develop branch on https://github.com/rust-bitcoin/bitcoin-payment-instructions that allows us do the same?

Hmm, good point, thanks for bringing it up. I think for this approach here is fine (at least until this PR is getting ready to land), but going forward we might want to figure out a more coherent strategy.

Moving to a develop branch here does solve some of the issues, but ofc. it's not magic bullet :(

ldk-reviews-bot · 2025-09-25T15:33:20Z

🔔 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.

tnull

This needs yet another rebase, as we've been moving fast on the develop branch. However, we probably want to hold on this until we found a way forward on the end-to-end tests allowing us to verify the whole flow works as expected. Of course feel free to request review in the meantime if you think I should take a look at a specific part!

tnull · 2025-09-26T09:31:28Z

Cargo.toml

 vss-client = "0.3"
 prost = { version = "0.11.6", default-features = false}
+#bitcoin-payment-instructions = { version = "0.5" }
+bitcoin-payment-instructions = { git = "https://github.com/chuksys/bitcoin-payment-instructions", branch = "bump-lightning" }


Hmm, good point, thanks for bringing it up. I think for this approach here is fine (at least until this PR is getting ready to land), but going forward we might want to figure out a more coherent strategy.

Moving to a develop branch here does solve some of the issues, but ofc. it's not magic bullet :(

tnull · 2025-10-08T07:29:27Z

Ugh, seems I missed this PR when changing the base branch of all open PRs before deleting the develop branch (as main now takes the same role). Apologies! Please reopen it whenever you're ready!

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 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

Open

tnull reviewed Sep 18, 2025

View reviewed changes

chuksys added 7 commits September 22, 2025 13:28

Rename unified_qr to unified

6b65662

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 …

eaf1a1a

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

add bitcoin-payment-instructions to cargo.toml

23f2846

Add hrn_resolver to Node and pass into UnifiedPayment

29ac03f

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 …

6546d48

…353 HRNs

Fix typo in unified_payment send test and improve test name.

2362714

Switch to explicit imports in unified.rs mod tests

cc94f39

chuksys added 4 commits September 23, 2025 10:37

Use the pay_for_offer_from_hrn method from LDK upstream

71624c5

This commit ensures that when using the unified API to send to a HRN, we use pay_for_offer_from_hrn

fixup! add bitcoin-payment-instructions to cargo.toml

88df493

This fixup points bitcoin-payment-instructions to my fork allowing me to bump some lightning dependencies in order to resolve dep conflicts here.

fixup! rename UnifiedQRPayment to UnifiedPayment, rename QRPaymentRes…

f45de4a

…ult to UnifiedPaymentResult

fixup! Use the pay_for_offer_from_hrn method from LDK upstream

68c33b7

Just fixing some formatting issues.

chuksys force-pushed the refactor/adopt-bitcoin-payment-instructions branch from 38e17d1 to 68c33b7 Compare September 23, 2025 10:12

chuksys changed the base branch from main to develop September 23, 2025 10:16

chuksys added 2 commits September 23, 2025 11:29

fixup! Use the pay_for_offer_from_hrn method from LDK upstream

d32e012

This fixup commit simply fixes issues with the bolt12 send_using_amount method signature in the tests.

fixup! Use the pay_for_offer_from_hrn method from LDK upstream

5c170c1

Just fixing a simple formatting issue.

chuksys commented Sep 23, 2025

View reviewed changes

chuksys requested a review from tnull September 23, 2025 15:33

tnull reviewed Sep 26, 2025

View reviewed changes

reez mentioned this pull request Oct 6, 2025

BOLT 12 / BIP 353 reez/Monday#205

Open

tnull deleted the branch lightningdevkit:develop October 7, 2025 08:54

tnull closed this Oct 7, 2025

chuksys mentioned this pull request Oct 16, 2025

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

Merged

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!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

chuksys commented Sep 18, 2025

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

chuksys commented Aug 11, 2025 •

edited

Loading

ldk-reviews-bot commented Aug 11, 2025 •

edited

Loading

chuksys Sep 23, 2025 •

edited

Loading

tnull commented Oct 8, 2025 •

edited

Loading