Supply & parse receiver response errors #120

jbesraa · 2023-11-28T17:26:08Z

supersedes #110

resolves #53

DanGould · 2023-11-30T17:16:08Z

since rust-bitcoin is our dependency and has decided to keep serde, shall we persist with it (perhaps without derive) rather than the unmaintained tinyjson? #110 (comment)

see rust-bitcoin/rust-bitcoin#2093 on excluding serde_derive

jbesraa · 2023-11-30T19:26:06Z

Yea, that makes sense. Will see what they did and try to follow that.

I started actually be reverting the last commit and then started to write test first.
I am setting up mock http server(with 'httpmock') in the integration tests for sender/receiver and testing the response in different scenarios.
Ill push some commits tomorrow.

jbesraa · 2023-12-01T17:26:20Z

Generally the functionality seems to be working.
I will handle the serde thingy and organize stuff and add more testing.
Changed to draft for now as its not so ready for review.

DanGould

Comprehensive looking 😎 Any reason this is review requested but still draft? Because it is, I haven't checked it out yet.

DanGould · 2023-12-02T16:09:24Z

payjoin/Cargo.toml

@@ -24,11 +24,15 @@ bitcoin = { version = "0.30.0", features = ["base64"] }
 bip21 = "0.3.1"
 log = { version = "0.4.14"}
 rand = { version = "0.8.4", optional = true }
+serde = { version = "1.0.107", features = ["derive"] }


if we're using serde I think we want to use it without "derive," That's where the static binary comes in

DanGould · 2023-12-02T18:19:29Z

payjoin/src/receive/error.rs

+            Self::Server(_) => write!(
+                f,
+                r#"{{ "errorCode": "unavailable", "message": "The payjoin endpoint is not available for now." }}"#
+            ),


Display is for human readable errors, not really json formamtted ones. This would be wherever the error is printed internally. We probably do not want to drop e for this purpose. Json response should be a separate method, this may have been an oversight in my original pr.

DanGould · 2023-12-02T18:24:15Z

payjoin/tests/integration.rs

+        let receiver_server = MockServer::start();
+        {
+            receiver_server.mock(|when, then| {
+                // let err = Error::BadRequest
+                let error_response = Error::Server("".to_string().into());
+                // dbg!(&error_response);
+                when.method(httpmock::Method::POST).path("/payjoin");
+                then.status(200)
+                    .header("content-type", "text/plain")
+                    .body(error_response.to_string());
+            });
+            let mut response = isahc::post(receiver_server.url("/payjoin"), "").unwrap();
+            let client_response = ctx.clone().process_response(response.body_mut());
+            dbg!(&client_response);
+        }


Is there any reason we couldn't check against a real integration to avoid mocking?

DanGould · 2023-12-02T18:25:30Z

payjoin/Cargo.toml

 url = "2.2.2"

 [dev-dependencies]
 env_logger = "0.9.0"
 bitcoind = { version = "0.31.1", features = ["0_21_2"] }
+httpmock = "0.7.0-rc.1"
+isahc = "1.7.2"


We're using ureq as msrv complianat http elsewhere in the codebase, and it's a bitcoind dependency, so it might make more sense than isahc

DanGould

Hate to burst your bubble because I see you've taken a valiant effort to dive into the more difficult error handling territory. But I think there's some confusion between the receiver's bip21 parameters and sender's optional parameters in this last commit.

The version unsupported error is actually already produced from receive.

This PR should attempt to parse and categorize the response from an independent send module just from the json it receives inside process_response

Where Psbt::from_str is called should try to parse the response as a json error if Psbt parsing fails and then return some error, perhaps a receiver::Error with ResponseError parsed from receiver json or a ValidationError variants. The ResponseError could contain both known and unknown variants, but only display messages from known errors, as per spec. I believe this was already done in #110 via WellKnownError::from_str inside ResponseError::from_json.

DanGould · 2023-12-05T20:35:35Z

payjoin/src/receive/optional_parameters.rs

