Resolve comments, make PendingPaymentDetails enum instead

Author: jurvis

lightning/src/ln/channelmanager.rs

@@ -1204,36 +1204,26 @@ impl ChannelDetails {
 	}
 }
 
-/// We store payments that are pending resolution as `PendingOutboundPayment` in `ChannelManager`,
-/// and they can take on either one of the following states as defined in this enum.
+/// Used to express the status of payments that are currently pending resolution.
 #[derive(Debug, PartialEq)]
-pub enum PendingPaymentStatus {
+pub enum PendingPaymentDetails {
 	/// When a payment is still being sent and awaiting successful delivery.
-	Retryable,
+	Retryable {
+		/// Total amount (in msat) across all paths for this payment, not just the amount currently
+		/// inflight.
+		total_msat: u64
+	},
 	/// When a pending payment is fulfilled, we continue tracking it until all pending HTLCs have
-	/// been resolved. This ensures we don't look up pending payments in ChannelMonitors on restart
-	/// and add a pending payment that was already fulfilled.
+	/// been resolved. These payments will only cease being tracked upon receiving or handling
+	/// [`Event::PaymentSent`].
 	Fulfilled,
-	/// When a payer gives up trying to retry a payment, they inform us, letting us generate a
-	/// `PaymentFailed` event when all HTLCs have irrevocably failed. This avoids a number of race
-	/// conditions in MPP-aware payment retriers (1), where the possibility of multiple
-	/// `PaymentPathFailed` events with `all_paths_failed` can be pending at once, confusing a
-	/// downstream event handler as to when a payment has actually failed.
-	///
-	/// (1) https://github.com/lightningdevkit/rust-lightning/issues/1164
+	/// After a payment is explicitly abandoned by calling [`ChannelManager::abandon_payment`], they
+	/// are marked as abandoned until an [`Event::PaymentFailed`] event is generated. A payment
+	/// could also be marked as abandoned if pathfinding fails repeatedly and we will no longer
+	/// retry sending.
 	Abandoned,
 }
 
-/// Details of a pending payment, as returned by [`ChannelManager::list_pending_payments`].
-pub struct PendingPaymentDetails {
-	/// Current status of a payment that is pending resolution.
-	pub status: PendingPaymentStatus,
-	/// Total amount (in msat) across all paths for this payment, not just the amount currently
-	/// inflight. This field is `None` if the payment's status is either
-	/// `PendingPaymentStatus::Fulfilled` or `PendingPaymentPaymentStatus::Abandoned`.
-	pub total_msat: Option<u64>,
-}
-
 /// If a payment fails to send, it can be in one of several states. This enum is returned as the
 /// Err() type describing which state the payment is in, see the description of individual enum
 /// states for more.
@@ -1806,13 +1796,13 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		self.pending_outbound_payments.lock().unwrap().iter()
 			.filter_map(|(_, pending_outbound_payment)| match pending_outbound_payment {
 				PendingOutboundPayment::Retryable { total_msat, .. } => {
-					Some(PendingPaymentDetails { status: PendingPaymentStatus::Retryable, total_msat: Some(*total_msat) })
+					Some(PendingPaymentDetails::Retryable { total_msat: *total_msat })
 				},
 				PendingOutboundPayment::Abandoned { .. } => {
-					Some(PendingPaymentDetails { status: PendingPaymentStatus::Abandoned, total_msat: None })
+					Some(PendingPaymentDetails::Abandoned)
 				},
 				PendingOutboundPayment::Fulfilled { .. } => {
-					Some(PendingPaymentDetails { status: PendingPaymentStatus::Fulfilled, total_msat: None })
+					Some(PendingPaymentDetails::Fulfilled)
 				},
 				PendingOutboundPayment::Legacy { .. } => None
 			})

lightning/src/ln/payment_tests.rs

@@ -16,7 +16,7 @@ use crate::chain::channelmonitor::{ANTI_REORG_DELAY, LATENCY_GRACE_PERIOD_BLOCKS
 use crate::chain::transaction::OutPoint;
 use crate::chain::keysinterface::KeysInterface;
 use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
-use crate::ln::channelmanager::{self, BREAKDOWN_TIMEOUT, ChannelManager, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PendingPaymentStatus, PaymentSendFailure, IDEMPOTENCY_TIMEOUT_TICKS};
+use crate::ln::channelmanager::{self, BREAKDOWN_TIMEOUT, ChannelManager, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, IDEMPOTENCY_TIMEOUT_TICKS, PendingPaymentDetails};
 use crate::ln::msgs;
 use crate::ln::msgs::ChannelMessageHandler;
 use crate::routing::router::{PaymentParameters, get_route};
@@ -1282,7 +1282,7 @@ fn test_trivial_inflight_htlc_tracking(){
 
 		let pending_payments = nodes[0].node.list_pending_payments();
 		assert_eq!(pending_payments.len(), 1);
-		assert_eq!(pending_payments[0].status, PendingPaymentStatus::Fulfilled);
+		assert_eq!(pending_payments[0], PendingPaymentDetails::Fulfilled);
 	}
 
 	// Remove fulfilled payment
@@ -1317,8 +1317,7 @@ fn test_trivial_inflight_htlc_tracking(){
 
 		let pending_payments = nodes[0].node.list_pending_payments();
 		assert_eq!(pending_payments.len(), 1);
-		assert_eq!(pending_payments[0].status, PendingPaymentStatus::Retryable);
-		assert_eq!(pending_payments[0].total_msat, Some(500000));
+		assert_eq!(pending_payments[0], PendingPaymentDetails::Retryable { total_msat: 500000 });
 	}
 
 	// Now, let's claim the payment. This should result in the used liquidity to return `None`.