Handle v2 error

[DanGould]

Fix #468 with a simple approach: Just add the methods to UncheckedProposal which can be cloned at the beginning of the receive process.

This seems to be is the least invasive way to make this change that I didn't even think of in my enumerated approaches in #468.

Alternatively, we could have some way to extract an ErrorContext from which an Error (request,context) could be extracted from an ErrorContext::extract_req(e: Error) -> (Request, ohttp_ctx)

But I think this gets us the progress we want and we can follow up if we decide that's a good idea with the rest of the receiver module error split.

[coveralls]

Pull Request Test Coverage Report for Build 12757549527

Details

• 74 of 119 (62.18%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.2%) to 60.758%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/receive/v2/error.rs	0	3	0.0%
payjoin/src/receive/v2/mod.rs	67	83	80.72%
payjoin-cli/src/app/v2.rs	0	26	0.0%

Totals
Change from base Build 12754183738:	-0.2%
Covered Lines:	2917
Relevant Lines:	4801

---

💛 - Coveralls

[spacebear21]

tACK 22a5281, though this seems like a great candidate for a new unit test.

[spacebear21]

This feels counterintuitive, it might be clearer if handle_request_error returns Ok(()) on success and this becomes:

Suggested change

	handle_request_error(e, receiver, &self.config.ohttp_relay).await?;
	unreachable!("handle_request_error always returns Err")
	handle_request_error(e, receiver, &self.config.ohttp_relay).await?;
	return Err(anyhow!("Failed to process proposal"));

[DanGould]

I do think there is a change to make, that a successful handle_request_error returns the receive::Error which gets propagated via the question mark, but I think that second line should indeed be unreachable if that's the case because handle_request_error always returns an error. The reason the handle_request_error signature uses Result is so that ? can be used in the body. Is this still counterintuitive?

Perhaps using

/// Handle request error by sending an error response over the directory
async fn handle_request_error(
    e: Error,
    mut receiver: UncheckedProposal,
    ohttp_relay: &payjoin::Url,
) -> anyhow::Error {
    let (err_req, err_ctx) = match receiver.extract_err_req(&e, ohttp_relay) {
        Ok(req_ctx) => req_ctx,
        Err(e) => return anyhow!("Failed to extract error request: {}", e),
    };

    let err_response = match post_request(err_req).await {
        Ok(response) => response,
        Err(e) => return anyhow!("Failed to post error request: {}", e),
    };

    let err_bytes = match err_response.bytes().await {
        Ok(bytes) => bytes,
        Err(e) => return anyhow!("Failed to get error response bytes: {}", e),
    };

    if let Err(e) = receiver.process_err_res(&err_bytes, err_ctx) {
        return anyhow!("Failed to process error response: {}", e);
    }

    e.into()
}

and

Err(e) => {
                return Err(handle_request_error(e, receiver, &self.config.ohttp_relay).await);
            }

makes more sense

[spacebear21]

This seems better to me.

[DanGould]

How would you unit test this? Manually produce a receive::error::Error and match it with a Receiver? I've been hacking around an integration test for this but it's lengthy and ugly

[spacebear21]

How would you unit test this? Manually produce a receive::error::Error and match it with a Receiver? I've been hacking around an integration test for this but it's lengthy and ugly

I think we could start with a simple unit test that hardcodes an UncheckedProposal, returns an Error on one of the checks, and validates the JSON output. For example something like this:

        // copied from receive::v1::test
        let original_psbt = "cHNidP8BAHMCAAAAAY8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////AtyVuAUAAAAAF6kUHehJ8GnSdBUOOv6ujXLrWmsJRDCHgIQeAAAAAAAXqRR3QJbbz0hnQ8IvQ0fptGn+votneofTAAAAAAEBIKgb1wUAAAAAF6kU3k4ekGHKWRNbA1rV5tR5kEVDVNCHAQcXFgAUx4pFclNVgo1WWAdN1SYNX8tphTABCGsCRzBEAiB8Q+A6dep+Rz92vhy26lT0AjZn4PRLi8Bf9qoB/CMk0wIgP/Rj2PWZ3gEjUkTlhDRNAQ0gXwTO7t9n+V14pZ6oljUBIQMVmsAaoNWHVMS02LfTSe0e388LNitPa1UQZyOihY+FFgABABYAFEb2Giu6c4KO5YW0pfw3lGp9jMUUAAA=";
        let body = original_psbt.as_bytes();
        let headers = MockHeaders::new(body.len() as u64);
        let proposal = UncheckedProposal {
            v1: v1::UncheckedProposal::from_request(
                body,
                "maxadditionalfeecontribution=182&additionalfeeoutputindex=0",
                headers,
            )
            .unwrap(),
            context,
        };

        let p = proposal
            .check_broadcast_suitability(None, |t| Err(Error::Server("mock error".into())));
        assert_eq!(
            p.err().unwrap().to_json(),
            "{{ \"errorCode\": \"server-error\", \"message\": \"Internal server error\" }}"
        );

[DanGould]

Fixed the payjoin-cli receive::error::Error handler and added a unit test

[spacebear21]

Updated the last commit to use crate::receive::v1::test::proposal_from_test_vector() instead of manually constructing the headers in the v2 test.

tACK 4f8f952

[DanGould]

Whoops, 4f8f952 was based on an old base that had conflicts so I needed to rebase

Wise switch to crate::receive::v1::test::proposal_from_test_vector() 🔥