Separate receive submodules to be additive #466

DanGould · 2025-01-06T22:47:32Z

kills the PjUriBuilder pattern which was a feature gating hell
make separate receive::{v1,v2} modules to enable error separation and additive use
Pass supported_versions from the calling module since that's where version comes from
Explicitly removes non-additive not(feature = "v2") so integration tests can all run on --all-features

We've got a ways to go to separate modules for readability, and then to make the features in payjoin-cli additive, but we're getting there.

This is work toward #392

This can be followed by separating the error variants into distinct receive, receive::v1, receive::v2 types and making the v1/v2 features in payjoin-cli additive

coveralls · 2025-01-06T22:51:26Z

Pull Request Test Coverage Report for Build 12676194397

Details

693 of 777 (89.19%) changed or added relevant lines in 6 files are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.3%) to 60.86%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/app/v2.rs	0	2	0.0%
payjoin/src/receive/error.rs	0	2	0.0%
payjoin/src/receive/optional_parameters.rs	5	7	71.43%
payjoin-cli/src/app/v1.rs	0	7	0.0%
payjoin/src/receive/v2/mod.rs	60	69	86.96%
payjoin/src/receive/v1.rs	628	690	91.01%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/uri/mod.rs	1	80.38%
payjoin/src/receive/v2/mod.rs	3	88.22%

Totals
Change from base Build 12674841563:	-0.3%
Covered Lines:	2886
Relevant Lines:	4742

💛 - Coveralls

DanGould · 2025-01-06T22:54:44Z

payjoin/src/receive/optional_parameters.rs

-pub(crate) const SUPPORTED_VERSIONS: [usize; 2] = [1, 2];
-#[cfg(not(feature = "v2"))]
-pub(crate) const SUPPORTED_VERSIONS: [usize; 1] = [1];
+pub(crate) const SUPPORTED_VERSIONS: &[usize] = if cfg!(feature = "v2") { &[1, 2] } else { &[1] };


I realize now that switching this on cfg is insufficient, since a compiled version may either be running a v1 or v2 receiver, depending on which functions are used. Those functions are going to have to pass supported versions instead. Perhaps there will be separate payjoin::v1, payjoin::v2 modules where you'd only have one of the two public in practice but v1 gets conditionally compiled as pub(crate) for use when v2 is active.

We may need a specific test for this, but I'm not even sure pjv2 uses this in practice

payjoin/src/receive/v2/mod.rs

spacebear21

There are a few other spots remaining that use not(feature = "v2"). In some cases I think it results in the same behavior but it would be more readable to replace those with feature = "v1")

spacebear21 · 2025-01-08T16:45:41Z

payjoin/src/receive/v2/mod.rs

-    // The contents of the `&pj=` query parameter.
-    // This identifies a session at the payjoin directory server.
-    pub fn pj_url(&self) -> Url {
+    /// The directory plus ShortID Url


Suggested change

/// The directory plus ShortID Url

/// The subdirectory for this Payjoin receiver session.

/// It consists of a directory URL and the session ShortID in the path.

spacebear21 · 2025-01-08T16:48:44Z

payjoin/src/receive/mod.rs

+pub fn build_v1_pj_uri<'a>(
+    address: &bitcoin::Address,
+    endpoint: &url::Url,
+    disable_output_substitution: bool,
+) -> crate::uri::PjUri<'a> {


Why not take an amount: Option<Amount> parameter here as well?

My thinking was that this could be done ad infinitum with every potential bip21 parameter (message, label, a non-exhaustive list) and we're better off only requiring the strictly necessary parameters

But there could be an argument for Option for convenience.

spacebear21 · 2025-01-08T16:59:42Z

payjoin/src/receive/v2/mod.rs


-    pub fn extract_v1_req(&self) -> String { self.inner.payjoin_psbt.to_string() }
+    pub fn extract_v1_req(&self) -> String { self.v1.psbt().to_string() }


Not directly related to this refactor, but I wonder if there's any benefit to having this function when callers could just use psbt().to_string() instead?

DanGould · 2025-01-08T17:19:56Z

There are a few other spots remaining that use not(feature = "v2"). In some cases I think it results in the same behavior but it would be more readable to replace those with feature = "v1")

I completely agree. I believe all of those missing spots are payjoin-cli, not the payjoin-crate, and the plan should absolutely be to make separate v1/v2 features and leave v2 as default in payjoin-cli

The builder pattern was feature gated and confusing when there were really only two distinct ways to build Uris: 1. From plain parameters in v1, from which to add amount, label, etc. 2. From a v2 receiver, from which to add amount, label, etc. amount, label, message, etc. can be set by mutating the bitcoin_uri::Uri struct directly, and we weren't enforcing anything special like pjos in the builder that we could not in the v2::Receiver directly. So we can delete code and have a simple interface that gets us closer to additive v1/v2 features.

