From de9f152c65eaa750ddc25d1b97957e63a94993b8 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 17 Jan 2023 15:09:57 +1100 Subject: [PATCH 1/5] Remove unnecessary visibility qualifiers --- swarm/src/dial_opts.rs | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/swarm/src/dial_opts.rs b/swarm/src/dial_opts.rs index acc1b69a617..4137a4a4d67 100644 --- a/swarm/src/dial_opts.rs +++ b/swarm/src/dial_opts.rs @@ -36,7 +36,7 @@ use std::num::NonZeroU8; /// /// - [`DialOpts::unknown_peer_id`] dialing an unknown peer #[derive(Debug)] -pub struct DialOpts(pub(super) Opts); +pub struct DialOpts(Opts); impl DialOpts { /// Dial a known peer. @@ -200,7 +200,7 @@ impl From for DialOpts { /// - [`DialOpts::peer_id`] dialing a known peer /// - [`DialOpts::unknown_peer_id`] dialing an unknown peer #[derive(Debug)] -pub(super) enum Opts { +enum Opts { WithPeerId(WithPeerId), WithPeerIdWithAddresses(WithPeerIdWithAddresses), WithoutPeerIdWithAddress(WithoutPeerIdWithAddress), @@ -208,10 +208,10 @@ pub(super) enum Opts { #[derive(Debug)] pub struct WithPeerId { - pub(crate) peer_id: PeerId, - pub(crate) condition: PeerCondition, - pub(crate) role_override: Endpoint, - pub(crate) dial_concurrency_factor_override: Option, + peer_id: PeerId, + condition: PeerCondition, + role_override: Endpoint, + dial_concurrency_factor_override: Option, } impl WithPeerId { @@ -262,12 +262,12 @@ impl WithPeerId { #[derive(Debug)] pub struct WithPeerIdWithAddresses { - pub(crate) peer_id: PeerId, - pub(crate) condition: PeerCondition, - pub(crate) addresses: Vec, - pub(crate) extend_addresses_through_behaviour: bool, - pub(crate) role_override: Endpoint, - pub(crate) dial_concurrency_factor_override: Option, + peer_id: PeerId, + condition: PeerCondition, + addresses: Vec, + extend_addresses_through_behaviour: bool, + role_override: Endpoint, + dial_concurrency_factor_override: Option, } impl WithPeerIdWithAddresses { @@ -323,8 +323,8 @@ impl WithoutPeerId { #[derive(Debug)] pub struct WithoutPeerIdWithAddress { - pub(crate) address: Multiaddr, - pub(crate) role_override: Endpoint, + address: Multiaddr, + role_override: Endpoint, } impl WithoutPeerIdWithAddress { From 7692c770a84e5eed08ba7b69fa743369e3b5a943 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 17 Jan 2023 15:11:32 +1100 Subject: [PATCH 2/5] Convert `DialOpts` to struct variant --- swarm/src/dial_opts.rs | 141 +++++++++++++++++++++++++---------------- 1 file changed, 87 insertions(+), 54 deletions(-) diff --git a/swarm/src/dial_opts.rs b/swarm/src/dial_opts.rs index 4137a4a4d67..1fb49ca1bd4 100644 --- a/swarm/src/dial_opts.rs +++ b/swarm/src/dial_opts.rs @@ -36,7 +36,9 @@ use std::num::NonZeroU8; /// /// - [`DialOpts::unknown_peer_id`] dialing an unknown peer #[derive(Debug)] -pub struct DialOpts(Opts); +pub struct DialOpts { + inner: Opts, +} impl DialOpts { /// Dial a known peer. @@ -74,11 +76,15 @@ impl DialOpts { /// Get the [`PeerId`] specified in a [`DialOpts`] if any. pub fn get_peer_id(&self) -> Option { match self { - DialOpts(Opts::WithPeerId(WithPeerId { peer_id, .. })) => Some(*peer_id), - DialOpts(Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { - peer_id, .. - })) => Some(*peer_id), - DialOpts(Opts::WithoutPeerIdWithAddress(_)) => None, + DialOpts { + inner: Opts::WithPeerId(WithPeerId { peer_id, .. }), + } => Some(*peer_id), + DialOpts { + inner: Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { peer_id, .. }), + } => Some(*peer_id), + DialOpts { + inner: Opts::WithoutPeerIdWithAddress(_), + } => None, } } @@ -93,13 +99,15 @@ impl DialOpts { /// See . pub(crate) fn get_or_parse_peer_id(&self) -> Result, Multihash> { match self { - DialOpts(Opts::WithPeerId(WithPeerId { peer_id, .. })) => Ok(Some(*peer_id)), - DialOpts(Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { - peer_id, .. - })) => Ok(Some(*peer_id)), - DialOpts(Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { - address, .. - })) => { + DialOpts { + inner: Opts::WithPeerId(WithPeerId { peer_id, .. }), + } => Ok(Some(*peer_id)), + DialOpts { + inner: Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { peer_id, .. }), + } => Ok(Some(*peer_id)), + DialOpts { + inner: Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { address, .. }), + } => { let peer_id = address .iter() .last() @@ -119,64 +127,83 @@ impl DialOpts { pub(crate) fn get_addresses(&self) -> Vec { match self { - DialOpts(Opts::WithPeerId(WithPeerId { .. })) => vec![], - DialOpts(Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { - addresses, .. - })) => addresses.clone(), - DialOpts(Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { - address, .. - })) => vec![address.clone()], + DialOpts { + inner: Opts::WithPeerId(WithPeerId { .. }), + } => vec![], + DialOpts { + inner: Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { addresses, .. }), + } => addresses.clone(), + DialOpts { + inner: Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { address, .. }), + } => vec![address.clone()], } } pub(crate) fn extend_addresses_through_behaviour(&self) -> bool { match self { - DialOpts(Opts::WithPeerId(WithPeerId { .. })) => true, - DialOpts(Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { - extend_addresses_through_behaviour, - .. - })) => *extend_addresses_through_behaviour, - DialOpts(Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { .. })) => true, + DialOpts { + inner: Opts::WithPeerId(WithPeerId { .. }), + } => true, + DialOpts { + inner: + Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { + extend_addresses_through_behaviour, + .. + }), + } => *extend_addresses_through_behaviour, + DialOpts { + inner: Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { .. }), + } => true, } } pub(crate) fn peer_condition(&self) -> PeerCondition { match self { - DialOpts( - Opts::WithPeerId(WithPeerId { condition, .. }) - | Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { condition, .. }), - ) => *condition, - DialOpts(Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { .. })) => { - PeerCondition::Always - } + DialOpts { + inner: + Opts::WithPeerId(WithPeerId { condition, .. }) + | Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { condition, .. }), + } => *condition, + DialOpts { + inner: Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { .. }), + } => PeerCondition::Always, } } pub(crate) fn dial_concurrency_override(&self) -> Option { match self { - DialOpts(Opts::WithPeerId(WithPeerId { - dial_concurrency_factor_override, - .. - })) => *dial_concurrency_factor_override, - DialOpts(Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { - dial_concurrency_factor_override, - .. - })) => *dial_concurrency_factor_override, - DialOpts(Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { .. })) => None, + DialOpts { + inner: + Opts::WithPeerId(WithPeerId { + dial_concurrency_factor_override, + .. + }), + } => *dial_concurrency_factor_override, + DialOpts { + inner: + Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { + dial_concurrency_factor_override, + .. + }), + } => *dial_concurrency_factor_override, + DialOpts { + inner: Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { .. }), + } => None, } } pub(crate) fn role_override(&self) -> Endpoint { match self { - DialOpts(Opts::WithPeerId(WithPeerId { role_override, .. })) => *role_override, - DialOpts(Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { - role_override, - .. - })) => *role_override, - DialOpts(Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { - role_override, - .. - })) => *role_override, + DialOpts { + inner: Opts::WithPeerId(WithPeerId { role_override, .. }), + } => *role_override, + DialOpts { + inner: Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { role_override, .. }), + } => *role_override, + DialOpts { + inner: + Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { role_override, .. }), + } => *role_override, } } } @@ -256,7 +283,9 @@ impl WithPeerId { /// Addresses to dial the peer are retrieved via /// [`NetworkBehaviour::addresses_of_peer`](crate::behaviour::NetworkBehaviour::addresses_of_peer). pub fn build(self) -> DialOpts { - DialOpts(Opts::WithPeerId(self)) + DialOpts { + inner: Opts::WithPeerId(self), + } } } @@ -304,7 +333,9 @@ impl WithPeerIdWithAddresses { /// Build the final [`DialOpts`]. pub fn build(self) -> DialOpts { - DialOpts(Opts::WithPeerIdWithAddresses(self)) + DialOpts { + inner: Opts::WithPeerIdWithAddresses(self), + } } } @@ -340,7 +371,9 @@ impl WithoutPeerIdWithAddress { } /// Build the final [`DialOpts`]. pub fn build(self) -> DialOpts { - DialOpts(Opts::WithoutPeerIdWithAddress(self)) + DialOpts { + inner: Opts::WithoutPeerIdWithAddress(self), + } } } From b492236f8cdeb6a0bc82326b454777b9b8fb6321 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 17 Jan 2023 15:25:27 +1100 Subject: [PATCH 3/5] Eagerly set defaults in `DialOpts` Instead of carrying around an enum and parsing its contents later, directly set the correct values as we construct the final `DialOpts`. --- swarm/src/dial_opts.rs | 170 +++++++++++++---------------------------- 1 file changed, 51 insertions(+), 119 deletions(-) diff --git a/swarm/src/dial_opts.rs b/swarm/src/dial_opts.rs index 1fb49ca1bd4..5362ad99e91 100644 --- a/swarm/src/dial_opts.rs +++ b/swarm/src/dial_opts.rs @@ -37,7 +37,12 @@ use std::num::NonZeroU8; /// - [`DialOpts::unknown_peer_id`] dialing an unknown peer #[derive(Debug)] pub struct DialOpts { - inner: Opts, + peer_id: Option, + condition: PeerCondition, + addresses: Vec, + extend_addresses_through_behaviour: bool, + role_override: Endpoint, + dial_concurrency_factor_override: Option, } impl DialOpts { @@ -75,17 +80,7 @@ impl DialOpts { /// Get the [`PeerId`] specified in a [`DialOpts`] if any. pub fn get_peer_id(&self) -> Option { - match self { - DialOpts { - inner: Opts::WithPeerId(WithPeerId { peer_id, .. }), - } => Some(*peer_id), - DialOpts { - inner: Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { peer_id, .. }), - } => Some(*peer_id), - DialOpts { - inner: Opts::WithoutPeerIdWithAddress(_), - } => None, - } + self.peer_id } /// Retrieves the [`PeerId`] from the [`DialOpts`] if specified or otherwise tries to parse it @@ -98,113 +93,48 @@ impl DialOpts { /// /// See . pub(crate) fn get_or_parse_peer_id(&self) -> Result, Multihash> { - match self { - DialOpts { - inner: Opts::WithPeerId(WithPeerId { peer_id, .. }), - } => Ok(Some(*peer_id)), - DialOpts { - inner: Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { peer_id, .. }), - } => Ok(Some(*peer_id)), - DialOpts { - inner: Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { address, .. }), - } => { - let peer_id = address - .iter() - .last() - .and_then(|p| { - if let Protocol::P2p(ma) = p { - Some(PeerId::try_from(ma)) - } else { - None - } - }) - .transpose()?; - - Ok(peer_id) - } + if let Some(peer_id) = self.peer_id { + return Ok(Some(peer_id)) } + + let first_address = match self.addresses.first() { + Some(first_address) => first_address, + None => return Ok(None) + }; + + let maybe_peer_id = first_address + .iter() + .last() + .and_then(|p| { + if let Protocol::P2p(ma) = p { + Some(PeerId::try_from(ma)) + } else { + None + } + }) + .transpose()?; + + Ok(maybe_peer_id) } pub(crate) fn get_addresses(&self) -> Vec { - match self { - DialOpts { - inner: Opts::WithPeerId(WithPeerId { .. }), - } => vec![], - DialOpts { - inner: Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { addresses, .. }), - } => addresses.clone(), - DialOpts { - inner: Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { address, .. }), - } => vec![address.clone()], - } + self.addresses.clone() } pub(crate) fn extend_addresses_through_behaviour(&self) -> bool { - match self { - DialOpts { - inner: Opts::WithPeerId(WithPeerId { .. }), - } => true, - DialOpts { - inner: - Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { - extend_addresses_through_behaviour, - .. - }), - } => *extend_addresses_through_behaviour, - DialOpts { - inner: Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { .. }), - } => true, - } + self.extend_addresses_through_behaviour } pub(crate) fn peer_condition(&self) -> PeerCondition { - match self { - DialOpts { - inner: - Opts::WithPeerId(WithPeerId { condition, .. }) - | Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { condition, .. }), - } => *condition, - DialOpts { - inner: Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { .. }), - } => PeerCondition::Always, - } + self.condition } pub(crate) fn dial_concurrency_override(&self) -> Option { - match self { - DialOpts { - inner: - Opts::WithPeerId(WithPeerId { - dial_concurrency_factor_override, - .. - }), - } => *dial_concurrency_factor_override, - DialOpts { - inner: - Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { - dial_concurrency_factor_override, - .. - }), - } => *dial_concurrency_factor_override, - DialOpts { - inner: Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { .. }), - } => None, - } + self.dial_concurrency_factor_override } pub(crate) fn role_override(&self) -> Endpoint { - match self { - DialOpts { - inner: Opts::WithPeerId(WithPeerId { role_override, .. }), - } => *role_override, - DialOpts { - inner: Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { role_override, .. }), - } => *role_override, - DialOpts { - inner: - Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { role_override, .. }), - } => *role_override, - } + self.role_override } } @@ -220,19 +150,6 @@ impl From for DialOpts { } } -/// Internal options type. -/// -/// Not to be constructed manually. Use either of the below instead: -/// -/// - [`DialOpts::peer_id`] dialing a known peer -/// - [`DialOpts::unknown_peer_id`] dialing an unknown peer -#[derive(Debug)] -enum Opts { - WithPeerId(WithPeerId), - WithPeerIdWithAddresses(WithPeerIdWithAddresses), - WithoutPeerIdWithAddress(WithoutPeerIdWithAddress), -} - #[derive(Debug)] pub struct WithPeerId { peer_id: PeerId, @@ -284,7 +201,12 @@ impl WithPeerId { /// [`NetworkBehaviour::addresses_of_peer`](crate::behaviour::NetworkBehaviour::addresses_of_peer). pub fn build(self) -> DialOpts { DialOpts { - inner: Opts::WithPeerId(self), + peer_id: Some(self.peer_id), + condition: self.condition, + addresses: vec![], + extend_addresses_through_behaviour: false, + role_override: self.role_override, + dial_concurrency_factor_override: self.dial_concurrency_factor_override, } } } @@ -334,7 +256,12 @@ impl WithPeerIdWithAddresses { /// Build the final [`DialOpts`]. pub fn build(self) -> DialOpts { DialOpts { - inner: Opts::WithPeerIdWithAddresses(self), + peer_id: Some(self.peer_id), + condition: self.condition, + addresses: self.addresses, + extend_addresses_through_behaviour: self.extend_addresses_through_behaviour, + role_override: self.role_override, + dial_concurrency_factor_override: self.dial_concurrency_factor_override, } } } @@ -372,7 +299,12 @@ impl WithoutPeerIdWithAddress { /// Build the final [`DialOpts`]. pub fn build(self) -> DialOpts { DialOpts { - inner: Opts::WithoutPeerIdWithAddress(self), + peer_id: None, + condition: PeerCondition::Always, + addresses: vec![self.address], + extend_addresses_through_behaviour: false, + role_override: self.role_override, + dial_concurrency_factor_override: None, } } } From 3547b53dd5a7c5ad883aaa89a9fc55025cf4b3a5 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 17 Jan 2023 15:42:26 +1100 Subject: [PATCH 4/5] Rustfmt --- swarm/src/dial_opts.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/swarm/src/dial_opts.rs b/swarm/src/dial_opts.rs index 5362ad99e91..5cbb5370d18 100644 --- a/swarm/src/dial_opts.rs +++ b/swarm/src/dial_opts.rs @@ -94,12 +94,12 @@ impl DialOpts { /// See . pub(crate) fn get_or_parse_peer_id(&self) -> Result, Multihash> { if let Some(peer_id) = self.peer_id { - return Ok(Some(peer_id)) + return Ok(Some(peer_id)); } let first_address = match self.addresses.first() { Some(first_address) => first_address, - None => return Ok(None) + None => return Ok(None), }; let maybe_peer_id = first_address From 75b30ac00315468dfa884524fdd46e1739b2be5b Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 17 Jan 2023 15:44:14 +1100 Subject: [PATCH 5/5] Fix wrong default --- swarm/src/dial_opts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/swarm/src/dial_opts.rs b/swarm/src/dial_opts.rs index 5cbb5370d18..eb375c6ce74 100644 --- a/swarm/src/dial_opts.rs +++ b/swarm/src/dial_opts.rs @@ -204,7 +204,7 @@ impl WithPeerId { peer_id: Some(self.peer_id), condition: self.condition, addresses: vec![], - extend_addresses_through_behaviour: false, + extend_addresses_through_behaviour: true, role_override: self.role_override, dial_concurrency_factor_override: self.dial_concurrency_factor_override, }