Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions payjoin/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ exclude = ["tests"]
[features]
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"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

directory = []
v1 = ["_core"]
v2 = ["_core", "bitcoin/serde", "hpke", "dep:http", "bhttp", "ohttp", "serde", "url/serde", "directory"]
v2 = ["_core", "bitcoin/serde", "hpke", "dep:http", "bhttp", "ohttp", "url/serde", "directory"]
#[doc = "Functions to fetch OHTTP keys via CONNECT proxy using reqwest. Enables `v2` since only `v2` uses OHTTP."]
io = ["v2", "reqwest/rustls-tls"]
_danger-local-https = ["reqwest/rustls-tls", "rustls"]
Expand Down
2 changes: 2 additions & 0 deletions payjoin/src/core/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@
//!
//! This module contains types and methods used to implement Payjoin.
pub mod error;

pub mod version;
47 changes: 47 additions & 0 deletions payjoin/src/core/version.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
use core::fmt;

use serde::{Serialize, Serializer};

/// 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.
#[repr(u8)]
#[derive(Clone, Copy, PartialEq, Eq)]
pub enum Version {
/// BIP 78 Payjoin
One = 1,
/// BIP 77 Async Payjoin
Two = 2,
}

impl fmt::Display for Version {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { (*self as u8).fmt(f) }
}

impl fmt::Debug for Version {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fmt::Display::fmt(self, f) }
}

impl Serialize for Version {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
(*self as u8).serialize(serializer)
}
}
Comment on lines +40 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this not missing a matching deserialize impl?

Copy link
Contributor Author

@shinghim shinghim May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

2 changes: 2 additions & 0 deletions payjoin/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,5 @@ pub mod core;
pub(crate) mod error_codes;
#[cfg(feature = "_core")]
pub use crate::core::error::ImplementationError;
#[cfg(feature = "_core")]
pub(crate) use crate::core::version::Version;
3 changes: 2 additions & 1 deletion payjoin/src/receive/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use optional_parameters::Params;

pub use crate::psbt::PsbtInputError;
use crate::psbt::{InternalInputPair, InternalPsbtInputError, PsbtExt};
use crate::Version;