These are distinct and need to be able to be switched for features to be additive.

This finally allows v1, v2 features to be additive. However, it does complicate the UnknownVersion variant (used only by v1 as far as I can tell). I'm inclined to leave this as an open issue until we separate the receive::{v1,v2} errors from each other.

This allows v1, v2 features to be additive and used side by side.

DanGould · 2025-01-08T17:36:26Z

requested changes applied

This function does NOT extract the request nor the complete body and is better represented by an direct call to the internal behavior.

DanGould · 2025-01-08T17:52:02Z

My last push included error separation that I would rather include in a follow up PR. fixed by force pushing ignoring that commit Should be in the same state as last review but including requested changes now.

spacebear21

ACK 5c78a15

DanGould changed the title ~~Separate receive~~ Separate receive submodules to be additive Jan 6, 2025

DanGould commented Jan 6, 2025

View reviewed changes

DanGould mentioned this pull request Jan 6, 2025

Rework receive::v2, send::v2 submodule into additive crate::v2 and crate::v1 submodules features #392

Open

6 tasks

DanGould commented Jan 7, 2025

View reviewed changes

payjoin/src/receive/v2/mod.rs Show resolved Hide resolved

DanGould force-pushed the separate-receive branch 2 times, most recently from 32efb5c to 9881815 Compare January 7, 2025 04:45

DanGould requested a review from spacebear21 January 8, 2025 03:58

DanGould mentioned this pull request Jan 8, 2025

Review usage of crate::receive::Error #312

Open

spacebear21 requested changes Jan 8, 2025

View reviewed changes

DanGould added 4 commits January 8, 2025 12:31

Separate receive::{v1,v2} modules

a20764f

These are distinct and need to be able to be switched for features to be additive.

Remove not(feature ="v2") in payjoin crate

149f40d

This allows v1, v2 features to be additive and used side by side.

DanGould force-pushed the separate-receive branch from 9881815 to 9b02a53 Compare January 8, 2025 17:35

DanGould requested a review from spacebear21 January 8, 2025 17:36

DanGould force-pushed the separate-receive branch from 9b02a53 to 5c78a15 Compare January 8, 2025 17:51

Remove unnecessary, misnamed fn extract_v1_req

5c78a15

This function does NOT extract the request nor the complete body and is better represented by an direct call to the internal behavior.

spacebear21 approved these changes Jan 8, 2025

View reviewed changes

spacebear21 merged commit eaf2398 into payjoin:master Jan 8, 2025
6 checks passed

DanGould mentioned this pull request Jan 8, 2025

Receiver::pj_url is missing fragment parameters #431

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate receive submodules to be additive #466

Separate receive submodules to be additive #466

DanGould commented Jan 6, 2025 •

edited

Loading

coveralls commented Jan 6, 2025 •

edited

Loading

DanGould Jan 6, 2025 •

edited

Loading

spacebear21 left a comment

spacebear21 Jan 8, 2025

spacebear21 Jan 8, 2025 •

edited

Loading

DanGould Jan 8, 2025 •

edited

Loading

spacebear21 Jan 8, 2025

DanGould commented Jan 8, 2025

DanGould commented Jan 8, 2025

DanGould commented Jan 8, 2025 •

edited

Loading

spacebear21 left a comment

	/// The directory plus ShortID Url
	/// The subdirectory for this Payjoin receiver session.
	/// It consists of a directory URL and the session ShortID in the path.


		pub fn extract_v1_req(&self) -> String { self.inner.payjoin_psbt.to_string() }
		pub fn extract_v1_req(&self) -> String { self.v1.psbt().to_string() }

Separate receive submodules to be additive #466

Separate receive submodules to be additive #466

Conversation

DanGould commented Jan 6, 2025 • edited Loading

coveralls commented Jan 6, 2025 • edited Loading

Pull Request Test Coverage Report for Build 12676194397

Details

💛 - Coveralls

DanGould Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

spacebear21 left a comment

Choose a reason for hiding this comment

spacebear21 Jan 8, 2025

Choose a reason for hiding this comment

spacebear21 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

DanGould Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

spacebear21 Jan 8, 2025

Choose a reason for hiding this comment

DanGould commented Jan 8, 2025

DanGould commented Jan 8, 2025

DanGould commented Jan 8, 2025 • edited Loading

spacebear21 left a comment

Choose a reason for hiding this comment

DanGould commented Jan 6, 2025 •

edited

Loading

coveralls commented Jan 6, 2025 •

edited

Loading

DanGould Jan 6, 2025 •

edited

Loading

spacebear21 Jan 8, 2025 •

edited

Loading

DanGould Jan 8, 2025 •

edited

Loading

DanGould commented Jan 8, 2025 •

edited

Loading