Add BIP21 Unified QR Code Support #302

slanesuke · 2024-05-29T20:55:22Z

Based on #182
This pull request introduces a payment submodule unifiedqr.rs that handles BIP21 URIs with support for BOLT11 invoices. Its goal is to facilitate the creation and payment of Unified QR codes with an additional lightning parameter.

Important features

Receive Method : Generates a BIP21 URI string with an on the address and BOLT11 invoice.
Send Method: Parses a URI string, attempts to pay a BOLT12 offer, BOLT11 invoice, then falls back to the on-chain address if the offer and invoice fail.
Tests: The parse_uri parses the URIs and verifies we're pulling the right values. The first URI was created from the receive method, but the others were created from zeus and muun. The unified_qr_send_and_receive test verifies the functionality of generating and making Unified QR payments. This includes handling BOLT12 offers, verifying payment types, and ensuring the correct payments between nodes. The generate_bip21_uri test creates a URI to visualize.

I'd appreciate some feedback on how things look as well as any critiques on the structure of the module or any errors I made

tnull

Did a first pass, already seems to go into the right direction.

A few general notes though:

Please format your code via cargo fmt before each commit (in particular, we use tabs, not spaces, to indent)
Please try to break the changes into logical units by creating commits that clearly state what they do and why they do it. If you need to make additional changes to the same code afterwards, please do so in clearly marked fixup commits (usually they start the message with f or fixup) which will be squashed before merging. You'll want to get to a point where the reviewer can review large change sets commit-by-commit. You can find a good tutorial here:
https://cbea.ms/git-commit/
We generally should only expose few, well documented API methods/objects to the user. Only these should be pub and everything pub needs a doc comment that will end up in the rendered documentation. After you wrote such a doc comment, it's always a good idea to check how things look and if the changes render nicely via cargo doc [--open].

src/payment/unifiedqr.rs

slanesuke · 2024-06-06T16:34:30Z

@tnull I read uppercasing characters in URIs make the qr code easier to scan. Should I format the URI so all characters are uppercase? Or maybe just the address and invoice? As of right now the URIs come out in this format with bitcoin in all caps: BITCOIN:TB1Q78S2PPDUEFLXEWDW5UAXHLMUED6UZJHXFP7DED?amount=0.01&message=Test%20message&lightning=lntb10m1pnxreqkdq523jhxapqd4jhxumpvajsnp4q2tev92txgkgjr0673w08dk033xwz8u2ks2thvpm5tw4kd2xn7apgpp5xefd0dw6da4r9ajfrs6dctf3xwd6agnfvat52s0vf627cnnzr5fssp5kmgq67kzm4rfmkpd2kmjzz60aw476zhsu4d46h5wjmfcqa28w3ls9qyysgqcqpcxqrraqqz74svlwzjpqna2tgq0497jfx0d28pm0fklp5rngdxgj6vqpkqyj8w6h2e7ck3mlsvd0jecnmjpz86zkr0yj2getjp6rzhc9ced72hcqucgzc3

tnull · 2024-06-10T09:56:31Z

@tnull I read uppercasing characters in URIs make the qr code easier to scan. Should I format the URI so all characters are uppercase? Or maybe just the address and invoice? As of right now the URIs come out in this format with bitcoin in all caps: BITCOIN:TB1Q78S2PPDUEFLXEWDW5UAXHLMUED6UZJHXFP7DED?amount=0.01&message=Test%20message&lightning=lntb10m1pnxreqkdq523jhxapqd4jhxumpvajsnp4q2tev92txgkgjr0673w08dk033xwz8u2ks2thvpm5tw4kd2xn7apgpp5xefd0dw6da4r9ajfrs6dctf3xwd6agnfvat52s0vf627cnnzr5fssp5kmgq67kzm4rfmkpd2kmjzz60aw476zhsu4d46h5wjmfcqa28w3ls9qyysgqcqpcxqrraqqz74svlwzjpqna2tgq0497jfx0d28pm0fklp5rngdxgj6vqpkqyj8w6h2e7ck3mlsvd0jecnmjpz86zkr0yj2getjp6rzhc9ced72hcqucgzc3