mod error;
pub(crate) mod optional_parameters;
Expand Down Expand Up @@ -73,7 +74,7 @@ impl<'a> From<&'a InputPair> for InternalInputPair<'a> {
pub(crate) fn parse_payload(
base64: String,
query: &str,
supported_versions: &'static [usize],
supported_versions: &'static [Version],
) -> Result<(Psbt, Params), PayloadError> {
let unchecked_psbt = Psbt::from_str(&base64).map_err(InternalPayloadError::ParsePsbt)?;

Expand Down
3 changes: 2 additions & 1 deletion payjoin/src/receive/multiparty/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use core::fmt;
use std::error;

use crate::uri::ShortId;
use crate::Version;

#[derive(Debug)]
pub struct MultipartyError(InternalMultipartyError);
Expand All @@ -13,7 +14,7 @@ pub(crate) enum InternalMultipartyError {
/// Duplicate proposals
IdenticalProposals(IdenticalProposalError),
/// Proposal version not supported
ProposalVersionNotSupported(usize),
ProposalVersionNotSupported(Version),
/// Optimistic merge not supported
OptimisticMergeNotSupported,
/// Bitcoin Internal Error
Expand Down
4 changes: 2 additions & 2 deletions payjoin/src/receive/multiparty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ use super::{v1, v2, Error, InputPair};
use crate::psbt::merge::merge_unsigned_tx;
use crate::receive::multiparty::error::{InternalMultipartyError, MultipartyError};
use crate::receive::v2::SessionContext;
use crate::ImplementationError;
use crate::{ImplementationError, Version};

pub(crate) mod error;

const SUPPORTED_VERSIONS: &[usize] = &[2];
const SUPPORTED_VERSIONS: &[Version] = &[Version::Two];

#[derive(Default)]
pub struct UncheckedProposalBuilder {
Expand Down
33 changes: 23 additions & 10 deletions payjoin/src/receive/optional_parameters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ use bitcoin::FeeRate;
use log::warn;

use crate::output_substitution::OutputSubstitution;
use crate::Version;

#[derive(Debug, Clone)]
pub(crate) struct Params {
// version
pub v: usize,
pub v: Version,
// disableoutputsubstitution
pub output_substitution: OutputSubstitution,
// maxadditionalfeecontribution, additionalfeeoutputindex
Expand All @@ -24,7 +25,7 @@ pub(crate) struct Params {
impl Default for Params {
fn default() -> Self {
Params {
v: 1,
v: Version::One,
output_substitution: OutputSubstitution::Enabled,
additional_fee_contribution: None,
min_fee_rate: FeeRate::BROADCAST_MIN,
Expand All @@ -37,7 +38,7 @@ impl Default for Params {
impl Params {
pub fn from_query_pairs<K, V, I>(
pairs: I,
supported_versions: &'static [usize],
supported_versions: &'static [Version],
) -> Result<Self, Error>
where
I: Iterator<Item = (K, V)>,
Expand All @@ -52,8 +53,9 @@ impl Params {
for (key, v) in pairs {
match (key.borrow(), v.borrow()) {
("v", version) =>
params.v = match version.parse::<usize>() {
Ok(version) if supported_versions.contains(&version) => version,
params.v = match version {
"1" => Version::One,
"2" => Version::Two,
_ => return Err(Error::UnknownVersion { supported_versions }),
},
("additionalfeeoutputindex", index) =>
Expand Down Expand Up @@ -111,9 +113,9 @@ impl Params {
}
}

#[derive(Debug)]
#[derive(Debug, PartialEq, Eq)]
pub(crate) enum Error {
UnknownVersion { supported_versions: &'static [usize] },
UnknownVersion { supported_versions: &'static [Version] },
FeeRate,
}

Expand All @@ -135,19 +137,30 @@ pub(crate) mod test {
use bitcoin::Amount;

use super::*;
use crate::receive::optional_parameters::Params;
use crate::Version;

#[test]
fn test_parse_params() {
use bitcoin::FeeRate;

let pairs = url::form_urlencoded::parse(b"maxadditionalfeecontribution=182&additionalfeeoutputindex=0&minfeerate=1&disableoutputsubstitution=true&optimisticmerge=true");
let params =
Params::from_query_pairs(pairs, &[1]).expect("Could not parse params from query pairs");
assert_eq!(params.v, 1);
let params = Params::from_query_pairs(pairs, &[Version::One])
.expect("Could not parse params from query pairs");
assert_eq!(params.v, Version::One);
assert_eq!(params.output_substitution, OutputSubstitution::Disabled);
assert_eq!(params.additional_fee_contribution, Some((Amount::from_sat(182), 0)));
assert_eq!(params.min_fee_rate, FeeRate::BROADCAST_MIN);
#[cfg(feature = "_multiparty")]
assert!(params.optimistic_merge)
}

#[test]
fn from_query_pairs_unsupported_versions() {
let invalid_pair: Vec<(&str, &str)> = vec![("v", "888")];
let supported_versions = &[Version::One, Version::Two];
let params = Params::from_query_pairs(invalid_pair.into_iter(), supported_versions);
assert!(params.is_err());
assert_eq!(params.err().unwrap(), Error::UnknownVersion { supported_versions });
}
}
5 changes: 3 additions & 2 deletions payjoin/src/receive/v1/exclusive/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@ pub use error::RequestError;

use super::*;
use crate::into_url::IntoUrl;
use crate::Version;

/// 4_000_000 * 4 / 3 fits in u32
const MAX_CONTENT_LENGTH: usize = 4_000_000 * 4 / 3;
const SUPPORTED_VERSIONS: &[usize] = &[1];
const SUPPORTED_VERSIONS: &[Version] = &[Version::One];

pub trait Headers {
fn get_header(&self, key: &str) -> Option<&str>;
Expand Down Expand Up @@ -131,7 +132,7 @@ mod tests {
Address::from_script(&witness_utxo.script_pubkey, bitcoin::params::Params::MAINNET)?;
assert_eq!(address.address_type(), Some(AddressType::P2sh));

assert_eq!(proposal.params.v, 1);
assert_eq!(proposal.params.v, Version::One);
assert_eq!(proposal.params.additional_fee_contribution, Some((Amount::from_sat(182), 0)));
Ok(())
}
Expand Down
5 changes: 3 additions & 2 deletions payjoin/src/receive/v1/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -782,11 +782,12 @@ pub(crate) mod test {

use super::*;
use crate::receive::PayloadError;
use crate::Version;

pub(crate) fn unchecked_proposal_from_test_vector() -> UncheckedProposal {
let pairs = url::form_urlencoded::parse(QUERY_PARAMS.as_bytes());
let params =
Params::from_query_pairs(pairs, &[1]).expect("Could not parse params from query pairs");
let params = Params::from_query_pairs(pairs, &[Version::One])
.expect("Could not parse params from query pairs");
UncheckedProposal { psbt: PARSED_ORIGINAL_PSBT.clone(), params }
}

Expand Down
6 changes: 3 additions & 3 deletions payjoin/src/receive/v2/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ use crate::output_substitution::OutputSubstitution;
use crate::persist::Persister;
use crate::receive::{parse_payload, InputPair};
use crate::uri::ShortId;
use crate::{ImplementationError, IntoUrl, IntoUrlError, Request};
use crate::{ImplementationError, IntoUrl, IntoUrlError, Request, Version};

mod error;
mod persist;

const SUPPORTED_VERSIONS: &[usize] = &[1, 2];
const SUPPORTED_VERSIONS: &[Version] = &[Version::One, Version::Two];

static TWENTY_FOUR_HOURS_DEFAULT_EXPIRY: Duration = Duration::from_secs(60 * 60 * 24);

Expand Down Expand Up @@ -219,7 +219,7 @@ impl Receiver {
// V2 proposals are authenticated and encrypted to prevent such attacks.
//
// see: https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#unsecured-payjoin-server
if params.v == 1 {
if params.v == Version::One {
params.output_substitution = OutputSubstitution::Disabled;

// Additionally V1 sessions never have an optimistic merge opportunity
Expand Down