Add a session::tokio module with a convenience function for executing a session
#91
Conversation
Pull Request Test Coverage Report for Build 13172264691Details
💛 - Coveralls |
|
@ameba23 this is supposed to be the replacement for https://github.com/entropyxyz/entropy-core/blob/master/crates/protocol/src/execute_protocol.rs#L73 so that you don't have to worry about executing the session correctly. Does the API look okay to you? I noticed that you're taking the channels by value and then returning them; will the proposed API with references work? Also, do you want more/less/different tracing messages? |
|
|
||
| /// The outgoing message from a local session. | ||
| #[derive(Debug)] | ||
| pub struct MessageOut<SP: SessionParameters> { |
There was a problem hiding this comment.
The MessageOut and MessageIn type do not seem to be tokio-specific. Are they generally useful you reckon or will users want to implement their own versions? If the latter, why would they want that?
There was a problem hiding this comment.
These are just named tuples with returned stuff. The user is free to re-wrap it differently.
| /// The outgoing message from a local session. | ||
| #[derive(Debug)] | ||
| pub struct MessageOut<SP: SessionParameters> { | ||
| /// The ID of the session that created the message. |
There was a problem hiding this comment.
Is this accurate? Given there is a public SessionId type, it is a bit confusing to read this and then read SP::Verifier which boils down to a PartyId. Is it "The ID of the party that created the message in this session" perhaps?
| /// The incoming message from a remote session. | ||
| #[derive(Debug)] | ||
| pub struct MessageIn<SP: SessionParameters> { | ||
| /// The ID of the session the message originated from. |
| // will be stored here and applied after the messages for this round are sent. | ||
| let mut cached_messages = Vec::new(); | ||
|
|
||
| let key = session.verifier(); |
There was a problem hiding this comment.
| let key = session.verifier(); | |
| let verifier = session.verifier(); |
…or something more specific to its role? Maybe just from?
| .map_err(|err| { | ||
| LocalError::new(format!( | ||
| "Failed to send a message from {:?} to {:?}: {err}", | ||
| session.verifier(), |
There was a problem hiding this comment.
| session.verifier(), | |
| key, |
Great that this is going in and we wont have to worry about / maintain this anymore ⭐ The reason we are returning the channels is so that we can use the same channels and websocket connection for the sub-protocols of the following:
So if synedrion were to offer these high level protocols in a single session, we would not need to do this and from our point of view everything would be much simpler. I expect that passing references to the channels will work, but i will check with our current code. But what might be a problem is that the payloads of these channels currently contain both the synedrion message and the session ID, to allow us to have messages from the different subprotocols and be sure that we are processing the right message for the current protocol: The only other thing that we would lose by using this is that we are calling The amount of logging looks great. Tagging @JesseAbram @HCastano |
This can be included in
That's exactly #22 which I mentioned above, so it'll be included as well. I saw that you have some offloading in place, but I wonder, wouldn't it be better to use |
It seems like we do need it in
Thats great!
I've never used |
Fixes #28
Add
session::tokio::run_session()and supporting types. Gated behind thetokiofeature.TODO: deal with #22 as well?