Retry pj on version error

[jbesraa]

resolves #144
blocked by #120

[DanGould]

No longer blocked

[jbesraa]

currently blocked by #182 as we need to access the supported versions array to check if v1 is supported

also by #184

[DanGould]

Rather than #182 I think we just want to expose WellKnownError in the API and then match against it:

            match ctx.process_response(&mut response.into_reader()) {
                Ok(Some(psbt)) => return Ok(psbt),
                Ok(None) => std::thread::sleep(std::time::Duration::from_secs(5)),
                Err(re) => {
                    println!("{}", re);
                    match re {
                        payjoin::send::ResponseError::WellKnown(k) => {
                            match k {
                                payjoin::send::WellKnownError::VersionUnsupported(_, v) => {
                                    if v.contains(&2) {
                                        todo!();
                                    }
                                }
                                _ => todo!()
                            }
                        }
                        _ => todo!(),
                    }
                    log::debug!("{:?}", re);
                }
            }

Note I don't recommend this level of nesting in place, it's merely an example. A function should probably take care of it.

[DanGould]

See the comment here for how to resolve the clone issue. I recommend cloning in the exceptional case rather than within extract_v1 #183 (comment)

[jbesraa]

While this is doing the trick, I think we should probably create an http module already with the content_type set and just providereq , that will save some duplicated lines of code.

[DanGould]

yes I think payjoin-cli would benefit from hexagonal design separation of concerns. The whole crate is an imperative mess at the moment, especially with multiple features

[DanGould]

Concept ACK.

The most important change I'd like to see is handle_response_error(e) logic happening inside the match ctx_process_response block rather than let error = assignment followed by a block of handler logic.

Since you haven't said this has been tested yet I converted it to draft. Open seems appropriate once at least a v2 sender to v1 receiver has a successful version downgrade payjoin.

[DanGould]

If we're going to spend the time to document functions let's write them with complete sentences and conventional grammar.

Suggested change

	/// Whether the error provided by the payjoin receiver is suggesting a specific version of
	/// payjoin.
	///
	/// This is useful for when you support multiple versions of payjoin. For example, if you
	/// support v1 and v2(BiP78 && BiP77 respectively), you can check if the error is suggesting a
	/// specific version and if it does, you can retry the request based on the asked version.
	/// Check whether or not the payjoin receiver supports the supplied version.
	///
	/// This is useful when you support multiple versions of payjoin. For example, if you
	/// support v1 and v2 (BIP78 and BIP77 respectively) and sent a v2 request that
	/// resulted in this error, you could pass `1` to test whether or not the receiver might
	/// respond well to another request using the older version.

[jbesraa]

thanks, changed.

[DanGould]

rather than assign let error = and follow this block with logic, I suggest writing be a handle_response_error(e) helper which can be called here when Err(e) is matched to contain attempt_version_downgrade logic and then handle the ultimate error if all else fails.

[jbesraa]

fixed.
decided to go with attemp_v1 and guarded that for v2 feature as it felt more correct

[DanGould]

I agree with the concept of abstraction into attempt_v1 (check spelling) and wonder if it could be abstracted to serve both v1 and v2 features? Does it for sure need to be feature gated? Both v1 and v2 sender attempt v1 sends. I think it's more common to name fns try_ instead of attempt_ too.

[jbesraa]

Ok so I think I went through all the comments, but I am yet to have time to test it.
Probably will do that tomorrow/monday.

[jbesraa]

we dont really handle the error here, but that might be too much error handling? what do you think?

[DanGould]

Right now on line 237 the error for v2 still gets printed if the retry happens. It seems logical to me to handle the error by retrying INSTEAD of returning, and then handle both v1 and v2 ctx.process_response errors with the same piece of code after if you still end up with an error. I think I'd just leave ctx.process_response(&mut response.into_reader()) to be returned here instead of printing. Do all the printing and merged error handling in long_poll_post

[DanGould]

nit: this file both sends and receives so this fn should probably be called try_send_v1

[DanGould]

returning early on line 236 here will skip printing the error if one is encountered. Rather, if an error is encountered there it should run on the same log statements. This will also require try_v1 returns Result<Psbt, ResponseError> instead of an anyhow::Error too. Then the last line of try_v1 can just be the process function and does not need to be wrapped in Ok()

Err(mut error) => {
    if error.is_version_supported(&1) {
        match self.try_v1(req_ctx).await {
            Ok(res) => return Ok(res),
            Err(e) => error = e,
        }
    };
    println!("{}", error);
    log::debug!("{:?}", error);
}

[DanGould]

In the PDK Contributors Sync today we discussed the possible downgrade paths. This PR describes a downgrade where a v2 sender tries to send a v2 request to a v1 receiver who responds that they only accept v1 requests, however, is it even possible for a v2 sender to make a v2 request to a v1 receiver without a v2 ohttp= param? I suppose it's possible that a payload with a v=2 param is passed to the v1 receiver, but that would be an oddity. @jbesraa How do you think about handling this circumstance?

[jbesraa]

TBH maybe its best to close the PR as the scenario is not really feasible. what do you think?

[DanGould]

This issue has been entrusted to you to resolve. However If it's going to be closed as wontfix please write up a thorough explanation as to why in a closing comment. It seems like there is still occasion where the downgrade happens on bip21 url parse vs on error which should still be handled by a v2 sender. So the PR was slightly misguided on this first attempt but the issue can still be resolved.

[jbesraa]

If a user is running v2, then their default is sending v2, we want to be able first to check if the receiver accepts v2, if not we should send v1.
So instead of trying twice, we can parse the uri and understand if the receiver accepts v2(by validating the ohttp parameter), if not, we can use the existing functionality to try and send v2.

Notice that there is a refactor(#198) in the cli, so pay attention to that before going on.