Remove use of payjoin::Url and url::Url in public Api by benalleng · Pull Request #1057 · payjoin/rust-payjoin

benalleng · 2025-09-10T18:53:48Z

This PR aims to remove the use of Url in the public API args and accepting Url of &Url in our IntoUrl interface.

ChatGpt was used in some error correction along the way and type conversions for strings to urls.

closes #513

Pull Request Checklist

Please confirm the following before requesting review:

I have disclosed my use of
AI
in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

coveralls · 2025-09-10T19:01:47Z

Pull Request Test Coverage Report for Build 17654923783

Details

47 of 53 (88.68%) changed or added relevant lines in 10 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.01%) to 86.011%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/receive/v2/mod.rs	12	13	92.31%
payjoin-cli/src/app/v2/ohttp.rs	2	7	28.57%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/core/into_url.rs	1	94.34%

Totals
Change from base Build 17654709120:	-0.01%
Covered Lines:	8251
Relevant Lines:	9593

💛 - Coveralls

DanGould

Glad to see this started. I have a feeling this overshot the mark by a bit in two main ways

Why is impl IntoUrl removed from the public API for &str when Urls are expected? payjoin::IntoUrl is our way to have a typesafe URL passed without exposing the explicit dependency on url::Url, letting us get rid of the dependency on url in the future. However, we still want a strong Url type contract in the API and IntoUrl gets us there in function definitions.
Why was url::Url removed from payjoin-cli? url::Url in payjoin-cli types still seems appropriate because it minimizes this change for review and allows url::Url to still be removed from the payjoin interface. It lets payjoin-cli continue to have validation on config where it may not otherwise and simply allows the core payjoin crate to go without for the dependency sticklers.

And undershot in one main way: it didn't remove url::Url from the public IntoUrl interface by removing the following lines from into_url.rs as described here

-    impl IntoUrl for &Url {}
-    impl IntoUrl for Url {}

payjoin-cli/src/app/v2/ohttp.rs

payjoin-cli/tests/e2e.rs

payjoin-ffi/src/request.rs

payjoin-test-utils/src/lib.rs

payjoin/src/core/receive/v2/mod.rs

payjoin/src/core/send/v2/mod.rs

benalleng · 2025-09-11T15:33:25Z

Thanks for the review, I perhaps misunderstood the exact issue that we really just want to disallow accepting a Url in IntoUrl and other methods instead of trying to transition over to using a string reference instead

As a result it was my intention to try and keep some type arg consistency on the external crates, but I will walk back some of these changes now that I know the scope is a little more narrow.

benalleng · 2025-09-11T15:36:26Z

And undershot in one main way: it didn't remove url::Url from the public IntoUrl interface by removing the following lines from into_url.rs as described here
-    impl IntoUrl for &Url {}
-    impl IntoUrl for Url {}

~~My only question about this is if we remove the Url and &Url from being accepted why not just accept a &str and type convert to a Url internally without using IntoUrl?~~
I think this is answered here #1057 (comment)

DanGould · 2025-09-11T15:40:59Z

why not just accept a &str

Also for ergonomics because then you can pass both &str and String. And if we eventually have a payjoin::Url type then we have consistency without contract change, too.

DanGould

Better.

A few things remain:

Why are we expecting valid urls for unparsed inputs. That sounds like a recipe for disaster. I know you know better than that and these instances are just a goofs but c'mon man a panic is srs bizns. This has gotta be fixed.

edit: this was a bit harsh I make booboos like this sometimes too. Hope you don't take it the wrong way, Ben.
2. Lots of to_string where as_str() will do. Indeed we're not auditing the enforcement but I think we can at least use best practice all over the place here to demonstrate minimized mem use.
3. Internal APIs can and should use Url to take advantage of the type's strong validation since we're not pulling that dependency out yet.

payjoin/src/core/receive/v2/mod.rs

payjoin/src/core/receive/v2/session.rs

payjoin/src/core/send/v2/mod.rs

payjoin/tests/integration.rs

DanGould · 2025-09-11T18:12:55Z

payjoin/tests/integration.rs

                let receiver: Arc<corepc_node::Client> = Arc::new(receiver);
                let receiver_clone = receiver.clone();
-                let ohttp_relay = services.ohttp_relay_url();
+                let ohttp_relay = services.ohttp_relay_url().to_string();


