RFC: Network::receive only returns the desired type of message

[akhi3030]

Currently, whenever we are doing Network::receive(), we need to handle the different variants that we do not care about. In most cases, we ignore the other variants and we simply call receive() again.

The idea here is to introduce a way to get the Network to keep looping till it receives the variant we care about. The PR is still incomplete. I wanted to get some feedback on whether this idea makes sense or not before I do the work to finish it up.

We introduce a new trait FromNetworkMessage. This trait is implemented by the various types of network messages. Then instead of calling receive(), we call e.g. receive::<RepairMessage>() and do not have to write additional looping code.

The only interesting part of the RFC is in src/network.rs. You can ignore the rest of the changes.

[codecov]

Codecov Report

❌ Patch coverage is 70.00000% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.78%. Comparing base (c4aa729) to head (c72ae2e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/network.rs	0.00%	5 Missing ⚠️
src/network/simulated/core.rs	55.55%	4 Missing ⚠️
src/network/simulated.rs	66.66%	2 Missing ⚠️
src/network/tcp.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
- Coverage   86.83%   86.78%   -0.06%     
==========================================
  Files          41       41              
  Lines        7864     7869       +5     
==========================================
  Hits         6829     6829              
- Misses       1035     1040       +5     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[qkniep]

Seems almost like manually reimplementing TryFrom

[qkniep]

I think it would be better to split up NetworkMessage. I am no longer convinced having a single network message type was good to begin with. Then each Network could be a Network<T> and only work with messages of type T. Instead of having NetworkMessage as a type itself we might want to turn it into a trait, so actually it would be Network<T: NetworkMessage>?

[qkniep]

Or, thinking of repair, it could even be Network<S: NetworkMessage, R: NetworkMessage> for two (potentially different) send and receive message types.

[akhi3030]

Hmm... how about making things even simpler.

We define Network as something like below:

pub trait Network {
    async fn send<T: Serialize>(&self, message: &T, to: SocketAddr) -> Result<(), NetworkSendError> {
        // implemented by using send_serialized below
    }

    // definition provided by the structs actually implementing this trait
    async fn send_serialized(&self, bytes: &[u8], to: SocketAddr) -> Result<(), NetworkSendError>;


    async fn receive<T: DeserializeOwned>(&self) -> Result<T, NetworkReceiveError> {
        // implemented by using receive_raw below.
    }

    // definition provided by the structs actually implementing this trait
    async fn receive_raw(&self, buf: &mut [u8]) -> Result<usize, NetworkReceiveError>;
}

Now components and send and receive anything they want as long as it can be [de]serialized.

[qkniep]

I'm not sure this would help. I also don't know what the purpose would be to try to receive different message types, if we receive them not in the expected order all calls to receive will fail.

And then you give the callers of send and receive a lot of responsibility: If I want to call receive on e.g. repair_network, I first have to figure out that I have to call repair_network.receive::<RepairResponse>(), and if anyone ever calls send with the wrong type this results in an error that will only be caught by receive or its caller.

[qkniep]

After consulting ChatGPT, I came up with this middle-ground approach using associated types. This keeps the type parameters out of any uses of T: Network and dyn Network. One other downside of the generics-on-functions approach is that it makes the Network trait incompatible to be used with dyn.

trait Network {
    type Send;
    type Recv;

    fn send(&self, msg: &Self::Send) -> anyhow::Result<()>;
    fn recv(&self) -> anyhow::Result<Self::Recv>;
}

struct UdpNetwork<S: Encode, R: Decode> { /* ... */ }

impl<S: Encode, R: Decode> Network for UdpNetwork<S, R> {
    type Send = S;
    type Recv = R;
    /* ... */
}

Also works with type inference:

let socket = UdpNetwork::new();
socket.send(&"hello".to_string()).unwrap();
let socket: String = udp.recv().unwrap();