chore: get rid of WsAdminClient #4520
Conversation
dpc
force-pushed
the
getting-rid-of-ws-admin-client
branch
from
88ec9a7
to
289a955
Compare
There was a problem hiding this comment.
I think this makes sense, having two clients annoyed me too.
As a next step towards the goal of a "guardian client" I could imagine being able to save our_id and the password to the client DB, which would lead to guardian functionality becoming available. To allow modules to access guardian APIs we should add something like ClientContext::guardian_auth(&self) -> Option<(PeerId, ApiAuth)> that can be used to determine if the user is a guardian and if so what the authentication token is.
| pub fn from_pre_peer_id_endpoint(url: SafeUrl) -> Self { | ||
| // PeerIds are used only for informational purposes, but just in case, make a | ||
| // big number so it stands out | ||
| GlobalFederationApiWithCache::new(WsFederationApi::new(vec![(PeerId::from(1024), url)])) | ||
| .into() | ||
| } | ||
|
|
||
| pub fn from_single_endpoint(peer: PeerId, url: SafeUrl) -> Self { | ||
| GlobalFederationApiWithCache::new(WsFederationApi::new(vec![(peer, url)])).into() | ||
| } |
There was a problem hiding this comment.
Is it really worth having two of these functions?
There was a problem hiding this comment.
Doesn't matter too much, just wanted a simplest patch possible, to unblock meta module. We'll clean up with time.
| /// Sets the password used to decrypt the configs and authenticate | ||
| /// | ||
| /// Must be called first before any other calls to the API | ||
| async fn set_password(&self, auth: ApiAuth) -> FederationResult<()>; |
There was a problem hiding this comment.
Must be called first before any other calls to the API
This doesn't make a lot of sense if read in the context of the normal client API. Maybe the admin API should be defined as an extension trait just like module APIs are? I think that would make it easier to understand the separate concepts of "normal API" and "admin API"
|
Needs rebase :( |
Meta module PR already does something like that, thought it does not store it in the DB. |
dpc
force-pushed
the
getting-rid-of-ws-admin-client
branch
from
6cf556a
to
0f214fa
Compare
|
Rebased. |
| 1, | ||
| "attempted to broadcast admin password?!" | ||
| ); | ||
| self.request_current_consensus(method.into(), params.with_auth(auth)) |
There was a problem hiding this comment.
Could we use request_single_peer here instead of request_current_consensus?
There was a problem hiding this comment.
It makes sense, but it changes the type of the result, which will lead further changes, and I have other PRs on top of this one, and just wanted to move these methods 1:1 to a new place to keep it small. We'll follow up with improvements. Added a TODO to #4519 4519
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Could you elaborate more on this goal. What functionalities would this guardian client have, differentiated from the current Federation/User client, and from the direct API calls to a Guardian app makes to ConfigGen / Consensus APIs |
I'm not sure about the original meaning but, the AFAICT next goals are:
|
Re #4519
I wrote it just to see if it's possible and everything still works.
Now I think we might tweak
IRawFederationApia bit and retain ability to do both. I.e. splitIRawFederationApi::all_peersintoall_peers+admin_peer, so thatFederationApiExt::request_admincan call only the admin, while other requests - all guardians.But one way or another I think getting rid of
WsAdminClientis easy and the right thing to do.