Update receiver typestate documentation for functions shared between v1 and v2 (and some InputPair documentation) #769

mehmetefeumit · 2025-06-13T22:41:42Z

I am reading through the PDK code base, and thought that one of the better ways I could contribute and learn at the same time would be to update the documentation so that it is easier for both people who're reading through the typestate progression and AI models who are doing the same.

This has a focus on the public functions so that documentation is detailed and up-to-date. I'll do the same for the sender when I read through that, and also want to make sure that session context and other parts (i.e. struct fields, etc.) also have a baseline documentation which posterity can build upon.

Do let me know if there are parts which you think are to detailed, can be refined, or simply incorrect, even though I tried to make sure that the latter will unlikely be the case :)

The updates in v1 and v2 are exactly the same.

coveralls · 2025-06-13T22:46:33Z

Pull Request Test Coverage Report for Build 16186272882

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 85.558%

Totals
Change from base Build 16173926139:	0.0%
Covered Lines:	7571
Relevant Lines:	8849

💛 - Coveralls

nothingmuch

This needs an update after #750 was merged, since a lot has changed.

I especially appreciate that the tone/phrasing is consistent, that helps reduce mental overhead when parsing docs, trying to figure out what something is for, this is a welcome change. In some places perhaps it goes a bit far, e.g. "and moves on to the next typestate" is mostly self evident if the typestate pattern is itself mentioned and briefly described (and with Receiver now being generic over that as in the usual convention, there's a logical place to document that).

Some nits: there's a few capitalization or formatting nits, e.g. TxIn vs. txin, or scriptPubKey vs. scriptpubkey which IMO can help readability a bit. Things like the numbered list of steps on finalize_proposal could also use markdown list formatting.

mehmetefeumit · 2025-06-26T19:25:44Z

@nothingmuch Made the changes so that reference to PSBT Input vs. TxIn are clearer and removed all of the redundant documentation.

I added a new docstring for the new Receiver construct as well, but not touching the session-related v2 specific functions for now to keep the scope of the PR smaller. I'm going to do the same for OHTTP/session related functions, and also for the sender functions/structs later.

nothingmuch

Thanks (again!) for such a valuable contribution. IMO this is an important PR, I hope others also review it with the care it deserves.

I posted a few comments on v1 stuff that also apply to the corresponding v2 methods.

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.

payjoin/src/receive/mod.rs

payjoin/src/receive/v1/mod.rs

nothingmuch · 2025-06-27T15:08:29Z

payjoin/src/receive/v1/mod.rs

+    /// Additionally, this function also checks whether the sender did not point to an output
+    /// which pays to the receiver when specifying an output the receiver can reduce the value of
+    /// to increase the fees the transaction pays. If the parameter does point to a output which
+    /// pays to the receiver, this function sets the parameter to None before letting the receive
+    /// process continue to the next steps, protecting the receiver from accidentally subtracting fees
+    /// from their own outputs.


It took me a few tries to parse the compound sentence opening this paragraph, but I'm not sure how to phrase it more simply... @DanGould how would you phrase this?

@nothingmuch Changed this to be more readable in the current version of the PR.

payjoin/src/receive/v1/mod.rs

payjoin/src/receive/v2/mod.rs

nothingmuch · 2025-06-27T16:35:01Z

crap, i absentmindedly pressed the green button instead of the scary click-outside-the-dialog-which-feels-like-it'd-lose-typed-text and ended up posting my review prematurely... i'm not fully done with the v2 side, so i may post additional comments. sorry!

nothingmuch

Sorry for not re-reviewing in a timely manner after the last changes, I added a merge commit resolving the conflict but honestly I think you would have better judgement about how to integrate these two concerns (using the API vs. the OHTTP warning) into the top level module docs.

Other than that I think this is good to go, if my conflict resolution is acceptable then it should just be rebased out of existence since that extra merge commit just adds noise.

@DanGould, @arminsabouri for consistency not only in the API but also the docs? IMO having the state machine enumerated under the main struct in the "implementations" subsection is much more intuitive and browsable, the difference between the v1 and the v2 docs on this branch is pretty stark IMO

nothingmuch · 2025-07-02T18:36:59Z

to be clear if we change v1 to also use the generics based typestate pattern and update the docs, that should be in a followup issue & PR, not this one

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

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.

I think the original was right, because there is no documentation for Receiver<Foo> that gets generated for docs.rs. I'm reviewing now and comfortable moving this forward as-is, but would want those docs replaced on pub struct Foo so that our generation is right in a follow up.

DanGould · 2025-07-02T20:27:50Z

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

DanGould

I sincerely appreciate this effort. it improves the readability significantly.

I've got some changes to request. This would be easier to rebase/review if it were split into modules, not sure if that's possible. This isn't too far off though.

Just noting: We may want some sort of script/macro (outside of rust, by necessity) to add these docs to payjoin-ffi eventually.

DanGould · 2025-07-02T20:11:26Z

payjoin/src/core/psbt/mod.rs

            .ok_or(AddressTypeError::UnknownAddressType)
    }

+    /// Returns the expected weight of this input based on the address type of the UTXO it is pointing to.


This is true at the time of writing, but will need to change when #583 is addressed to handle weight to specify that additional info other than the address type is used to describe it.

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

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

+/// interactive (where the receiver takes manual actions to respond to the
+/// payjoin proposal) or a non-interactive (ex. a donation page which automatically generates a new QR code
+/// for each visit) payment receiver. For the latter, you should call [`Self::check_broadcast_suitability`] to check


