Refactor: Address Feedback

1. Move 'handle_onion_message_response' to its original location.
2. Add proper indentations for readability.
3. Change the 'respond' function to take 'self' by value to prevent unnecessary cloning. Also, update 'handle_message' and 'handle_custom_message' to take 'message' by value instead of reference.
4. Make 'reply_path' a private variable.
5. Remove unnecessary comments.
6. Reorganize code by moving the introduced enum above the structs it's used within for better code readability.
7. Make 'responder' the last member of 'WithReplyPath'.
8. Replace 'match' with 'if-else' for initialization.
9. Shorten 'reply_path_option' to 'reply_path_opt'.
10. DRY up the code for improved clarity.

fuzz/src/onion_message.rs

@@ -96,7 +96,7 @@ impl MessageRouter for TestMessageRouter {
 struct TestOffersMessageHandler {}
 
 impl OffersMessageHandler for TestOffersMessageHandler {
-	fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, _message: &ReceivedOnionMessage<OMH, OffersMessage>) {}
+	fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, _message: ReceivedOnionMessage<OMH, OffersMessage>) {}
 }
 
 #[derive(Debug)]
@@ -121,7 +121,7 @@ struct TestCustomMessageHandler {}
 
 impl CustomOnionMessageHandler for TestCustomMessageHandler {
 	type CustomMessage = TestCustomMessage;
-	fn handle_custom_message<F: Fn(Self::CustomMessage, BlindedPath), T: OnionMessageContents>(&self, message: &ReceivedOnionMessage<F, T>) {
+	fn handle_custom_message<F: Fn(Self::CustomMessage, BlindedPath), T: OnionMessageContents>(&self, message: ReceivedOnionMessage<F, T>) {
 		if let ReceivedOnionMessage::WithReplyPath({_, responder}) = message {
 			responder.respond(TestCustomMessage {})
 		}

lightning/src/ln/channelmanager.rs

@@ -9274,7 +9274,7 @@ where
 	R::Target: Router,
 	L::Target: Logger,
 {
-	fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, message: &ReceivedOnionMessage<F, OffersMessage>) {
+	fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, message: ReceivedOnionMessage<F, OffersMessage>) {
 		let secp_ctx = &self.secp_ctx;
 		let expanded_key = &self.inbound_payment_key;
 

lightning/src/ln/msgs.rs

@@ -1592,8 +1592,6 @@ pub trait OnionMessageHandler: EventsProvider {
 	/// Handle an incoming `onion_message` message from the given peer.
 	fn handle_onion_message(&self, peer_node_id: &PublicKey, msg: &OnionMessage);
 
-	fn handle_onion_message_response<T: OnionMessageContents>(&self, response: T, reply_path: BlindedPath, log_suffix: fmt::Arguments);
-
 	/// Returns the next pending onion message for the peer with the given node id.
 	fn next_onion_message_for_peer(&self, peer_node_id: PublicKey) -> Option<OnionMessage>;
 

lightning/src/ln/peer_handler.rs

@@ -124,7 +124,6 @@ impl RoutingMessageHandler for IgnoringMessageHandler {
 
 impl OnionMessageHandler for IgnoringMessageHandler {
 	fn handle_onion_message(&self, _their_node_id: &PublicKey, _msg: &msgs::OnionMessage) {}
-	fn handle_onion_message_response<T: OnionMessageContents>(&self, _response: T, _reply_path: BlindedPath, _log_suffix: fmt::Arguments) {}
 	fn next_onion_message_for_peer(&self, _peer_node_id: PublicKey) -> Option<msgs::OnionMessage> { None }
 	fn peer_connected(&self, _their_node_id: &PublicKey, _init: &msgs::Init, _inbound: bool) -> Result<(), ()> { Ok(()) }
 	fn peer_disconnected(&self, _their_node_id: &PublicKey) {}
@@ -136,11 +135,11 @@ impl OnionMessageHandler for IgnoringMessageHandler {
 }
 
 impl OffersMessageHandler for IgnoringMessageHandler {
-	fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, _message: &ReceivedOnionMessage<F, OffersMessage>) {}
+	fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, _message: ReceivedOnionMessage<F, OffersMessage>) {}
 }
 impl CustomOnionMessageHandler for IgnoringMessageHandler {
 	type CustomMessage = Infallible;
-	fn handle_custom_message<F: Fn(Self::CustomMessage, BlindedPath)>(&self, _message: &ReceivedOnionMessage<F, Self::CustomMessage>) {
+	fn handle_custom_message<F: Fn(Self::CustomMessage, BlindedPath)>(&self, _message: ReceivedOnionMessage<F, Self::CustomMessage>) {
 		// Since we always return `None` in the read the handle method should never be called.
 		unreachable!();
 	}

lightning/src/onion_message/functional_tests.rs

@@ -70,7 +70,7 @@ impl MessageRouter for TestMessageRouter {
 struct TestOffersMessageHandler {}
 
 impl OffersMessageHandler for TestOffersMessageHandler {
-	fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, _message: &ReceivedOnionMessage<F, OffersMessage>) {}
+	fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, _message: ReceivedOnionMessage<F, OffersMessage>) {}
 }
 
 #[derive(Clone, Debug, PartialEq)]
@@ -132,10 +132,10 @@ impl Drop for TestCustomMessageHandler {
 
 impl CustomOnionMessageHandler for TestCustomMessageHandler {
 	type CustomMessage = TestCustomMessage;
-	fn handle_custom_message<F: Fn(Self::CustomMessage, BlindedPath)>(&self, message: &ReceivedOnionMessage<F, Self::CustomMessage>) {
+	fn handle_custom_message<F: Fn(Self::CustomMessage, BlindedPath)>(&self, message: ReceivedOnionMessage<F, Self::CustomMessage>) {
 		if let ReceivedOnionMessage::WithReplyPath{responder, message, path_id: _} = message {
 			match self.expected_messages.lock().unwrap().pop_front() {
-				Some(expected_msg) => assert_eq!(expected_msg, *message),
+				Some(expected_msg) => assert_eq!(expected_msg, message),
 				None => panic!("Unexpected message: {:?}", message),
 			}
 

lightning/src/onion_message/messenger.rs

@@ -244,89 +244,86 @@ impl OnionMessageRecipient {
 	}
 }
 
+/// A enum to handle received [`OnionMessage`]
+pub enum ReceivedOnionMessage<R, T>
+where
+	R: RespondFunction<T>,
+	T: OnionMessageContents
+{
+	WithReplyPath {
+		message: T,
+		path_id: Option<[u8; 32]>,
+		responder: Responder<R, T>,
+	},
+	WithoutReplyPath {
+		message: T,
+		path_id: Option<[u8; 32]>,
+	},
+}
+
+impl<R, T> ReceivedOnionMessage<R, T>
+where
+	R: RespondFunction<T>,
+	T: OnionMessageContents
+{
+	fn new(messenger_function: R, message: T, reply_path_opt: Option<BlindedPath>, path_id: Option<[u8; 32]> ) -> Self {
+		if let Some(reply_path) = reply_path_opt {
+			let responder = Responder {
+				messenger_function,
+				reply_path,
+				_phantom: PhantomData
+			};
+			ReceivedOnionMessage::WithReplyPath {
+				message,
+				path_id,
+				responder
+			}
+		} else {
+			ReceivedOnionMessage::WithoutReplyPath {
+				message,
+				path_id
+			}
+		}
+    }
+}
+
 pub trait RespondFunction<T: OnionMessageContents>{
-	fn respond(&self, response: T, reply_path: BlindedPath);
+	fn respond(self, response: T, reply_path: BlindedPath);
 }
 
 impl<F, T> RespondFunction<T> for F
 where
-T: OnionMessageContents,
-F: Fn(T, BlindedPath)
+	F: Fn(T, BlindedPath),
+	T: OnionMessageContents
 {
-	fn respond(&self, response: T, reply_path: BlindedPath) {
+	fn respond(self, response: T, reply_path: BlindedPath) {
 		self(response, reply_path)
 	}
 }
 
 /// A struct handling response to an [`OnionMessage`]
 pub struct Responder<R, T>
 where
-R: RespondFunction<T>,
-T: OnionMessageContents
+	R: RespondFunction<T>,
+	T: OnionMessageContents
 {
 	messenger_function: R,
-	pub reply_path: BlindedPath,
+	reply_path: BlindedPath,
 	// This phantom Data is used to ensure that we use T in the struct definition
 	// This allow us to ensure at compile time that the received message type and response type will be same
 	_phantom: PhantomData<T>
 }
 
 impl<R, T> Responder<R, T>
 where
-R: RespondFunction<T>,
-T: OnionMessageContents
+	R: RespondFunction<T>,
+	T: OnionMessageContents
 {
-    pub fn respond(&self, response: T) {
-		// Utilising the fact that we ensure at compile time that
-		// received message type, and response type will be same
-		self.messenger_function.respond(response, self.reply_path.clone())
+    pub fn respond(self, response: T) {
+		self.messenger_function.respond(response, self.reply_path)
 	}
 }
 
-/// A enum to handle received [`OnionMessage`]
-pub enum ReceivedOnionMessage<R, T>
-where
-R: RespondFunction<T>,
-T: OnionMessageContents
-{
-	WithReplyPath {
-		responder: Responder<R, T>,
-		message: T,
-		path_id: Option<[u8; 32]>
-	},
-	WithoutReplyPath {
-		message: T,
-		path_id: Option<[u8; 32]>,
-	},
-}
-
-impl<R, T> ReceivedOnionMessage<R, T>
-where
-R: RespondFunction<T>,
-T: OnionMessageContents
-{
-	fn new(messenger_function: R, message: T, reply_path_option: Option<BlindedPath>, path_id: Option<[u8; 32]> ) -> Self {
-		match reply_path_option {
-			Some(reply_path) => {
-				let responder = Responder {
-					messenger_function,
-					reply_path,
-					_phantom: PhantomData
-				};
-				ReceivedOnionMessage::WithReplyPath {
-					responder,
-					message,
-					path_id
-				}
-			}
-			None => ReceivedOnionMessage::WithoutReplyPath {
-				message,
-				path_id
-			}
-		}
-    }
-}
-
 /// An [`OnionMessage`] for [`OnionMessenger`] to send.
 ///
 /// These are obtained when released from [`OnionMessenger`]'s handlers after which they are
@@ -584,7 +581,7 @@ pub trait CustomOnionMessageHandler {
 	/// Called with the custom message that was received, returning a response to send, if any.
 	///
 	/// The returned [`Self::CustomMessage`], if any, is enqueued to be sent by [`OnionMessenger`].
-	fn handle_custom_message<F: Fn(Self::CustomMessage, BlindedPath)>(&self, message: &ReceivedOnionMessage<F, Self::CustomMessage>);
+	fn handle_custom_message<F: Fn(Self::CustomMessage, BlindedPath)>(&self, message: ReceivedOnionMessage<F, Self::CustomMessage>);
 
 	/// Read a custom message of type `message_type` from `buffer`, returning `Ok(None)` if the
 	/// message type is unknown.
@@ -912,6 +909,14 @@ where
 		)
 	}
 
+	fn handle_onion_message_response<T: OnionMessageContents>(
+		&self, response: T, reply_path: BlindedPath, log_suffix: fmt::Arguments
+	) {
+		let _ = self.find_path_and_enqueue_onion_message(
+			response, Destination::BlindedPath(reply_path), None, log_suffix
+		);
+	}
+
 	#[cfg(test)]
 	pub(super) fn release_pending_msgs(&self) -> HashMap<PublicKey, VecDeque<OnionMessage>> {
 		let mut message_recipients = self.message_recipients.lock().unwrap();
@@ -987,11 +992,9 @@ where
 				   "Received an onion message with path_id {:02x?} and {} reply_path: {:?}",
 					path_id, if reply_path.is_some() { "a" } else { "no" }, message);
 
+				let message_type = message.msg_type();
 				match message {
 					ParsedOnionMessageContents::Offers(msg) => {
-						// message_type is defined outside of closure to avoid burrowing
-						// issues when using msg for creating ReceivedOnionMessage::New()
-						let message_type = msg.msg_type();
 						let closure = |response, reply_path: BlindedPath| {
 							self.handle_onion_message_response(
 								response, reply_path, format_args!(
@@ -1002,12 +1005,9 @@ where
 							);
 						};
 						let message = ReceivedOnionMessage::new(closure, msg, reply_path, path_id);
-						self.offers_handler.handle_message(&message);
+						self.offers_handler.handle_message(message);
 					},
 					ParsedOnionMessageContents::Custom(msg) => {
-						// message_type is defined outside of closure to avoid burrowing
-						// issues when using msg for creating ReceivedOnionMessage::New()
-						let message_type = msg.msg_type();
 						let closure = |response, reply_path: BlindedPath| {
 							self.handle_onion_message_response(
 								response, reply_path, format_args!(
@@ -1018,7 +1018,7 @@ where
 							);
 						};
 						let message = ReceivedOnionMessage::new(closure, msg, reply_path, path_id);
-						self.custom_handler.handle_custom_message(&message);
+						self.custom_handler.handle_custom_message(message);
 					},
 				}
 			},
@@ -1053,14 +1053,6 @@ where
 		}
 	}
 
-	fn handle_onion_message_response<T: OnionMessageContents>(
-		&self, response: T, reply_path: BlindedPath, log_suffix: fmt::Arguments
-	) {
-		let _ = self.find_path_and_enqueue_onion_message(
-			response, Destination::BlindedPath(reply_path), None, log_suffix
-		);
-	}
-
 	fn peer_connected(&self, their_node_id: &PublicKey, init: &msgs::Init, _inbound: bool) -> Result<(), ()> {
 		if init.features.supports_onion_messages() {
 			self.message_recipients.lock().unwrap()

lightning/src/onion_message/offers.rs

@@ -43,7 +43,7 @@ pub trait OffersMessageHandler {
 	/// The returned [`OffersMessage`], if any, is enqueued to be sent by [`OnionMessenger`].
 	///
 	/// [`OnionMessenger`]: crate::onion_message::messenger::OnionMessenger
-	fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, message: &ReceivedOnionMessage<F, OffersMessage>);
+	fn handle_message<F: Fn(OffersMessage, BlindedPath)>(&self, message: ReceivedOnionMessage<F, OffersMessage>);
 
 	/// Releases any [`OffersMessage`]s that need to be sent.
 	///