While not apparent in the top squashed commit, I played around with some
different approaches here.
The one I landed on adds the auth token as a parameter to the API traits
themselves. There's an additional UserAuthenticator which handles
re-auth'ing when a token is about to expire or has expired. The
authenticator is also share-able between multiple tasks, and only allows
one task to auth at a time if it's used concurrently. A nice benefit is that
it's very easy to see which APIs need auth (since they need a token param).
Another approach I tried was hiding all the auth details in the
RestClient, which didn't work well as the RestClient logic started getting
too complicated. I also tried a wrapper type around the NodeApiClient,
but this just led to excessive repetition and wasn't adding enough value.
I've also experimented with a builder-oriented RestClient API, where
you (1) build the request then (2) send the request
let request = self.rest.request_builder(POST, "/foo")
.json(&data)
.bearer_auth(&token);
self.rest.send(request).await?
The builder API avoids adding too many parameters to the
rest.request(..) function, especially now that we have more combinations
(like json/signed-bcs/query, maybe auth, maybe retries).
The reqwest builder also has a built-in serde query string serializer,
like request_builder(GET, "/bar").query(&data), which would let us remove
the serde_qs dependency.
I've left in the rest.request(..) APIs for now. If the new builder
APIs seem nicer, we can remove the prev. one.
add ByteStr, a wrapper around cheaply cloneable Bytes, so we
can cheaply clone UserAuthTokens.
```text
Diff in /Users/fang/lexe/dev/client/common/src/api/def.rs at line 21:
use async_trait::async_trait;
use super::auth::{
- UserAuthToken, UserAuthRequest, UserAuthResponse, UserSignupRequest,
+ UserAuthRequest, UserAuthResponse, UserAuthToken, UserSignupRequest,
};
use crate::api::error::{BackendApiError, NodeApiError, RunnerApiError};
use crate::api::node::{ListChannels, NodeInfo};
```
While not apparent in the top squashed commit, I played around with some different approaches here.
The one I landed on adds the auth token as a parameter to the API traits themselves. There's an additional
UserAuthenticator
which handles re-auth'ing when a token is about to expire or has expired. The authenticator is also share-able between multiple tasks, and only allows one task to auth at a time if it's used concurrently. A nice benefit is that it's very easy to see which APIs need auth (since they need a token param).Another approach I tried was hiding all the auth details in the
RestClient
, which didn't work well as theRestClient
logic started getting too complicated. I also tried a wrapper type around theNodeApiClient
, but this just led to excessive repetition and wasn't adding enough value.I've also experimented with a builder-oriented
RestClient
API, where you (1) build the request then (2) send the requestThe builder API avoids adding too many parameters to the
rest.request(..)
function, especially now that we have more combinations (like json/signed-bcs/query, maybe auth, maybe retries).The
reqwest
builder also has a built-inserde
query string serializer, likerequest_builder(GET, "/bar").query(&data)
, which would let us remove theserde_qs
dependency.I've left in the
rest.request(..)
APIs for now. If the new builder APIs seem nicer, we can remove the prev. one.add
ByteStr
, a wrapper around cheaply cloneableBytes
, so we can cheaply cloneUserAuthToken
s.