Remove extract_err_req and process_err_res

Previously extract_err_req was exposed as a public method on
SessionHistory, and process_err_res as a pure function, for implementers
to manually process error responses.

Now this can and should be done via the HasReplyableError typestate, so
these functions are no longer needed.

Author: spacebear21

payjoin-ffi/src/receive/mod.rs

@@ -159,22 +159,6 @@ impl SessionHistory {
     pub fn fallback_tx(&self) -> Option<Arc<crate::Transaction>> {
         self.0.fallback_tx().map(|tx| Arc::new(tx.into()))
     }
-
-    /// Construct the error request to be posted on the directory if an error occurred.
-    /// To process the response, use [process_err_res]
-    pub fn extract_err_req(
-        &self,
-        ohttp_relay: String,
-    ) -> Result<Option<RequestResponse>, SessionError> {
-        match self.0.extract_err_req(ohttp_relay) {
-            Ok(Some((request, ctx))) => Ok(Some(RequestResponse {
-                request: request.into(),
-                client_response: Arc::new(ctx.into()),
-            })),
-            Ok(None) => Ok(None),
-            Err(e) => Err(SessionError::from(e)),
-        }
-    }
 }
 
 #[derive(uniffi::Object)]
@@ -475,12 +459,6 @@ impl UncheckedOriginalPayload {
     }
 }
 
-/// Process an OHTTP Encapsulated HTTP POST Error response
-/// to ensure it has been posted properly
-#[uniffi::export]
-pub fn process_err_res(body: &[u8], context: &ClientResponse) -> Result<(), SessionError> {
-    payjoin::receive::v2::process_err_res(body, context.into()).map_err(Into::into)
-}
 #[derive(Clone, uniffi::Object)]
 pub struct MaybeInputsOwned(payjoin::receive::v2::Receiver<payjoin::receive::v2::MaybeInputsOwned>);
 

payjoin/src/core/receive/v2/mod.rs

@@ -34,7 +34,7 @@ pub(crate) use error::InternalSessionError;
 pub use error::SessionError;
 use serde::de::Deserializer;
 use serde::{Deserialize, Serialize};
-pub use session::{replay_event_log, SessionEvent, SessionHistory};
+pub use session::{replay_event_log, SessionEvent, SessionHistory, SessionStatus};
 use url::Url;
 
 use super::error::{Error, InputContributionError};
@@ -270,34 +270,6 @@ impl<State> core::ops::DerefMut for Receiver<State> {
     fn deref_mut(&mut self) -> &mut Self::Target { &mut self.state }
 }
 
-/// Construct an OHTTP Encapsulated HTTP POST request to return
-/// a Receiver Error Response
-fn extract_err_req(
-    err: &JsonReply,
-    ohttp_relay: impl IntoUrl,
-    session_context: &SessionContext,
-) -> Result<(Request, ohttp::ClientResponse), SessionError> {
-    if session_context.expiration.elapsed() {
-        return Err(InternalSessionError::Expired(session_context.expiration).into());
-    }
-    let mailbox = mailbox_endpoint(&session_context.directory, &session_context.reply_mailbox_id());
-    let (body, ohttp_ctx) = ohttp_encapsulate(
-        &session_context.ohttp_keys.0,
-        "POST",
-        mailbox.as_str(),
-        Some(err.to_json().to_string().as_bytes()),
-    )
-    .map_err(InternalSessionError::OhttpEncapsulation)?;
-    let req = Request::new_v2(&session_context.full_relay_url(ohttp_relay)?, &body);
-    Ok((req, ohttp_ctx))
-}
-
-/// Process an OHTTP Encapsulated HTTP POST Error response
-/// to ensure it has been posted properly
-pub fn process_err_res(body: &[u8], context: ohttp::ClientResponse) -> Result<(), SessionError> {
-    process_post_res(body, context).map_err(|e| InternalSessionError::DirectoryResponse(e).into())
-}
-
 #[derive(Debug, Clone)]
 pub struct ReceiverBuilder(SessionContext);
 
