Refactor function signatures where appropriate

[benalleng]

Many of these function signatures were audited in #405 and implemented here.

I am going to leave any new function signatures outside of the ones here to a new PR so I don't bloat this PR anymore than it already is.

[benalleng]

Does this seem like an ok way to turn this into a string?

[nothingmuch]

i think so.

previously the response body was copied into a Vec<u8>, and then given as a io::Read. .text(), otoh, treats it as a utf8 string, not all &[u8]-ish things can be made into a String and subsequently a &str, but since this is v1 that should be OK, the data should be base64 followed by query param encoded data, all of which should be ASCII.

however, it seems a bit more defensive, and consistent with v2 to think of all responses as &[u8], so that the payjoin crate is responsible for handling that error, if we change the signature to take a &str, then payjoin-cli and other wallets would be responsible for enforcing this part of BIP78's rules which they should be rather agnostic about, similarly to how the payjoin crate should be agnostic to transport errors etc (and by the same token should not accept a response object as its argument).

nit:

Suggested change

	let response_str = response.text().await.with_context(|| "Failed to read seponse")?;
	let response_str = response.text().await.with_context(|| "Failed to read reponse")?;

[spacebear21]

It seems fine, though the extract context is probably unnecessary. I might also suggest doing it inline like it was before:

let psbt = ctx.process_response(&response.text().await?).map_err(|e| {
...

[nothingmuch]

Partial review, I'm not sure how strongly I feel about &[u8] in place of &str, I think I could be convinced that that's just bikeshedding but I think the separation of responsibilities arguments is more than just bikeshedding.

I would very much appreciate it if the commits were split up, some of it is trivial refactoring, other parts are a bit more semantic, it all looks good to me but I found it a little noisy to read the large diff as small changes touch many different files, and reviewing commit by commit would be easier, we can always squash later with no effort if that feels too noisy (but I'm personally partial to small commits):

1. validate_input_utxos remove treat_missing_as_error param
2. process_response, &mut impl std::io::Read -> &[u8]
3. extract_v2, Url -> &Url
4. new_v1 (Url, Vec<u8>) -> (&Url, &[u8])
5. making ohttp_decapsulate pub(crate) (seems appropriate but not sure i understand the rationale, if it's just because that's possible then cACK)
6. decrypt_message_b ...

some of these are really trivial but touch many files

some of these are a bit more subtle

it would make sense IMO to have one commit per changed fn, and maybe even separate v1 and v2 code into separate pull requests?

[nothingmuch]

i think so.

previously the response body was copied into a Vec<u8>, and then given as a io::Read. .text(), otoh, treats it as a utf8 string, not all &[u8]-ish things can be made into a String and subsequently a &str, but since this is v1 that should be OK, the data should be base64 followed by query param encoded data, all of which should be ASCII.

however, it seems a bit more defensive, and consistent with v2 to think of all responses as &[u8], so that the payjoin crate is responsible for handling that error, if we change the signature to take a &str, then payjoin-cli and other wallets would be responsible for enforcing this part of BIP78's rules which they should be rather agnostic about, similarly to how the payjoin crate should be agnostic to transport errors etc (and by the same token should not accept a response object as its argument).

nit:

Suggested change

	let response_str = response.text().await.with_context(|| "Failed to read seponse")?;
	let response_str = response.text().await.with_context(|| "Failed to read reponse")?;

[spacebear21]

The note in #405 says response could be a &[u8], but I agree using &str seems more straightforward.

@DanGould is there a reason we should use bytes instead of a string?

[spacebear21]

Seems @nothingmuch addressed this same question in his review and I missed it when writing this.

[spacebear21]

It seems fine, though the extract context is probably unnecessary. I might also suggest doing it inline like it was before:

let psbt = ctx.process_response(&response.text().await?).map_err(|e| {
...

[nothingmuch]

Re &[u8] vs impl std::io::Read, BIP 78 does not restrict the size of the response, but specifies that content length must be included. the v1 receiver enforces a reasonable limit on this:

rust-payjoin/payjoin/src/receive/v1.rs

Lines 95 to 97 in 34ee78d

	if content_length > 4_000_000 * 4 / 3 {
	return Err(InternalRequestError::ContentLengthTooLarge(content_length).into());
	}

however, a similar limit was not included in payjoin-cli prior to this PR, response.bytes().to_vec() could potentially make unbounded allocations given a large enough response body:

https://github.com/payjoin/rust-payjoin/pull/481/files#diff-40ff0a2747a0024cf54e5c3831bdd0a9e5ae32e714ce629826e5d8455ed0d50bL99

or in the sender:

rust-payjoin/payjoin/src/send/v1.rs

Line 277 in 34ee78d

response.read_to_string(&mut res_str).map_err(InternalValidationError::Io)?;

I opened #483, but since this PR changes the process_response function signature, maybe it'd be more convenient to address the payjoin-cli part of this in this PR at your discretion

[DanGould]

This is closer but still needs work. I notice separate commits and updated comments are headed in the right direction.

In order to be merged, each commit needs to build and should pass checks, lints, and tests. See the README's Contributing section and ask me questions to run these locally to prepare for a push. From what I can tell, none of these commits yet build or pass tests, perhaps because commits depend on changes from each other.

It seems like some changes like ee2ddfb are sufficient to stand alone as independent PRs to merge in short order.

To get some of this merged I recommend picking just one change out of the many split commits here, starting a fresh branch from master, and organizing the new commit message to follow the seven rules listed here. That'd let you model an ideal PR flow in the most minimal sense, in isolation. Then, I think you'd have an easier time packing a few of these remaining changes together in a PR with multiple commits for another more ambitious PR review and merge after.

[benalleng]

Changing to draft for now to have as an easy reference but will eventually plan to close in favor of separating these into more easily digested PRs

[spacebear21]

Closing this as it's being re-implemented in smaller PR chunks