Move shared response processing code to ohttp.rs by spacebear21 · Pull Request #660 · payjoin/rust-payjoin

spacebear21 · 2025-04-18T01:33:50Z

The process_res methods across send and receive have a lot of shared code that can be extracted into reusable functions. process_get_res and process_post_res essentially do the same thing, and could conceivably be combined into one function. They were kept separate because "202 Accepted" is not a valid response for POST requests.

spacebear21 · 2025-04-18T01:35:53Z

Leaving in draft for now because some follow-up items need to be addressed:

process_response methods return inconsistent error types when POSTing vs. GETting, specifically: EncapsulationError vs. ResponseError for send and SessionError vs. Error for receive.
There are four duplicated variants between InternalEncapsulationError and InternalSessionError.
- Also, EncapsulationError is a misnomer since it afaict it's exclusively raised when decapsulating responses.
- The Hpke variant has nothing to do with en/decapsulation.

coveralls · 2025-04-18T01:38:59Z

Pull Request Test Coverage Report for Build 15500115610

Details

51 of 75 (68.0%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.01%) to 85.043%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/receive/v2/error.rs	0	2	0.0%
payjoin/src/send/v2/error.rs	0	3	0.0%
payjoin/src/ohttp.rs	28	47	59.57%

Totals
Change from base Build 15449709719:	-0.01%
Covered Lines:	6772
Relevant Lines:	7963

💛 - Coveralls

DanGould · 2025-06-06T17:30:47Z

are there other from conversions that can be deleted?

spacebear21 · 2025-06-06T18:23:40Z

The error audit I mentioned in my other comment can be addressed later in a follow-up, I'll make a new issue.

are there other from conversions that can be deleted?

I had a second commit locally that I hadn't pushed which simplifies the From mappings. The PR is still a net addition but I think the simplification is significant.

DanGould

1 request and 1 concern

I think there's a more concise and semantically correct way to write process_get_res which might solve a mutant. See below.
I think some of the From impls for DirectoryResponseError may be even more verbose than changes to make map_err work while expanding our public API and should be reconsidered

Fighting a head cold so warning these comments might be less sane than usual.

DanGould · 2025-06-06T18:50:54Z

payjoin/src/ohttp.rs

+    let body = match response.status() {
+        http::StatusCode::OK => response.body().to_vec(),
+        http::StatusCode::ACCEPTED => return Ok(None),
+        _ => return Err(DirectoryResponseError::UnexpectedStatusCode(response.status())),
+    };
+    Ok(Some(body))


I know this was carried over, but I think it's more appropriately written as the following, which might address @benalleng's mutant concern as well

Suggested change

let body = match response.status() {

http::StatusCode::OK => response.body().to_vec(),

http::StatusCode::ACCEPTED => return Ok(None),

_ => return Err(DirectoryResponseError::UnexpectedStatusCode(response.status())),

};

Ok(Some(body))

match response.status() {

http::StatusCode::OK => Ok(Some(response.body().to_vec())),

http::StatusCode::ACCEPTED => Ok(None),

code => Err(DirectoryResponseError::UnexpectedStatusCode(code)),

}

this addresses 2 separate issues: 1. replying with Some(body) outside of the match and using the single response.status() call as the context for the UnexpectedStatusCode error.

DanGould · 2025-06-06T18:54:08Z

payjoin/src/receive/v2/error.rs

+            DirectoryResponseError::InvalidSize(e) =>
+                InternalSessionError::UnexpectedResponseSize(e),
+            DirectoryResponseError::OhttpDecapsulation(e) =>
+                InternalSessionError::OhttpEncapsulation(e),
+            DirectoryResponseError::UnexpectedStatusCode(e) =>
+                InternalSessionError::UnexpectedStatusCode(e),


Why should InternalSessionError not have a BadResponse variant that encapsulates DirectoryResponseError?

I believe this is addressed in the second commit.

DanGould · 2025-06-06T18:56:00Z

payjoin/src/receive/v2/error.rs

+impl From<DirectoryResponseError> for Error {
+    fn from(value: DirectoryResponseError) -> Self { V2(value.into()) }
+}


It doesn't seem to me like this should necessarily be coerced into a V2 error. It could be a multiparty or later error, and so should be explicitly mapped at the call site.

DanGould · 2025-06-06T19:01:20Z

payjoin/src/send/v2/error.rs

