Use PeerState::{register,remove}_request instead of map access

We introduce two new methods on `PeerState` to avoid direct access to
the internal `pending_requests` map.

Author: tnull

lightning-liquidity/src/lsps1/peer_state.rs

@@ -14,10 +14,12 @@ use super::msgs::{LSPS1OrderId, LSPS1OrderParams, LSPS1PaymentInfo, LSPS1Request
 use crate::lsps0::ser::{LSPSDateTime, LSPSRequestId};
 use crate::prelude::HashMap;
 
+use core::fmt;
+
 #[derive(Default)]
 pub(super) struct PeerState {
 	outbound_channels_by_order_id: HashMap<LSPS1OrderId, OutboundCRChannel>,
-	pub(super) pending_requests: HashMap<LSPSRequestId, LSPS1Request>,
+	pending_requests: HashMap<LSPSRequestId, LSPS1Request>,
 }
 
 impl PeerState {
@@ -33,11 +35,42 @@ impl PeerState {
 		self.outbound_channels_by_order_id.get(order_id).map(|channel| &channel.order)
 	}
 
+	pub(super) fn register_request(
+		&mut self, request_id: LSPSRequestId, request: LSPS1Request,
+	) -> Result<(), PeerStateError> {
+		if self.pending_requests.contains_key(&request_id) {
+			return Err(PeerStateError::DuplicateRequestId);
+		}
+		self.pending_requests.insert(request_id, request);
+		Ok(())
+	}
+
+	pub(super) fn remove_request(
+		&mut self, request_id: &LSPSRequestId,
+	) -> Result<LSPS1Request, PeerStateError> {
+		self.pending_requests.remove(request_id).ok_or(PeerStateError::UnknownRequestId)
+	}
+
 	pub(super) fn has_active_requests(&self) -> bool {
 		!self.outbound_channels_by_order_id.is_empty()
 	}
 }
 
+#[derive(Debug, Copy, Clone)]
+pub(super) enum PeerStateError {
+	UnknownRequestId,
+	DuplicateRequestId,
+}
+
+impl fmt::Display for PeerStateError {
+	fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+		match self {
+			Self::UnknownRequestId => write!(f, "unknown request id"),
+			Self::DuplicateRequestId => write!(f, "duplicate request id"),
+		}
+	}
+}
+
 pub(super) struct ChannelOrder {
 	pub(super) order_params: LSPS1OrderParams,
 	pub(super) created_at: LSPSDateTime,

lightning-liquidity/src/lsps1/service.rs

@@ -162,9 +162,12 @@ where
 				.or_insert(Mutex::new(PeerState::default()));
 			let mut peer_state_lock = inner_state_lock.lock().unwrap();
 
-			peer_state_lock
-				.pending_requests
-				.insert(request_id.clone(), LSPS1Request::CreateOrder(params.clone()));
+			let request = LSPS1Request::CreateOrder(params.clone());
+			peer_state_lock.register_request(request_id.clone(), request).map_err(|e| {
+				let err = format!("Failed to handle request due to: {}", e);
+				let action = ErrorAction::IgnoreAndLog(Level::Error);
+				LightningError { err, action }
+			})?;
 		}
 
 		event_queue_notifier.enqueue(LSPS1ServiceEvent::RequestForPaymentDetails {
@@ -191,11 +194,15 @@ where
 		match outer_state_lock.get(counterparty_node_id) {
 			Some(inner_state_lock) => {
 				let mut peer_state_lock = inner_state_lock.lock().unwrap();
-
-				match peer_state_lock.pending_requests.remove(&request_id) {
-					Some(LSPS1Request::CreateOrder(params)) => {
+				let request = peer_state_lock.remove_request(&request_id).map_err(|e| {
+					debug_assert!(false, "Failed to send response due to: {}", e);
+					let err = format!("Failed to send response due to: {}", e);
+					APIError::APIMisuseError { err }
+				})?;
+
+				match request {
+					LSPS1Request::CreateOrder(params) => {
 						let order_id = self.generate_order_id();
-
 						peer_state_lock.new_order(
 							order_id.clone(),
 							params.order.clone(),
@@ -206,6 +213,9 @@ where
 						let response = LSPS1Response::CreateOrder(LSPS1CreateOrderResponse {
 							order: params.order,
 							order_id,
+
+							// TODO, we need to set this in the peer/channel state, and send the
+							// set value here:
 							order_state: LSPS1OrderState::Created,
 							created_at,
 							payment,
@@ -215,14 +225,22 @@ where
 						message_queue_notifier.enqueue(counterparty_node_id, msg);
 						Ok(())
 					},
-
-					_ => Err(APIError::APIMisuseError {
-						err: format!("No pending buy request for request_id: {:?}", request_id),
-					}),
+					t => {
+						debug_assert!(
+							false,
+							"Failed to send response due to unexpected request type: {:?}",
+							t
+						);
+						let err = format!(
+							"Failed to send response due to unexpected request type: {:?}",
+							t
+						);
+						return Err(APIError::APIMisuseError { err });
+					},
 				}
 			},
 			None => Err(APIError::APIMisuseError {
-				err: format!("No state for the counterparty exists: {:?}", counterparty_node_id),
+				err: format!("No state for the counterparty exists: {}", counterparty_node_id),
 			}),
 		}
 	}
@@ -236,9 +254,13 @@ where
 		match outer_state_lock.get(counterparty_node_id) {
 			Some(inner_state_lock) => {
 				let mut peer_state_lock = inner_state_lock.lock().unwrap();
-				peer_state_lock
-					.pending_requests
-					.insert(request_id.clone(), LSPS1Request::GetOrder(params.clone()));
+
+				let request = LSPS1Request::GetOrder(params.clone());
+				peer_state_lock.register_request(request_id.clone(), request).map_err(|e| {
+					let err = format!("Failed to handle request due to: {}", e);
+					let action = ErrorAction::IgnoreAndLog(Level::Error);
+					LightningError { err, action }
+				})?;
 
 				event_queue_notifier.enqueue(LSPS1ServiceEvent::CheckPaymentConfirmation {
 					request_id,