Introduce Payjoin version enum by shinghim · Pull Request #668 · payjoin/rust-payjoin

shinghim · 2025-04-25T15:24:02Z

Fixes #643

coveralls · 2025-04-25T15:31:52Z

Pull Request Test Coverage Report for Build 14868573150

Details

20 of 28 (71.43%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.03%) to 81.995%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/version.rs	0	8	0.0%

Totals
Change from base Build 14867604163:	-0.03%
Covered Lines:	5442
Relevant Lines:	6637

💛 - Coveralls

payjoin/src/receive/optional_parameters.rs

DanGould · 2025-04-25T16:57:13Z

have you considered an explicit discriminant enum? I can't imagine our Version enum would ever have fields unless we did something wonky to have sub-versions.

Something like the following is what I was thinking about. I'm not suggesting a change per se but trying to understand the rationale for one over the other if there was one

#[repr(u8)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum Version {
    One = 1,
    Two = 2,
}

shinghim · 2025-04-26T13:28:09Z

@DanGould I hadn't considered it, but I think it makes more sense to have it as an explicit discriminant enum so we don't need the extra logic to convert to a usize. I'm not too familiar with the #[repr(u8)] bit but did some reading, and it looks like it'll be necessary since it might get included in the ffi work

payjoin/src/version.rs

DanGould

I'm glad you picked this up and wrote some tests. Thank you.

I wonder which branches aren't tested making our code coverage go down.

I know we use some differing BIP XX/BIP-XX/BIPXX formatting in this repo but at least keep them consistent in one PR please

payjoin/src/version.rs

payjoin/src/receive/optional_parameters.rs

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

DanGould · 2025-04-30T15:02:02Z

payjoin/Cargo.toml

 default = ["v2"]
 #[doc = "Core features for payjoin state machines"]
-_core = ["bitcoin/rand-std", "serde_json", "url", "bitcoin_uri"]
+_core = ["bitcoin/rand-std", "serde_json", "url", "bitcoin_uri", "serde"]


Is this change strictly necessary? for Version compilation i.e. all _core-enabled compilations? If so, shouldn't the redundant explicit feature be removed from v2 = ["serde"...] be removed?

it's needed for Serialize - I'll remove the redundant feature in v2

And is that serde::Serialize required for the v1 feature used exclusively?

From BIP-77:

HPKE binds ciphertexts to application-specific info strings. Because
of this domain separation, BIP 78's v parameter is redundant and
should be omitted for version 2.

If we aren't using the v parameter, we can probably keep it to v1 exclusively

I'm not sure our code reflects that suggested omission yet, and it also seems based on your later comment that serde is still required for the error. If for some reason that error that serializes a version array never gets hit, v1 exclusivity would be great. If not, it's ok to keep in core.

payjoin/src/version.rs

Closes #647 Create a new `core` module since there isn't a module to put common/core stuff. The new Version enum in #668 could also be put into this mod, among other things

shinghim · 2025-05-01T04:36:04Z

Also moved Version into the new core module

DanGould

ser/de behavior, both having a serializer/deserializer and their feature gated applicability are my only remaining concerns

DanGould · 2025-05-05T19:06:31Z

payjoin/src/core/version.rs

+impl Serialize for Version {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        (*self as u8).serialize(serializer)
+    }
+}


is this not missing a matching deserialize impl?

~~you're right, i'll add it~~ actually, i'm not seeing anywhere it's being used, unlike Serialize, which is being used here:

rust-payjoin/payjoin/src/receive/error.rs

Line 220 in eb26494

serde_json::to_string(supported_versions).unwrap_or_default();

is it okay to leave out until we are actually using it?

Ah! very well.

In this case, please document the omission and design decisions on the Version type itself. That is, both the docstring on impl fmt::Display (which right now isn't actually linked to Version::fmt because it comes before the impl block), and notes about what is implemented need to go on the Version enum itself and then this is golden.

/// The Payjoin version /// /// From [BIP 78](https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#optional-parameters), /// the supported versions should be output as numbers: /// /// ```json /// { /// "errorCode": "version-unsupported", /// "supported" : [ 2, 3, 4 ], /// "message": "The version is not supported anymore" /// } /// ``` /// /// # Note /// - Only [`Serialize`] is implemented for json array serialization in the `unsupported-version` error message, /// not [`serde::Deserialize`], as deserialization is not required for this type. /// - [`fmt::Display`] and [`fmt::Debug`] output the `u8` representation for compatibility with BIP 78/77 /// and to match the expected wire format.

DanGould

Thanks for the clarifications. I noticed one more thing. Version should be pub(crate) in lib.rs.

And please put the documentation on the Version type itself so that it's discoverable.

DanGould · 2025-05-06T15:02:38Z

payjoin/Cargo.toml

 default = ["v2"]
 #[doc = "Core features for payjoin state machines"]
-_core = ["bitcoin/rand-std", "serde_json", "url", "bitcoin_uri"]
+_core = ["bitcoin/rand-std", "serde_json", "url", "bitcoin_uri", "serde"]


I'm not sure our code reflects that suggested omission yet, and it also seems based on your later comment that serde is still required for the error. If for some reason that error that serializes a version array never gets hit, v1 exclusivity would be great. If not, it's ok to keep in core.

DanGould · 2025-05-06T15:15:09Z

payjoin/src/core/version.rs

+impl Serialize for Version {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        (*self as u8).serialize(serializer)
+    }
+}