+
+impl From<DirectoryResponseError> for EncapsulationError {
+    fn from(value: DirectoryResponseError) -> Self {
+        match value {
+            DirectoryResponseError::InvalidSize(e) => InternalEncapsulationError::InvalidSize(e),
+            DirectoryResponseError::OhttpDecapsulation(e) => InternalEncapsulationError::Ohttp(e),
+            DirectoryResponseError::UnexpectedStatusCode(e) =>
+                InternalEncapsulationError::UnexpectedStatusCode(e),
+        }
+        .into()
+    }
+}
+
+impl From<DirectoryResponseError> for ResponseError {
+    fn from(value: DirectoryResponseError) -> Self {
+        ResponseError::Validation(
+            super::InternalValidationError::V2Encapsulation(value.into()).into(),
+        )
+    }
+}


Same as above, it seems like this conversion is only done once (which could be done with map_err or an Err(e) match arm) making the From unnecessarily verbose, and the EncapsulationError seems like it could just have a DirectoryResponseError unless these InvalidSize, OhttpDecapsulation, UnexpectedStatusCode variants crop up outside of the context of a directory response.

DanGould · 2025-06-06T19:13:05Z

payjoin/src/receive/v2/error.rs

+impl From<DirectoryResponseError> for Error {
+    fn from(e: DirectoryResponseError) -> Self { InternalSessionError::DirectoryResponse(e).into() }
+}
+
+impl From<DirectoryResponseError> for SessionError {
+    fn from(e: DirectoryResponseError) -> Self { InternalSessionError::DirectoryResponse(e).into() }
+}
+


Ah, I see this commit obviates my earlier comments. However, it still seems like it depends on context which one of these it gets converted to, which suggests to me you still want a map instead of the conversion (which happens on only few occasions)

spacebear21 · 2025-06-06T19:59:08Z

The latest push fixes process_get_res and replaces the From implementations with map_err.

payjoin/src/receive/v2/mod.rs

DanGould

Assuming checking if the get body is empty was a bug ACK 3b2dc34

payjoin/src/ohttp.rs

The `process_res` methods across send and receive have a lot of shared code that can be extracted into reusable functions. `process_get_res` and `process_post_res` essentially do the same thing, and could conceivably be combined into one function. They were kept separate because "202 Accepted" is not a valid response for POST requests.

Instead of mapping DirectoryResponseError to the corresponding variants on InternalSessionError and InternalCreateRequestError, nest those variants by replacing them with DirectoryResponseError.

spacebear21 · 2025-06-06T21:30:02Z

ec4b67e only applies your suggestions re: response.status().

In payjoin#660 we added a process_get_res and process_post_res methods in ohttps to pull away some of the logic from our send module but multiparty remained the same. This PR attempts pull sync the multiparty sender error handling with this new approach. As well this change gets rid of the remaining mutant on this status code match block.

spacebear21 mentioned this pull request Apr 18, 2025

Handle bad key configuration #617

Closed

spacebear21 force-pushed the ohttp-reqs-refactor branch from d72153b to 141b559 Compare June 5, 2025 21:49

spacebear21 mentioned this pull request Jun 6, 2025

Audit SessionError, CreateRequestError, and EncapsulationError internal variants #749

Open

spacebear21 marked this pull request as ready for review June 6, 2025 18:26

spacebear21 requested a review from DanGould June 6, 2025 18:26

DanGould requested changes Jun 6, 2025

View reviewed changes

spacebear21 force-pushed the ohttp-reqs-refactor branch from daef0ac to 3b2dc34 Compare June 6, 2025 19:58

DanGould reviewed Jun 6, 2025

View reviewed changes

payjoin/src/receive/v2/mod.rs Show resolved Hide resolved

DanGould approved these changes Jun 6, 2025

View reviewed changes

DanGould reviewed Jun 6, 2025

View reviewed changes

payjoin/src/ohttp.rs Show resolved Hide resolved

payjoin/src/ohttp.rs Show resolved Hide resolved

spacebear21 added 2 commits June 6, 2025 17:28

Refactor DirectoryResponseError mappings

ec4b67e

Instead of mapping DirectoryResponseError to the corresponding variants on InternalSessionError and InternalCreateRequestError, nest those variants by replacing them with DirectoryResponseError.

spacebear21 force-pushed the ohttp-reqs-refactor branch from 3b2dc34 to ec4b67e Compare June 6, 2025 21:28

spacebear21 merged commit d8c37c0 into payjoin:master Jun 6, 2025
7 checks passed

Conversation

spacebear21 commented Apr 18, 2025

Uh oh!

spacebear21 commented Apr 18, 2025

Uh oh!

coveralls commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15500115610

Details

💛 - Coveralls

Uh oh!

DanGould commented Jun 6, 2025

Uh oh!

spacebear21 commented Jun 6, 2025

Uh oh!

DanGould left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

spacebear21 Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

spacebear21 commented Jun 6, 2025

Uh oh!

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spacebear21 commented Jun 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Apr 18, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading