RelayManager does not completely manage relays

[DanGould]

rust-payjoin/payjoin-cli/src/app/v2/mod.rs

Line 885 in 2c9085c

async fn unwrap_relay_or_else_fetch(

We're doing this relay health check outside of the relay manager. This means the selection seems to be happening in multiple places. If all of this business logic were abstracted in a single manager, I think it'd make more sense in our implementation and be easier for downstream implementations to reference. This came up in the bdk-cli persistence integration pr because @Mshehu5 implementing was using our reference, and this reference is confusing.

[va-an]

I'd like to pick this one up.

From a quick look, the relay selection logic currently lives in three places outside RelayManager:

1. App::unwrap_relay_or_else_fetch (v2/mod.rs)
2. unwrap_ohttp_keys_or_else_fetch (free fn in ohttp.rs)
3. fetch_ohttp_keys (free fn in ohttp.rs)

Plan is to move all three into RelayManager so callers just ask it for a relay / ohttp keys and never touch its internal state.

Do you prefer a single PR for the whole move, or a separate PR per function?

[benalleng]

imo I think would be the most pragmatic to approach this in a single PR to best be able to manage the context. I would just note to take some extra care in the commits so that it is easy to track context across them

[DanGould]

Plan is to move all three into RelayManager so callers just ask it for a relay / ohttp keys and never touch its internal state.

I suppose this makes sense to hits this issue specifically and in isolation (as a commit).

#1328 is beyond the scope of this but related. However you choose to touch or ignore it, please ensure to share the motivations behind the moves you make

@benalleng do you mean to tackle both this issue and #1328 in one PR? Or just to tackle the unification of the 3 separate locations in one PR?

[DanGould]

Leaving another comment @va-an because I'd like Ben to clarify, I think his comment could be interpreted in at least 2 ways

[benalleng]

Sorry, just responding to this #1582 (comment) specifically, I think that it would be helpful to look over #1328 but the PR i am hoping to see should only deal with moving the following under the RelayManager

1. App::unwrap_relay_or_else_fetch (v2/mod.rs)
2. unwrap_ohttp_keys_or_else_fetch (free fn in ohttp.rs)
3. fetch_ohttp_keys (free fn in ohttp.rs)