RPC module de business logicification

[ceyhunsen]

Description

This PR moves out business logic code from RPC module.

• Adds another transportation layer (tokio mpsc channels) between actor modules and rpc module (only for streams) in order to achieve this
• Maintains async nature of the streams
• Adds temporary expect() calls for error handling in tokio threads, soon to be fixed by Handling errors in RPC Tokio tasks #502
• Only required changes to the actual code apart from moving the code
• Tests are untouched to ensure previous working state is preserved

Linked Issues

• Related Clean-up RPC module from business logic and code repetition (for encoding/decoding) #469
• Closes Move business logic code outside of rpc module #515

[mmtftr]

I believe we can write the stream operations more clearly. I've added some improvements. Some may be out of scope but I believe some others are much needed simplification of stream handling in our codebase.

[mmtftr]

why are these panics?

these look very much like recoverable errors. Please return recoverable errors here.

[ceyhunsen]

All of the new expects should be inside tokio::spawns. Because we didn't decided how we will handle this errors yet, I just left them as is. I think it is better to convert them while working on #502 because Results will be discarded anyways rn. For today, we can identify which line returned an error while testing.

[mmtftr]

I really can't accept including recoverable errors as panics into the codebase when they can be returned properly. I think it's a matter of our code quality standards that we don't include such code. Panics have the ability to put objects in invalid states.

I also disagree that the first solution #502 is an acceptable solution. Panicking should be reserved for unrecoverable errors.

I can't think of a valid reason to keep panics in. We removed unwraps to avoid panics in our runtime, let's not abuse except for recoverable errors.

[ceyhunsen]

Yeah, you are right. Fixed in 0b5f52b.

[mmtftr]

I believe the following happens here:

This background task will complete immediately, since the buffer length == total items sent.

If that's the case, this doesn't need to be a background task, simply send the entire vec into the channel within the functino. You know for a fact that it won't block. Since the tx is dropped, the rx will drop values as they're consumed.

In fact, the semantics would be even better if you returned:

ReceiverStream::from(stream::iter(winternitz_public_keys))

which will likely have the same effect but gets rid of the extra background task and all the confusing error handling (which is nothing but handling the closure and early dropping of each stream)

[ceyhunsen]

Good catch. This approach is only useful if calculations can be made async but in this case it is serial. Fixed in 22a9b2c