the word "interactive" bugs me because all payjoin is interactive. You can tell this vocab is insufficient because it requries 2 parentheticals to explain. I think "automated" or "automatic" payment processor / receiver is more descriptive. however, this is the language of bip 78 and so I'm ok leaving it as-is in our docs for now.

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

DanGould · 2025-07-02T20:58:33Z

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

+//! This Payjoin v2 module, in addition to what is described above, also provides tools to leverage
+//! Oblivious HTTP to make collaboratively building a Payjoin transaction asynchronous between the


OHTTP does not make payjoin asynchronous, the store-and-forward directory server does.

Yeah not sure why I made that connection there. I'll give a better definition for both OHTTP and the store-and-forward directory while revising this.

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

DanGould · 2025-07-02T21:08:02Z

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

-/// A checked proposal that the receiver may contribute inputs to to make a payjoin
-///
-/// Call [`Receiver<WantsOutputs>::commit_inputs`] to proceed.


I'm skeptical that we want to delete the original sentence. Seems like some documentation is better than none here, even if the Call [\Receiver::commit_input`] to proceed.` typestate info is moved to the impl

We want documentation on both the structs and the Receiver<...> implementation definitions? I can add that if we do, but currently I'm just moving the entire thing as is (with minor modifications) to impl Receiver<WantsInputs>.

ah my mistake. I meant the first sentence of the original

Cool! There are some modifications with that one, but the theme of moving all documentation from the structs is common in the commit. Are you saying that we should still keep the documentation in both the implementation and the struct itself, or something else? I want to make sure that I understood correctly :D

I cleaned up the PR with the rest of the changes and will update the remote branch soon and re-publish the PR.

I think having something on the State struct is better than nothing. What you've done moving to impls is more appropriate but I'd rather not completely delete the state docs.

Happy to merge what you have for now and follow up after the main module comment about OHTTP being used for async (it's not, as you recognized) is fixed

Done! All changes are added 👍🏻

DanGould

A cursory look suggests that my concerns were addressed and that this is ready to go. However it requires a rebase.

mehmetefeumit · 2025-07-10T16:48:01Z

@DanGould Conflict resolved!

nothingmuch

reACK

DanGould

ACK ac341b7

nothingmuch · 2025-07-10T17:31:31Z

I edited the PR text, removing some stuff that no longer applies, I hope you don't mind @mehmetefeumit

mehmetefeumit force-pushed the doc-update branch from 4f9fe46 to 0ddb4b7 Compare June 13, 2025 22:58

nothingmuch requested changes Jun 20, 2025

View reviewed changes

DanGould force-pushed the master branch from 61351cb to 9f403f1 Compare June 20, 2025 20:17

mehmetefeumit force-pushed the doc-update branch 2 times, most recently from c1a143c to 7b1caef Compare June 26, 2025 19:14

mehmetefeumit force-pushed the doc-update branch from 7b1caef to 63df38b Compare June 26, 2025 21:30

nothingmuch requested changes Jun 27, 2025

View reviewed changes

mehmetefeumit force-pushed the doc-update branch 3 times, most recently from 53c7551 to 8e57c2c Compare June 28, 2025 20:22

nothingmuch mentioned this pull request Jun 28, 2025

Clarify request construction methods for sender and receiver #814

Merged

2 tasks

mehmetefeumit force-pushed the doc-update branch 2 times, most recently from 230399c to 205de56 Compare June 29, 2025 05:32

nothingmuch approved these changes Jul 2, 2025

View reviewed changes

DanGould requested changes Jul 2, 2025

View reviewed changes

mehmetefeumit marked this pull request as draft July 6, 2025 23:53

mehmetefeumit force-pushed the doc-update branch from e9497f5 to 4741f1f Compare July 7, 2025 05:49

mehmetefeumit marked this pull request as ready for review July 7, 2025 05:54

mehmetefeumit requested a review from DanGould July 8, 2025 05:28

DanGould requested changes Jul 9, 2025

View reviewed changes

mehmetefeumit added 2 commits July 9, 2025 21:32

Add documentation for InputPair functions

ea6ab29

Update receiver function documentation for version-agnostic functions

ac341b7

mehmetefeumit force-pushed the doc-update branch from 4741f1f to ac341b7 Compare July 10, 2025 04:38

nothingmuch approved these changes Jul 10, 2025

View reviewed changes

DanGould approved these changes Jul 10, 2025

View reviewed changes

nothingmuch merged commit 6d1be60 into payjoin:master Jul 10, 2025
9 checks passed

nothingmuch mentioned this pull request Jul 10, 2025

cargo doc improvements tracking issue #865

Open

18 tasks

		//! This Payjoin v2 module, in addition to what is described above, also provides tools to leverage
		//! Oblivious HTTP to make collaboratively building a Payjoin transaction asynchronous between the

Update receiver typestate documentation for functions shared between v1 and v2 (and some InputPair documentation) #769

Update receiver typestate documentation for functions shared between v1 and v2 (and some InputPair documentation) #769

Uh oh!

Conversation

mehmetefeumit commented Jun 13, 2025

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 16186272882

Details

💛 - Coveralls

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

mehmetefeumit commented Jun 26, 2025

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nothingmuch commented Jun 27, 2025

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

nothingmuch commented Jul 2, 2025

Uh oh!

DanGould commented Jul 2, 2025

Uh oh!

DanGould commented Jul 2, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

mehmetefeumit commented Jul 10, 2025

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

coveralls commented Jun 13, 2025 •

edited

Loading