Error if send or receive session expired #299
Conversation
Close #298 This introduces a `Url` extension trait called `payjoin::uri::UrlExt` which can set and get `ohttp=` as a fragment rather than a URI parameter. Doing so allows BIP 77 payjoin v2 parameters to be compliant with any BIP 21 bitcoin URI parser that already supports &pj= without introducing new params. This seems to simplify the v1/v2 feature gates too. This change also handles subdirectory parsing differently since the URL no longer ends with the subdirectory, but also the fragment. This introduces new V2 errors. On my first attempt I added subdirectory parsing and setting to `UrlExt`, but opted to remove that since it's out of scope for fragment handling and it would probably need to be considered to be feature gated behind `send`/`receive` which could make the `uri` module messier. (But also perhaps not since `UriExt` is private and `send`/`receive` gates can be enforced in those higher module abstractions where `UriExt` is depended upon). The `UriExt` is unit tested. I'm not sure how to best handle the difference between Ul paths with or without a trailing slash. Technically the latter is a true subdirectory but for our purposes, we're actually using HTTP endpoints that don't differentiate. Our parser should probably prefer one to another. I'm inclined to prefer fewer characters for simplicity. e.g. ``` https://localhost:53429/A8xaMow612_1aiU15nyhQz0RorgIegqftrT7Yo19xKWc#ohttp=AQAgJLHcnSjkWH3Qc6ix2GOSPdyPH3OSu0-YL3NPbLd2Dx8ABAABAAM ``` vs ``` https://localhost:53429/A8xaMow612_1aiU15nyhQz0RorgIegqftrT7Yo19xKWc/#ohttp=AQAgJLHcnSjkWH3Qc6ix2GOSPdyPH3OSu0-YL3NPbLd2Dx8ABAABAAM ``` This was done as prerequisite to #299 since I believe `exp` should be a payjoin-specific fragment parameter as well that gets added to `UriExt`
DanGould
force-pushed
the
expire
branch
4 times, most recently
from
3b8599d
to
eb3c64e
Compare
DanGould
force-pushed
the
expire
branch
2 times, most recently
from
623ba94
to
938f811
Compare
There was a problem hiding this comment.
tACK 938f811 with minor style suggestions.
A more sophisticated expiration method would have the directory put an expiration ceiling at the time the session was initialized, since ultimately the directory only wants to hold sessions in storage for a limited time.
An even more sophisticated way to deal with expiration would to have the database filter expired requests so that no attempts are made to resume expired requests.
+1 for both of these improvements.
payjoin/src/receive/v2/mod.rs
Outdated
| expire_after: Duration, | ||
| custom_expiry_duration: Option<Duration>, |
There was a problem hiding this comment.
Nit: I prefer expire_after. It's concise, and "custom" is implied for any parameter.
| #[cfg(feature = "v2")] ohttp_keys: Option<OhttpKeys>, | ||
| #[cfg(feature = "v2")] expiry: Option<std::time::SystemTime>, |
There was a problem hiding this comment.
Github won't let me add a comment on line 109 but we should have a docstring for expiry.
Relatedly, it might be clearer if we extract these params into a custom struct for v2-only params? e.g.:
#[cfg(feature = "v2")]
struct V2Params {
/// Optional OHTTP keys for v2 (only available if the "v2" feature is enabled).
ohttp_keys: Option<OhttpKeys>,
/// Optional expiry time for the payjoin session.
expiry: Option<std::time::SystemTime>,
}
impl PjUriBuilder {
pub fn new(
address: Address,
origin: Url,
#[cfg(feature = "v2")] v2_params: V2Params,
) -> Self {
...
#[cfg(feature = "v2")]
pj.set_v2_params(v2_params);
}
}
There was a problem hiding this comment.
I think this could work. Where would v2_params live?
There was a problem hiding this comment.
You mean the V2Params struct? It specifically relates to URI params so it should live in uri/mod.rs I would think.
The bip21 crate already percent encodes query parameters so doing it in UrlExt accidentally doubled the percent encoding. This makes an infallable api.
DanGould
force-pushed
the
expire
branch
2 times, most recently
from
6f84707
to
cc36572
Compare
Default to 24 hour expiration. Throw an error when the relevant request is extracted so that polling can be done until the expiration is thrown. The expiration error type should be accessable so that the caller knows if an error was thrown for expiration or for some other reason.
|
I've rebased on master to call the |
Payjoin V2 is asynchronous and introduces an expiration parameter after which sessions are no longer valid. The payjoin directory should dictate the length of time for which it is willing to maintain an open session and communicate that during session initialization. The expiration can then be communicated in the bip21 URI for the sender to stop attempts after expiration. The receiver should also stop after expiration.
This PR is a simple implementation of this idea that only filters expired send/recv sessions at the time their request is extracted.
A more sophisticated expiration method would have the directory put an expiration ceiling at the time the session was initialized, since ultimately the directory only wants to hold sessions in storage for a limited time.
An even more sophisticated way to deal with expiration would to have the database filter expired requests so that no attempts are made to resume expired requests.
Note that the first commit changes the
UrlExttrait NOT to percent encode the fragment, since I found out thatbip21::Urialready percent-encodes the parameter in itsDisplayimplementation