Extracted client API in near_network. - #7839
Conversation
matklad
left a comment
There was a problem hiding this comment.
Overall, looks most excellent to me!
Didn't yet do review for correctness here
| pub type Result<T> = std::result::Result<T, ReasonForBan>; | ||
|
|
||
| impl Client { | ||
| pub async fn tx_status_request( |
There was a problem hiding this comment.
yup, love this interface with a bunch of functions very much, kudos!
Merging Client & ViewClient behind a single interface is also a very good idea imo: VC should really be just an impl detail of Client.
| match self.view_client_addr.send(NetworkViewClientMessages::BlockRequest(hash)).await { | ||
| Ok(NetworkViewClientResponses::Block(block)) => Ok(Some(*block)), | ||
| Ok(NetworkViewClientResponses::NoResponse) => Ok(None), | ||
| Ok(NetworkViewClientResponses::Ban { ban_reason }) => Err(ban_reason), | ||
| Ok(resp) => panic!("unexpected ViewClientResponse: {resp:?}"), | ||
| Err(err) => { | ||
| tracing::error!("mailbox error: {err}"); | ||
| Ok(None) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we could extract some of the boilerplate here into a helper:
self.send_view(NetworkViewClientMessages::BlockRequest(hash), |res| match res {
NetworkViewClientResponses::Block(block) => Some(*block),
NetworkViewClientResponses::NoResponse => None,
resp => panic!("unexpected ViewClientResponse: {resp:?}"),
})
(eg, have the MailBox error be unwrap_or_defaulted, and handling Ban automatically. Perhaps even merging mailbox closed & no_response).
I don't think we should though, just explicitly writing out matches is simpler.
There was a problem hiding this comment.
I don't think we should either. All of the content of this file is boilerplate, so let it be as straightforward as possible.
| } | ||
|
|
||
| pub struct RawRoutedMessage { | ||
| pub(crate) struct RawRoutedMessage { |
| let peer_id = conn.peer_info.id.clone(); | ||
| ctx.spawn( | ||
| wrap_future(async move { | ||
| network_state.receive_message(&clock, peer_id, msg, was_requested).await |
There was a problem hiding this comment.
It looks a bit strange that this is routed via the global network_state rather than local connection.
There was a problem hiding this comment.
I want this PeerMessage handler to be agnostic to where the request came from (in particular the response should be sent outside of the handler). It is true that right now it is far from perfect, because even Client wants to get sometimes the peer_id of the sender for some reason. I'll move it back to PeerActor for now, and I'll think how to rearrange it later.
matklad
left a comment
There was a problem hiding this comment.
I think this has higher than usual chance of breaking pytests, please keep an eye on that!
This way the client actors are abstracted out from the network crate. Also moved the multiplexing PeerMessage -> (View)ClientActor API, from PeerActor to NetworkState. Improved tests which started failing (this PR doesn't introduce semantic changes, therefore those tests were apparently too fragile).
|
This breaks a whole bunch of tests, see #7916 |
|
I know, I'm working on a fix + mechanism to prevent this kind of bugs in the future: #7895 |
This way the client actors are abstracted out from the network crate. Also moved the multiplexing PeerMessage -> (View)ClientActor API, from PeerActor to NetworkState. Improved tests which started failing (this PR doesn't introduce semantic changes, therefore those tests were apparently too fragile).
I have incorrectly removed it in #7839 (by accident). Also I've dropped producing message of this type in this PR, so that support for receiving StateResponse can be dropped in the future release.
I have incorrectly removed it in near#7839 (by accident). Also I've dropped producing message of this type in this PR, so that support for receiving StateResponse can be dropped in the future release.
I have incorrectly removed it in near#7839 (by accident). Also I've dropped producing message of this type in this PR, so that support for receiving StateResponse can be dropped in the future release.
This way the client actors are abstracted out from the network crate.
Also moved the multiplexing PeerMessage -> (View)ClientActor API, from PeerActor to NetworkState.
Improved tests which started failing (this PR doesn't introduce semantic changes, therefore those tests were apparently too fragile).