Rename _danger-local-https to _manual-tls by 0xZaddyy · Pull Request #763 · payjoin/rust-payjoin

0xZaddyy · 2025-06-12T19:38:33Z

This PR updates the feature flag name _danger-local-https to _manual-tls for improved clarity and consistency.

What’s Changed

Renamed all occurrences of _danger-local-https to _manual-tls across the codebase.
Updated all feature references in code, documentation, and tests

closes #729

benalleng · 2025-06-12T19:50:45Z

Taking a look! Let me know if you need any help with making sure rust_fmt is running correctly as I see a lot of formatting changes.

zealsham · 2025-06-12T23:55:52Z

Looking good , Did you run the code formating on rust nightly build, that should help with the many unrelated code changes

0xZaddyy · 2025-06-13T00:58:39Z

Looking good , Did you run the code formating on rust nightly build, that should help with the many unrelated code changes

i think the mistake i made was running cargo fmt

benalleng

Good start! We obviously need to get our formatting worked out but as well just so you are reminded it looks like the test-utils still is looking for the _danger-local-https flag. https://github.com/0xZaddyy/rust-payjoin/blob/935bfbc505d3ee14ccf5eb8fb8cd03ae3178a532/payjoin-test-utils/Cargo.toml#L20-L21

benalleng · 2025-06-13T14:44:30Z

payjoin-cli/src/app/v1.rs

