change request struct to tuple-struct

[jbesraa]

resolves #80

[DanGould]

What I'd initially imagined was a change to more easily deconstruct (req, ctx) into ((url, body), ctx), but I see that one may easily interpret that to instead be deconstructed as in this PR:

let req_url = req.0;
let req_body = req.1;

In hindsight, it actually may make more sense to put context into the request as a named variable so that you have req.url, req.body, req.context and don't have to know about deconstructing the tuple and guess the names. The context is directly related to the particular request anyhow.

I'm going to update the issue to reflect this preference. Thanks for engaging with this so we could come up with a better design.

[DanGould]

Another reason to keep the names is to keep the documentation. Adding pub context: context, to Request with a docstring should make it easier to handle downstream

[jbesraa]

@DanGould I moved Context into Request. I needed to add Clone attribute to Context so I can use it in the integration test after passing req to a function(see the changed lines in integration.rs please).

Question about the inline tests:
when I run cargo t locally the the inline tests(tests above functions) fail because the different steps dont consume code from each other, for example:

//! ### 3. (optional) Spawn a thread or async task that will broadcast the transaction after delay (e.g. 1 minute) unless canceled
//!
//! This was written in the original docs, but it should be clarified: In case the payjoin goes through but you still want to pay by default. This is missing from the `payjoin-cli`.
//!
//! Writing this, I think of [Signal's contributing guidelines](https://github.com/signalapp/Signal-iOS/blob/main/CONTRIBUTING.md#development-ideology):
//! > "The answer is not more options. If you feel compelled to add a preference that's exposed to the user, it's very possible you've made a wrong turn somewhere."
//!
//! ### 4. Construct the request with the PSBT and parameters
//!
//! ```
//! let req = link
//!     .create_pj_request(psbt, pj_params)
//!     .with_context(|| "Failed to create payjoin request")?;
//! ```
//!
//! ### 5. Send the request and receive response
//!
//! Senders request a payjoin from the receiver with a payload containing the Original PSBT  and optional parameters. They require a secure endpoint for authentication and message secrecy to prevent that transaction from being modified by a malicious third party during transit or being snooped on. Only https and .onion endpoints are spec-compatible payjoin endpoints.
//!
//! Avoiding the secure endpoint requirement is convenient for testing.
//!
//! ```
//! let client = reqwest::blocking::Client::builder()
//!     .danger_accept_invalid_certs(danger_accept_invalid_certs)
//!     .build()
//!     .with_context(|| "Failed to build reqwest http client")?;
//! let response = client
//!     .post(req.url)
//!     .body(req.body)
//!     .header("Content-Type", "text/plain")
//!     .send()
//!     .with_context(|| "HTTP request failed")?;
//! ```
//!
//! ### 6. Process the response
//!
//! An `Ok` response should include a Payjoin Proposal PSBT. Check that it's signed, following protocol, not trying to steal or otherwise error.
//!
//! ```
//! // TODO display well-known errors and log::debug the rest
//! // ctx is the context returned from create_pj_request in step 4.
//! let psbt = req.ctx.clone().process_response(response).with_context(|| "Failed to process response")?;
//! log::debug!("Proposed psbt: {:#?}", psbt);

"req.ctx" in step 6 would return an error here because req is defined in a different step(4).. does that happen to you as well?

[DanGould]

@jbesraa Thanks for this. I modified the code to make use of borrow instead of making Request Clone and demonstrated the type of deconstruction for readability that that makes possible. Let me know what you think

[DanGould]

Question about the inline tests: when I run cargo t locally the the inline tests(tests above functions) fail because the different steps dont consume code from each other ... "req.ctx" in step 6 would return an error here because req is defined in a different step(4).. does that happen to you as well?

It does fail for me as well, and I was wondering why, oops! This documentation would be better suited to a tutorial at https://payjoindevkit.org but I wrote it before that existed!

Once that exists, it would make more sense to link to the post from the docstring. In the future if we choose to proceed with Rust Documentation tests, we can use preprocessing and hiding to fix the errors you've come across

[DanGould]

Now I see why @Kixunil separated (req, ctx) in the first place. req data gets shuttled off to the http client and ctx sticks around. By using this new abstraction I'm not actually sure this is more ergonomic since ... we eneded up needing to build this separation back in with .clone()

After working through it I think it might actually make sense to leave functionally as-is and document why the separation exists in the create_pj_request function signature. working this through was NOT a waste of time if we end up merging something else @jbesraa, clearly there was a lack of clarity and I think we finally found it.

The ergonomic solution may be as simple as

    /// Prepare an HTTP request and request context to process the response
    ///
    /// An HTTP client will own the Request data while Context sticks around so
    /// a `(Request, Context)` tuple is returned to keep them separated. call:
    ///
    /// `let (request, context) = uri.create_pj_request(psbt, params);`
    #[cfg(feature = "send")]
    fn create_pj_request(
        self,
        psbt: bitcoin::psbt::Psbt,
        params: send::Configuration,
    ) -> Result<(send::Request, send::Context), send::CreateRequestError>;

[jbesraa]

that makes sense @DanGould. pushed the short docs and reverted previous commits