Add BIP21 Unified QR Code Support #302
Conversation
There was a problem hiding this comment.
Did a first pass, already seems to go into the right direction.
A few general notes though:
- Please format your code via
cargo fmtbefore 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
forfixup) 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
puband everythingpubneeds 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 viacargo doc [--open].
e296eee to
8ba6166
Compare
|
@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: |
Yes, for the on-chain part |
f273f70 to
acb56b7
Compare
|
@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! |
f93b217 to
a1c5629
Compare
a1c5629 to
5967e14
Compare
a9b7fca to
98ce5e3
Compare
src/payment/unified_qr.rs
Outdated
| String::try_from(value).map_err(|_| Error::UriParameterParsingFailed)?; | ||
|
|
||
| for param in lighting_str.split('&') { | ||
| if let Ok(offer) = param.parse::<Offer>() { |
There was a problem hiding this comment.
BOLT12 offers go in the "lno" parameter, not the "lightning" parameter.
There was a problem hiding this comment.
Ah, good to know. Thanks!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ah, I guess this is the relevant resource here?: bitcoin/bips#1555
118b479 to
94efe34
Compare
|
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 |
|
@tnull ready for review! |
tnull
left a comment
There was a problem hiding this comment.
Mostly looks good to me, mod a few minor things!
|
Btw, feel free to rebase on |
94efe34 to
f17da17
Compare
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.
f17da17 to
7384507
Compare
|
rebased |
tnull
left a comment
There was a problem hiding this comment.
Mostly LGTM, docs might need another round of clarification though.
src/payment/unified_qr.rs
Outdated
| /// # 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
Cleaned up the docs so they are easier to understand for the user. Also changed the message param in receive to description.
tnull
left a comment
There was a problem hiding this comment.
One last nit, but otherwise should be good to go.
src/payment/unified_qr.rs
Outdated
| /// # Return Value | ||
| /// | ||
| /// **Success**: Returns a `QrPaymentResult` indicating the result of the payment. | ||
| /// * `QrPaymentResult::Bolt12 { payment_id }` if the BOLT12 offer was paid. |
There was a problem hiding this comment.
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).
Should I squash down to one commit? |
Based on #182
This pull request introduces a payment submodule
unifiedqr.rsthat 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
parse_uriparses the URIs and verifies we're pulling the right values. The first URI was created from thereceivemethod, but the others were created from zeus and muun. Theunified_qr_send_and_receivetest 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. Thegenerate_bip21_uritest 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