Refactor unified_qr.rs to use bitcoin-payment-instructions #666
Conversation
|
I've assigned @tnull as a reviewer! |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
This needs a rebase by now, sorry!
chuksys
force-pushed
the
refactor/adopt-bitcoin-payment-instructions
branch
2 times, most recently
from
1e7d616 to
3e30347
Compare
|
This need a rebase now. |
chuksys
force-pushed
the
refactor/adopt-bitcoin-payment-instructions
branch
2 times, most recently
from
a8ed274 to
4abb530
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Alright, now that we published v0.7.0-rc.0 and branched off release/0.7 we should ready to move forward with this as soon as bitcoin-payment-instructions supporting the new LDK version is released.
To this end, please rebase this PR and clean up the commit history. In particular, please avoid line wrapping for the first line in the commit description and also clearly describe the what, how and why for each change in the commit descriptions. Please refer to https://cbea.ms/git-commit/ for guidance on how to write good commit messages.
src/ffi/types.rs
Outdated
| pub use crate::payment::QrPaymentResult; | ||
| pub use crate::payment::UnifiedPaymentResult; | ||
|
|
||
| pub use lightning::onion_message::dns_resolution::HumanReadableName as LdkHumanReadableName; |
There was a problem hiding this comment.
I don't think we want to expose the LdkHumanReadableName variant, this is just an internal type alias, no?
src/ffi/types.rs
Outdated
| } | ||
|
|
||
| impl HumanReadableName { | ||
| pub fn into_inner(&self) -> LdkHumanReadableName { |
There was a problem hiding this comment.
This shouldn't be pub, and I think we can simply inline this into impl From<HumanReadableName> for LdkHumanReadableName?
| self.inner.clone() | ||
| } | ||
|
|
||
| pub fn from_encoded(encoded: &str) -> Result<Self, Error> { |
There was a problem hiding this comment.
Please provide docs on all public methods. Feel free to copy them over from the LDK counterpart.
| pub fn send_using_amount( | ||
| &self, offer: &Offer, amount_msat: u64, quantity: Option<u64>, payer_note: Option<String>, | ||
| route_parameters: Option<RouteParametersConfig>, | ||
| route_parameters: Option<RouteParametersConfig>, hrn: Option<HumanReadableName>, |
There was a problem hiding this comment.
I don't think we should change the public API here. Please create a pub(crate) send_using_amount_inner method that you can use here and that is simply used with None for the hrn parameter here in send_using_amount.
src/payment/unified.rs
Outdated
| }, | ||
| }; | ||
|
|
||
| if let Some(PaymentMethod::LightningBolt12(offer)) = |
There was a problem hiding this comment.
Why not simply use a match instead of if let Some(..) = .. matches!`?
Rename the source file for the Unified QR payment logic from unified_qr.rs to unified.rs. This change is made in preparation for expanding the module's scope beyond just QR code payment processing.
Rename the primary payment handler struct from UnifiedQRPayment to UnifiedPayment. Also rename QRPaymentResult to UnifiedPaymentResult. This change aligns with the prior module rename and reflects that the component handles a broader range of payment inputs.
Introduce the bitcoin-payment-instructions library as a dependency in Cargo.toml. This library provides the necessary types and parsing logic to handle BIP 21 URIs and BIP 353 HRNs.
Introduce the hrn_resolver dependency into the main Node struct and plumb it through to the UnifiedPayment component. This is necessary to allow the node to resolve offers presented via a Human-Readable Name (HRN).
Restructure the payment initiation logic in unified.rs to handle both BIP 21 URIs and BIP 353 Human-Readable Names (HRNs) as input sources. This provides a single, unified entry point for initiating payments.
Correct a minor typo in the unified_payment send test. The test name is also updated to be more descriptive of the behavior being validated.
Switch the module tests within unified.rs to use explicit use statements instead of glob imports (*). This improves code clarity by clearly indicating which items are being brought into scope.
chuksys
force-pushed
the
refactor/adopt-bitcoin-payment-instructions
branch
from
4abb530 to
ac99cf6
Compare
Adopt the upstream LDK change to use the dedicated pay_for_offer_from_hrn method when initiating a payment for an offer resolved via a Human-Readable Name (HRN). This ensures better alignment with LDK best practices.
chuksys
force-pushed
the
refactor/adopt-bitcoin-payment-instructions
branch
from
ac99cf6 to
ed451bb
Compare
|
Just pushed some changes. Currently fixing the issue that's making CI fail. |
3 participants
This PR is a continuation of #607 which was closed mistakenly when the
developbase branch was deleted.This PR introduces a
unified.rsmodule (which is a refactor of theunified_qr.rsmodule) - this refactor allows us to use this module as a single API for sending payments toBIP 21/321 URIsas well asBIP 353 HRNs, creating a simpler interface for users.https://github.com/rust-bitcoin/bitcoin-payment-instructions is used to parse
BIP 21/321 URIsas well as theBIP 353 HRNs.Changes
unified_qr.rsmodule has been renamed tounified.rs.UnifiedQrPaymentstruct has been renamed toUnifiedPayment.QRPaymentResultenum has been renamed toUnifiedPaymentResult.sendmethod inunified.rsnow supports sending to bothBIP 21/321 URIsas well asBIP 353 HRNs.This PR closes #521 and #435