Skip to content

Allow for custom HTTPHrnResolvers #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

benthecarman
Copy link
Contributor

Closes #3

This allows for giving a custom reqwest client when creating a HTTPHrnResolver. This is useful for adding custom headers, proxying connections, etc. This also has the added benefit of using the same reqwest client across every call so it'll be better and reusing tls connections.

@TheBlueMatt
Copy link
Member

We probably don't want someone to be able to configure a full client including headers, right? We just care about proxying, AFAIU.

@benthecarman
Copy link
Contributor Author

We probably don't want someone to be able to configure a full client including headers, right? We just care about proxying, AFAIU.

I don't really see the downside. Being able to provide the full client is nice because then you can reuse the client across uses.

@TheBlueMatt
Copy link
Member

Looking at the current source for Client (https://docs.rs/reqwest/latest/src/reqwest/async_impl/client.rs.html#2760) its kinda mixed. We may want to allow configuring a read timeout, of course, but we definitely don't want someone to use a Client if the cookies feature is on (which would trivially allow tracking a single payer across lots of requests).

@tnull
Copy link

tnull commented Jul 2, 2025

Looking at the current source for Client (https://docs.rs/reqwest/latest/src/reqwest/async_impl/client.rs.html#2760) its kinda mixed. We may want to allow configuring a read timeout, of course, but we definitely don't want someone to use a Client if the cookies feature is on (which would trivially allow tracking a single payer across lots of requests).

FWIW, most of the crates in the space (e.g., esplora-client) provide a from_client constructor to allow overriding some of these values, but also to reuse a client.

@TheBlueMatt
Copy link
Member

Hmm, though in those cases its probably okay to have things like cookies coped over? You might set an auth cookie once and use it both for esplora and other APIs to the same host. Whereas in this case, any difference in requests across clients is pretty bad (not that we can avoid it too well...HTTP sucks)

@benthecarman benthecarman force-pushed the better-http branch 4 times, most recently from 47d15b8 to 7237ce0 Compare July 2, 2025 21:49
This allows for giving a custom reqwest client when creating a
HTTPHrnResolver. This is useful for adding custom headers, proxying
connections, etc. This also has the added benefit of using the same
reqwest client across every call so it'll be better and reusing
tls connections.
@TheBlueMatt
Copy link
Member

Did we ever resolve this discussion?

@benthecarman
Copy link
Contributor Author

benthecarman commented Jul 11, 2025

I tried to see if we could turn the from_client function into a Result type and we could do some validation on if they're doing any of the foot guns but doesn't seem like they expose the config after the client is built.

I still stand by the current changes. I think this api is by far the simplest and most convenient for the user. There are small privacy tradeoffs but lnurl isn't really meant to be a private protocol and often is used for the opposite (zaps)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose easy way to proxy requests through e.g. tor
3 participants