Clarify request construction methods for sender and receiver #814

0xZaddyy · 2025-06-27T14:01:56Z

Refactor: Clarify request construction methods for sender and receiver

closes #813

This PR focuses on improving method naming consistency across the sender and receiver components, particularly around request creation. These changes help better reflect the behavior of these methods and align with OHTTP semantics

✅ Completed
1. Renamed extract_v1 → create_v1_post_request
2. Renamed extract_v2 → create_v2_post_request
3. Renamed extract_req → create_poll_request across receiver typestates
4. Updated all extract_req call sites.
5. Renamed process_res → process_response where applicable for consistency

TODO

Rename internal subdir/subdirectory terminology to mailbox for clarity and consistency
Improve or add doc comments for renamed methods

Let me know if this direction looks good, i'll be happy to adjust.

nothingmuch

Looks good, fairly straightforward.

I'll ACK after my suggestion, but before merging this should at least get an additional CACK, since I proposed the names.

The CHANGELOG.md modification suggests find and replace was used, but for what it's worth rust-analyzer and ast-grep support doing renaming a bit more conservatively than textual find & replace, let me know if you need help setting these tools up (ping on discord or something).

nothingmuch · 2025-06-28T10:58:41Z

payjoin/CHANGELOG.md

+- Shorten mailbox IDs to 64 pseudorandom bits [#386](https://github.com/payjoin/rust-payjoin/pull/386)
 - Clarify send and receive module documentation [#407](https://github.com/payjoin/rust-payjoin/pull/407)
 - Pad ohttp messages to consistent 8192 bytes [#395](https://github.com/payjoin/rust-payjoin/pull/395)
- encode subdirectory IDs in bech32 and other QR optimizations [#417](https://github.com/payjoin/rust-payjoin/pull/417)
+- encode mailbox IDs in bech32 and other QR optimizations [#417](https://github.com/payjoin/rust-payjoin/pull/417)


This was already released, so i don't think we should change it retroactively

nothingmuch · 2025-06-28T11:06:57Z

Make Sender::endpoint() private if no valid external use cases exist

Group typestate structs into an internal module to reduce cargo doc clutter

Improve or add doc comments for renamed methods

IMO these can and should be separate PRs

flake.nix

payjoin/src/send/multiparty/mod.rs

coveralls · 2025-06-28T16:45:42Z

Pull Request Test Coverage Report for Build 16272674154

Details

63 of 66 (95.45%) changed or added relevant lines in 8 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.02%) to 85.714%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-directory/src/lib.rs	6	7	85.71%
payjoin-cli/src/app/v2/mod.rs	7	9	77.78%

Totals
Change from base Build 16271409661:	-0.02%
Covered Lines:	7614
Relevant Lines:	8883

💛 - Coveralls

0xZaddyy · 2025-06-28T21:59:04Z

Make Sender::endpoint() private if no valid external use cases exist

Group typestate structs into an internal module to reduce cargo doc clutter

Improve or add doc comments for renamed methods

IMO these can and should be separate PRs

Sounds good, I’ll follow up with a separate PR to address:

Making Sender::endpoint() private (if no valid external use case exists)
Grouping typestate structs into an internal module to reduce Cargo doc clutter

nothingmuch

Solid progress. I have a number of new suggestions, some things I didn't catch in the previous round.

I didn't comment in all places where these comments apply, so please generalize to other places

If you want to do comprehensive doc comment changes maybe it's better to make a PR based on or even into #769, or just do a test merge to see if any conflicts are created. Either way feel free to skip those suggestions in this one, and we can open an issue instead.

payjoin-ffi/src/receive/mod.rs

payjoin-ffi/src/receive/uni.rs

payjoin/src/receive/v2/mod.rs

DanGould · 2025-07-02T20:06:07Z

Apart from the specific comments I've made, I think the docs for all of the v2 typestates, e.g.
/// Foo
pub struct Foo {
...
should all be moved to the corresponding impl block:
/// Foo
impl Receiver<Foo> {
...
because that way they will be displayed next to the methods that they relate to in the Receiver docs.

@nothingmuch I don't think this is right, because impl block docstrings don't make it into the generated docs. I think the original method was correct for generating documentation that makes it into docs.rs.

DanGould · 2025-07-02T20:28:12Z

My last comment is just wrong. It just shows up in the impl block and not the structs. Doh!

DanGould · 2025-07-02T20:30:19Z

It also looks like you might need help rebasing. You don't want to create a merge commit, you want to fetch out #769, get it on a local branch called doc-update and then checkout this branch Renaming-changes, from which you should be able to git rebase doc-update and these commits will be applied on top. you can git rebase -i doc-update from Renaming-changes if you want to interactively apply the changes.

I'd do the same with master(first). git rebase master from the Renaming-changes branch. That will apply each commit in Renaming-changes on top of the current state of master. make sure master is up to date before doing this rebase.

0xZaddyy · 2025-07-02T20:32:34Z

It also looks like you might need help rebasing. You don't want to create a merge commit, you want to fetch out #769, get it on a local branch called doc-update and then checkout this branch Renaming-changes, from which you should be able to git rebase doc-update and these commits will be applied on top. you can git rebase -i doc-update from Renaming-changes if you want to interactively apply the changes.

I'd do the same with master(first). git rebase master from the Renaming-changes branch. That will apply each commit in Renaming-changes on top of the current state of master. make sure master is up to date before doing this rebase.

Thanks Dan, i'll do this now

DanGould · 2025-07-09T00:40:29Z

What happened here, did the commit get squashed into Efe's rather than rebased on top of it? If you need help with a rebase we can get on a call some afternoon Easter US time to show you how. Efe's commit will not require a rebase of its own.

DanGould

My only real issue, which was at first a nit but did appear twice, is that the word 'extract' for PSBT extraction was replaced with 'constructs' which I believe is in error. I would like to see that reverted.

DanGould · 2025-07-10T19:40:52Z

payjoin-ffi/python/test/test_payjoin_integration_test.py

            # **********************
            # Inside the Sender:
-            # Sender checks, signs, finalizes, extracts, and broadcasts
+            # Sender checks, signs, finalizes, constructs, and broadcasts


extraction here refers to PSBT transaction extraction and the sender does not do PSBT construction in this step. I'm guessing this is a search and replace straggler.

I'd rather revert than change this line.

DanGould · 2025-07-10T19:44:10Z

payjoin/src/core/receive/v2/mod.rs

+fn short_id_from_pubkey(pubkey: &HpkePublicKey) -> ShortId {
    sha256::Hash::hash(&pubkey.to_compressed_bytes()).into()
 }


Since this crate defines HpkePublicKey and ShortId in the public interface I think it would be considered better practice to define impl From<HpkePublicKey> for ShortId than have this one-off function.

This is a follow-up concern since you did not define this function, you're just renaming it.

Disagree, From consumes, and is supposed to be an information preserving conversion, but hashing and truncating is definitely lossy

Maybe payjoin should define an extension trait for HpkePublicKey instead?

Where have you seen information suggesting the following?

From consumes, and is supposed to be an information preserving conversion

I have not heard of this convention before. I thought it was just an infallible conversion

DanGould · 2025-07-10T19:47:00Z

payjoin/src/core/receive/v2/session.rs

-    /// Extract the error request to be posted on the directory if an error occurred.
+    /// Construct the error request to be posted on the directory if an error occurred.
    /// To process the response, use [crate::receive::v2::process_err_res]
    pub fn extract_err_req(


did you want to leave the name of this one extract on purpose? ok with me just curious.

DanGould · 2025-07-10T19:47:51Z

payjoin/tests/integration.rs

            // **********************
            // Inside the Sender:
-            // Sender checks, signs, finalizes, extracts, and broadcasts
+            // Sender checks, signs, finalizes, constructs, and broadcasts


Again, "extracts" is correct here, referring to PSBT transaction extraction. Construction is not.

Feel free to specify "The PSBT" in both of these comments if you find that helpful for clarity.

DanGould · 2025-07-14T15:03:04Z

payjoin-ffi/python/test/test_payjoin_integration_test.py

+
+

Were these whitespace changes included in prior reviews of this PR? They seem unrelated. I might just remove them before merging to move this along

Typically these belong in their own commit

DanGould · 2025-07-14T16:38:55Z

payjoin-ffi/python/test/test_payjoin_integration_test.py

-    
+


There are still quite a few of these stragglers I'll take care of

- Rename `extract_v1` to `create_v1_post_request` to better reflect behavior - Rename `extract_v2` to `create_v2_post_request` for naming consistency - Rename `extract_req` to `create_poll_request` across receiver typestates - Rename all internal uses of `subdir`/`subdirectory` to `mailbox` - Includes variables, function names, and documentation comments

Co-authored-by: oyindamola oladapo <oyinoladapo5@gmail.com>

DanGould

ACK 5ec16de

nothingmuch requested changes Jun 28, 2025

View reviewed changes

nothingmuch reviewed Jun 28, 2025

View reviewed changes

flake.nix Outdated Show resolved Hide resolved

nothingmuch reviewed Jun 28, 2025

View reviewed changes

payjoin/src/send/multiparty/mod.rs Outdated Show resolved Hide resolved

0xZaddyy force-pushed the Renaming-changes branch 5 times, most recently from 5475825 to 687fcf9 Compare June 28, 2025 16:42

0xZaddyy force-pushed the Renaming-changes branch from 687fcf9 to b1a566b Compare June 28, 2025 17:45

0xZaddyy requested a review from nothingmuch June 28, 2025 17:47

0xZaddyy force-pushed the Renaming-changes branch 6 times, most recently from 22cc794 to 0e65fff Compare June 28, 2025 20:53

0xZaddyy marked this pull request as ready for review June 28, 2025 21:46

0xZaddyy force-pushed the Renaming-changes branch from 0e65fff to 369fc55 Compare June 28, 2025 21:49

nothingmuch requested changes Jun 28, 2025

View reviewed changes

0xZaddyy force-pushed the Renaming-changes branch 6 times, most recently from 5269a2f to 42eefa2 Compare June 29, 2025 04:15

0xZaddyy requested a review from nothingmuch June 29, 2025 04:30

0xZaddyy force-pushed the Renaming-changes branch from 42eefa2 to 900ac63 Compare June 29, 2025 11:06

0xZaddyy force-pushed the Renaming-changes branch from 9f1e0df to eee58e0 Compare July 2, 2025 19:49

0xZaddyy force-pushed the Renaming-changes branch 2 times, most recently from e63c9d6 to 330aa35 Compare July 2, 2025 20:26

0xZaddyy force-pushed the Renaming-changes branch from 330aa35 to 7b3b94b Compare July 2, 2025 20:28

0xZaddyy force-pushed the Renaming-changes branch 2 times, most recently from 812cf8d to 48927d8 Compare July 3, 2025 12:27

0xZaddyy requested a review from nothingmuch July 3, 2025 12:46

0xZaddyy force-pushed the Renaming-changes branch 2 times, most recently from 6b43f4a to 3871e81 Compare July 3, 2025 15:41

0xZaddyy force-pushed the Renaming-changes branch 4 times, most recently from d183d6b to 5278124 Compare July 10, 2025 18:15

nothingmuch approved these changes Jul 10, 2025

View reviewed changes

DanGould requested changes Jul 10, 2025

View reviewed changes

0xZaddyy force-pushed the Renaming-changes branch 2 times, most recently from bad5bef to 05060b3 Compare July 11, 2025 23:59

DanGould reviewed Jul 14, 2025

View reviewed changes

0xZaddyy force-pushed the Renaming-changes branch from 05060b3 to edb500b Compare July 14, 2025 15:34

DanGould reviewed Jul 14, 2025

View reviewed changes

0xZaddyy and others added 2 commits July 14, 2025 12:40

Remove errant whitespace

5ec16de

Co-authored-by: oyindamola oladapo <oyinoladapo5@gmail.com>

DanGould force-pushed the Renaming-changes branch from edb500b to 5ec16de Compare July 14, 2025 16:42

DanGould approved these changes Jul 14, 2025

View reviewed changes

DanGould merged commit b9f19f3 into payjoin:master Jul 14, 2025
15 checks passed

Clarify request construction methods for sender and receiver #814

Clarify request construction methods for sender and receiver #814

Uh oh!

Conversation

0xZaddyy commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refactor: Clarify request construction methods for sender and receiver

TODO

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

nothingmuch Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

nothingmuch commented Jun 28, 2025

Uh oh!

Uh oh!

Uh oh!

coveralls commented Jun 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16272674154

Details

💛 - Coveralls

Uh oh!

0xZaddyy commented Jun 28, 2025

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanGould commented Jul 2, 2025

Uh oh!

DanGould commented Jul 2, 2025

Uh oh!

DanGould commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xZaddyy commented Jul 2, 2025

Uh oh!

DanGould commented Jul 9, 2025

Uh oh!

DanGould left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

nothingmuch Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

0xZaddyy commented Jun 27, 2025 •

edited

Loading

coveralls commented Jun 28, 2025 •

edited

Loading

DanGould commented Jul 2, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading

DanGould Jul 10, 2025 •

edited

Loading

DanGould Jul 10, 2025 •

edited

Loading

DanGould Jul 14, 2025 •

edited

Loading

DanGould Jul 10, 2025 •

edited

Loading

DanGould Jul 14, 2025 •

edited

Loading