chore: clean component server client tests

[uriel-starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.74%. Comparing base (8090c8b) to head (adb1d08).
Report is 4 commits behind head on main.

Files	Patch %	Lines
crates/mempool_infra/src/component_client_rpc.rs	87.50%	1 Missing ⚠️
crates/mempool_infra/src/component_server_rpc.rs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
+ Coverage   74.78%   76.74%   +1.96%     
==========================================
  Files          23       27       +4     
  Lines         916     1088     +172     
  Branches      916     1088     +172     
==========================================
+ Hits          685      835     +150     
- Misses        187      201      +14     
- Partials       44       52       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Itay-Tsabary-Starkware]

Reviewed 2 of 10 files at r1, all commit messages.
Reviewable status: 2 of 10 files reviewed, 12 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_client.rs line 39 at r1 (raw file):

    #[error("Got an unexpected response type.")]
    UnexpectedResponse,
    #[error("Could not connect to server.")]

Please change to "Failed to connect to the server".

---

crates/mempool_infra/src/component_client.rs line 41 at r1 (raw file):

    #[error("Could not connect to server.")]
    ConnectionFailure,
    #[error("Could not get response from server.")]

Please change to "Failed to get a response from the server".

---

crates/mempool_infra/src/component_definitions.rs line 9 at r1 (raw file):

}

pub struct RequestWithResponder<Request, Response>

1. This should be in a different pr.
2. Why do you prefer this name?

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 44 at r1 (raw file):

#[async_trait]
impl ClientATrait for ComponentAClientRpc {

Please stick to the naming convention defined in the mempool: the trait name is XClient (X being the component name, in our case, A).

Code quote:

ClientATrait

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 47 at r1 (raw file):