+        #[cfg(feature = "_manual-tls")]
        let tls_acceptor = Self::init_tls_acceptor()?;
        while let Ok((stream, _)) = listener.accept().await {
            let app = app.clone();
-            #[cfg(feature = "_danger-local-https")]
+            #[cfg(feature = "_manual-tls")]
            let tls_acceptor = tls_acceptor.clone();
            tokio::spawn(async move {
-                #[cfg(feature = "_danger-local-https")]
+                #[cfg(feature = "_manual-tls")]


I think I'd like to explore moving the init_tls_acceptor() to the e2e tests and add these flags as a feature locked option. This would help to further reduce the amount of feature flagged logic in the source code.

arminsabouri

Besides the fmt changes and the applying the unrelated files this looks good.
One more thing: do you mind formatting your commits using the following guide:
https://cbea.ms/git-commit/

For your current commits please squash them once we figure out the fmt changes and reword to "Rename _danger-local-https to _manual-tls"
You'll notice that I dropped the "and simplify TLS handling". If you do simplify tls handling please do it in a seperate commit. Commits should do just one thing well. That helps us write our release notes, reduces review burden and allows us to revert specific changes if we need to. Thank you!

arminsabouri · 2025-07-16T13:03:26Z

payjoin/src/core/receive/multiparty/error.rs

+            InternalMultipartyError::IdenticalProposals(e) => {
+                write!(f, "More than one identical participant: {e}")
+            }
+            InternalMultipartyError::ProposalVersionNotSupported(v) => {
+                write!(f, "Proposal version not supported: {v}")
+            }
+            InternalMultipartyError::OptimisticMergeNotSupported => {
+                write!(f, "Optimistic merge not supported")
+            }
+            InternalMultipartyError::BitcoinExtractTxError(e) => {
+                write!(f, "Bitcoin extract tx error: {e:?}")
+            }
+            InternalMultipartyError::InputMissingWitnessOrScriptSig => {
+                write!(f, "Input in Finalized Proposal is missing witness or script_sig")
+            }
+            InternalMultipartyError::FailedToCombinePsbts(e) => {
+                write!(f, "Failed to combine psbts: {e:?}")


Looks like some unrelated formatting changes were applied.

arminsabouri · 2025-07-16T13:04:31Z

payjoin/src/receive/v2/persist.rs

@@ -0,0 +1,35 @@
+use std::fmt::{self, Display};


This file was removed in favor of session.rs. Looks like you need to rebase against master.

coveralls · 2025-07-31T23:46:59Z

Pull Request Test Coverage Report for Build 17130350626

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 86.529%

Totals
Change from base Build 17129145427:	0.0%
Covered Lines:	7862
Relevant Lines:	9086

💛 - Coveralls

nothingmuch

ACK, but this PR can be trivial if rebased on #913 which introduces certificate fields to the app config & cli params

i think we can also consider making this manual_tls not _manual_tls, supporting this as feature in payjoin-cli for people who want to run v1 receivers manually provided certificates (--certificate-key seems generally useful, but not sure what value --root-certificate or more generally self signed certificaes have outside of testing). @DanGould thoughts?

nothingmuch · 2025-08-02T23:40:26Z

payjoin-cli/src/app/mod.rs

+#[cfg(feature = "_manual-tls")]
+fn http_agent(cert: Option<Vec<u8>>) -> Result<reqwest::Client> {
+    match cert {
+        Some(cert_der) => build_http_agent_with_cert(&cert_der),
+        None =>
+            if let Ok(base64_cert) = std::env::var("MANUAL_TLS_CERT") {
+                let cert_der = base64::decode(base64_cert)?;
+                build_http_agent_with_cert(&cert_der)
+            } else {
+                Err(anyhow::anyhow!("No certificate provided for manual TLS mode"))
+            },
+    }
+}


i think the #913 approach is a bit cleaner since http_agent just accepts the config regardless of feature flags, so if you agree i would prefer if this PR was rebased on top of it, at which point it should be a trivial change that just renames the feature flag and does not introduce new logic

Alright sounds good :)
I’ll drop the new logic and just go with the feature flag rename so it aligns with #913

spacebear21 · 2025-08-21T13:31:59Z

@0xZaddyy looks like there have been significant conflicts since #913 got merged, could you rebase this PR?

0xZaddyy · 2025-08-21T13:33:52Z

@0xZaddyy looks like there have been significant conflicts since #913 got merged, could you rebase this PR?

sure will get this fixed now

This change updates the feature flag `_danger-local-https` to `_manual-tls` to better reflect the actual behavior and reduce misleading implications about its risk level. The new name clarifies that TLS certificates are only accepted manually and not validated by default.

0xZaddyy · 2025-08-21T14:58:09Z

@spacebear21 all set
it’s fixed!

spacebear21 · 2025-08-21T15:39:13Z

ACK, the CI failure is unrelated and getting fixed in #986 . Thanks @0xZaddyy

benalleng requested changes Jun 13, 2025

View reviewed changes

DanGould force-pushed the master branch from 61351cb to 9f403f1 Compare June 20, 2025 20:17

0xZaddyy force-pushed the change-danger branch 7 times, most recently from 0655d03 to eb7cfa6 Compare July 16, 2025 05:32

arminsabouri requested changes Jul 16, 2025

View reviewed changes

0xZaddyy changed the title ~~Rename _danger-local-https to _manual-tls & Refactor TLS Certificate Handling~~ Rename _danger-local-https to _manual-tls Jul 16, 2025

0xZaddyy force-pushed the change-danger branch from eb7cfa6 to f21e9a1 Compare July 16, 2025 20:33

0xZaddyy force-pushed the change-danger branch 6 times, most recently from fbf5d4a to 2ec825d Compare July 31, 2025 23:43

0xZaddyy marked this pull request as ready for review July 31, 2025 23:44

0xZaddyy requested review from arminsabouri and benalleng August 1, 2025 00:14

0xZaddyy force-pushed the change-danger branch 2 times, most recently from 9f7308c to 57f3292 Compare August 1, 2025 12:50

nothingmuch reviewed Aug 2, 2025

View reviewed changes

0xZaddyy force-pushed the change-danger branch 2 times, most recently from 04b0aa5 to d79884f Compare August 3, 2025 18:15

0xZaddyy force-pushed the change-danger branch 2 times, most recently from a31ab51 to 2f58524 Compare August 3, 2025 18:28

0xZaddyy requested a review from nothingmuch August 3, 2025 18:28

0xZaddyy force-pushed the change-danger branch from 2f58524 to 1eb9d29 Compare August 3, 2025 18:32

0xZaddyy force-pushed the change-danger branch from 1eb9d29 to 3faff3a Compare August 21, 2025 13:43

0xZaddyy force-pushed the change-danger branch from 3faff3a to 594a00b Compare August 21, 2025 14:43

spacebear21 merged commit c3c39dc into payjoin:master Aug 21, 2025
15 of 20 checks passed

spacebear21 mentioned this pull request Aug 21, 2025

Change _danger-local-https to _manual-tls #729

Closed

benalleng mentioned this pull request Jan 9, 2026

The current release versions of payjoin-directory and payjoin-test-utils have conflicting features when trying do depend on them externally #1261

Open

Conversation

0xZaddyy commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What’s Changed

Uh oh!

benalleng commented Jun 12, 2025

Uh oh!

zealsham commented Jun 12, 2025

Uh oh!

0xZaddyy commented Jun 13, 2025

Uh oh!

benalleng left a comment

Choose a reason for hiding this comment

Uh oh!

benalleng Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

arminsabouri Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

arminsabouri Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 17130350626

Details

💛 - Coveralls

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

nothingmuch Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

0xZaddyy Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

spacebear21 commented Aug 21, 2025

Uh oh!

0xZaddyy commented Aug 21, 2025

Uh oh!

0xZaddyy commented Aug 21, 2025

Uh oh!

spacebear21 commented Aug 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

0xZaddyy commented Jun 12, 2025 •

edited

Loading

coveralls commented Jul 31, 2025 •

edited

Loading