Audit public API to take arguments by reference where possible#1373
Audit public API to take arguments by reference where possible#1373spacebear21 merged 2 commits intopayjoin:masterfrom
Conversation
Pull Request Test Coverage Report for Build 22587632286Details
💛 - Coveralls |
| INFO_B, | ||
| &mut OsRng, | ||
| )?; | ||
| let mut plaintext = plaintext.to_vec(); |
There was a problem hiding this comment.
The only part of this I screwed up was putting docs on half of this (encrypt_message_a) and not encrypt_message b. It could be doc'd as /// Takes &[u8]: HPKE pads and mutates internally, copy is unavoidable. but tbh I think passing as the mutable Vec may have been fine. @benalleng
There was a problem hiding this comment.
Well its nice to see some movement here. Hopefully we can finally take #525 out back and put it down 😆
decrypt_message_a and decrypt_message_b only borrow their secret-key arguments, so accept &HpkeSecretKey instead of owned HpkeSecretKey. Removes .clone() at every call site. Also make encrypt_message_a take `mut body` in the signature to match encrypt_message_b, dropping the `let mut body = body` rebind. Note: the entire hpke module is pub(crate), so none of these are public API changes.
The cert_der parameter is only passed to Certificate::from_der which borrows it, so accept &[u8] instead of Vec<u8>. Fix call sites in payjoin-cli and payjoin-test-utils accordingly. Other pub fns in io.rs and ohttp.rs keep ownership where consumed (ohttp_context, impl IntoUrl).
There was a problem hiding this comment.
Just a high-level question about function visibility is it reasonable to pub(crate) these functions instead of the whole module? I guess I'm not sure I understand the tradeoffs especially if someone or and AI is looking around they might mistake these for being actually public endpoints
There was a problem hiding this comment.
whole module pub(crate) is just less noisy and what I've seen as the idiom, then let the compiler enforce from there.
If docs.rs gets an accurate view of what the public API actually is, I'm sure we can feed that same info to a bot for linting. Just need the right setup and I didn't quite get there yet.
4 participants
Systematic LLM-driven audit of every
pub fninpayjoin/src/for parameters that take owned types where references would suffice, per the C-CALLER-CONTROL API guideline. to close #402Signature changes to two handfuls of functions.
Intentional ownership (kept as-is, with justification)
decrypt_message_b(receiver_pk: HpkePublicKey)-- consumed byOpModeR::Auth()ohttp_decapsulate(res_ctx: ClientResponse)--.decapsulate()consumes selffetch_ohttp_keys(impl IntoUrl, ...)--into_url()consumesTypestate documentation
All 28 typestate types derive
Clonedespite taking owned prior typestates, which is intentional for the v2 async protocol (sessions are serialized/deserialized across HTTP round-trips) but was previously undocumented. I chose not to document as understood, but maybe we ought to?Note: We could be using
#[must_use]on these Typestate transition methods and then enforcing that API invariant, too. Then compiler would then warn if any transition result is unused.Methodology
There was definitely more to the prompts than this but this was the core check.
Breaking changes
This changes public API signatures. The
Vec<u8>->&[u8]changes eliminate unnecessary allocations at call sites but require callers to update. Appropriate for pre-1.0 or a semver-major bump.Future enforcement
Rather than have manual review catch this every time, this is the sort of probabilistic lint we may be able to lean on LLMs for. I'm proposing an issue. I think we can add this one and then use the robot to check as we see repeated slips. I think it could hold us to this type of checklist with much less effort than a human reviewer following this checklist rust-bitcoin/rust-bitcoin#843.
Disclosure: Authored by Claude Code and reviewed by me.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.