Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce ManualRoutingParameters #3342

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Introduce ManualRoutingParameters #3342

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c52ab5f
Introduce ManualRoutingParameters
shaavan Sep 25, 2024
e0600cc
Update AwaitingInvoice to Include ManualRoutingParameters
shaavan Sep 25, 2024
2297ce3
Extend pay_for_offer to accept ManualRoutingParameters
shaavan Sep 25, 2024
271522d
Utilize ManualRoutingParameters to create new RouteParams
shaavan Sep 25, 2024
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
16 changes: 11 additions & 5 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ use crate::ln::channel_state::ChannelDetails;
use crate::ln::features::{Bolt12InvoiceFeatures, ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures};
#[cfg(any(feature = "_test_utils", test))]
use crate::ln::features::Bolt11InvoiceFeatures;
use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, Payee, PaymentParameters, Route, RouteParameters, Router};
use crate::routing::router::{BlindedTail, InFlightHtlcs, ManualRoutingParameters, Path, Payee, PaymentParameters, Route, RouteParameters, Router};
use crate::ln::onion_payment::{check_incoming_htlc_cltv, create_recv_pending_htlc_info, create_fwd_pending_htlc_info, decode_incoming_update_add_htlc_onion, InboundHTLCErr, NextPacketDetails};
use crate::ln::msgs;
use crate::ln::onion_utils;
Expand Down Expand Up @@ -1893,10 +1893,11 @@ where
/// # use lightning::events::{Event, EventsProvider};
/// # use lightning::ln::channelmanager::{AChannelManager, PaymentId, RecentPaymentDetails, Retry};
/// # use lightning::offers::offer::Offer;
/// # use lightning::routing::router::ManualRoutingParameters;
/// #
/// # fn example<T: AChannelManager>(
/// # channel_manager: T, offer: &Offer, quantity: Option<u64>, amount_msats: Option<u64>,
/// # payer_note: Option<String>, retry: Retry, max_total_routing_fee_msat: Option<u64>
/// # payer_note: Option<String>, retry: Retry, max_total_routing_fee_msat: Option<ManualRoutingParameters>
/// # ) {
/// # let channel_manager = channel_manager.get_cm();
/// let payment_id = PaymentId([42; 32]);
Expand Down Expand Up @@ -9207,10 +9208,15 @@ macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {

let _persistence_guard = PersistenceNotifierGuard::notify_on_drop($self);

let manual_routing_params = max_total_routing_fee_msat.map(
|fee_msat| ManualRoutingParameters::new()
.with_max_total_routing_fee_msat(fee_msat)
);

let expiration = StaleExpiration::AbsoluteTimeout(absolute_expiry);
$self.pending_outbound_payments
.add_new_awaiting_invoice(
payment_id, expiration, retry_strategy, max_total_routing_fee_msat, None,
payment_id, expiration, retry_strategy, manual_routing_params, None,
)
.map_err(|_| Bolt12SemanticError::DuplicatePaymentId)?;

Expand Down Expand Up @@ -9303,7 +9309,7 @@ where
pub fn pay_for_offer(
&self, offer: &Offer, quantity: Option<u64>, amount_msats: Option<u64>,
payer_note: Option<String>, payment_id: PaymentId, retry_strategy: Retry,
max_total_routing_fee_msat: Option<u64>
manual_routing_params: Option<ManualRoutingParameters>
) -> Result<(), Bolt12SemanticError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

OK no needed option here but: what do you think about having the function call like pay_for_offer(offer, OfferParams::default()) where OfferParams is something like that

struct OfferParams {
    quantity: Option<u64>,
    amount_msats: Option<u64>,
    manual_routing_params: Option<ManualRoutingParameters>,
}

I think is a lot of more verbose in this way, but I also this that this will be a lot of cleaner.

Now I do not think this will fit well in this PR maybe can be a followup one, but with this patter, it is possible to hide future updates to offers (e.g: recurrence or market place feature) without breaking too much the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a great improvement! It will help keep the function signature cleaner while remaining maintainable for future parameter increases.

Maybe we can take this in a follow-up. @TheBlueMatt, what do you think?

let expanded_key = &self.inbound_payment_key;
let entropy = &*self.entropy_source;
Expand Down Expand Up @@ -9345,7 +9351,7 @@ where
};
self.pending_outbound_payments
.add_new_awaiting_invoice(
payment_id, expiration, retry_strategy, max_total_routing_fee_msat,
payment_id, expiration, retry_strategy, manual_routing_params,
Some(retryable_invoice_request)
)
.map_err(|_| Bolt12SemanticError::DuplicatePaymentId)?;
Expand Down
16 changes: 8 additions & 8 deletions lightning/src/ln/outbound_payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1598,13 +1598,9 @@ impl OutboundPayments {

pub(super) fn add_new_awaiting_invoice(
&self, payment_id: PaymentId, expiration: StaleExpiration, retry_strategy: Retry,
max_total_routing_fee_msat: Option<u64>, retryable_invoice_request: Option<RetryableInvoiceRequest>
manual_routing_params: Option<ManualRoutingParameters>, retryable_invoice_request: Option<RetryableInvoiceRequest>
) -> Result<(), ()> {
let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
let manual_routing_params = max_total_routing_fee_msat.map(
|fee_msats| ManualRoutingParameters::new()
.with_max_total_routing_fee_msat(fee_msats)
);
match pending_outbounds.entry(payment_id) {
hash_map::Entry::Occupied(_) => Err(()),
hash_map::Entry::Vacant(entry) => {
Expand Down Expand Up @@ -2281,7 +2277,7 @@ mod tests {
use crate::offers::offer::OfferBuilder;
use crate::offers::test_utils::*;
use crate::routing::gossip::NetworkGraph;
use crate::routing::router::{InFlightHtlcs, Path, PaymentParameters, Route, RouteHop, RouteParameters};
use crate::routing::router::{InFlightHtlcs, ManualRoutingParameters, Path, PaymentParameters, Route, RouteHop, RouteParameters};
use crate::sync::{Arc, Mutex, RwLock};
use crate::util::errors::APIError;
use crate::util::hash_tables::new_hash_map;
Expand Down Expand Up @@ -2695,10 +2691,12 @@ mod tests {
.build().unwrap()
.sign(recipient_sign).unwrap();

let manual_routing_params = ManualRoutingParameters::new().with_max_total_routing_fee_msat(invoice.amount_msats() / 100 + 50_000);

assert!(
outbound_payments.add_new_awaiting_invoice(
payment_id, expiration, Retry::Attempts(0),
Some(invoice.amount_msats() / 100 + 50_000), None,
Some(manual_routing_params), None,
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
Expand Down Expand Up @@ -2796,9 +2794,11 @@ mod tests {
assert!(!outbound_payments.has_pending_payments());
assert!(pending_events.lock().unwrap().is_empty());

let manual_routing_params = ManualRoutingParameters::new().with_max_total_routing_fee_msat(1234);

assert!(
outbound_payments.add_new_awaiting_invoice(
payment_id, expiration, Retry::Attempts(0), Some(1234), None,
payment_id, expiration, Retry::Attempts(0), Some(manual_routing_params), None,
).is_ok()
);
assert!(outbound_payments.has_pending_payments());
Expand Down