f - Merge InvoicePayer and PaymentRetryHandler

Author: jkczyz

lightning-invoice/src/payment.rs

@@ -14,10 +14,10 @@
 //! to send a payment over a [`Route`]. Implementations of [`Router`] find a [`Route`] between payer
 //! and payee using information provided by the payer and from the payee's [`Invoice`].
 //!
-//! [`InvoicePayer`] caches each [`Invoice`] by its `payment_hash` so that [`PaymentRetryHandler`]
-//! can retry the payment if it fails. It accomplishes this by implementing [`EventHandler`] which
-//! decorates a user-provided handler. It will intercepts any [`Event::PaymentFailed`] events and
-//! retry the payment a fixed number of times before failing or succeeding as needed.
+//! [`InvoicePayer`] caches each [`Invoice`] by its `payment_hash` so that it can retry the payment
+//! if it fails. It accomplishes this by implementing [`EventHandler`] which decorates a
+//! user-provided handler. It will intercepts any [`Event::PaymentFailed`] events and retry the
+//! payment a fixed number of times before failing or succeeding as needed.
 //!
 //! # Example
 //!
@@ -28,7 +28,7 @@
 //! The [`Route`] is compute before each payment attempt. Any updates affecting path finding such as
 //! updates to the network graph or changes to channels scores should be applied prior to retries.
 //! This typically means any [`EventHandler`] decorators responsible for this should decorate
-//! [`PaymentRetryHandler`] so that such changes take effect before retrying.
+//! [`InvoicePayer`] so that such changes take effect before retrying.
 
 use crate::Invoice;
 
@@ -44,7 +44,6 @@ use lightning::routing::router::{Route, RouteHint};
 use lightning::util::events::{Event, EventHandler};
 use lightning::util::logger::Logger;
 
-use std::cell::RefCell;
 use std::collections::hash_map::{self, HashMap};
 use std::ops::Deref;
 use std::sync::Mutex;
@@ -54,10 +53,20 @@ use std::sync::Mutex;
 const MAX_PAYMENT_ATTEMPTS: usize = 3;
 
 /// A utility for paying [`Invoice]`s.
