docs: clarify send,receive function documentation by arminsabouri · Pull Request #407 · payjoin/rust-payjoin

arminsabouri · 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


 #[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

    }
 }

+/// 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.

arminsabouri · 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

    /// 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

arminsabouri · 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

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

DanGould requested changes Nov 26, 2024

View reviewed changes

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

arminsabouri 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

arminsabouri added 2 commits November 26, 2024 14:12

docs(send): add additional function docs

f664aa5

chore(receive): fix typos

3b0f0be

arminsabouri 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

arminsabouri 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

Closed

17 tasks

	/// 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

Conversation

arminsabouri commented Nov 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Nov 26, 2024 • 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!

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 Nov 26, 2024 • 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!

arminsabouri Nov 26, 2024 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Nov 26, 2024

Uh oh!

DanGould commented Nov 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Nov 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DanGould Nov 26, 2024 •

edited

Loading

DanGould Nov 26, 2024 •

edited

Loading

arminsabouri Nov 26, 2024 •

edited

Loading