Start `payjoin-io` #220

jbesraa · 2024-03-20T15:40:07Z

#211

DanGould

Great start. I went through each commit and left comments on the code.

Two high level comments

Have payjoin-io (Are we set on that name? payjoin-defaults?) depend on payjoin so that you can make use of its types. It should be an abstraction on top of payjoin after all. This should require fewer lines of changes and be easier to reason about and maintain.
I'd like to see the last three commits combined to show a clean change history from two separate implementations, one in payjoin/tests/integration.rs and one in payjoin-cli/src/app/v2.rs into one to demonstrate that the abstraction works and is a drop-in replacement.

This crate is going to be an enormous help to all of our implementations. I can't wait to see it published.

DanGould · 2024-03-20T16:23:26Z

payjoin-io/README.md

+This repository contains a collection of I/O utilities for working
+with `rust-payjoin` crate. It is a work in progress and will be
+updated as new tools are developed.


This doesn't open a new repository, just a library. We may post this to crates.io so this should be catered to an audience there. I also get sad any time I come to a website that says WIP/coming soon. Just tell me what it does! All software is WIP. This isn't a draft PR, it's published software ;)

Try something like

This provides a collection of I/O utilities for working with the payjoin crate.
The payjoin crate only deals with the low-level protocol. payjoin-io provides sane defaults to make implementing payjoin easy.

DanGould · 2024-03-20T16:25:06Z

payjoin-io/Cargo.toml

+ohttp = { version = "0.5.1" }
+bhttp = { version = "0.5.1" }
+url = "2.2.2"
+tokio = { version = "1.12.0", features = ["full"] }
+ureq = "2.9.4"


The idea of this crate is to encapsulate payjoin, so we can depend on a version of payjoin with relevant features directly.

yea I thought about this but was not sure, because I thought we might use payjoin-io in integration tests and then we would have circular deps(although it can be dev-dep to payjoin). what do you think?

I think it should be a dev-dep in payjoin. That shouldn't be circular, is it?

DanGould · 2024-03-20T16:25:36Z

payjoin-io/Cargo.toml


 [dev-dependencies]
+http = "1"
+ohttp-relay = "0.0.4"


Check for the latest version of ohttp-relay before this is merged

DanGould · 2024-03-20T16:26:05Z

payjoin-io/src/error.rs

+macro_rules! impl_from_error {
+    ($from:ty, $to:ident) => {
+        impl From<$from> for Error {
+            fn from(value: $from) -> Self { Self(InternalError::$to(value)) }
+        }
+    };
+}


This is very neat and thought through. However, we should be able to get all of these errors from the payjoin crate instead of re-implementing them.

What I mean by this is to encapsulate a payjoin_io::Error variant payjoin_io::Error::PayjoinV2(payjoin::v2::Error)

DanGould · 2024-03-20T16:26:41Z

payjoin-io/src/lib.rs

+pub mod error;
+pub mod ohttp_keys;


Since the very first iteration does only one thing it makes more sense to me to put all of the functionality in lib.rs. We probably don't have modules to separate yet.

I did this after I added the Error and ProxyServer and lib.rs didnt feel like the right place for them. with that said, if following the comment above we make payjoin-io depend on payjoin for the error object and other types, that could trim some of the code I think then it would make sense to move everything to lib.rs

DanGould · 2024-03-20T16:32:04Z

payjoin-io/src/ohttp_keys.rs

+    let res = spawn_blocking(move || {
+        let proxy = match ProxyServer::new(&proxy_endpoint) {
+            Ok(p) => p,
+            Err(e) => return Err(e),
+        };
+        proxy.get(ohttp_keys_url.as_str()).call().map_err(Error::from)
+    })
+    .await?


I'm not sure we want to or need to depend on tokio or async operations in this crate yet. I'd prefer we didn't and focused on simple but correct I/O operations and the consumer upstream can still make their own decision on whether or not to use tokio/async-std etc. Having tokio as a feature that exposes async functions may make sense in later versions.