This actually probably should remain a string type with the ownership within the receiver loop

arminsabouri · 2025-09-12T12:23:33Z

Heads up @benalleng there is a conflict on this branch after #1046

DanGould

Needs a slight signature change now that crate::uri is pub and visibility lets us internalize some functions to allow them to take validated url::Url and not have to return Results

payjoin/src/core/receive/v2/mod.rs

payjoin/src/core/uri/v2.rs

payjoin/src/core/send/v2/mod.rs

This commit removes the Url from the public Api where it is an arg in public api methods and also as an impl in the IntoUrl interface so that it is not accepted where IntoUrl is used. This commit does not remove Url internally from the payjoin crate or remove the use of Url in any external crates, it simply transitions the type to a `&str` or `String` when passing into payjoin methods.

This is a minor commit in a simplification I found could be made in some send tests using existing consts

DanGould

tACK c46687f

Clean 🧽

benalleng self-assigned this Sep 10, 2025

benalleng added this to the payjoin-1.0 milestone Sep 10, 2025

benalleng force-pushed the remove-public-url branch from 72d450b to c4b9a96 Compare September 10, 2025 18:57

benalleng force-pushed the remove-public-url branch 3 times, most recently from a8892e0 to a4a85f1 Compare September 10, 2025 20:37

arminsabouri requested a review from DanGould September 11, 2025 14:53

benalleng force-pushed the remove-public-url branch from 739cc4b to b53dc45 Compare September 11, 2025 15:22

DanGould requested changes Sep 11, 2025

View reviewed changes

benalleng force-pushed the remove-public-url branch 2 times, most recently from 1d66a52 to 3220474 Compare September 11, 2025 17:35

benalleng marked this pull request as ready for review September 11, 2025 17:36

DanGould requested changes Sep 11, 2025

View reviewed changes

benalleng force-pushed the remove-public-url branch 3 times, most recently from 65ab3db to 31b3403 Compare September 11, 2025 19:16

DanGould mentioned this pull request Sep 11, 2025

Make payjoin::uri module public #1048

Merged

2 tasks

benalleng force-pushed the remove-public-url branch from 6c39e5e to 04cff71 Compare September 11, 2025 20:39

benalleng force-pushed the remove-public-url branch from 04cff71 to 48bcda4 Compare September 12, 2025 14:08

DanGould requested changes Sep 12, 2025

View reviewed changes

payjoin/src/core/receive/v2/mod.rs Outdated Show resolved Hide resolved

payjoin/src/core/uri/v2.rs Outdated Show resolved Hide resolved

benalleng force-pushed the remove-public-url branch 3 times, most recently from e4f30aa to f301b1e Compare September 12, 2025 14:48

DanGould reviewed Sep 12, 2025

View reviewed changes

payjoin/src/core/send/v2/mod.rs Outdated Show resolved Hide resolved

benalleng force-pushed the remove-public-url branch from f301b1e to 8872c34 Compare September 12, 2025 15:42

benalleng force-pushed the remove-public-url branch from 8872c34 to aa7a7c3 Compare September 12, 2025 15:44

benalleng added 2 commits September 12, 2025 11:45

Use test-utils example url const

c46687f

This is a minor commit in a simplification I found could be made in some send tests using existing consts

benalleng force-pushed the remove-public-url branch from aa7a7c3 to c46687f Compare September 12, 2025 15:46

DanGould approved these changes Sep 12, 2025

View reviewed changes

DanGould merged commit 2ad79cd into payjoin:master Sep 12, 2025
10 checks passed

DanGould mentioned this pull request Sep 12, 2025

Remove url::Url types from public API #513

Closed

benalleng deleted the remove-public-url branch October 6, 2025 18:40

Conversation

benalleng commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 17654923783

Details

💛 - Coveralls

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!

Uh oh!

Uh oh!

benalleng commented Sep 11, 2025

Uh oh!

benalleng commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould commented Sep 11, 2025

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanGould Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

benalleng Sep 11, 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!

arminsabouri commented Sep 12, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

4 participants

benalleng commented Sep 10, 2025 •

edited

Loading

coveralls commented Sep 10, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading

benalleng commented Sep 11, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading

benalleng Sep 11, 2025 •

edited

Loading