Remove `url::Url` types from public API

[DanGould]

Our dependency is twofold, in function arguments and in errors. We can more easily maintain stability of our public API if we internalize our url dependencies in both function arguments and in error variants.

Using url::Url in the public API may be a form of tech debt, in that we took it on to get the API out without full consideration at the time the dep was first introduced by Kixunil.

Don't' depend on url::Url in function arguments

Instead, take Url arguments as IntoUrl trait implementations in core typestate machine instead of url::Url.

We have to re-export url::Url, bind to it, and have downstream implementers manually parse URLs before payjoin will accept them as arguments to many functions that produce requests e.g. Sender::extract_v2.

Instead, an IntoUrl trait like reqwest over &'a str, &'a String, String that handles Url validation inside the typestate machine would abstract this burden from an end user and make one less error for them to handle since it'd be encapsulated in our variants resulting from those extract or setup functions.

related: #459

Don't depend on url in public error variants.

We have no explicit url::ParseError variants in our Error types because they're only internal variants, but if we were to make those variants public we would. We also make downstream users construct their own Urls to pass to our functions which makes them handle the ParseError on their own rather than encapsulating that logic and error handling in our extract_ functions or similar.

related: #403

[nothingmuch]

reqwest's IntoUrl is sealed, but what you're thinking of wouldn't be, right?

[DanGould]

It would be sealed and we'd implement it for &'a str, &'a String and String just like reqwest.

[DanGould]

I realize now that reqwest also re-exports url::Url, which is nice for convenience. There are circumstances where you want to parse URLs early, as in payjoin-cli configuration, and having the preferred type available alongside IntoUrl, with IntoUrl trait implementations on &Url and Url even more ergonomic.

[DanGould]

This is somewhat covered by #520, we're not going to remove url::Url (immediately), but we are using IntoUrl to make the signatures more ergonomic. Whether we want to depend on the url::ParseUrl variants in the public API is a question I haven't answered yet, but am leaning to NOT. Better if we encapsulate it in our own so that downstream won't get a break from the dependency being upgraded.

[arminsabouri]

From @DanGould "If we get rid of this [impl IntoUrl for [url::]Url], then we can use string types everywhere" https://docs.rs/payjoin/latest/payjoin/trait.IntoUrl.html#implementors

[DanGould]

While #1057 removed the use of Url in function arguments, it's still in the public API. Reopening

A typesafe future proof solution would be to introduce

struct Url(url::Url);

impl Url {

    fn as_str(&self) -> &str;

    fn to_string(&self) -> String;
}

impl IntoUrl for Url;
impl IntoUrl for &Url;

and then return this Url type from the remaining functions.

e.g.

PayjoinExtras::endpoint() -> Url PjParams::endpoint() -> Url

[benalleng]

After work on #1077 we realized that perhaps we want to move towards our own implementation of payjoin::Url instead of trying to marry the url crate with our halfway approach.

However we still need to decide if that is the best approach, there is still the option of using the url crate with no default features and enabling only the serde feature which removes some of the more problematic features like idna

Ideally we can use our on payjoin::Url with string manipulation methods similar to those on the url crate that we use. I need to explore exactly what is still needed as Dan pointed out we can probably remove parse() all together.

Url methods we wwould need to recreate

• path_segments(): needed for PjParam parse but can possibly be done with some string manipulation
• fragment(): again used on PjParam parse but seems like could be done with out own string manipulation
• parse(): this can probably be removed
• query(): This is only used in v1 receiver so perhaps there is a hacky way to avoid this?

Action Items right now are to remove url::Url from payjoin-test-utils and the still publicly available Request::url before moving on to start getting rid of the url crate entirely

[arminsabouri]

From @DanGould Remove public accessers from struct req fields. Use getters for read only field access. Get a string method to get the contents of Request::url field (or otherwise document exactly why we need to reteurn a proper Url from a getter (either Request::url() or Request::url_as_str()` or similar)

[benalleng]

#1121 should close this issue but does not remove the url crate as a dep. I think we should make a new issue for that as it seems outside of the scope of this issue

[DanGould]

Noted un-completed by #1141

[DanGould]

closed by #1141