Consider spliting `ContextV2::process_response` into two functions

[jbesraa]

Currently ContextV2::process_response is doing few things: 1) read response data 2) convert response data to proposal 3) validate proposal

It would be useful to split this function into two separate functions:

1) ContextV2::extract_proposal_from_response(&self, &mut impl std::io::Read) 2) ContextV2::process_proposal(self, proposal)

Why this is useful? 1) process_response takes self even though its not always necessarily needed (in case of empty success response) 2) process_response return argument with three different options including Ok(None) is confusing

[DanGould]

process_response takes self even though its not always necessarily needed (in case of empty success response)

Are you suggesting to get rid of self in a function signature? Both of your suggestions contain self in some form as an argument. They're methods, not pure functions.

Or are you suggesting that taking ownership of self is a problem? If so, can you show me an example of where?

process_response return argument with three different options including Ok(None) is confusing

Returning Result<Option<T>> Is a common rust pattern as far as I understand. In addition to how it's used to process the response, a similar pattern is also used in payjoin-directory's Database to handle the other side of the same function as Option<Result> on peek_req.

To be explicit, the three variants that can be returned are as follows:

1. Success with a value (Ok(Some(value))): The operation succeeded and returned a value.
2. Success without a value (Ok(None))): The operation succeeded but there was no value to return.
3. Failure (Err(error)): The operation failed, and an error was returned.

[jbesraa]

If I am not mistaken, (2) is possible only when a response with status code 204 received from the directory. But it would be nice to be able to handle only responses with status code 200 and in that case Ok(None) is unreachable.

[DanGould]

Correct Ok(None) is returned when the encapsulated response from the directory has status code 204.

When would you need to handle only encapsulated status code 200 and not 204? v2 receivers that receive either v1 or v2 requests (ideally both) need to handle encapsulated 204 responses from the payjoin directory.

[jbesraa]

The consideration for handling only 200 is performance. While implementing payjoin in ldk-node, I didnt see a reason to pass by responses that are not 200. Do you think there is a reason to handle 204 as well?

[DanGould]

You need to handle 204 and 200 in order to implement asynchronous payjoin using HTTP long polling. When a proposal is available at the directory, a 200 response is produced. If no proposal is yet available, a 204 response is returned and the client should make another request. They should continue to poll until the session expires or process_response returns an error.

[DanGould]

Unless I misunderstood the issue, I'm going to close it for now. I think the implementation is doing what we want it to do.

Thanks for raising the issue