@@ -22,17 +46,19 @@ impl Default for Params {
            disable_output_substitution: false,
            additional_fee_contribution: None,
            min_feerate: FeeRate::ZERO,
+            version: 1,


version: 1 is default and does not need to be explicitly specified when sending v1 payjoin requests

If not specified, the receiver will assume the sender is v=1.

DanGould · 2023-12-05T20:38:35Z

payjoin/src/receive/optional_parameters.rs

-    pub fn from_query_pairs<K, V, I>(pairs: I) -> Result<Self, Error>
+    pub fn from_query_pairs<K, V, I>(pairs: I) -> Result<Self, ResponseError>


from_query_pairs probably should not return a ResponseError, as that type encapsulates the error presented in the response. Error on the other hand can contain richness only relevant to the internal receiver state

DanGould · 2023-12-05T20:42:20Z

payjoin/src/uri.rs

@@ -110,6 +111,7 @@ impl<'a> bip21::de::DeserializeParams<'a> for Payjoin {
 pub struct DeserializationState {
    pj: Option<Url>,
    pjos: Option<bool>,
+    v: Option<usize>,


"&v=" is a sender optional parameter, not a bip21 uri parameter.

bip21 receiver parameters are specified by the receiver and unrecognized by v1 senders.

sender optional parameters are attached to the http request that sends a payjoin.

DanGould · 2023-12-05T20:46:11Z

payjoin/src/receive/optional_parameters.rs

+use crate::send::error::WellKnownError;
+use crate::send::ResponseError;


optional_parameters mod lives inside receive and should not depend on anything that's not common to both send and receive or explicitly inside receive. receive depending on send should be a red flag that the design might be going down the wrong path

DanGould · 2023-12-05T20:46:50Z

payjoin/src/receive/optional_parameters.rs

+type MaxAdditionalFeeContribution = bitcoin::Amount;
+type AdditionalFeeOutputIndex = usize;


How does this newtype help someone reading the error signature better understand what they're looking at?

jbesraa · 2023-12-06T11:07:33Z

thanks for the input @DanGould
last commit was not really great..

pushed some changes to revert some of the mistakes introduced previously.
this is still wip and not ready for review.
i am planning to test the other KnownError scenarios and then remove derive from serde

jbesraa · 2023-12-06T11:20:42Z

payjoin-cli/src/app.rs

@@ -91,7 +91,8 @@ impl App {
        let psbt = Psbt::from_str(&psbt).with_context(|| "Failed to load PSBT from base64")?;
        log::debug!("Original psbt: {:#?}", psbt);
        let fallback_tx = psbt.clone().extract_tx();
-        let (req, ctx) = payjoin::send::RequestBuilder::from_psbt_and_uri(psbt, uri)
+        let version = 1;
+        let (req, ctx) = payjoin::send::RequestBuilder::from_psbt_and_uri(psbt, uri, version)


motivation > we need a way to provide the sender to specify the version that will be appended in the url

jbesraa · 2023-12-14T13:24:14Z

Thanks for the input!

TBH I was sure we dont want serde at all in the project, but now that I see that u are using it in V2, I could indeed just impl serde::Ser.. and serde::Des...

i agree, we shouldnt expose RawResponse but I do think we should keep it and use it internally in ResponseError. that way, we can make changes to the internal struct without breaking a pub struct. please see my latest push.

in general I think all is implemented for the ResponseError and its added to process_response and that is tested as well.

We have the integration tests and cli that are not using ResponseError and using AnyHow, should I tackle that in the next commit or we want to do that in a different pr?

DanGould · 2023-12-14T16:36:30Z

I think we should at least have the errors created here handled in integration tests. It would be nice to have them done properly in the app too with proper println!(errorCode) println!(message) for WellKnownErrors and println!(generic error message about unknown error) for UnknownErrors + debug logs including the code+messages for both

DanGould · 2023-12-14T16:40:45Z

payjoin/src/send/error.rs

+        let known_str_error = "{\"errorCode\":\"version-unsupported\",
+                    \"message\":\"This version of payjoin is not supported.\"}";


This is a good opportunity to take advantage of string literals

Suggested change

let known_str_error = "{\"errorCode\":\"version-unsupported\",

\"message\":\"This version of payjoin is not supported.\"}";

let known_str_error = r#"{"errorCode":"version-unsupported", "message":"This version of payjoin is not supported."}"#;

DanGould · 2023-12-14T16:44:47Z

payjoin/src/send/mod.rs

+
+    const ORIGINAL_PSBT: &str = "cHNidP8BAHMCAAAAAY8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////AtyVuAUAAAAAF6kUHehJ8GnSdBUOOv6ujXLrWmsJRDCHgIQeAAAAAAAXqRR3QJbbz0hnQ8IvQ0fptGn+votneofTAAAAAAEBIKgb1wUAAAAAF6kU3k4ekGHKWRNbA1rV5tR5kEVDVNCHAQcXFgAUx4pFclNVgo1WWAdN1SYNX8tphTABCGsCRzBEAiB8Q+A6dep+Rz92vhy26lT0AjZn4PRLi8Bf9qoB/CMk0wIgP/Rj2PWZ3gEjUkTlhDRNAQ0gXwTO7t9n+V14pZ6oljUBIQMVmsAaoNWHVMS02LfTSe0e388LNitPa1UQZyOihY+FFgABABYAFEb2Giu6c4KO5YW0pfw3lGp9jMUUAAA=";
+
+    const ORIGINAL_PROPOSAL: &str = "cHNidP8BAJwCAAAAAo8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////jye60aAl3JgZdaIERvjkeh72VYZuTGH/ps2I4l0IO4MBAAAAAP7///8CJpW4BQAAAAAXqRQd6EnwadJ0FQ46/q6NcutaawlEMIcACT0AAAAAABepFHdAltvPSGdDwi9DR+m0af6+i2d6h9MAAAAAAQEgqBvXBQAAAAAXqRTeTh6QYcpZE1sDWtXm1HmQRUNU0IcBBBYAFMeKRXJTVYKNVlgHTdUmDV/LaYUwIgYDFZrAGqDVh1TEtNi300ntHt/PCzYrT2tVEGcjooWPhRYYSFzWUDEAAIABAACAAAAAgAEAAAAAAAAAAAEBIICEHgAAAAAAF6kUyPLL+cphRyyI5GTUazV0hF2R2NWHAQcXFgAUX4BmVeWSTJIEwtUb5TlPS/ntohABCGsCRzBEAiBnu3tA3yWlT0WBClsXXS9j69Bt+waCs9JcjWtNjtv7VgIge2VYAaBeLPDB6HGFlpqOENXMldsJezF9Gs5amvDQRDQBIQJl1jz1tBt8hNx2owTm+4Du4isx0pmdKNMNIjjaMHFfrQABABYAFEb2Giu6c4KO5YW0pfw3lGp9jMUUIgICygvBWB5prpfx61y1HDAwo37kYP3YRJBvAjtunBAur3wYSFzWUDEAAIABAACAAAAAgAEAAAABAAAAAAA=";


This should be called PAYJOIN_PROPOSAL in BIP 78 parlance

let's integration test it first

DanGould

I see that WellKnown now ignores the message. It should still hang on to it for debug, but only return the template for display. So close!

I also now see what you were saying about additional json fields. In spec "version-unsupported" error contains a supported-versions array for example. We're not using it yet, so I don't think it's awful to ignore for the first round, but we must consider that we may support additional json fields in our design (saying we're OK with breaking changes is one way to handle that) in the future as you mentioned over voice

https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#optional-parameters

I also pinned "which" in order to pass msrv

DanGould · 2023-12-18T18:01:38Z

payjoin/src/send/error.rs

+    fn from_str(s: &str) -> Result<Self, ()> {
+        match s {
+            "unavailable" => Ok(WellKnownError::Unavailable),
+            "not-enough-money" => Ok(WellKnownError::NotEnoughMoney),
+            "version-unsupported" => Ok(WellKnownError::VersionUnsupported),
+            "original-psbt-rejected" => Ok(WellKnownError::OriginalPsbtRejected),
+            _ => Err(()),
+        }


Yes, we want wellknown error to only match on errorCode, but it should still contain a message which may be different than the template. However, only template message will be displayed to the user and the actual message printed to debug log

DanGould · 2023-12-18T18:07:59Z

payjoin/src/send/error.rs

+
+    #[test]
+    fn test_parse_json() {
+        let known_str_error = r#"{"errorCode":"version-unsupported", "message":"This version of payjoin is not supported."}"#;


This test could make the message distinct from the template, and check that it prints the message to debug and displays the canned "This version of payjoin is not supported"

DanGould · 2023-12-18T18:10:02Z

payjoin/src/send/error.rs

+                r#"Well known error: {{ "errorCode": "{}",
+                "message": "{}" }}"#,
+                e.error_code(),
+                e.to_string(),


we should print the saved json error message here and use to_string just for display

jbesraa · 2023-12-21T19:50:43Z

fixed up code to add message and actually separate "message" and "meaning" ugh

DanGould

I made a small nit to WellKnownError getter return ownership and applied this change to work in the payjoin-cli If that floats your boat. I say we merge should you approve my changes on top.

DanGould · 2023-12-22T18:01:36Z

payjoin/tests/integration.rs

+            assert_eq!(
+                format!("{:?}", response_error),
+                "Well known error: { \"errorCode\": \"version-unsupported\",\n                \"message\": \"Debug Message\" }"
+            );
+            match response_error {
+                payjoin::send::ResponseError::WellKnown(e) => {
+                    assert_eq!(e.clone().error_code(), "version-unsupported");
+                    assert_eq!(e.message(), "Debug Message");
+                }
+                _ => panic!("Unexpected error type"),
+            };


I definitely prefer the match asserts to the assert_eq. Seems like that assert_eq could start failing while the response is still spec valid.

DanGould · 2023-12-22T18:02:55Z

payjoin/tests/integration.rs

+            let unrecognized_error = r#"{"errorCode":"not-recognized", "message":"o"}"#;
+            let response_error = ctx.clone().process_response(&mut unrecognized_error.as_bytes());
+            assert_eq!(
+                response_error.unwrap_err().to_string(),
+                "The receiver sent an unrecognized error."
+            );


This seems to be more of a unit test than related to this particular integration since we're mocking the unrecognized error

DanGould · 2023-12-27T20:43:26Z

payjoin/src/send/error.rs

+}
+
+impl WellKnownError {
+    fn from_error_code(s: &str, message: String) -> Result<Self, ()> {


Seems like this could shortcut return directly to an unrecognized ResponseError insted of Err(()). Just a note in case this gets refactored in the future.

DanGould · 2023-12-27T20:45:42Z

payjoin/src/send/mod.rs

+        assert_eq!(
+            ctx.process_response(&mut invalid_json_error.as_bytes()).unwrap_err().to_string(),
+            "The receiver sent an invalid response: couldn't decode as PSBT or JSON"
+        )


I might check Eq between error types instead of the magic string representation next time

DanGould · 2023-12-27T20:49:53Z

A nit in the commit log would be that there's a send module not sender module

DanGould · 2023-12-27T20:50:19Z

Excellent review volley here. Huge congrats that this is merged 🎉

jbesraa requested a review from DanGould November 28, 2023 17:26

jbesraa added the receive receiving payjoin label Nov 28, 2023

jbesraa changed the title ~~Receiver/well known errors~~ Supply & parse receiver response errors Nov 28, 2023

DanGould mentioned this pull request Nov 28, 2023

Supply & parse receiver response errors #110

Closed

2 tasks

jbesraa force-pushed the receiver/well-known-errors branch from 210ca40 to 59121d5 Compare December 1, 2023 17:24

jbesraa marked this pull request as draft December 1, 2023 17:24

jbesraa force-pushed the receiver/well-known-errors branch from 59121d5 to 693ab86 Compare December 1, 2023 17:26

DanGould requested changes Dec 2, 2023

View reviewed changes

DanGould requested changes Dec 5, 2023

View reviewed changes

jbesraa force-pushed the receiver/well-known-errors branch 4 times, most recently from 3f138a3 to c76ad71 Compare December 6, 2023 11:04

jbesraa commented Dec 6, 2023

View reviewed changes

jbesraa force-pushed the receiver/well-known-errors branch 12 times, most recently from 1f9fc3d to d0575aa Compare December 13, 2023 17:18

DanGould reviewed Dec 14, 2023

View reviewed changes

DanGould previously approved these changes Dec 14, 2023

View reviewed changes

DanGould reviewed Dec 14, 2023

View reviewed changes

jbesraa force-pushed the receiver/well-known-errors branch 3 times, most recently from cae632b to 874b874 Compare December 15, 2023 12:04

DanGould added this to the 0.13.0 milestone Dec 15, 2023

jbesraa force-pushed the receiver/well-known-errors branch 3 times, most recently from 32f3f3c to 16907ad Compare December 18, 2023 14:30

DanGould force-pushed the receiver/well-known-errors branch from 16907ad to e989bd4 Compare December 18, 2023 18:31

DanGould requested changes Dec 18, 2023

View reviewed changes

DanGould force-pushed the receiver/well-known-errors branch from e989bd4 to 08533a3 Compare December 18, 2023 18:43

DanGould mentioned this pull request Dec 20, 2023

Send v1 requests to v1 receivers even if v2 feature is enabled #144

Open

jbesraa force-pushed the receiver/well-known-errors branch 2 times, most recently from 1ac76e1 to 24e09b0 Compare December 21, 2023 19:47

This was referenced Dec 22, 2023

[WIP] Retry pj on version error jbesraa/rust-payjoin#1

Closed

Retry pj on version error #151

Closed

DanGould approved these changes Dec 22, 2023

View reviewed changes

Allow sender to handle well known errors

b867f69

jbesraa force-pushed the receiver/well-known-errors branch from b083792 to b867f69 Compare December 27, 2023 16:56

DanGould reviewed Dec 27, 2023

View reviewed changes

DanGould merged commit 8ffa8ef into payjoin:master Dec 27, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supply & parse receiver response errors #120

Supply & parse receiver response errors #120

jbesraa commented Nov 28, 2023 •

edited

DanGould commented Nov 30, 2023 •

edited

jbesraa commented Nov 30, 2023 •

edited

jbesraa commented Dec 1, 2023

DanGould left a comment

DanGould Dec 2, 2023

DanGould Dec 2, 2023

DanGould Dec 2, 2023

DanGould Dec 2, 2023

DanGould left a comment

DanGould Dec 5, 2023

DanGould Dec 5, 2023

DanGould Dec 5, 2023

DanGould Dec 5, 2023

DanGould Dec 5, 2023

jbesraa commented Dec 6, 2023 •

edited

jbesraa Dec 6, 2023

jbesraa commented Dec 14, 2023

DanGould commented Dec 14, 2023 •

edited

DanGould Dec 14, 2023

DanGould Dec 14, 2023

DanGould left a comment •

edited

DanGould Dec 18, 2023

DanGould Dec 18, 2023

DanGould Dec 18, 2023

jbesraa commented Dec 21, 2023

DanGould left a comment

DanGould Dec 22, 2023

DanGould Dec 22, 2023

DanGould Dec 27, 2023 •

edited

DanGould Dec 27, 2023 •

edited

DanGould commented Dec 27, 2023

DanGould commented Dec 27, 2023

		pub fn from_query_pairs<K, V, I>(pairs: I) -> Result<Self, Error>
		pub fn from_query_pairs<K, V, I>(pairs: I) -> Result<Self, ResponseError>

		use crate::send::error::WellKnownError;
		use crate::send::ResponseError;

		type MaxAdditionalFeeContribution = bitcoin::Amount;
		type AdditionalFeeOutputIndex = usize;

		let known_str_error = "{\"errorCode\":\"version-unsupported\",
		\"message\":\"This version of payjoin is not supported.\"}";


		const ORIGINAL_PSBT: &str = "cHNidP8BAHMCAAAAAY8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////AtyVuAUAAAAAF6kUHehJ8GnSdBUOOv6ujXLrWmsJRDCHgIQeAAAAAAAXqRR3QJbbz0hnQ8IvQ0fptGn+votneofTAAAAAAEBIKgb1wUAAAAAF6kU3k4ekGHKWRNbA1rV5tR5kEVDVNCHAQcXFgAUx4pFclNVgo1WWAdN1SYNX8tphTABCGsCRzBEAiB8Q+A6dep+Rz92vhy26lT0AjZn4PRLi8Bf9qoB/CMk0wIgP/Rj2PWZ3gEjUkTlhDRNAQ0gXwTO7t9n+V14pZ6oljUBIQMVmsAaoNWHVMS02LfTSe0e388LNitPa1UQZyOihY+FFgABABYAFEb2Giu6c4KO5YW0pfw3lGp9jMUUAAA=";

		const ORIGINAL_PROPOSAL: &str = "cHNidP8BAJwCAAAAAo8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////jye60aAl3JgZdaIERvjkeh72VYZuTGH/ps2I4l0IO4MBAAAAAP7///8CJpW4BQAAAAAXqRQd6EnwadJ0FQ46/q6NcutaawlEMIcACT0AAAAAABepFHdAltvPSGdDwi9DR+m0af6+i2d6h9MAAAAAAQEgqBvXBQAAAAAXqRTeTh6QYcpZE1sDWtXm1HmQRUNU0IcBBBYAFMeKRXJTVYKNVlgHTdUmDV/LaYUwIgYDFZrAGqDVh1TEtNi300ntHt/PCzYrT2tVEGcjooWPhRYYSFzWUDEAAIABAACAAAAAgAEAAAAAAAAAAAEBIICEHgAAAAAAF6kUyPLL+cphRyyI5GTUazV0hF2R2NWHAQcXFgAUX4BmVeWSTJIEwtUb5TlPS/ntohABCGsCRzBEAiBnu3tA3yWlT0WBClsXXS9j69Bt+waCs9JcjWtNjtv7VgIge2VYAaBeLPDB6HGFlpqOENXMldsJezF9Gs5amvDQRDQBIQJl1jz1tBt8hNx2owTm+4Du4isx0pmdKNMNIjjaMHFfrQABABYAFEb2Giu6c4KO5YW0pfw3lGp9jMUUIgICygvBWB5prpfx61y1HDAwo37kYP3YRJBvAjtunBAur3wYSFzWUDEAAIABAACAAAAAgAEAAAABAAAAAAA=";

Supply & parse receiver response errors #120

Supply & parse receiver response errors #120

Conversation

jbesraa commented Nov 28, 2023 • edited

DanGould commented Nov 30, 2023 • edited

jbesraa commented Nov 30, 2023 • edited

jbesraa commented Dec 1, 2023

DanGould left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Dec 6, 2023 • edited

Choose a reason for hiding this comment

jbesraa commented Dec 14, 2023

DanGould commented Dec 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Dec 21, 2023

DanGould left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould Dec 27, 2023 • edited

Choose a reason for hiding this comment

DanGould Dec 27, 2023 • edited

Choose a reason for hiding this comment

DanGould commented Dec 27, 2023

DanGould commented Dec 27, 2023

jbesraa commented Nov 28, 2023 •

edited

DanGould commented Nov 30, 2023 •

edited

jbesraa commented Nov 30, 2023 •

edited

jbesraa commented Dec 6, 2023 •

edited

DanGould commented Dec 14, 2023 •

edited

DanGould left a comment •

edited

DanGould Dec 27, 2023 •

edited

DanGould Dec 27, 2023 •

edited