Ah! very well.

In this case, please document the omission and design decisions on the Version type itself. That is, both the docstring on impl fmt::Display (which right now isn't actually linked to Version::fmt because it comes before the impl block), and notes about what is implemented need to go on the Version enum itself and then this is golden.

/// The Payjoin version /// /// From [BIP 78](https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#optional-parameters), /// the supported versions should be output as numbers: /// /// ```json /// { /// "errorCode": "version-unsupported", /// "supported" : [ 2, 3, 4 ], /// "message": "The version is not supported anymore" /// } /// ``` /// /// # Note /// - Only [`Serialize`] is implemented for json array serialization in the `unsupported-version` error message, /// not [`serde::Deserialize`], as deserialization is not required for this type. /// - [`fmt::Display`] and [`fmt::Debug`] output the `u8` representation for compatibility with BIP 78/77 /// and to match the expected wire format.

DanGould · 2025-05-06T15:19:32Z

payjoin/src/lib.rs

+#[cfg(feature = "_core")]
+pub use crate::core::version::Version;


I think pub(crate) is sufficient since Version is an internal API

Suggested change

#[cfg(feature = "_core")]

pub use crate::core::version::Version;

#[cfg(feature = "_core")]

pub(crate) use crate::core::version::Version;

In a follow up, all of these internal crate re-exports can be done inside core/mod.rs, and then something like

#[cfg(feature = "_core")] pub use crate::core::*;

can be used in lib.rs to clean up the verbose syntax here. Thanks for starting the cleanup by adding the core mod 😎

DanGould

ACK 292cc79

shinghim force-pushed the 643-version branch 3 times, most recently from 8b05efa to badd2c7 Compare April 25, 2025 15:29

shinghim force-pushed the 643-version branch from badd2c7 to 1c83012 Compare April 25, 2025 15:36

shinghim marked this pull request as ready for review April 25, 2025 15:39

arminsabouri reviewed Apr 25, 2025

View reviewed changes

payjoin/src/receive/optional_parameters.rs Show resolved Hide resolved

shinghim force-pushed the 643-version branch from 1c83012 to 0068fea Compare April 26, 2025 13:34

shinghim mentioned this pull request Apr 27, 2025

Dedupe ImplementationError #669

Merged

shinghim force-pushed the 643-version branch from 0068fea to ed1690d Compare April 28, 2025 20:18

spacebear21 reviewed Apr 29, 2025

View reviewed changes

payjoin/src/version.rs Outdated Show resolved Hide resolved

shinghim force-pushed the 643-version branch from ed1690d to d141809 Compare April 30, 2025 01:10

DanGould requested changes Apr 30, 2025

View reviewed changes

DanGould added a commit that referenced this pull request Apr 30, 2025

Dedupe ImplementationError (#669)

a9b461a

Closes #647 Create a new `core` module since there isn't a module to put common/core stuff. The new Version enum in #668 could also be put into this mod, among other things

shinghim force-pushed the 643-version branch 3 times, most recently from cdce962 to 25f3d44 Compare May 1, 2025 04:27

shinghim requested a review from DanGould May 1, 2025 04:36

DanGould reviewed May 5, 2025

View reviewed changes

DanGould requested changes May 6, 2025

View reviewed changes

DanGould mentioned this pull request May 6, 2025

Catch new match arm cargo mutants #686

Merged

Introduce Payjoin version enum

292cc79

shinghim force-pushed the 643-version branch from 25f3d44 to 292cc79 Compare May 6, 2025 19:58

shinghim mentioned this pull request May 6, 2025

Clean up re-exports #688

Closed

DanGould approved these changes May 6, 2025

View reviewed changes

DanGould merged commit a854386 into payjoin:master May 6, 2025
7 checks passed

		#[cfg(feature = "_core")]
		pub use crate::core::version::Version;

Conversation

shinghim commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 14868573150

Details

💛 - Coveralls

Uh oh!

Uh oh!

DanGould commented Apr 25, 2025

Uh oh!

shinghim commented Apr 26, 2025

Uh oh!

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!

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!

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!

shinghim commented May 1, 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!

shinghim May 6, 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!

Choose a reason for hiding this comment

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shinghim commented Apr 25, 2025 •

edited

Loading

coveralls commented Apr 25, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading

shinghim May 6, 2025 •

edited

Loading