DanGould · 2024-03-20T16:33:28Z

payjoin-io/src/ohttp_keys.rs

+// Proxy server struct.
+// This is used to make requests to the payjoin server.
+struct ProxyServer {
+    agent: ureq::Agent,
+}


ureq::Agent represents an Http client, not a servers.

Use conventional BIP 77 naming. s/payjoin server/payjoin directory/ s/Proxy server/Ohttp relay/

DanGould · 2024-03-20T16:35:53Z

payjoin-io/src/ohttp_keys.rs

+impl ProxyServer {
+    fn new(proxy: &Url) -> Result<Self, Error> {
+        let proxy = ureq::Proxy::new(Self::normalize_proxy_url(proxy)?)?;
+        let agent = ureq::AgentBuilder::new().proxy(proxy).build();
+        Ok(Self { agent })
+    }
+
+    fn get(&self, url: &str) -> ureq::Request { self.agent.get(url) }
+
+    // Normalize the Url to include the port for ureq. ureq has a bug
+    // which makes Proxy::new(...) use port 8080 for all input with scheme
+    // http regardless of the port included in the Url. This prevents that.
+    // https://github.com/algesten/ureq/pull/717
+    fn normalize_proxy_url(proxy: &Url) -> Result<String, Error> {
+        let host = match proxy.host_str() {
+            Some(host) => host,
+            None => return Err(Error(InternalError::ParseUrl(url::ParseError::EmptyHost))),
+        };
+        match proxy.scheme() {
+            "http" | "https" => Ok(format!("{}:{}", host, proxy.port().unwrap_or(80))),
+            _ => Ok(proxy.as_str().to_string()),
+        }
+    }
+}


Not sure we need a Struct impl here, simple private fn implementations that return an agent may do. We also want to support a danger-local-https feature right away to make this crate compatible with local integration testing as well as payjoin-cli

DanGould · 2024-03-20T16:36:41Z

payjoin-io/src/ohttp_keys.rs

+#[cfg(test)]
+mod tests {
+    use http::uri::Uri;
+
+    use super::*;
+
+    fn find_free_port() -> u16 {
+        let listener = std::net::TcpListener::bind("0.0.0.0:0").unwrap();
+        listener.local_addr().unwrap().port()
+    }
+
+    // This test depends on the production payjo.in server being live.
+    #[tokio::test]
+    async fn test_fetch_ohttp_keys() {
+        let relay_port = find_free_port();
+        let relay_url = Url::parse(&format!("http://0.0.0.0:{}", relay_port)).unwrap();
+        let pj_endpoint = Url::parse("https://payjo.in:443").unwrap();
+        tokio::select! {
+            _ = ohttp_relay::listen_tcp(relay_port, Uri::from_static("payjo.in:443")) => {
+                assert!(false, "Relay is long running");
+            }
+            res = fetch(relay_url, &pj_endpoint) => {
+                assert!(res.is_ok());
+            }
+        }
+    }
+}


I love seeing the tests this early. However, now that we've got a common implementation, couldn't this go into integration tests with a local directory server instead of the production payjo.in server hack?

DanGould · 2024-03-20T16:37:51Z

payjoin-cli/Cargo.toml

@@ -40,6 +40,7 @@ serde = { version = "1.0.160", features = ["derive"] }
 tokio = { version = "1.12.0", features = ["full"] }
 ureq = "2.9.4"
 url = { version = "2.3.1", features = ["serde"] }
+payjoin-io = { path = "../payjoin-io" }


👍 This makes a lot of sense to me vs a versioned dependency before our first release. I think we'll have to version it in order to release payjoin-cli with a payjoin-io dependency

Since at this commit payjoin-io depends on payjoin with v2 feature, It should also only be optionally compiled

v2 = ["payjoin/v2", "payjoin-io"] ... payjoin-io = { path = "../payjoin-io", optional = true }

And included in the v2 payjoin-cli feature

jbesraa · 2024-03-21T18:39:29Z