Yes, for the on-chain part bip21/rust-bitcoin already do the right thing if you use the {:#} formatter (see https://github.com/Kixunil/bip21/blob/88ac4516ccf19ccbf747a874f9725accac08fe17/src/ser.rs#L122-L143 / https://github.com/rust-bitcoin/rust-bitcoin/blob/45433095182e103430c33f07c9746f71e1b4fbcf/bitcoin/src/address/mod.rs#L146-L182), but for the rest we probably want to do it manually for now. Eventually we'll need to test that none of that interferes with wallet compatibility too much.

src/payment/unified_qr.rs

src/error.rs

tests/integration_tests_rust.rs

slanesuke · 2024-06-20T01:39:52Z

@tnull I added the ability to pay an offer in the send method. And since we aren't adding an offer when creating the URI I just concatenated an offer to an existing URI in the integration tests and it worked fine! Also, I had to determine whether the value after the lighting parameter was an invoice or an offer based on the prefix (lnbt, lntb, lno, etc) so thats a bit quirky but let me know what you think!

tnull

Looks mostly good!

src/payment/unified_qr.rs

src/lib.rs

bindings/ldk_node.udl

src/error.rs

TheBlueMatt · 2024-07-06T12:39:00Z

src/payment/unified_qr.rs

+				String::try_from(value).map_err(|_| Error::UriParameterParsingFailed)?;
+
+			for param in lighting_str.split('&') {
+				if let Ok(offer) = param.parse::<Offer>() {


BOLT12 offers go in the "lno" parameter, not the "lightning" parameter.

Ah, good to know. Thanks!

BOLT12 offers go in the "lno" parameter, not the "lightning" parameter.

Seems the examples on https://bitcoinqr.dev/ are also wrong/outdated then? Could you point us to an updated resource (besides BIP353, which seems orthogonal?) that explains the usage of lno? I assume there might be other updates we might be missing then? Seems https://delvingbitcoin.org/t/revisiting-bip21/ proposes also the slightly different b12 as the parameter?

Ah, I guess this is the relevant resource here?: bitcoin/bips#1555

slanesuke · 2024-07-09T20:01:25Z

After some research, a BIP21 URI containing both an invoice and an offer should scan fine as a QR code. According to ISO/IEC 18004, which is the international standard for QR codes, a static QR code can store up to 3 KB of data (4,296 alphanumeric characters). This should easily accommodate our URI size.

So, our BIP21 QR code would be scannable by most modern smartphones, including older iPhones and Androids, if its printed with fair resolution and within a minimum physical dimension of 2.5x2.5cm (which is larger than all wallets I've checked). Plus, error correction levels in QR codes ensure reliability, even if the QR code is damaged

slanesuke · 2024-07-09T20:02:49Z

@tnull ready for review!

tnull

Mostly looks good to me, mod a few minor things!

Cargo.toml

src/payment/unified_qr.rs

tnull · 2024-07-12T11:26:05Z

Btw, feel free to rebase on main since we just fixed the broken CI (mod flaky jobs, of course ..)

Firstly, I thought I staged and made commits for `unified_qr.rs` so sorry if this is out of order! But in `unified_qr.rs` I - I introduced the `UnifiedQrPayment` struct to handle creating and paying BIP21 URIs - `receive` generates a URI with an on-chain address and BOLT11 invoice and returns the URI as a string - `send` will parse a given URI string and attempt to send the BOLT12 offer, BOLT11 invoice, then if those fail the fallback on-chain address will be paid to. - Then I included tests for URI generation and URI parsing - Also has logging and error handling for payment operations

- implement unified_qr_payment method to create the Unified QR payment handler - Includes conditional UniFFI features and updates docs with BIP21 and BOLT11 links

- Included unified_qr in payment module - Added `PaymentResult` and `UnifiedQrPayment` from unified_qr for public use

- Introduced `UnifiedQrPayment` method to `Node` interface - Add `UnifiedQrPayment` interface with `receieve and `send` methods - Add `PaymentResult` interface (enum) with `Onchain`, `Bolt11` and `Bolt12` fields These changes add support for our UniFFI bindings and enable the use of `unified_qr_payment` payment handler in Swift, and Kotlin.

- Add `UriParameterFailed` and `InvalidUri` fields to the `Error` enum - Added related error messages in the Display impl for the new fields

- Added `PaymentResult` so the .udl could access the enum - Added comment to explain the need to import any re-exported items to enure they're accessible in UniFFI. (becasue rustc says to add them in `lib.rs`

- Added `unified_qr_send_receive` test to verify the `UnifedQrPayment` functionality - Added logic to handle paying a `BOLT12` offer, `BOLT11` invoice, and if those fail `On-chain` tx from a URI. - Validated each payments successful event - Ensured the off-chain and on-chain balacnes reflected the payment attempts

The changes include: - Fixed a handful of nits for better readability in docs and simple grammar errors and made various name changes that affected the committed files. - Added a helper function in unified_qr.rs called capitalize_qr_params to format the lightning param in the receive method - Removed the optional message in the receive method and made it a required &str - Adjusted UDL formatting to use tabs instead of spaces These changes were made to improve code quality and maintainability based on the review feedback

Changes include: - Modified serialize_params to serialize both invoices and offers - Refactored deserialize_temp by removing the code that was parsing based on the lightning invoice/offer prefix. I instead used for loop to iterate over each lightning parameter, attempting to parse the string as an offer first, and then as an invoice. May need to log an error if neither succeeds - Added support for Bolt12 offers in the receive method - Updated capitalize_params function to handle multiple lightning parameters - Added a generate_bip21_uri test to show what the uri looks like in integration_tests_rust - Adjusted integration tests. Still needs work Still trying to figure out a bug related to Bolt12 offers being "paid" when it should fall back to an on-chain tx

In this commit: - In serialize_params, BOLT12 offers were changed to be serialized with the `lno` key rather than the `lightning` key - During deserializing, I had to make the same update. Used a match to check whether it was a `lightning` or `lno` key and then parsed accordingly. - Next, a small name change: capitalize_qr_params to format_uri. Previously I changed the value after "&lightning" to all caps, but the "&lno=" value wasn't being changed. So, added a helper method inside format_uri to capitalize the values given the key! - Updated corresponding tests with `lno` update Small nits: - Updated QrPaymentResult with more thorough docs - Added a parsing test with an offer

This commit fixes a handful of minor comments/nits that include: - Updates to set the `bip21` crates default-features to false, to minimize dependencies. - Enable the `std` feature since we use/benefit from it. - In `receive` return `InvoiceCreationFailed` or `OfferCreationFailed` when creating an invoice or offer. Rather than silently logging the error. - Also in `receive` we first check if an amount is specified, and if not, return an error and abort. - Pass in `Config` to `UnifiedQrPayment` struct to use the users config network. - In `send` instead of checking each network for the `NetworkChecked` URI, we pass in the `Config::Network`. - Simplifed param parsing in `deserialize_temp` by directly finding the key and parsing the corresponding value. - General documentation fixes. - In parsing tests, moved longer invoice/offer strings into. variables that start with expected_ for clarity.

slanesuke · 2024-07-15T22:12:36Z

rebased

tnull

Mostly LGTM, docs might need another round of clarification though.

tnull · 2024-07-16T08:52:50Z

src/payment/unified_qr.rs

+	/// # Parameters
+	/// - `amount_sats`: The amount to be received, specified in satoshis.
+	/// - `message`: A description or note associated with the payment.
+	///   This message is visible to the payee and can provide context or details about the payment.


Is it? I think neither the BOLT11 / BOLT12 descriptions nor the BIP21 message are ever sent to the payee, but will be seen by the payer, no?

Also, should this parameter rather be called description to make it less confusing for Lightning users?

Is it? I think neither the BOLT11 / BOLT12 descriptions nor the BIP21 message are ever sent to the payee, but will be seen by the payer, no?

Also, should this parameter rather be called description to make it less confusing for Lightning users?

No, you're right! I meant payer. I'll update and use description since it's used elsewhere. Thanks for pointing that out!

src/payment/unified_qr.rs

Cleaned up the docs so they are easier to understand for the user. Also changed the message param in receive to description.

tnull

One last nit, but otherwise should be good to go.

tnull · 2024-07-17T09:02:10Z

src/payment/unified_qr.rs

+	/// # Return Value
+	///
+	/// **Success**: Returns a `QrPaymentResult` indicating the result of the payment.
+	/// * `QrPaymentResult::Bolt12 { payment_id }` if the BOLT12 offer was paid.


Let's drop these specifics for the variants here as they are redundant with the QrPaymentResult docs. Could also summarize the success and error case in one short paragraph (same above for receive).

slanesuke · 2024-07-17T13:36:38Z

One last nit, but otherwise should be good to go.

Should I squash down to one commit?

tnull

LGTM, thanks!

slanesuke marked this pull request as draft May 29, 2024 20:55

tnull reviewed May 31, 2024

View reviewed changes

slanesuke force-pushed the 2024-05-wip-introduce-unified-qr branch from e296eee to 8ba6166 Compare June 5, 2024 21:49

tnull reviewed Jun 10, 2024

View reviewed changes

tests/integration_tests_rust.rs Outdated Show resolved Hide resolved

slanesuke force-pushed the 2024-05-wip-introduce-unified-qr branch from f273f70 to acb56b7 Compare June 20, 2024 01:30

slanesuke changed the title ~~WIP: Add BIP21 Unified QR Code Support~~ Add BIP21 Unified QR Code Support Jun 20, 2024

slanesuke marked this pull request as ready for review June 20, 2024 21:46

slanesuke force-pushed the 2024-05-wip-introduce-unified-qr branch from f93b217 to a1c5629 Compare June 21, 2024 16:16

slanesuke force-pushed the 2024-05-wip-introduce-unified-qr branch from a1c5629 to 5967e14 Compare July 1, 2024 15:35

tnull reviewed Jul 2, 2024

View reviewed changes

slanesuke force-pushed the 2024-05-wip-introduce-unified-qr branch 3 times, most recently from a9b7fca to 98ce5e3 Compare July 5, 2024 20:43

TheBlueMatt reviewed Jul 6, 2024

View reviewed changes

slanesuke force-pushed the 2024-05-wip-introduce-unified-qr branch from 118b479 to 94efe34 Compare July 9, 2024 19:55

tnull reviewed Jul 12, 2024

View reviewed changes

slanesuke force-pushed the 2024-05-wip-introduce-unified-qr branch from 94efe34 to f17da17 Compare July 12, 2024 16:30

slanesuke added 6 commits July 15, 2024 16:11

Add unified_qr_payment payment handler to Node

0132f7a

- implement unified_qr_payment method to create the Unified QR payment handler - Includes conditional UniFFI features and updates docs with BIP21 and BOLT11 links

Add support for unified_qr in payment mod

67c4b41

- Included unified_qr in payment module - Added `PaymentResult` and `UnifiedQrPayment` from unified_qr for public use

Add bip21 crate to handle BIP21 URIs

7e1eb16

Update Error enum with URI related errors

b2c6707

- Add `UriParameterFailed` and `InvalidUri` fields to the `Error` enum - Added related error messages in the Display impl for the new fields

slanesuke added 6 commits July 15, 2024 16:11

Add PaymentResult import for UniFFI bindings

59bea4d

- Added `PaymentResult` so the .udl could access the enum - Added comment to explain the need to import any re-exported items to enure they're accessible in UniFFI. (becasue rustc says to add them in `lib.rs`

slanesuke force-pushed the 2024-05-wip-introduce-unified-qr branch from f17da17 to 7384507 Compare July 15, 2024 22:12

tnull reviewed Jul 16, 2024

View reviewed changes

Fix docs for clarity

8be8083

Cleaned up the docs so they are easier to understand for the user. Also changed the message param in receive to description.

tnull reviewed Jul 17, 2024

View reviewed changes

f docs

3b863ea

tnull approved these changes Jul 18, 2024

View reviewed changes

tnull merged commit 77a0bbe into lightningdevkit:main Jul 18, 2024
7 of 13 checks passed

This was referenced Jul 19, 2024

Socratic Seminar #21 Topics NashBitDevs/nashbitdevs.org#33

Closed

bip21 reez/Monday#110

Merged

feat: bip21 reez/BitcoinUI#101

Merged

tnull mentioned this pull request Oct 7, 2024

Add helpers for BIP21/Unified QR Support #182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BIP21 Unified QR Code Support #302

Add BIP21 Unified QR Code Support #302

slanesuke commented May 29, 2024 •

edited

Loading

tnull left a comment •

edited

Loading

slanesuke commented Jun 6, 2024

tnull commented Jun 10, 2024 •

edited

Loading

slanesuke commented Jun 20, 2024 •

edited

Loading

tnull left a comment

TheBlueMatt Jul 6, 2024

slanesuke Jul 8, 2024

tnull Jul 9, 2024 •

edited

Loading

tnull Jul 9, 2024 •

edited

Loading

slanesuke commented Jul 9, 2024

slanesuke commented Jul 9, 2024

tnull left a comment

tnull commented Jul 12, 2024

slanesuke commented Jul 15, 2024

tnull left a comment

tnull Jul 16, 2024

slanesuke Jul 16, 2024

tnull left a comment

tnull Jul 17, 2024 •

edited

Loading

slanesuke commented Jul 17, 2024

tnull left a comment

Add BIP21 Unified QR Code Support #302

Add BIP21 Unified QR Code Support #302

Conversation

slanesuke commented May 29, 2024 • edited Loading

tnull left a comment • edited Loading

Choose a reason for hiding this comment

slanesuke commented Jun 6, 2024

tnull commented Jun 10, 2024 • edited Loading

slanesuke commented Jun 20, 2024 • edited Loading

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt Jul 6, 2024

Choose a reason for hiding this comment

slanesuke Jul 8, 2024

Choose a reason for hiding this comment

tnull Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

tnull Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

slanesuke commented Jul 9, 2024

slanesuke commented Jul 9, 2024

tnull left a comment

Choose a reason for hiding this comment

tnull commented Jul 12, 2024

slanesuke commented Jul 15, 2024

tnull left a comment

Choose a reason for hiding this comment

tnull Jul 16, 2024

Choose a reason for hiding this comment

slanesuke Jul 16, 2024

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

tnull Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

slanesuke commented Jul 17, 2024

tnull left a comment

Choose a reason for hiding this comment

slanesuke commented May 29, 2024 •

edited

Loading

tnull left a comment •

edited

Loading

tnull commented Jun 10, 2024 •

edited

Loading

slanesuke commented Jun 20, 2024 •

edited

Loading

tnull Jul 9, 2024 •

edited

Loading

tnull Jul 9, 2024 •

edited

Loading

tnull Jul 17, 2024 •

edited

Loading