Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bindings/ldk_node.udl
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,8 @@ interface FeeRate {
interface UnifiedPayment {
[Throws=NodeError]
string receive(u64 amount_sats, [ByRef]string message, u32 expiry_sec);
[Throws=NodeError]
PaymentResult send([ByRef]string uri_str);
[Throws=NodeError, Async]
PaymentResult send([ByRef]string uri_str, u64? amount_msat);
};

interface LSPS1Liquidity {
Expand Down
118 changes: 84 additions & 34 deletions src/payment/unified.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use lightning_invoice::{Bolt11Invoice, Bolt11InvoiceDescription, Description};

use bip21::de::ParamKind;
use bip21::{DeserializationError, DeserializeParams, Param, SerializeParams};
use bitcoin::address::{NetworkChecked, NetworkUnchecked};
use bitcoin::address::NetworkChecked;
use bitcoin::{Amount, Txid};

use std::sync::Arc;
Expand Down Expand Up @@ -143,54 +143,104 @@ impl UnifiedPayment {
Ok(format_uri(uri))
}

/// Sends a payment given a [BIP 21] URI.
/// Sends a payment given a [BIP 21] URI or [BIP 353] HRN.
///
/// This method parses the provided URI string and attempts to send the payment. If the URI
/// has an offer and or invoice, it will try to pay the offer first followed by the invoice.
/// If they both fail, the on-chain payment will be paid.
///
/// Returns a `QrPaymentResult` indicating the outcome of the payment. If an error
/// Returns a `PaymentResult` indicating the outcome of the payment. If an error
/// 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> {
let uri: bip21::Uri<NetworkUnchecked, Extras> =
uri_str.parse().map_err(|_| Error::InvalidUri)?;

let uri_network_checked =
uri.clone().require_network(self.config.network).map_err(|_| Error::InvalidNetwork)?;

if let Some(offer) = uri_network_checked.extras.bolt12_offer {
let offer = maybe_wrap(offer);
match self.bolt12_payment.send(&offer, None, None) {
Ok(payment_id) => return Ok(QrPaymentResult::Bolt12 { payment_id }),
Err(e) => log_error!(self.logger, "Failed to send BOLT12 offer: {:?}. This is part of a unified QR code payment. Falling back to the BOLT11 invoice.", e),
/// [BIP 353]: https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki
pub async fn send(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the first async method (mod Node::next_event_async) we do expose in the API, which makes it a bit incongruent with the rest of the API.

That said, I'm not super opposed to it, let's use this as a first experimental venture into async-API world. If any users complain, we could reuse our runtime to drive this and expose a blocking API, which is what we did so far.

&self, uri_str: &str, amount_msat: Option<u64>,
) -> Result<PaymentResult, Error> {
let instructions = match PaymentInstructions::parse(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

uri_str,
self.config.network,
self.hrn_resolver.as_ref(),
true,
)
.await
{
Ok(instr) => instr,
Err(e) => {
log_error!(self.logger, "Failed to parse payment instructions: {:?}", e);
return Err(Error::UriParameterParsingFailed);
},
};

let resolved = match instructions {
PaymentInstructions::ConfigurableAmount(ref instr) => {
if let Some(amount) = amount_msat {
let amt = match BPIAmount::from_sats(amount) {
Ok(amt) => amt,
Err(e) => {
log_error!(self.logger, "Error while coverting amount : {:?}", e);
return Err(Error::InvalidAmount);
},
};
match instr.clone().set_amount(amt, self.hrn_resolver.as_ref()).await {
Ok(resolved) => resolved,
Err(e) => {
log_error!(self.logger, "Failed to set amount: {:?}", e);
return Err(Error::InvalidAmount);
},
}
} else {
log_error!(self.logger, "No amount specified. Aborting the payment.");
return Err(Error::InvalidAmount);
}
},
PaymentInstructions::FixedAmount(ref instr) => instr.clone(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

if let Some(PaymentMethod::LightningBolt12(offer)) =
resolved.methods().iter().find(|m| matches!(m, PaymentMethod::LightningBolt12(_)))
{
let offer = maybe_wrap(offer.clone());
if let Some(amount_msat) = amount_msat {
match self.bolt12_payment.send_using_amount(&offer, amount_msat, None, None) {
Ok(payment_id) => return Ok(PaymentResult::Bolt12 { payment_id }),
Err(e) => log_error!(self.logger, "Failed to send BOLT12 offer: {:?}. This is part of a unified payment. Falling back to the BOLT11 invoice.", e),
}
} else {
match self.bolt12_payment.send(&offer, None, None) {
Ok(payment_id) => return Ok(PaymentResult::Bolt12 { payment_id }),
Err(e) => log_error!(self.logger, "Failed to send BOLT12 offer: {:?}. This is part of a unified payment. Falling back to the BOLT11 invoice.", e),
}
}
}

if let Some(invoice) = uri_network_checked.extras.bolt11_invoice {
let invoice = maybe_wrap(invoice);
if let Some(PaymentMethod::LightningBolt11(invoice)) =
resolved.methods().iter().find(|m| matches!(m, PaymentMethod::LightningBolt11(_)))
{
let invoice = maybe_wrap(invoice.clone());
match self.bolt11_invoice.send(&invoice, None) {
Ok(payment_id) => return Ok(QrPaymentResult::Bolt11 { payment_id }),
Err(e) => log_error!(self.logger, "Failed to send BOLT11 invoice: {:?}. This is part of a unified QR code payment. Falling back to the on-chain transaction.", e),
Ok(payment_id) => return Ok(PaymentResult::Bolt11 { payment_id }),
Err(e) => log_error!(self.logger, "Failed to send BOLT11 invoice: {:?}. This is part of a unified payment. Falling back to the on-chain transaction.", e),
}
}

let amount = match uri_network_checked.amount {
Some(amount) => amount,
None => {
log_error!(self.logger, "No amount specified in the URI. Aborting the payment.");
return Err(Error::InvalidAmount);
},
};

let txid = self.onchain_payment.send_to_address(
&uri_network_checked.address,
amount.to_sat(),
None,
)?;
if let Some(PaymentMethod::OnChain(address)) =
resolved.methods().iter().find(|m| matches!(m, PaymentMethod::OnChain(_)))
{
let amount = match resolved.onchain_payment_amount() {
Some(amount) => amount,
None => {
log_error!(self.logger, "No amount specified. Aborting the payment.");
return Err(Error::InvalidAmount);
},
};

Ok(QrPaymentResult::Onchain { txid })
let txid =
self.onchain_payment.send_to_address(&address, amount.sats().unwrap(), None)?;
return Ok(PaymentResult::Onchain { txid });
}
log_error!(self.logger, "Payable methods not found in URI");
Err(Error::PaymentSendingFailed)
}
}

Expand Down Expand Up @@ -319,7 +369,7 @@ impl DeserializationError for Extras {
mod tests {
use super::*;
use crate::payment::unified::Extras;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

use bitcoin::{Address, Network};
use bitcoin::{address::NetworkUnchecked, Address, Network};
use std::str::FromStr;

#[test]
Expand Down