@DanGould This is ready for review

DanGould

The overall shape of the integration looks good to me now. There are some nits you may pick up if you like and an payjoin-cli/danger-local-https issue to fix. The error handling is clever to workaround an issue with OhttpKeys::decode: It returns a private error! That api can be addressed in another PR. You handled it gracefully for now.

However, The payjoin/v1 integration test no longer ever runs because payjoin-io is a dev-dependency of payjoin which always depends on payjoin/v2. You'll see the test payjoin integration v1 task in CI only runs the v2 tests.

Perhaps it makes more sense for the payjoin v2 integration tests to migrate to payjoin-io while the v1 test stays in payjoin for now until the i/o operations for v1 are handled by that crate. Another way to possibly solve this would be to feature gate fetch-ohttp-keys with a payjoin-io/v2 feature. But I'm not sure the second potential solution would work, and as you mentioned before, testing this way is sort of a circular dependency that may have caused more of a problem than I initially imagined.

DanGould · 2024-03-24T01:34:14Z

payjoin-io/Cargo.toml

+keywords = ["bip77", "payjoin", "bitcoin", "networking"]
+license = "MITNFA"
+name = "payjoin-io"
+readme = "./README.md"


nit: It seems like this could just be readme = "README.md"

payjoin-io/README.md

DanGould · 2024-03-24T01:41:49Z

payjoin-io/src/lib.rs

