docs: clarify send,receive function documentation #407

0xBEEFCAF3 · 2024-11-25T18:12:17Z

Took some notes while I was reading the sender functions. Thought this could be useful for future contributors.
Also noticed that functions had docs but were not using doc comments ///. Let me know if that was an intentional decision, I can revert that. But doc comments are nice b/c my ide will ussually describe the function when I hover over it.

DanGould · 2024-11-26T15:34:09Z

payjoin/src/send/mod.rs

@@ -350,6 +355,7 @@ impl V1Context {

 #[cfg(feature = "v2")]
 pub struct V2PostContext {
+    /// The endpoint to send the request to. This is either a pj directory or a receiver's endpoint.


Suggested change

/// The endpoint to send the request to. This is either a pj directory or a receiver's endpoint.

/// The payjoin directory subdirectory to send the request to.

I may have misinformed you. This is always the payjoin directory subdirectory in this typestate. Only V1Context's endpoint is a V1 receiver's endpoint.

DanGould · 2024-11-26T15:34:52Z

payjoin/src/send/mod.rs

+    /// This is either the pj directory or the receiver's endpoint
    endpoint: Url,


Same comment as above. This is always the payjoin directory subdirectory to GET from

This still hasn't been addressed

DanGould · 2024-11-26T15:37:22Z

payjoin/src/send/mod.rs

+/// Ensure that the payee output spk is found the list of outputs only once.
+/// And that the amount is the same as in the original PSBT.


Prefer "output script" or "output scriptPubKey" to "output spk." In general less jargon makes for clearer communication. Refer to Murch's terminology BIP for unambiguous consistent vocabulary

This is checking in the build step not between PSBTs. Do you think "And that the amount is the same as in the original PSBT" is sufficiently clear?

Got it. Thats my misunderstanding.
rephrashing to "Ensure that the payee's output scriptPubKey appears in the list of outputs exactly once, and that the payee's output amount matches the requested amount."

DanGould · 2024-11-26T15:40:45Z

payjoin/src/send/mod.rs

@@ -763,6 +772,7 @@ fn clear_unneeded_fields(psbt: &mut Psbt) {
    }
 }

+/// Check that the output contributing to the fee is not less than the fee.


Suggested change

/// Check that the output contributing to the fee is not less than the fee.

/// Ensure that an additional fee output is sufficient to pay for the specified additional fee

DanGould · 2024-11-26T15:59:30Z

payjoin/src/send/mod.rs

+/// Find the sender's change output.
+/// The first output should always be the payee spk.
+/// If the fee contribution is clamped, any non-payee spk can be the change output.


Suggested change

/// Find the sender's change output.

/// The first output should always be the payee spk.

/// If the fee contribution is clamped, any non-payee spk can be the change output.

/// Find the sender's change output index by eliminating the payee's as a candidate.

@spacebear21 "The first output should always be the payee script" screams privacy leak to me since receivers maintain relative ordering. I think this comment is wrong but please help me double check. If that is the behavior, must randomize this to address the issue. However, I believe the second line of the proposed comment is actually wrong since later in the function we check for the payee with .find() in the case of 2 outputs. This docstring suggests a readability refactor since the unit type return branch (2, _) => (), was misread to produce this docstring. An inline comment would help fix the misread.

I do NOT believe clamped fee contribution suggests which script can be a change output either. Rather, it ensures that in the case that a SenderBuilder-parameterized additional fee output can't pay for the parameterized max additional fee contribution, then the SenderBuilder will return a Sender with a decreased additional fee contribution rather than producing an error.

The first output should always be the payee script

This seems to only be true if there is no second output. Which makes sense as then we only have one output and that has to the the payee spk

payjoin/src/send/mod.rs

DanGould · 2024-11-26T16:06:37Z

payjoin/src/send/mod.rs

+/// Checks that the change output index is valid
+/// and that the amount is not less than the fee we are suppose to
+/// be contributing.


Suggested change

/// Checks that the change output index is valid

/// and that the amount is not less than the fee we are suppose to

/// be contributing.

/// Check that the change output index is not out of bounds

/// and that the additional fee contribution is not less than specified.

Use imperative mood.

Saying "and that the additional fee contribution is not less than specified" covers both amount and clamp parameters.

The "and..." suggests to me the name is incorrect (should be check_change) but that's a nit and suggests the two *_change_index calls in determine_fee_contribution could both be named *_change instead. But this is a major nit and does not actually need to change.

0xBEEFCAF3 · 2024-11-26T17:35:07Z

@DanGould snuck in one more commit (9a73fa2) fixing some typos I found

DanGould · 2024-11-26T17:42:26Z

Also, this isn't this definitely docs(x) in conventional commits parlance

DanGould · 2024-11-26T18:20:38Z

payjoin/src/receive/v2/mod.rs

@@ -237,7 +237,7 @@ impl UncheckedProposal {
    ///
    /// Receiver MUST check that the Original PSBT from the sender
    /// can be broadcast, i.e. `testmempoolaccept` bitcoind rpc returns { "allowed": true,.. }
-    /// for `extract_tx_to_sheculed_broadcast()` before calling this method.
+    /// for `extract_tx_to_scheduled_broadcast()` before calling this method.


Should be extract_tx_to_schedule_broadcast

DanGould

ACK 3b0f0be

Many thanks for persisting with me to get all these details right

0xBEEFCAF3 · 2024-11-26T19:49:13Z

ACK 3b0f0be

Many thanks for persisting with me to get all these details right

Yeah you got it. Thanks for the review

0xBEEFCAF3 requested review from DanGould and spacebear21 November 25, 2024 18:12

DanGould requested changes Nov 26, 2024

View reviewed changes

0xBEEFCAF3 force-pushed the arm/chore/doc-comments branch from fc3afc6 to 9a73fa2 Compare November 26, 2024 17:34

0xBEEFCAF3 force-pushed the arm/chore/doc-comments branch 2 times, most recently from 0b0e2c3 to 86fb9e1 Compare November 26, 2024 17:46

DanGould reviewed Nov 26, 2024

View reviewed changes

0xBEEFCAF3 added 2 commits November 26, 2024 14:12

docs(send): add additional function docs

f664aa5

chore(receive): fix typos

3b0f0be

0xBEEFCAF3 force-pushed the arm/chore/doc-comments branch from 86fb9e1 to 3b0f0be Compare November 26, 2024 19:13

DanGould approved these changes Nov 26, 2024

View reviewed changes

DanGould changed the title ~~chore(send): additional function docs~~ docs: clarify send,receive function documentation Nov 26, 2024

DanGould merged commit 0148630 into payjoin:master Nov 26, 2024
6 checks passed

0xBEEFCAF3 deleted the arm/chore/doc-comments branch November 26, 2024 19:48

DanGould mentioned this pull request Dec 3, 2024

Release 0.21.0 #390

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: clarify send,receive function documentation #407

docs: clarify send,receive function documentation #407

0xBEEFCAF3 commented Nov 25, 2024

DanGould Nov 26, 2024

DanGould Nov 26, 2024 •

edited

Loading

DanGould Nov 26, 2024

DanGould Nov 26, 2024

DanGould Nov 26, 2024

0xBEEFCAF3 Nov 26, 2024

DanGould Nov 26, 2024

DanGould Nov 26, 2024 •

edited

Loading

0xBEEFCAF3 Nov 26, 2024 •

edited

Loading

DanGould Nov 26, 2024

0xBEEFCAF3 commented Nov 26, 2024

DanGould commented Nov 26, 2024

DanGould Nov 26, 2024

DanGould left a comment

0xBEEFCAF3 commented Nov 26, 2024

	/// The endpoint to send the request to. This is either a pj directory or a receiver's endpoint.
	/// The payjoin directory subdirectory to send the request to.

		/// This is either the pj directory or the receiver's endpoint
		endpoint: Url,

		/// Ensure that the payee output spk is found the list of outputs only once.
		/// And that the amount is the same as in the original PSBT.

	/// Check that the output contributing to the fee is not less than the fee.
	/// Ensure that an additional fee output is sufficient to pay for the specified additional fee

docs: clarify send,receive function documentation #407

docs: clarify send,receive function documentation #407

Conversation

0xBEEFCAF3 commented Nov 25, 2024

Choose a reason for hiding this comment

DanGould Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

0xBEEFCAF3 Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xBEEFCAF3 commented Nov 26, 2024

DanGould commented Nov 26, 2024

Choose a reason for hiding this comment

DanGould left a comment

Choose a reason for hiding this comment

0xBEEFCAF3 commented Nov 26, 2024

DanGould Nov 26, 2024 •

edited

Loading

DanGould Nov 26, 2024 •

edited

Loading

0xBEEFCAF3 Nov 26, 2024 •

edited

Loading