From d82c2a1a7a4032a4715bd079043a7609ef56080e Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 20 Jan 2023 10:30:09 +1100 Subject: [PATCH] refactor(swarm): simplify `DialOpts` (#3335) Instead of tracking an inner enum, move all fields of `Opts` into `DialOpts`, setting them directly once we construct them. This makes the getters a lot simpler to implement and reduces code duplication. --- swarm/src/dial_opts.rs | 177 +++++++++++++++++------------------------ 1 file changed, 71 insertions(+), 106 deletions(-) diff --git a/swarm/src/dial_opts.rs b/swarm/src/dial_opts.rs index acc1b69a617..eb375c6ce74 100644 --- a/swarm/src/dial_opts.rs +++ b/swarm/src/dial_opts.rs @@ -36,7 +36,14 @@ use std::num::NonZeroU8; /// /// - [`DialOpts::unknown_peer_id`] dialing an unknown peer #[derive(Debug)] -pub struct DialOpts(pub(super) Opts); +pub struct DialOpts { + peer_id: Option, + condition: PeerCondition, + addresses: Vec, + extend_addresses_through_behaviour: bool, + role_override: Endpoint, + dial_concurrency_factor_override: Option, +} impl DialOpts { /// Dial a known peer. @@ -73,13 +80,7 @@ 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, - } + self.peer_id } /// Retrieves the [`PeerId`] from the [`DialOpts`] if specified or otherwise tries to parse it @@ -92,92 +93,48 @@ 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, .. - })) => { - 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(Opts::WithPeerId(WithPeerId { .. })) => vec![], - DialOpts(Opts::WithPeerIdWithAddresses(WithPeerIdWithAddresses { - addresses, .. - })) => addresses.clone(), - DialOpts(Opts::WithoutPeerIdWithAddress(WithoutPeerIdWithAddress { - address, .. - })) => vec![address.clone()], - } + self.addresses.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, - } + self.extend_addresses_through_behaviour } 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 - } - } + self.condition } 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, - } + self.dial_concurrency_factor_override } 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, - } + self.role_override } } @@ -193,25 +150,12 @@ 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)] -pub(super) enum Opts { - WithPeerId(WithPeerId), - WithPeerIdWithAddresses(WithPeerIdWithAddresses), - WithoutPeerIdWithAddress(WithoutPeerIdWithAddress), -} - #[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 { @@ -256,18 +200,25 @@ 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 { + peer_id: Some(self.peer_id), + condition: self.condition, + addresses: vec![], + extend_addresses_through_behaviour: true, + role_override: self.role_override, + dial_concurrency_factor_override: self.dial_concurrency_factor_override, + } } } #[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 { @@ -304,7 +255,14 @@ impl WithPeerIdWithAddresses { /// Build the final [`DialOpts`]. pub fn build(self) -> DialOpts { - DialOpts(Opts::WithPeerIdWithAddresses(self)) + DialOpts { + 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, + } } } @@ -323,8 +281,8 @@ impl WithoutPeerId { #[derive(Debug)] pub struct WithoutPeerIdWithAddress { - pub(crate) address: Multiaddr, - pub(crate) role_override: Endpoint, + address: Multiaddr, + role_override: Endpoint, } impl WithoutPeerIdWithAddress { @@ -340,7 +298,14 @@ impl WithoutPeerIdWithAddress { } /// Build the final [`DialOpts`]. pub fn build(self) -> DialOpts { - DialOpts(Opts::WithoutPeerIdWithAddress(self)) + DialOpts { + 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, + } } }