payjoin
Create fallback ohttp-relay logic for payjoin-cli
The reliance on ohttp-relays can cause some distruption if they ever go offline but we did not have any examples of a fallback mechanism for choosing the relays at random and falling back to other relays in case they fail.
This adds a new RelayState to manage which relays have already been tried and ulitmately hangs on to the first successful relay that it reaches per session.
Closes #528
Pull Request Test Coverage Report for Build 15071763691
Details
- 48 of 83 (57.83%) changed or added relevant lines in 4 files are covered.
- 1 unchanged line in 1 file lost coverage.
- Overall coverage decreased (-0.1%) to 82.135%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| payjoin-cli/src/app/v2.rs | 33 | 47 | 70.21% |
| payjoin/src/io.rs | 6 | 27 | 22.22% |
| <!-- | Total: | 48 | 83 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| payjoin/src/io.rs | 1 | 58.26% |
| <!-- | Total: | 1 |
| Totals | |
|---|---|
| Change from base Build 15031183812: | -0.1% |
| Covered Lines: | 5508 |
| Relevant Lines: | 6706 |
💛 - Coveralls
Nice lookin' draft. There are some code repeats that can probably be squashed but you probably know that and are just sharing early. I suppose this fixes #528? A mention in the OP will close the issue when this merges, if so. Thanks Ben
Thanks for the feedback, always helpful to get some nudges in the right direction
is the http dependency new or is it already in cargo tree? If so, which version? May you please specify exactly instead of just "1"
It looks like the payjoin tree uses 1.1.0 though we also need to specify the version there as well.
same for rand. Do we need rand explicitly or is payjoin::bitcoin::rand available in all configurations?
rand can definitely be just from within payjoin, we can remove the independent dep there.
How necessary are the dual code paths for _danger-local-https? To what extent may they be reduced?
~~These were holdouts from v1 and I think ultimately they can be somewhat reduced though there will be some separation assuming we desire to keep the tests local and not having to actually reach out to a server.~~
Ultimately I think that the _danger-local-https flag here is useful for our tests as we can use local relays and directories without needing a good connection to an actual directory.
Will the relay state mutex unlock ever be contentious (lots of clients during resume?) If so, we should use tokio::sync::Mutex to await instead of blocking the async fn. If not, the synchronous one is fine.
~~I'll need to take a closer look at this.~~ See below
Will the relay state mutex unlock ever be contentious (lots of clients during resume?) If so, we should use tokio::sync::Mutex to await instead of blocking the async fn. If not, the synchronous one is fine.
I do not think we need to use async Mutex as all of our mutex unlocking and usage is done within small blocks and our mutexes are dropped before any sort of await call is made.
I think I spoke imprecisely. The Internal Error should remain private. If it's time to make certain variants that are public, but keep an Internal Variant that's private, I think that would solve the problem.
what msrv is #[allow(private_interfaces)] included in?
https://github.com/DanGould/rust-payjoin/actions/runs/15049802630/job/42301513021#step:5:803
This isn't included until 1.72.0.
Since using this #[allow(public_interfaces)] + #[doc(hidden)] approach won't work with our .msrv is it ok to make the InternalError variants public as well while keeping them doc hidden like I did in c97f23f? Or should we find another approach?
I guess we could do something along the lines of just making all the variants of InternalError doc hidden except our UnexpectedStatusCode error.
One fix would be to make the current InternalError into InternalErrorInner and then make a pub struct InternalError(InternalErrorInner).
Otherwise, #[doc(hidden)] on both the Internal Variant AND pub enum InternalError could work.