@@ -1106,13 +1078,31 @@ pub struct HasReplyableError {
 }
 
 impl Receiver<HasReplyableError> {
+    /// Construct an OHTTP Encapsulated HTTP POST request to return
+    /// a Receiver Error Response
     pub fn create_error_request(
         &self,
         ohttp_relay: impl IntoUrl,
     ) -> Result<(Request, ohttp::ClientResponse), SessionError> {
-        extract_err_req(&self.error_reply, ohttp_relay, &self.session_context)
+        let session_context = &self.session_context;
+        if session_context.expiration.elapsed() {
+            return Err(InternalSessionError::Expired(session_context.expiration).into());
+        }
+        let mailbox =
+            mailbox_endpoint(&session_context.directory, &session_context.reply_mailbox_id());
+        let (body, ohttp_ctx) = ohttp_encapsulate(
+            &session_context.ohttp_keys.0,
+            "POST",
+            mailbox.as_str(),
+            Some(self.error_reply.to_json().to_string().as_bytes()),
+        )
+        .map_err(InternalSessionError::OhttpEncapsulation)?;
+        let req = Request::new_v2(&session_context.full_relay_url(ohttp_relay)?, &body);
+        Ok((req, ohttp_ctx))
     }
 
+    /// Process an OHTTP Encapsulated HTTP POST Error response
+    /// to ensure it has been posted properly
     pub fn process_error_response(
         &self,
         res: &[u8],
@@ -1422,11 +1412,7 @@ pub mod test {
     }
 
     #[test]
-    fn test_extract_err_req() -> Result<(), BoxError> {
-        let receiver = Receiver {
-            state: unchecked_proposal_v2_from_test_vector(),
-            session_context: SHARED_CONTEXT.clone(),
-        };
+    fn test_create_error_request() -> Result<(), BoxError> {
         let mock_err = mock_err();
         let expected_json = serde_json::json!({
             "errorCode": "unavailable",
@@ -1435,38 +1421,26 @@ pub mod test {
 
         assert_eq!(mock_err.to_json(), expected_json);
 
-        let (_req, _ctx) = extract_err_req(&mock_err, EXAMPLE_URL, &receiver.session_context)?;
+        let receiver = Receiver {
+            state: HasReplyableError { error_reply: mock_err.clone() },
+            session_context: SHARED_CONTEXT.clone(),
+        };
+
+        let (_req, _ctx) = receiver.create_error_request(EXAMPLE_URL)?;
 
-        let internal_error: Error = InternalPayloadError::MissingPayment.into();
-        let (_req, _ctx) =
-            extract_err_req(&(&internal_error).into(), EXAMPLE_URL, &receiver.session_context)?;
         Ok(())
     }
 
     #[test]
-    fn test_extract_err_req_expiration() -> Result<(), BoxError> {
+    fn test_create_error_request_expiration() -> Result<(), BoxError> {
         let now = crate::time::Time::now();
-        let noop_persister = NoopSessionPersister::default();
         let context = SessionContext { expiration: now, ..SHARED_CONTEXT.clone() };
         let receiver = Receiver {
-            state: UncheckedOriginalPayload {
-                original: crate::receive::tests::original_from_test_vector(),
-            },
+            state: HasReplyableError { error_reply: mock_err() },
             session_context: context.clone(),
         };
 
-        let server_error = || {
-            receiver
-                .clone()
-                .check_broadcast_suitability(None, |_| Err("mock error".into()))
-                .save(&noop_persister)
-        };
-
-        let error = server_error().expect_err("Server error should be populated with mock error");
-        let res = error.api_error().expect("check_broadcast error should propagate to api error");
-        let actual_json = JsonReply::from(&res);
-
-        let expiration = extract_err_req(&actual_json, EXAMPLE_URL, &context);
+        let expiration = receiver.create_error_request(EXAMPLE_URL);
 
         match expiration {
             Err(error) => assert_eq!(

payjoin/src/core/receive/v2/session.rs

@@ -4,9 +4,8 @@ use super::{ReceiveSession, SessionContext};
 use crate::error::{InternalReplayError, ReplayError};
 use crate::output_substitution::OutputSubstitution;
 use crate::persist::SessionPersister;
-use crate::receive::v2::{extract_err_req, SessionError};
 use crate::receive::{InputPair, JsonReply, OriginalPayload, PsbtContext};
-use crate::{ImplementationError, IntoUrl, PjUri, Request};
+use crate::{ImplementationError, PjUri};
 
 /// Replay a receiver event log to get the receiver in its current state [ReceiveSession]
 /// and a session history [SessionHistory]
@@ -112,35 +111,6 @@ impl SessionHistory {
         })
     }
 
-    /// Construct the error request to be posted on the directory if an error occurred.
-    /// To process the response, use [crate::receive::v2::process_err_res]
-    pub fn extract_err_req(
-        &self,
-        ohttp_relay: impl IntoUrl,
-    ) -> Result<Option<(Request, ohttp::ClientResponse)>, SessionError> {
-        // FIXME ideally this should be more like a method of
-        // Receiver<UncheckedOriginalPayload> and subsequent states instead of the
-        // history as a whole since it doesn't make sense to call it before,
-        // reaching that state.
-        if !self.received_sender_proposal() {
-            return Ok(None);
-        }
-
-        let session_context = self.session_context();
-        let json_reply = match self.terminal_error() {
-            Some(json_reply) => json_reply,
-            _ => return Ok(None),
-        };
-        let (req, ctx) = extract_err_req(&json_reply, ohttp_relay, &session_context)?;
-        Ok(Some((req, ctx)))
-    }
-
-    fn received_sender_proposal(&self) -> bool {
-        self.events
-            .iter()
-            .any(|event| matches!(event, SessionEvent::RetrievedOriginalPayload { .. }))
-    }
-
     fn session_context(&self) -> SessionContext {
         let mut initial_session_context = self
             .events
@@ -703,78 +673,4 @@ mod tests {
 
         Ok(())
     }
-
-    #[test]
-    fn test_skipped_session_extract_err_request() -> Result<(), BoxError> {
-        let ohttp_relay = EXAMPLE_URL;
-        let mock_err = mock_err();
-
-        let session_history =
-            SessionHistory { events: vec![SessionEvent::CheckedBroadcastSuitability()] };
-        let err_req = session_history.extract_err_req(ohttp_relay)?;
-        assert!(err_req.is_none());
-
-        let session_history = SessionHistory {
-            events: vec![
-                SessionEvent::CheckedBroadcastSuitability(),
-                SessionEvent::GotReplyableError(mock_err.clone()),
-            ],
-        };
-
-        let err_req = session_history.extract_err_req(ohttp_relay)?;
-        assert!(err_req.is_none());
-
-        let session_history = SessionHistory {
-            events: vec![
-                SessionEvent::Created(SHARED_CONTEXT.clone()),
-                SessionEvent::CheckedBroadcastSuitability(),
-                SessionEvent::GotReplyableError(mock_err.clone()),
-            ],
-        };
-
-        let err_req = session_history.extract_err_req(ohttp_relay)?;
-        assert!(err_req.is_none());
-        Ok(())
-    }
-
-    #[test]
-    fn test_session_extract_err_req_reply_key() -> Result<(), BoxError> {
-        let proposal = original_from_test_vector();
-        let ohttp_relay = EXAMPLE_URL;
-        let mock_err = mock_err();
-
-        let session_history_one = SessionHistory {
-            events: vec![
-                SessionEvent::Created(SHARED_CONTEXT.clone()),
-                SessionEvent::RetrievedOriginalPayload {
-                    original: proposal.clone(),
-                    reply_key: Some(crate::HpkeKeyPair::gen_keypair().1),
-                },
-                SessionEvent::GotReplyableError(mock_err.clone()),
-            ],
-        };
-
-        let err_req_one = session_history_one.extract_err_req(ohttp_relay)?;
-        assert!(err_req_one.is_some());
-
-        let session_history_two = SessionHistory {
-            events: vec![
-                SessionEvent::Created(SHARED_CONTEXT.clone()),
-                SessionEvent::RetrievedOriginalPayload {
-                    original: proposal.clone(),
-                    reply_key: Some(crate::HpkeKeyPair::gen_keypair().1),
-                },
-                SessionEvent::GotReplyableError(mock_err.clone()),
-            ],
-        };
-
-        let err_req_two = session_history_two.extract_err_req(ohttp_relay)?;
-        assert!(err_req_two.is_some());
-        assert_ne!(
-            session_history_one.session_context().reply_key,
-            session_history_two.session_context().reply_key
-        );
-
-        Ok(())
-    }
 }

payjoin/tests/integration.rs

@@ -198,8 +198,8 @@ mod integration {
         use http::StatusCode;
         use payjoin::persist::{NoopSessionPersister, OptionalTransitionOutcome};
         use payjoin::receive::v2::{
-            replay_event_log as replay_receiver_event_log, PayjoinProposal, Receiver,
-            ReceiverBuilder, UncheckedOriginalPayload,
+            replay_event_log as replay_receiver_event_log, PayjoinProposal, ReceiveSession,
+            Receiver, ReceiverBuilder, SessionStatus, UncheckedOriginalPayload,
         };
         use payjoin::send::v2::SenderBuilder;
         use payjoin::{OhttpKeys, PjUri, UriExt};
@@ -317,12 +317,12 @@ mod integration {
             let result = tokio::select!(
             err = services.take_ohttp_relay_handle() => panic!("Ohttp relay exited early: {:?}", err),
             err = services.take_directory_handle() => panic!("Directory server exited early: {:?}", err),
-            res = process_err_res(&services) => res
+            res = do_err_test(&services) => res
             );
 
             assert!(result.is_ok(), "v2 send receive failed: {:#?}", result.unwrap_err());
 
-            async fn process_err_res(services: &TestServices) -> Result<(), BoxError> {
+            async fn do_err_test(services: &TestServices) -> Result<(), BoxError> {
                 let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver(None, None)?;
                 let agent = services.http_agent();
                 services.wait_for_services_ready().await?;
@@ -419,10 +419,14 @@ mod integration {
                     "Protocol error: Can't broadcast. PSBT rejected by mempool."
                 );
 
-                let (_, session_history) = replay_receiver_event_log(&persister)?;
-                let (err_req, err_ctx) = session_history
-                    .extract_err_req(services.ohttp_relay_url().as_str())?
-                    .expect("error request should exist");
+                let (session, session_history) = replay_receiver_event_log(&persister)?;
+                assert_eq!(session_history.status(), SessionStatus::Active);
+                let has_error = match session {
+                    ReceiveSession::HasReplyableError(r) => r,
+                    _ => panic!("Expected HasError"),
+                };
+                let (err_req, err_ctx) =
+                    has_error.create_error_request(services.ohttp_relay_url().as_str())?;
                 let err_response = agent
                     .post(err_req.url)
                     .header("Content-Type", err_req.content_type)
@@ -431,8 +435,17 @@ mod integration {
                     .await?;
 
                 let err_bytes = err_response.bytes().await?;
-                // Ensure that the error was handled properly
-                assert!(payjoin::receive::v2::process_err_res(&err_bytes, err_ctx).is_ok());
+                has_error.process_error_response(&err_bytes, err_ctx).save(&persister)?;
+
+                // Ensure the session is closed properly
+                let (_, session_history) = replay_receiver_event_log(&persister)?;
+                assert_eq!(session_history.status(), SessionStatus::Failed);
+                assert_eq!(
+                    session_history.terminal_error().expect("should have error"),
+                    (&server_error).into()
+                );
+
+                // TODO: Sender should retrieve the error response to complete the error flow
 
                 Ok(())
             }