-pub struct InvoicePayer<P: Deref, R> where P::Target: Payer, R: Router {
+pub struct InvoicePayer<P: Deref, R, L: Deref, E>
+where
+	P::Target: Payer,
+	R: Router,
+	L::Target: Logger,
+	E: EventHandler,
+{
 	payer: P,
 	router: R,
+	logger: L,
+	event_handler: E,
+	// Lock order: payment_attempts -> invoice_cache
 	invoice_cache: Mutex<HashMap<PaymentHash, Invoice>>,
+	payment_attempts: Mutex<HashMap<PaymentHash, usize>>,
 }
 
 /// A trait defining behavior of an [`Invoice`] payer.
@@ -101,28 +110,22 @@ pub enum PaymentError {
 	Sending(PaymentSendFailure),
 }
 
-/// An [`EventHandler`] decorator for retrying failed payments.
-pub struct PaymentRetryHandler<I, P: Deref, R, L: Deref, E>
+impl<P: Deref, R, L: Deref, E> InvoicePayer<P, R, L, E>
 where
-	I: Deref<Target = InvoicePayer<P, R>>,
 	P::Target: Payer,
 	R: Router,
 	L::Target: Logger,
 	E: EventHandler,
 {
-	invoice_payer: I,
-	payment_attempts: RefCell<HashMap<PaymentHash, usize>>,
-	logger: L,
-	event_handler: E,
-}
-
-impl<P: Deref, R> InvoicePayer<P, R> where P::Target: Payer, R: Router {
 	/// Creates an invoice payer.
-	pub fn new(payer: P, router: R) -> Self {
+	pub fn new(payer: P, router: R, logger: L, event_handler: E) -> Self {
 		Self {
 			payer,
 			router,
+			logger,
+			event_handler,
 			invoice_cache: Mutex::new(HashMap::new()),
+			payment_attempts: Mutex::new(HashMap::new()),
 		}
 	}
 
@@ -175,35 +178,15 @@ impl<P: Deref, R> InvoicePayer<P, R> where P::Target: Payer, R: Router {
 
 	/// Removes the [`Invoice`] cached by the given payment hash.
 	///
-	/// Should be called once a payment has failed or succeeded. This is taken care of by
-	/// [`PaymentRetryHandler`], but can be called independently as well.
+	/// Should be called once a payment has failed or succeeded. This is taken care of when
+	/// [`InvoicePayer`] is used as an [`EventHandler`] but can be called independently as well.
 	pub fn remove_cached_invoice(&self, payment_hash: &PaymentHash) {
 		self.invoice_cache.lock().unwrap().remove(payment_hash);
 	}
 }
 
-impl<I, P: Deref, R, L: Deref, E> PaymentRetryHandler<I, P, R, L, E>
-where
-	I: Deref<Target = InvoicePayer<P, R>>,
-	P::Target: Payer,
-	R: Router,
-	L::Target: Logger,
-	E: EventHandler,
-{
-	/// Creates a payment retry handler.
-	pub fn new(invoice_payer: I, logger: L, event_handler: E) -> Self {
-		Self {
-			invoice_payer,
-			payment_attempts: RefCell::new(HashMap::new()),
-			logger,
-			event_handler,
-		}
-	}
-}
-
-impl<I, P: Deref, R, L: Deref, E> EventHandler for PaymentRetryHandler<I, P, R, L, E>
+impl<P: Deref, R, L: Deref, E> EventHandler for InvoicePayer<P, R, L, E>
 where
-	I: Deref<Target = InvoicePayer<P, R>>,
 	P::Target: Payer,
 	R: Router,
 	L::Target: Logger,
@@ -212,14 +195,14 @@ where
 	fn handle_event(&self, event: &Event) {
 		match event {
 			Event::PaymentFailed { payment_hash, rejected_by_dest, .. } => {
-				let mut attempts_by_payment_hash = self.payment_attempts.borrow_mut();
+				let mut attempts_by_payment_hash = self.payment_attempts.lock().unwrap();
 				let attempts = attempts_by_payment_hash
 					.entry(*payment_hash)
 					.and_modify(|attempts| *attempts += 1)
 					.or_insert(1);
 				if !rejected_by_dest {
 					if *attempts < MAX_PAYMENT_ATTEMPTS {
-						if self.invoice_payer.pay_cached_invoice(payment_hash).is_ok() {
+						if self.pay_cached_invoice(payment_hash).is_ok() {
 							log_trace!(self.logger, "Payment {} failed; retrying (attempts: {})", log_bytes!(payment_hash.0), attempts);
 							return;
 						} else {
@@ -233,18 +216,18 @@ where
 				}
 
 				// Either the payment was rejected, exceeded the maximum attempts, or failed retry.
-				self.invoice_payer.remove_cached_invoice(payment_hash);
+				self.remove_cached_invoice(payment_hash);
 				attempts_by_payment_hash.remove(payment_hash);
 			},
 			Event::PaymentSent { payment_preimage } => {
 				let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
-				self.invoice_payer.remove_cached_invoice(&payment_hash);
-
-				let attempts = self.payment_attempts
-					.borrow_mut()
+				let mut attempts_by_payment_hash = self.payment_attempts.lock().unwrap();
+				let attempts = attempts_by_payment_hash
 					.remove(&payment_hash)
 					.map_or(1, |attempts| attempts + 1);
 				log_trace!(self.logger, "Payment {} succeeded (attempts: {})", log_bytes!(payment_hash.0), attempts);
+
+				self.remove_cached_invoice(&payment_hash);
 			},
 			_ => {},
 		}
@@ -284,35 +267,33 @@ mod tests {
 
 	#[test]
 	fn pays_invoice_on_first_attempt() {
+		let event_handled = core::cell::RefCell::new(false);
+		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+
 		let payer = TestPayer::new();
 		let router = NullRouter {};
-		let invoice_payer = InvoicePayer::new(&payer, router);
-
 		let logger = TestLogger::new();
-		let event_handled = core::cell::RefCell::new(false);
-		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
-		let retry_handler = PaymentRetryHandler::new(&invoice_payer, &logger, event_handler);
+		let invoice_payer = InvoicePayer::new(&payer, router, &logger, event_handler);
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
 		assert!(invoice_payer.pay_invoice(&invoice).is_ok());
 		assert_eq!(*payer.attempts.borrow(), 1);
 
-		retry_handler.handle_event(&Event::PaymentSent { payment_preimage });
+		invoice_payer.handle_event(&Event::PaymentSent { payment_preimage });
 		assert_eq!(*event_handled.borrow(), true);
 		assert_eq!(*payer.attempts.borrow(), 1);
 	}
 
 	#[test]
 	fn pays_invoice_on_retry() {
+		let event_handled = core::cell::RefCell::new(false);
+		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+
 		let payer = TestPayer::new();
 		let router = NullRouter {};
-		let invoice_payer = InvoicePayer::new(&payer, router);
-
 		let logger = TestLogger::new();
-		let event_handled = core::cell::RefCell::new(false);
-		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
-		let retry_handler = PaymentRetryHandler::new(&invoice_payer, &logger, event_handler);
+		let invoice_payer = InvoicePayer::new(&payer, router, &logger, event_handler);
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -324,25 +305,24 @@ mod tests {
 			network_update: None,
 			rejected_by_dest: false,
 		};
-		retry_handler.handle_event(&event);
+		invoice_payer.handle_event(&event);
 		assert_eq!(*event_handled.borrow(), false);
 		assert_eq!(*payer.attempts.borrow(), 2);
 
-		retry_handler.handle_event(&Event::PaymentSent { payment_preimage });
+		invoice_payer.handle_event(&Event::PaymentSent { payment_preimage });
 		assert_eq!(*event_handled.borrow(), true);
 		assert_eq!(*payer.attempts.borrow(), 2);
 	}
 
 	#[test]
 	fn fails_paying_invoice_after_max_retries() {
+		let event_handled = core::cell::RefCell::new(false);
+		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+
 		let payer = TestPayer::new();
 		let router = NullRouter {};
-		let invoice_payer = InvoicePayer::new(&payer, router);
-
 		let logger = TestLogger::new();
-		let event_handled = core::cell::RefCell::new(false);
-		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
-		let retry_handler = PaymentRetryHandler::new(&invoice_payer, &logger, event_handler);
+		let invoice_payer = InvoicePayer::new(&payer, router, &logger, event_handler);
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -354,29 +334,28 @@ mod tests {
 			network_update: None,
 			rejected_by_dest: false,
 		};
-		retry_handler.handle_event(&event);
+		invoice_payer.handle_event(&event);
 		assert_eq!(*event_handled.borrow(), false);
 		assert_eq!(*payer.attempts.borrow(), 2);
 
-		retry_handler.handle_event(&event);
+		invoice_payer.handle_event(&event);
 		assert_eq!(*event_handled.borrow(), false);
 		assert_eq!(*payer.attempts.borrow(), 3);
 
-		retry_handler.handle_event(&event);
+		invoice_payer.handle_event(&event);
 		assert_eq!(*event_handled.borrow(), true);
 		assert_eq!(*payer.attempts.borrow(), 3);
 	}
 
 	#[test]
 	fn fails_paying_invoice_after_retry_error() {
+		let event_handled = core::cell::RefCell::new(false);
+		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+
 		let payer = TestPayer::new().fails_on_attempt(2);
 		let router = NullRouter {};
-		let invoice_payer = InvoicePayer::new(&payer, router);
-
 		let logger = TestLogger::new();
-		let event_handled = core::cell::RefCell::new(false);
-		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
-		let retry_handler = PaymentRetryHandler::new(&invoice_payer, &logger, event_handler);
+		let invoice_payer = InvoicePayer::new(&payer, router, &logger, event_handler);
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -388,21 +367,20 @@ mod tests {
 			network_update: None,
 			rejected_by_dest: false,
 		};
-		retry_handler.handle_event(&event);
+		invoice_payer.handle_event(&event);
 		assert_eq!(*event_handled.borrow(), true);
 		assert_eq!(*payer.attempts.borrow(), 2);
 	}
 
 	#[test]
 	fn fails_paying_invoice_after_rejected_by_payee() {
+		let event_handled = core::cell::RefCell::new(false);
+		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+
 		let payer = TestPayer::new();
 		let router = NullRouter {};
-		let invoice_payer = InvoicePayer::new(&payer, router);
-
 		let logger = TestLogger::new();
-		let event_handled = core::cell::RefCell::new(false);
-		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
-		let retry_handler = PaymentRetryHandler::new(&invoice_payer, &logger, event_handler);
+		let invoice_payer = InvoicePayer::new(&payer, router, &logger, event_handler);
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -414,21 +392,20 @@ mod tests {
 			network_update: None,
 			rejected_by_dest: true,
 		};
-		retry_handler.handle_event(&event);
+		invoice_payer.handle_event(&event);
 		assert_eq!(*event_handled.borrow(), true);
 		assert_eq!(*payer.attempts.borrow(), 1);
 	}
 
 	#[test]
 	fn fails_repaying_invoice_with_pending_payment() {
+		let event_handled = core::cell::RefCell::new(false);
+		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
+
 		let payer = TestPayer::new();
 		let router = NullRouter {};
-		let invoice_payer = InvoicePayer::new(&payer, router);
-
 		let logger = TestLogger::new();
-		let event_handled = core::cell::RefCell::new(false);
-		let event_handler = |_: &_| { *event_handled.borrow_mut() = true; };
-		let retry_handler = PaymentRetryHandler::new(&invoice_payer, &logger, event_handler);
+		let invoice_payer = InvoicePayer::new(&payer, router, &logger, event_handler);
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -453,15 +430,16 @@ mod tests {
 			network_update: None,
 			rejected_by_dest: false,
 		};
-		retry_handler.handle_event(&event);
+		invoice_payer.handle_event(&event);
 		assert_eq!(*event_handled.borrow(), true);
 	}
 
 	#[test]
 	fn fails_paying_invoice_with_routing_errors() {
 		let payer = TestPayer::new();
 		let router = FailingRouter {};
-		let invoice_payer = InvoicePayer::new(&payer, router);
+		let logger = TestLogger::new();
+		let invoice_payer = InvoicePayer::new(&payer, router, &logger, |_: &_| {});
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);
@@ -476,7 +454,8 @@ mod tests {
 	fn fails_paying_invoice_with_sending_errors() {
 		let payer = TestPayer::new().fails_on_attempt(1);
 		let router = NullRouter {};
-		let invoice_payer = InvoicePayer::new(&payer, router);
+		let logger = TestLogger::new();
+		let invoice_payer = InvoicePayer::new(&payer, router, &logger, |_: &_| {});
 
 		let payment_preimage = PaymentPreimage([1; 32]);
 		let invoice = invoice(payment_preimage);