Skip to content

Commit

Permalink
Use IntoUrl for more ergonomic function signatures (#520)
Browse files Browse the repository at this point in the history
Re: #513 
Inspired by
[`reqwest::IntoUrl`](https://docs.rs/reqwest/latest/reqwest/trait.IntoUrl.html)

After some consideration, the goal is no longer to
remove `url` types from the public API. That crate is ancient and has a
stable release cadence and conservative MSRV targets (1.63 even with the
latest) that we can track.

However, I still think the `IntoUrl` interface makes our typestate
machines easier to work downstream with for the tradeoff of added
complexity in our crate.

Note, in payjoin-cli We're still validating URLs in configuration, and
using the `url::Url` abstraction in function signatures makes more sense
than becoming more loose for testing.

The greatest advantage of this change may accrue to downstream FFI
users, who I imagine use their native language's Url type and conversion
to a stringly type that payjoin-ffi handles using IntoUrl rather than
forcing them to use the `payjoin::Url` re-export and error handling.
  • Loading branch information
DanGould authored Feb 19, 2025
2 parents 0451383 + b3f0966 commit 4ba2d8c
Show file tree
Hide file tree
Showing 11 changed files with 207 additions and 67 deletions.
2 changes: 1 addition & 1 deletion payjoin-cli/src/app/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ impl App {
.map_err(|e| anyhow!("Failed to parse pj_endpoint: {}", e))?;

let mut pj_uri =
payjoin::receive::v1::build_v1_pj_uri(&pj_receiver_address, &pj_part, false);
payjoin::receive::v1::build_v1_pj_uri(&pj_receiver_address, &pj_part, false)?;
pj_uri.amount = Some(amount);

Ok(pj_uri.to_string())
Expand Down
2 changes: 1 addition & 1 deletion payjoin-cli/src/app/v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl AppTrait for App {
let address = self.bitcoind()?.get_new_address(None, None)?.assume_checked();
let ohttp_keys = unwrap_ohttp_keys_or_else_fetch(&self.config).await?;
let session =
Receiver::new(address, self.config.pj_directory.clone(), ohttp_keys.clone(), None);
Receiver::new(address, self.config.pj_directory.clone(), ohttp_keys.clone(), None)?;
self.db.insert_recv_session(session.clone())?;
self.spawn_payjoin_receiver(session, Some(amount)).await
}
Expand Down
114 changes: 114 additions & 0 deletions payjoin/src/into_url.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
use url::{ParseError, Url};

#[derive(Debug)]
pub enum Error {
BadScheme,
ParseError(ParseError),
}

impl std::fmt::Display for Error {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
use Error::*;

match self {
BadScheme => write!(f, "URL scheme is not allowed"),
ParseError(e) => write!(f, "{}", e),
}
}
}

impl std::error::Error for Error {}

impl From<ParseError> for Error {
fn from(err: ParseError) -> Error { Error::ParseError(err) }
}

type Result<T> = core::result::Result<T, Error>;

/// Try to convert some type into a [`Url`].
///
/// This trait is "sealed", such that only types within payjoin can
/// implement it.
///
/// This design is inspired by the `reqwest` crate's design:
/// see <https://docs.rs/reqwest/latest/reqwest/trait.IntoUrl.html>
pub trait IntoUrl: IntoUrlSealed {}

impl IntoUrl for &Url {}
impl IntoUrl for Url {}
impl IntoUrl for &str {}
impl IntoUrl for &String {}
impl IntoUrl for String {}

pub trait IntoUrlSealed {
/// Besides parsing as a valid `Url`, the `Url` must be a valid
/// `http::Uri`, in that it makes sense to use in a network request.
fn into_url(self) -> Result<Url>;

fn as_str(&self) -> &str;
}

impl IntoUrlSealed for &Url {
fn into_url(self) -> Result<Url> { self.clone().into_url() }

fn as_str(&self) -> &str { self.as_ref() }
}

impl IntoUrlSealed for Url {
fn into_url(self) -> Result<Url> {
if self.has_host() {
Ok(self)
} else {
Err(Error::BadScheme)
}
}

fn as_str(&self) -> &str { self.as_ref() }
}

impl IntoUrlSealed for &str {
fn into_url(self) -> Result<Url> { Url::parse(self)?.into_url() }

fn as_str(&self) -> &str { self }
}

impl IntoUrlSealed for &String {
fn into_url(self) -> Result<Url> { (&**self).into_url() }

fn as_str(&self) -> &str { self.as_ref() }
}

impl IntoUrlSealed for String {
fn into_url(self) -> Result<Url> { (&*self).into_url() }

fn as_str(&self) -> &str { self.as_ref() }
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn http_uri_scheme_is_allowed() {
let url = "http://localhost".into_url().unwrap();
assert_eq!(url.scheme(), "http");
}

#[test]
fn https_uri_scheme_is_allowed() {
let url = "https://localhost".into_url().unwrap();
assert_eq!(url.scheme(), "https");
}

#[test]
fn into_url_file_scheme() {
let err = "file:///etc/hosts".into_url().unwrap_err();
assert_eq!(err.to_string(), "URL scheme is not allowed");
}

#[test]
fn into_url_blob_scheme() {
let err = "blob:https://example.com".into_url().unwrap_err();
assert_eq!(err.to_string(), "URL scheme is not allowed");
}
}
27 changes: 16 additions & 11 deletions payjoin/src/io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
use reqwest::{Client, Proxy};

use crate::{OhttpKeys, Url};
use crate::into_url::IntoUrl;
use crate::OhttpKeys;

/// Fetch the ohttp keys from the specified payjoin directory via proxy.
///
Expand All @@ -13,11 +14,11 @@ use crate::{OhttpKeys, Url};
/// * `payjoin_directory`: The payjoin directory from which to fetch the ohttp keys. This
/// directory stores and forwards payjoin client payloads.
pub async fn fetch_ohttp_keys(
ohttp_relay: Url,
payjoin_directory: Url,
ohttp_relay: impl IntoUrl,
payjoin_directory: impl IntoUrl,
) -> Result<OhttpKeys, Error> {
let ohttp_keys_url = payjoin_directory.join("/ohttp-keys")?;
let proxy = Proxy::all(ohttp_relay.as_str())?;
let ohttp_keys_url = payjoin_directory.into_url()?.join("/ohttp-keys")?;
let proxy = Proxy::all(ohttp_relay.into_url()?.as_str())?;
let client = Client::builder().proxy(proxy).build()?;
let res = client.get(ohttp_keys_url).send().await?;
let body = res.bytes().await?.to_vec();
Expand All @@ -36,12 +37,12 @@ pub async fn fetch_ohttp_keys(
/// * `cert_der`: The DER-encoded certificate to use for local HTTPS connections.
#[cfg(feature = "_danger-local-https")]
pub async fn fetch_ohttp_keys_with_cert(
ohttp_relay: Url,
payjoin_directory: Url,
ohttp_relay: impl IntoUrl,
payjoin_directory: impl IntoUrl,
cert_der: Vec<u8>,
) -> Result<OhttpKeys, Error> {
let ohttp_keys_url = payjoin_directory.join("/ohttp-keys")?;
let proxy = Proxy::all(ohttp_relay.as_str())?;
let ohttp_keys_url = payjoin_directory.into_url()?.join("/ohttp-keys")?;
let proxy = Proxy::all(ohttp_relay.into_url()?.as_str())?;
let client = Client::builder()
.danger_accept_invalid_certs(true)
.use_rustls_tls()
Expand All @@ -58,14 +59,18 @@ pub struct Error(InternalError);

#[derive(Debug)]
enum InternalError {
ParseUrl(crate::ParseError),
ParseUrl(crate::into_url::Error),
Reqwest(reqwest::Error),
Io(std::io::Error),
#[cfg(feature = "_danger-local-https")]
Rustls(rustls::Error),
InvalidOhttpKeys(String),
}

impl From<url::ParseError> for Error {
fn from(value: url::ParseError) -> Self { Self(InternalError::ParseUrl(value.into())) }
}

macro_rules! impl_from_error {
($from:ty, $to:ident) => {
impl From<$from> for Error {
Expand All @@ -74,8 +79,8 @@ macro_rules! impl_from_error {
};
}

impl_from_error!(crate::into_url::Error, ParseUrl);
impl_from_error!(reqwest::Error, Reqwest);
impl_from_error!(crate::ParseError, ParseUrl);
impl_from_error!(std::io::Error, Io);
#[cfg(feature = "_danger-local-https")]
impl_from_error!(rustls::Error, Rustls);
Expand Down
5 changes: 4 additions & 1 deletion payjoin/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ pub(crate) mod bech32;
#[cfg_attr(docsrs, doc(cfg(feature = "directory")))]
pub mod directory;

#[cfg(feature = "_core")]
pub(crate) mod into_url;
#[cfg(feature = "io")]
#[cfg_attr(docsrs, doc(cfg(feature = "io")))]
pub mod io;
Expand All @@ -51,10 +53,11 @@ pub use request::*;
#[cfg(feature = "_core")]
mod uri;

#[cfg(feature = "_core")]
pub use into_url::{Error as IntoUrlError, IntoUrl};
#[cfg(feature = "_core")]
pub use uri::{PjParseError, PjUri, Uri, UriExt};
#[cfg(feature = "_core")]
pub use url::{ParseError, Url};

#[cfg(feature = "_core")]
pub(crate) mod error_codes;
9 changes: 5 additions & 4 deletions payjoin/src/receive/v1/exclusive/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pub(crate) use error::InternalRequestError;
pub use error::RequestError;

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

/// 4_000_000 * 4 / 3 fits in u32
const MAX_CONTENT_LENGTH: usize = 4_000_000 * 4 / 3;
Expand All @@ -14,12 +15,12 @@ pub trait Headers {

pub fn build_v1_pj_uri<'a>(
address: &bitcoin::Address,
endpoint: &url::Url,
endpoint: impl IntoUrl,
disable_output_substitution: bool,
) -> crate::uri::PjUri<'a> {
) -> Result<crate::uri::PjUri<'a>, crate::into_url::Error> {
let extras =
crate::uri::PayjoinExtras { endpoint: endpoint.clone(), disable_output_substitution };
bitcoin_uri::Uri::with_extras(address.clone(), extras)
crate::uri::PayjoinExtras { endpoint: endpoint.into_url()?, disable_output_substitution };
Ok(bitcoin_uri::Uri::with_extras(address.clone(), extras))
}

impl UncheckedProposal {
Expand Down
34 changes: 22 additions & 12 deletions payjoin/src/receive/v2/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ impl From<InternalSessionError> for Error {

#[derive(Debug)]
pub(crate) enum InternalSessionError {
/// Url parsing failed
ParseUrl(crate::into_url::Error),
/// The session has expired
Expired(std::time::SystemTime),
/// OHTTP Encapsulation failed
Expand All @@ -35,6 +37,10 @@ pub(crate) enum InternalSessionError {
UnexpectedStatusCode(http::StatusCode),
}

impl From<crate::into_url::Error> for SessionError {
fn from(e: crate::into_url::Error) -> Self { InternalSessionError::ParseUrl(e).into() }
}

impl From<std::time::SystemTime> for Error {
fn from(e: std::time::SystemTime) -> Self { InternalSessionError::Expired(e).into() }
}
Expand All @@ -51,31 +57,35 @@ impl From<HpkeError> for Error {

impl fmt::Display for SessionError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
use InternalSessionError::*;

match &self.0 {
InternalSessionError::Expired(expiry) => write!(f, "Session expired at {:?}", expiry),
InternalSessionError::OhttpEncapsulation(e) =>
write!(f, "OHTTP Encapsulation Error: {}", e),
InternalSessionError::Hpke(e) => write!(f, "Hpke decryption failed: {}", e),
InternalSessionError::UnexpectedResponseSize(size) => write!(
ParseUrl(e) => write!(f, "URL parsing failed: {}", e),
Expired(expiry) => write!(f, "Session expired at {:?}", expiry),
OhttpEncapsulation(e) => write!(f, "OHTTP Encapsulation Error: {}", e),
Hpke(e) => write!(f, "Hpke decryption failed: {}", e),
UnexpectedResponseSize(size) => write!(
f,
"Unexpected response size {}, expected {} bytes",
size,
crate::directory::ENCAPSULATED_MESSAGE_BYTES
),
InternalSessionError::UnexpectedStatusCode(status) =>
write!(f, "Unexpected status code: {}", status),
UnexpectedStatusCode(status) => write!(f, "Unexpected status code: {}", status),
}
}
}

impl error::Error for SessionError {
fn source(&self) -> Option<&(dyn error::Error + 'static)> {
use InternalSessionError::*;

match &self.0 {
InternalSessionError::Expired(_) => None,
InternalSessionError::OhttpEncapsulation(e) => Some(e),
InternalSessionError::Hpke(e) => Some(e),
InternalSessionError::UnexpectedResponseSize(_) => None,
InternalSessionError::UnexpectedStatusCode(_) => None,
ParseUrl(e) => Some(e),
Expired(_) => None,
OhttpEncapsulation(e) => Some(e),
Hpke(e) => Some(e),
UnexpectedResponseSize(_) => None,
UnexpectedStatusCode(_) => None,
}
}
}
Loading

0 comments on commit 4ba2d8c

Please sign in to comment.