    async fn a_get_value(&self) -> Result<ValueA, ClientError> {
        let Ok(mut client) = RemoteAClient::connect(self.dst.clone()).await else {
            return Err(ClientError::ConnectionFailure);

Tonic Result's captures information that is foregone by this; please propagate the error.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 51 at r1 (raw file):

        let Ok(response) = client.remote_a_get_value(AGetValueMessage {}).await else {
            return Err(ClientError::ResponseFailure);

Ditto.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 140 at r1 (raw file):

    match a_client.a_get_value().await {
        Ok(returned_value) => assert_eq!(returned_value, expected_value),
        Err(error) => panic!("{error}"),

Please revert and use expect.

Code quote:

        Ok(returned_value) => assert_eq!(returned_value, expected_value),
        Err(error) => panic!("{error}"),

---

crates/mempool_infra/tests/component_server_client_test.rs line 82 at r1 (raw file):

        Ok(returned_value) => assert_eq!(returned_value, expected_value),
        Err(error) => panic!("{error}"),
    }

Ditto.

Code quote:

    match a_client.a_get_value().await {
        Ok(returned_value) => assert_eq!(returned_value, expected_value),
        Err(error) => panic!("{error}"),
    }

---

crates/mempool_infra/tests/common/mod.rs line 10 at r1 (raw file):

#[async_trait]
pub(crate) trait ClientATrait: Send + Sync {

Rename according to the convention (mentioned in a different comment as well)

Code quote:

ClientATrait

---

crates/mempool_infra/tests/common/mod.rs line 11 at r1 (raw file):

#[async_trait]
pub(crate) trait ClientATrait: Send + Sync {
    async fn a_get_value(&self) -> Result<ValueA, ClientError>;

Please wrap in a type.

Code quote:

Result<ValueA, ClientError>

---

crates/mempool_infra/tests/common/mod.rs line 17 at r1 (raw file):

pub(crate) trait ClientBTrait: Send + Sync {
    async fn b_get_value(&self) -> Result<ValueB, ClientError>;
}

Ditto on both changes.

Code quote:

pub(crate) trait ClientBTrait: Send + Sync {
    async fn b_get_value(&self) -> Result<ValueB, ClientError>;
}

---

crates/mempool_infra/tests/common/mod.rs line 20 at r1 (raw file):

pub(crate) struct ComponentA {
    b: Box<dyn ClientBTrait>,

Change to arc (in a different pr), for B as well. See how the gateway uses the mempool client for inspiration.

[Itay-Tsabary-Starkware]

Reviewable status: 2 of 10 files reviewed, 13 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 54 at r1 (raw file):

        };

        Ok(response.get_ref().value)

IIUC get_ref can be into_inner.

[Itay-Tsabary-Starkware]

Reviewable status: 2 of 10 files reviewed, 14 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 41 at r1 (raw file):

        Self { dst: construct_url(ip_address, port) }
    }
}

As a first step, please define the client once, and then define types for A and B clients. The construct_url function can be defined in the scope of ithe client implementation.

Code quote:

struct ComponentAClientRpc {
    dst: String,
}

impl ComponentAClientRpc {
    fn new(ip_address: IpAddr, port: u16) -> Self {
        Self { dst: construct_url(ip_address, port) }
    }
}

[Itay-Tsabary-Starkware]

Reviewable status: 2 of 10 files reviewed, 14 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 41 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

As a first step, please define the client once, and then define types for A and B clients. The construct_url function can be defined in the scope of ithe client implementation.

This will allow to delete the duplicated A and B clients

[Itay-Tsabary-Starkware]

Reviewable status: 2 of 10 files reviewed, 14 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 41 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

This will allow to delete the duplicated A and B clients

Also, move this definition to the src dir in a relevant module.

[Itay-Tsabary-Starkware]

Reviewable status: 2 of 10 files reviewed, 15 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 124 at r1 (raw file):

}

impl ComponentBServerRpc {

These can be generic in the component and the server.
See for example the channels definition:

impl<Component, Request, Response> ComponentServer<Component, Request, Response>

Also make sure to move these to a relevant module under src

Code quote:

struct ComponentBServerRpc {
    b: Option<ComponentB>,
    address: SocketAddr,
}

impl ComponentBServerRpc {

[Itay-Tsabary-Starkware]

Reviewable status: 2 of 10 files reviewed, 15 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 124 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

These can be generic in the component and the server.
See for example the channels definition:

impl<Component, Request, Response> ComponentServer<Component, Request, Response>

Also make sure to move these to a relevant module under src

And, wrap with a suitable type for A and B :-)

[uriel-starkware]

Reviewable status: 2 of 11 files reviewed, 15 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/mempool_infra/src/component_client.rs line 39 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please change to "Failed to connect to the server".

Done.

---

crates/mempool_infra/src/component_client.rs line 41 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please change to "Failed to get a response from the server".

Done.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 41 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Also, move this definition to the src dir in a relevant module.

Done.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 44 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please stick to the naming convention defined in the mempool: the trait name is XClient (X being the component name, in our case, A).

Done.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 47 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Tonic Result's captures information that is foregone by this; please propagate the error.

Done.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 51 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Ditto.

Done.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 54 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

IIUC get_ref can be into_inner.

Done.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 124 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

And, wrap with a suitable type for A and B :-)

My slack message is addressing this

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 140 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please revert and use expect.

Done.

---

crates/mempool_infra/tests/component_server_client_test.rs line 82 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Ditto.

Done.

---

crates/mempool_infra/tests/common/mod.rs line 10 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Rename according to the convention (mentioned in a different comment as well)

Done.

---

crates/mempool_infra/tests/common/mod.rs line 11 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please wrap in a type.

Done.

---

crates/mempool_infra/tests/common/mod.rs line 17 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Ditto on both changes.

Done.

---

crates/mempool_infra/tests/common/mod.rs line 20 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Change to arc (in a different pr), for B as well. See how the gateway uses the mempool client for inspiration.

will do

---

crates/mempool_infra/src/component_definitions.rs line 9 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

1. This should be in a different pr.
2. Why do you prefer this name?

Ok. I think this name is more concise and is shorter

[Itay-Tsabary-Starkware]

Reviewed 2 of 8 files at r2, all commit messages.
Reviewable status: 2 of 13 files reviewed, 10 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 44 at r1 (raw file):

Previously, uriel-starkware wrote…

Done.

The trait name should be AClient.
You can define a type alias for ComponentClientRpc<ComponentA>, e.g., ARpcClientImpl.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 47 at r1 (raw file):

Previously, uriel-starkware wrote…

Done.

IIUC this can be simplified using the map_err syntax, along with a ?
There's an example in the gateway.rs.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 51 at r1 (raw file):

Previously, uriel-starkware wrote…

Done.

Ditto ^

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 51 at r3 (raw file):

            Ok(client) => client,
            Err(e) => return Err(ClientError::ConnectionFailure(e)),
        };

map_err + ?

Code quote:

        let mut client = match RemoteBClient::connect(self.dst.clone()).await {
            Ok(client) => client,
            Err(e) => return Err(ClientError::ConnectionFailure(e)),
        };

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 54 at r3 (raw file):

        let response = match client.remote_b_get_value(BGetValueMessage {}).await {
            Ok(response) => response,

map_err + ?

---

crates/mempool_infra/tests/common/mod.rs line 10 at r1 (raw file):

Previously, uriel-starkware wrote…

Done.

Unblocking here, please change accoring to the other comment.

---

crates/mempool_infra/tests/common/mod.rs line 11 at r1 (raw file):

Previously, uriel-starkware wrote…

Done.

Great. Also please type alias AClientResult. Ditto for B.

[Itay-Tsabary-Starkware]

Reviewable status: 2 of 13 files reviewed, 11 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_infra/src/component_server_rpc.rs line 23 at r3 (raw file):

        self.component.take().unwrap().start_server(self.address).await;
    }
}

Let's break this down:

1. There can be a start_component fn in a designated trait for components to impl, that enable them to run whatever functionality they want on start up. I don't think this is relevant for our case.

2. There should be a start_server fn in a designated trait for servers to implement, in which they start their operation. This function can also call the inner component's start_component fn. In the rpc use case, the impl can be to start listening to messages (when the service is created at the server constructor).

3. The service and the component should be separated -- a component is not a service. That is, a component should not implement service/server functions.

4. The service/server should find a way to invoke the component. Check this trait from our code base for inspiration

#[async_trait]
pub trait ComponentRequestHandler<Request, Response> {
    async fn handle_request(&mut self, request: Request) -> Response;
}

Code quote:

#[async_trait]
pub trait ServerStart {
    async fn start_server(self, address: SocketAddr);
}

pub struct ComponentServerRpc<Component> {
    component: Option<Component>,
    address: SocketAddr,
}

impl<Component: ServerStart> ComponentServerRpc<Component> {
    pub fn new(component: Component, ip_address: IpAddr, port: u16) -> Self {
        Self { component: Some(component), address: SocketAddr::new(ip_address, port) }
    }

    pub async fn start(&mut self) {
        self.component.take().unwrap().start_server(self.address).await;
    }
}

[uriel-starkware]

Reviewable status: 2 of 13 files reviewed, 11 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 44 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

The trait name should be AClient.
You can define a type alias for ComponentClientRpc<ComponentA>, e.g., ARpcClientImpl.

Done.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 47 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

IIUC this can be simplified using the map_err syntax, along with a ?
There's an example in the gateway.rs.

Done.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 51 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Ditto ^

Done.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 51 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

map_err + ?

Done.

---

crates/mempool_infra/tests/component_server_client_rpc_test.rs line 54 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

map_err + ?

Done.

---

crates/mempool_infra/tests/common/mod.rs line 11 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Great. Also please type alias AClientResult. Ditto for B.

Done.