-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(connection-pool): Keep only fluence peers in the connection pool #1440
Conversation
58a5468
to
640f13f
Compare
crates/kademlia/src/behaviour.rs
Outdated
@@ -160,7 +160,7 @@ impl Kademlia { | |||
} | |||
} | |||
|
|||
pub fn add_kad_node(&mut self, peer: PeerId, addresses: Vec<Multiaddr>) { | |||
pub fn add_kad_node(&mut self, peer: PeerId, addresses: &Vec<Multiaddr>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&[]
? or clippy will complain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
connection-pool/src/behaviour.rs
Outdated
@@ -250,12 +251,12 @@ impl ConnectionPoolBehaviour { | |||
self.subscribers.push(outlet); | |||
} | |||
|
|||
pub fn add_discovered_addresses(&mut self, peer_id: PeerId, addresses: Vec<Multiaddr>) { | |||
pub fn add_discovered_addresses(&mut self, peer_id: PeerId, addresses: &[Multiaddr]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You clone unconditionally, maybe no need to accept &[Multiaddr]
and accept Vec
instead? So that the calling side is responsible for cloning and it's more transparent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
crates/kademlia/src/behaviour.rs
Outdated
@@ -160,7 +160,7 @@ impl Kademlia { | |||
} | |||
} | |||
|
|||
pub fn add_kad_node(&mut self, peer: PeerId, addresses: Vec<Multiaddr>) { | |||
pub fn add_kad_node(&mut self, peer: PeerId, addresses: &[Multiaddr]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Maybe pass Vec
if we clone it anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
} else { | ||
log::debug!( | ||
target: "protocols", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets change to target: network
or target: blocked
. protocols
is too narrow, don't think we'll have much of these so no sense in grouping them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -61,18 +90,18 @@ impl FluenceNetworkBehaviour { | |||
} | |||
} | |||
|
|||
fn filter_addresses(addresses: Vec<Multiaddr>, allow_local: bool) -> Vec<Multiaddr> { | |||
fn filter_addresses(addresses: &[Multiaddr], allow_local: bool) -> Vec<Multiaddr> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you also clone in all cases, so makes sense to pass Vec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -160,7 +160,7 @@ impl Kademlia { | |||
} | |||
} | |||
|
|||
pub fn add_kad_node(&mut self, peer: PeerId, addresses: &[Multiaddr]) { | |||
pub fn add_kad_node(&mut self, peer: PeerId, addresses: Vec<Multiaddr>) { | |||
for addr in addresses { | |||
self.kademlia.add_address(&peer, addr.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to remove .clone
No description provided.