+/// Returns the ohttp keys from the specified payjoin directory.
+///
+/// `ohttp_relay` - The proxy the user want to use when requesting the ohttp keys from a payjoin
+/// server.
+/// This is important in order to preserve privacy when communication with the payjoin directory.
+///
+/// `payjoin_directory` - The payjoin server endpoint to fetch the ohttp keys from.
+/// This directory will host your responses and the payjoin sender requests.
+pub fn fetch_ohttp_keys(
+    ohttp_relay: Url,
+    payjoin_directory: Url,
+    #[cfg(feature = "danger-local-https")] cert_der: Vec<u8>,
+) -> Result<payjoin::OhttpKeys, Error> {


I'm glad this is documented however there are a few things to consider. This function actually returns a Result. I tend to like letting the function signature speak for itself and documenting the why rather than the what. The signature could change easily which we could forget to update. Make sure to be consistent with bip77 naming (server vs relay vs proxy vs directory)

To improve readability and follow Rust's conventions more closely, consider separating the parameter name from its description with a colon and a space, and ensure that each description is a grammatically complete sentence. Here's a slightly refined version of your documentation:

/// Fetch the ohttp keys from the specified payjoin directory via proxy. /// /// * `ohttp_relay`: The http CONNNECT method proxy to requesti the ohttp keys from a payjoin directory. /// Proxying requests for ohttp keys ensures a client IP address is never revealed to the payjoin directory. /// /// * `payjoin_directory`: The payjoin directory from which to fetch the ohttp keys. /// This directory stores and forwards payjoin client payloads. /// /// * `cert_der` (optional): The DER-encoded certificate to use for local HTTPS connections. /// This parameter is only available when the "danger-local-https" feature is enabled. pub fn fetch_ohttp_keys( ohttp_relay: Url, payjoin_directory: Url, #[cfg(feature = "danger-local-https")] cert_der: Vec<u8>, ) -> Result<payjoin::OhttpKeys, Error> { // Function implementation goes here }

the payjoin directory endpoint from which to fetch ohttp keys is actually directory.url/ohttp-keys and the function just takes the directory url.

DanGould · 2024-03-24T01:42:55Z

payjoin-io/src/lib.rs

+    #[cfg(feature = "danger-local-https")] cert_der: Vec<u8>,
+) -> Result<payjoin::OhttpKeys, Error> {
+    let ohttp_keys_url = payjoin_directory.join("/ohttp-keys")?;
+    println!("Fetching ohttp keys from {}", ohttp_keys_url.as_str());


nit: Except for certain parts of payjoin-cli, our crates use log to better organize log messages.

DanGould · 2024-03-24T02:03:05Z

payjoin-cli/Cargo.toml

@@ -40,6 +40,7 @@ serde = { version = "1.0.160", features = ["derive"] }
 tokio = { version = "1.12.0", features = ["full"] }
 ureq = "2.9.4"
 url = { version = "2.3.1", features = ["serde"] }
+payjoin-io = { path = "../payjoin-io" }


Since at this commit payjoin-io depends on payjoin with v2 feature, It should also only be optionally compiled

v2 = ["payjoin/v2", "payjoin-io"] ... payjoin-io = { path = "../payjoin-io", optional = true }

And included in the v2 payjoin-cli feature

DanGould · 2024-03-24T02:05:14Z

payjoin-cli/src/app/v1.rs

+        // None is needed for for the OhttpKeys because we fetch `payjoin-io` which fetch `v2`
+        // feature from `payjoin` crate by default.
+        let pj_uri = PjUriBuilder::new(pj_receiver_address, pj_part, None).amount(amount).build();


Are you sure? This v1 module should only compile withOUT payjoin v2 feature since the signature of that function looks like this:

pub fn new( address: Address, pj: Url, #[cfg(feature = "v2")] ohttp_keys: Option<OhttpKeys>, ) -> Self {

The inclusion of payjoin-io for all feature configurations of payjoin-cli must have forced payjoin/v2 even for payjoin-cli/not(v2) and caused a compile time error here. Making payjoin-io an explicit dependency of payjoin-cli/v2 will render this change unnecessary.

DanGould · 2024-03-24T02:14:14Z

payjoin-cli/src/app/v2.rs

-async fn unwrap_ohttp_keys_or_else_fetch(config: &AppConfig) -> Result<payjoin::OhttpKeys> {
+async fn unwrap_ohttp_keys_or_else_fetch(config: AppConfig) -> Result<payjoin::OhttpKeys> {
    if let Some(keys) = config.ohttp_keys.clone() {
        Ok(keys)
    } else {
-        fetch_ohttp_keys(&config.ohttp_relay, &config.pj_endpoint).await
+        Ok(tokio::task::spawn_blocking(move || {
+            payjoin_io::fetch_ohttp_keys(config.ohttp_relay.clone(), config.pj_endpoint.clone())
+        })
+        .await
+        .map_err(|e| anyhow!("Failed to fetch ohttp keys: {}", e))??)
    }
 }


I think we need some danger-local-https logic here. Otherwise, if that feature is enabled the fetch_ohttp_keys function call has too few arguments and will not compile.

I think you could also get the Result::Ok you want out with a couple fewer conversions

if let Some(keys) = config.ohttp_keys.clone() { Ok(keys) } else { tokio::task::spawn_blocking(move || { payjoin_io::fetch_ohttp_keys(config.ohttp_relay.clone(), config.pj_endpoint.clone()) }) .await? .map_err(|e| anyhow!("Failed to fetch ohttp keys: {}", e)) } }

jbesraa · 2024-03-25T14:05:33Z

one standing problem I still face is if i run cargo b in the workspace root it will fail due to v2 error in the payjoin-cli.
I think cargo build payjoin-io in the same context and some things are conflicting even tho in payjoin-cli its guarded behind v2. we might need to guard payjoin-io behind v2 as well

jbesraa · 2024-03-25T14:06:46Z

decided to move v2 integration tests to payjoin-io, I agree that it fits there and doesnt make much sense to import payjoin-io to payjoin..

also I think I agree about the naming, we should probably go with payjoin-defaults

DanGould · 2024-03-25T15:09:52Z

I just had success with cargo b in the workspace root and in payjoin-cli on stable on 06d3397. Are you sure you don't need to cargo clean the old dependency with the same version out?

jbesraa · 2024-03-25T15:27:59Z

oops I was on nightly. working for me as well

DanGould · 2024-03-25T15:35:07Z

Nightly must build too. I just had success with cargo b in building with 1.78.0-nightly the workspace root and in payjoin-cli on stable on 06d3397.

#220 uncovered an issue with the OhttpKeys::decode return error type returning an internal error. I think it still makes sense to return the internal `ohttp::Error` rather than wrapping it in another type that gives no additional context. That also led me to clean up the whole module's error handling. Fix #222 Fix #142

jbesraa · 2024-03-26T11:45:39Z

can you please dump the output of rustup show here?

cargo build is not consistent on my machine, now the stable-rust is again not building(because of v2 incompatibility)

DanGould · 2024-03-26T14:57:08Z

Most of the time I don't use rustup, but a nix profile installed with https://github.com/nix-community/fenix.

However to test nightly I did use the rustup installed in ohttp-relay:

$ ohttp-relay % rustup --version
rustup 1.26.0 (1980-01-01)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.63.0 (4b91a6ea7 2022-08-08)`
$ ohttp-relay % rustup show
Default host: aarch64-apple-darwin
rustup home:  /Users/dan/.rustup

installed toolchains
--------------------

stable-aarch64-apple-darwin
nightly-aarch64-apple-darwin (default)
1.63-aarch64-apple-darwin

active toolchain
----------------

1.63-aarch64-apple-darwin (directory override for '/Users/dan/f/dev/ohttp-relay')
rustc 1.63.0 (4b91a6ea7 2022-08-08)

Are you sure you don't just need to call cargo clean to get rid of the previous dependency tree?

DanGould · 2024-03-26T15:05:35Z

Aha! I have a feeling you're calling cargo build at the top level workspace where payjoin-io defaults to depend on payjoin/v2 which is then tried to use for payjoin-cli and fails. I bet if you called cargo build from payjoin-cli it would build. A v2 feature on payjoin-io could fix this.

jbesraa · 2024-03-26T16:02:19Z

Aha! I have a feeling you're calling cargo build at the top level workspace where payjoin-io defaults to depend on payjoin/v2 which is then tried to use for payjoin-cli and fails. I bet if you called cargo build from payjoin-cli it would build. A v2 feature on payjoin-io could fix this.

yup, thats what I was trying to say here #220 (comment)

should I go on and add the v2 feature to payjoin-io ?

DanGould · 2024-03-26T16:07:54Z

I somehow failed to replicate that on first attempt, apologies. Yes gate fetch function behind v2.

DanGould · 2024-03-26T17:03:12Z

Another simple option may be to have an optional io feature in the payjoin crate instead of a new one. The feature would depends on ureq, rustls, etc. payjoin would still be possible with use without io. Do you think that might be a smaller change or easier to maintain?

jbesraa · 2024-03-27T04:06:50Z

I prefer the current approach of a new crate.
The 'payjoin' crate should be isolated IMO and have very few deps and features.
I think the current feature flagging is not perfect but as we add more functionality to 'payjoin-defaults' we will be smarter about it and maybe get rid of some of those.

jbesraa · 2024-03-27T17:19:08Z

@DanGould let me know when its ok to fixup the commits

DanGould

There's just one problem blocking this from merge in my view. payjoin-cli will not compile with --features v2, danger-local-https because that branch of payjoin_defaults::fetch_ohttp_keys has not been handled. Otherwise looking good 🪽

DanGould · 2024-03-24T17:26:35Z

payjoin/Cargo.toml

@@ -46,6 +46,7 @@ tokio = { version = "1.12.0", features = ["full"] }
 tracing = "0.1.40"
 tracing-subscriber = { version = "0.3.17", features = ["env-filter"] }
 ureq = "2.9.4"
+payjoin-io = { path = "../payjoin-io", features = ["danger-local-https"]}


nit: try to keep dependencies listed in alphabetical order. And keep spacing consistent.

Suggested change

payjoin-io = { path = "../payjoin-io", features = ["danger-local-https"]}

payjoin-io = { path = "../payjoin-io", features = ["danger-local-https"] }

DanGould · 2024-03-25T15:41:13Z

payjoin/tests/integration.rs

@@ -22,7 +23,6 @@ mod integration {

    static INIT_TRACING: OnceCell<()> = OnceCell::new();

-    #[cfg(not(feature = "v2"))]


since the module exists I wouldn't necessarily remove the module feature gate.

Feature gating here is still a minor problem. Integration tests shouldn't necessarily be feature gated wholesale. that's what the the modules were there for.

DanGould · 2024-03-25T15:42:05Z

Cargo.toml

-path = "payjoin"
+path = "payjoin"


try not to include changes that don't have effects

DanGould · 2024-03-25T15:42:32Z

payjoin-cli/Cargo.toml

@@ -40,6 +40,7 @@ serde = { version = "1.0.160", features = ["derive"] }
 tokio = { version = "1.12.0", features = ["full"] }
 ureq = "2.9.4"
 url = { version = "2.3.1", features = ["serde"] }
+payjoin-io = { path = "../payjoin-io", optional = true}


nit:

Suggested change

payjoin-io = { path = "../payjoin-io", optional = true}

payjoin-io = { path = "../payjoin-io", optional = true }

DanGould · 2024-03-28T18:14:09Z

payjoin-defaults/src/lib.rs

+
+/// Fetch the ohttp keys from the specified payjoin directory via proxy.
+///
+/// * `ohttp_relay`: The http CONNNECT method proxy to requesti the ohttp keys from a payjoin


sp. "requesti"

DanGould · 2024-03-28T18:32:20Z

payjoin-defaults/src/lib.rs

+struct PayjoinProxy {
+    client: ureq::Agent,
+}


I think I realized you probably have an instinct to put this in a struct because you might be used to the object oriented paradigm. Just a comment.

DanGould · 2024-03-28T18:35:34Z

payjoin-defaults/src/lib.rs

+    // which makes Proxy::new(...) use port 8080 for all input with scheme
+    // http regardless of the port included in the Url. This prevents that.
+    // https://github.com/algesten/ureq/pull/717
+    fn normalize_proxy_url(proxy: &Url) -> Result<String, Error> {


I'm seeing both Error variants returned by this impl block return clippy warnings. In a follow up pr they can probably be made opaque and have the error source which is not actionable ignored. I read "Thinking about errors" this week which helped me re-think the whole error design space. Unless a downstream user can make critical decisions from these internal errors, we don't have to pass them up to each caller.

warning: the `Err`-variant returned from this function is very large --> payjoin-defaults/src/lib.rs:123:44 | 123 | fn normalize_proxy_url(proxy: &Url) -> Result<String, Error> { | ^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 272 bytes | = help: try reducing the size of `Error`, for example by boxing large elements or replacing it with `Box<Error>` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err

DanGould · 2024-03-28T18:39:27Z

payjoin-cli/src/app/v2.rs

+        Ok(tokio::task::spawn_blocking(move || {
+            payjoin_defaults::fetch_ohttp_keys(
+                config.ohttp_relay.clone(),
+                config.pj_endpoint.clone(),
+            )
+        })


Two comments:

This won't compile with #[cfg(features = "danger-local-https")] because it's missing the local cert parameter.

Cloning the Url types that need to be owned will clone fewer bytes than cloning the whole AppConfig struct

async fn unwrap_ohttp_keys_or_else_fetch(config: &AppConfig) -> Result<payjoin::OhttpKeys> { if let Some(keys) = config.ohttp_keys.clone() { Ok(keys) } else { let ohttp_relay = config.ohttp_relay.clone(); let payjoin_directory = config.pj_endpoint.clone(); Ok(tokio::task::spawn_blocking(move || { payjoin_defaults::fetch_ohttp_keys( ohttp_relay, payjoin_directory, ) }) .await .map_err(|e| anyhow!("Failed to fetch ohttp keys: {}", e))??) } }```

the danger-local-https feature from payjoin-default is not used by the cli and its not included in the cli cargo.toml. should we still accommodate for that?

Yes, it should be used if is danger-local-https && v2 features are set. It can probably be switched on by the payjoin-cli/danger-local-https feature since it's a test feature and an extra dependency shouldn't hurt there.

This PR deletes danger-local-https fetch code which the new crate replaces and it should maintain feature parity.

The goal of this new crate is to provide i/o utilities for safer and easier integration with `rust-payjoin` crate.

Using a proxy server setup and `ureq` we access the payjoin directory and return the ohttp keys.

DanGould

Gah, the one blocking issue was resolved but another opened.

We can't default payjoin-cli to a dangerous feature set. If we do, the default mode for payjoin-cli will be one where users can lose money. That should be for those who want to compile the dangerous feature set explicitly only.

DanGould · 2024-04-09T17:29:53Z

payjoin-cli/src/app/v2.rs

+        Ok(tokio::task::spawn_blocking(move || {
+            payjoin_defaults::fetch_ohttp_keys(
+                ohttp_relay,
+                payjoin_directory,
+                #[cfg(feature = "danger-local-https")]
+                cert_der,
+            )
+        })
+        .await
+        .map_err(|e| anyhow!("Failed to fetch ohttp keys: {}", e))??)


It's a bit over complicated a fallible expression in Ok() without first handling the error. Better to handle one error at a time and then return Ok if the result is actually Ok. We can fix this in a follow up.

DanGould · 2024-04-09T17:32:21Z

payjoin-defaults/src/lib.rs

+        #[cfg(feature = "danger-local-https")]
+        cert_der,
+    )?;
+    let res = proxy.get(ohttp_keys_url.as_str()).call()?;


Since proxy is your own abstraction, it probably makes sense more to have it take your abstraction (Url). Otherwise, what's the point of the abstraction? I don't really think we need this abstraction / impl layer though so I'm OK ignoring it for now.

DanGould · 2024-04-09T17:34:06Z

payjoin-cli/Cargo.toml

+danger-local-https = ["rcgen", "rustls", "hyper-rustls", "payjoin-defaults/danger-local-https"]
+v2 = ["payjoin/v2", "payjoin-defaults/v2"]
+
+default = ["danger-local-https", "v2"]


danger-local-https is a test-only feature. If it is used by default then the CLI tool won't check https certs. That's dangerous! We can't ship a production build with this feature set. Where did the idea to set this default come from?

aye sorry that's a leftover from some manual local testing

DanGould · 2024-04-09T17:37:07Z

payjoin/tests/integration.rs

@@ -22,7 +23,6 @@ mod integration {

    static INIT_TRACING: OnceCell<()> = OnceCell::new();

-    #[cfg(not(feature = "v2"))]


Feature gating here is still a minor problem. Integration tests shouldn't necessarily be feature gated wholesale. that's what the the modules were there for.

Make sure `payjoin-cli` builds with `v2 ` and `danger-local-https` feature flags.

jbesraa force-pushed the use-io-in-cli branch 2 times, most recently from c0c7e75 to e650c7e Compare March 20, 2024 15:49

DanGould requested changes Mar 20, 2024

View reviewed changes

jbesraa force-pushed the use-io-in-cli branch 2 times, most recently from 5e60aa2 to 482055d Compare March 21, 2024 16:11

jbesraa changed the title ~~initial payjoin-io integration in payjoin-cli~~ Start payjoin-io Mar 21, 2024

jbesraa force-pushed the use-io-in-cli branch 8 times, most recently from 14d4f8d to 3f9308f Compare March 21, 2024 18:32

jbesraa requested a review from DanGould March 21, 2024 18:39

DanGould requested changes Mar 24, 2024

View reviewed changes

jbesraa requested a review from DanGould March 24, 2024 16:54

DanGould mentioned this pull request Mar 24, 2024

Clean up V2 errors #223

Merged

jbesraa force-pushed the use-io-in-cli branch 2 times, most recently from 38bca64 to 06d3497 Compare March 25, 2024 13:50

jbesraa force-pushed the use-io-in-cli branch 4 times, most recently from bc6f4ef to c4c17ca Compare March 26, 2024 16:48

jbesraa force-pushed the use-io-in-cli branch 2 times, most recently from 579f068 to e76d5f9 Compare March 27, 2024 16:50

jbesraa force-pushed the use-io-in-cli branch 2 times, most recently from fcfe68a to b13e2b4 Compare March 27, 2024 19:09

DanGould requested changes Mar 28, 2024

View reviewed changes

jbesraa force-pushed the use-io-in-cli branch from b13e2b4 to ab409cc Compare April 8, 2024 08:59

jbesraa added 2 commits April 8, 2024 12:04

Initialize new payjoin-defaults crate

84d4b38

The goal of this new crate is to provide i/o utilities for safer and easier integration with `rust-payjoin` crate.

Add fetch_ohttp_keys function

79d6155

Using a proxy server setup and `ureq` we access the payjoin directory and return the ohttp keys.

jbesraa force-pushed the use-io-in-cli branch 2 times, most recently from 599ae0c to 9b7d926 Compare April 8, 2024 11:30

DanGould requested changes Apr 9, 2024

View reviewed changes

jbesraa added 3 commits April 9, 2024 21:24

Use payjoin-defaults in payjoin-cli

462c57d

Move payjoin v2 integration tests to payjoin-defaults

a3137f7

Add new payjoin-cli build flow to CI

1044afd

Make sure `payjoin-cli` builds with `v2 ` and `danger-local-https` feature flags.

jbesraa force-pushed the use-io-in-cli branch from 9b7d926 to 1044afd Compare April 9, 2024 18:25

DanGould approved these changes Apr 11, 2024

View reviewed changes

DanGould merged commit 41f703b into payjoin:master Apr 11, 2024
5 checks passed

	payjoin-io = { path = "../payjoin-io", features = ["danger-local-https"]}
	payjoin-io = { path = "../payjoin-io", features = ["danger-local-https"] }

		@@ -22,7 +23,6 @@ mod integration {

		static INIT_TRACING: OnceCell<()> = OnceCell::new();

		#[cfg(not(feature = "v2"))]

	payjoin-io = { path = "../payjoin-io", optional = true}
	payjoin-io = { path = "../payjoin-io", optional = true }

Start payjoin-io #220

Start payjoin-io #220

Conversation

jbesraa commented Mar 20, 2024 • edited

DanGould left a comment

Choose a reason for hiding this comment

DanGould Mar 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Mar 21, 2024

DanGould left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould Mar 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Mar 25, 2024

jbesraa commented Mar 25, 2024 • edited

DanGould commented Mar 25, 2024 • edited

jbesraa commented Mar 25, 2024

DanGould commented Mar 25, 2024 • edited

jbesraa commented Mar 26, 2024 • edited

DanGould commented Mar 26, 2024

DanGould commented Mar 26, 2024

jbesraa commented Mar 26, 2024

DanGould commented Mar 26, 2024 • edited

DanGould commented Mar 26, 2024

jbesraa commented Mar 27, 2024

jbesraa commented Mar 27, 2024

DanGould left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould Mar 28, 2024 • edited

Choose a reason for hiding this comment

DanGould Mar 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould Apr 1, 2024 • edited

Choose a reason for hiding this comment

DanGould left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Start `payjoin-io` #220

Start `payjoin-io` #220

jbesraa commented Mar 20, 2024 •

edited

DanGould Mar 20, 2024 •

edited

DanGould Mar 21, 2024 •

edited

DanGould Mar 24, 2024 •

edited

jbesraa commented Mar 25, 2024 •

edited

DanGould commented Mar 25, 2024 •

edited

DanGould commented Mar 25, 2024 •

edited

jbesraa commented Mar 26, 2024 •

edited

DanGould commented Mar 26, 2024 •

edited

DanGould Mar 28, 2024 •

edited

DanGould Mar 28, 2024 •

edited

DanGould Apr 1, 2024 •

edited