feat: add relay server functionality#101
Conversation
varex83
changed the title
|
Coverage (base → head): |
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
A couple of questions and comments based on the extension of the PR.
|
|
||
| /// Shared application state for HTTP handlers. | ||
| #[derive(Clone)] | ||
| pub struct AppState { |
There was a problem hiding this comment.
AppState can be private to the module
There was a problem hiding this comment.
Not sure I follow the change? We can remove the pub modifier here, follow the warnings and remove the associated pub modifiers since there are no usages outside this module.
There was a problem hiding this comment.
I just made a whole module accessible only on crate level
/// Web.
pub(crate) mod web;
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
LGTM, some small details only.
|
|
||
| /// Shared application state for HTTP handlers. | ||
| #[derive(Clone)] | ||
| pub struct AppState { |
There was a problem hiding this comment.
Not sure I follow the change? We can remove the pub modifier here, follow the warnings and remove the associated pub modifiers since there are no usages outside this module.
| fn default() -> Self { | ||
| Self { | ||
| gater: None, | ||
| identify_protocol: "/pluto/relay/1.0.0-alpha".into(), |
There was a problem hiding this comment.
Just for consistency, the same way that we have defaults in PlutoBehaviourBuilder we should have them in RelayServerBehaviourBuilder (extract the identify_protocol into a lazy static)
| return Err(error); | ||
| } | ||
| }, | ||
| event = node.swarm.select_next_some() => { |
There was a problem hiding this comment.
Mostly due to code structuring: this block that handles events will grow larger (according to the TODO) and having everything inside this function might get complicated in the long run, that's why I mentioned that this will need to be revisited in a future PR (we might not even need to do anything).
| // todo(varex83): check if this is correct, since it's aligned with the original | ||
| // implementation, but I'm not sure if it's the correct way to do it. | ||
| // Would it be better to use max_res_per_peer * max_conns? |
There was a problem hiding this comment.
A couple of things:
- The Go implementation does not have an equivalent of
max_circuits_per_peer max_circuitsmatches the GoMaxCircuit(based on default values and docs), and in the original implementation this is set toconfig.MaxResPerPeer. This means thatmax_circuits: config.max_res_per_peeris correct.
Due to the confusion, I'd leave it as a TODO and revisit it later. We might need to ask someone with a bit more experience in libp2p about it.
2 participants
No description provided.