Fixup comments

mullvad-relay-selector/src/relay_selector/detailer.rs

@@ -2,10 +2,10 @@
 //! OpenVPN relay chosen by the relay selector.
 //!
 //! [`MullvadEndpoint`] contains all the necessary information for establishing a connection
-//! between the client and Mullvad VPN. It is the daemon's responsibillity of actually establishing
-//! this connection.
+//! between the client and Mullvad VPN. It is the daemon's responsibility to establish this
+//! connection.
 //!
-//! TODO(markus): Revise comments
+//! [`MullvadEndpoint`]: mullvad_types::endpoint::MullvadEndpoint
 
 use std::net::{IpAddr, SocketAddr, SocketAddrV4};
 

mullvad-relay-selector/src/relay_selector/helpers.rs

@@ -45,8 +45,20 @@ pub fn pick_random_relay_fn<RelayType>(
     if total_weight == 0 {
         relays.choose(&mut rng)
     } else {
+        // Assign each relay a subset of the range 0..total_weight with size equal to its weight.
         // Pick a random number in the range 1..=total_weight. This choses the relay with a
         // non-zero weight.
+        //
+        //                                rng(1..=total_weight)
+        //                                |
+        //                                v
+        //   _____________________________i___________________________________________________
+        // 0|_____________|__________________________|___________|_____|___________|__________| total_weight
+        //  ^             ^                          ^                             ^          ^
+        //  |             |                          |                             |          |
+        //  ------------------------------------------                             ------------
+        //         |                    |                                                |
+        //   weight(relay 0)       weight(relay 1)        ..       ..       ..     weight(relay n)
         let mut i: u64 = rng.gen_range(1..=total_weight);
         Some(
             relays

mullvad-relay-selector/src/relay_selector/mod.rs

@@ -535,6 +535,8 @@ impl RelaySelector {
     /// * An `Err` if no entry relay can be chosen (if multihop is enabled on `query`)
     /// * an `Err` if no [`MullvadEndpoint`] can be derived from the selected relay(s).
     /// * `Ok(GetRelay::Wireguard)` otherwise
+    ///
+    /// [`MullvadEndpoint`]: mullvad_types::endpoint::MullvadEndpoint
     fn get_wireguard_relay(
         query: &RelayQuery,
         config: &NormalSelectorConfig<'_>,
@@ -623,7 +625,9 @@ impl RelaySelector {
         })
     }
 
-    /// Constructs a `MullvadEndpoint` with details for how to connect to `relay`.
+    /// Constructs a [`MullvadEndpoint`] with details for how to connect to `relay`.
+    ///
+    /// [`MullvadEndpoint`]: mullvad_types::endpoint::MullvadEndpoint
     fn get_wireguard_endpoint(
         query: &RelayQuery,
         parsed_relays: &ParsedRelays,
@@ -671,6 +675,8 @@ impl RelaySelector {
     /// * An `Err` if no entry bridge can be chosen (if bridge mode is enabled on `query`)
     /// * an `Err` if no [`MullvadEndpoint`] can be derived from the selected relay
     /// * `Ok(GetRelay::OpenVpn)` otherwise
+    ///
+    /// [`MullvadEndpoint`]: mullvad_types::endpoint::MullvadEndpoint
     #[cfg(not(target_os = "android"))]
     fn get_openvpn_relay(
         query: &RelayQuery,
@@ -689,7 +695,9 @@ impl RelaySelector {
         })
     }
 
-    /// Constructs a `MullvadEndpoint` with details for how to connect to `relay`.
+    /// Constructs a [`MullvadEndpoint`] with details for how to connect to `relay`.
+    ///
+    /// [`MullvadEndpoint`]: mullvad_types::endpoint::MullvadEndpoint
     #[cfg(not(target_os = "android"))]
     fn get_openvpn_endpoint(
         query: &RelayQuery,

mullvad-relay-selector/src/relay_selector/query.rs

@@ -79,8 +79,20 @@ pub struct RelayQuery {
 }
 
 impl RelayQuery {
-    /// Create a new [`RelayQuery`] with no opinionated defaults. This
-    /// should be the const equivalent to [`Default::default`].
+    /// Create a new [`RelayQuery`] with no opinionated defaults. This query matches every relay
+    /// with any configuration by setting each of its fields to [`Constraint::Any`]. Should be the
+    /// const equivalent to [`Default::default`].
+    ///
+    /// Note that the following identity applies for any `other_query`:
+    /// ```rust
+    /// # use mullvad_relay_selector::query::RelayQuery;
+    /// # use crate::mullvad_relay_selector::query::Intersection;
+    ///
+    /// # let other_query = RelayQuery::new();
+    /// assert_eq!(RelayQuery::new().intersection(other_query.clone()), Some(other_query));
+    /// # let other_query = RelayQuery::new();
+    /// assert_eq!(other_query.clone().intersection(RelayQuery::new()), Some(other_query));
+    /// ```
     pub const fn new() -> RelayQuery {
         RelayQuery {
             location: Constraint::Any,
@@ -94,20 +106,84 @@ impl RelayQuery {
 }
 
 impl Intersection for RelayQuery {
-    /// `intersection` defines a cautious merge strategy between two
-    /// [`RelayQuery`].
+    /// Return a new [`RelayQuery`] which matches the intersected queries.
     ///
-    /// * If two [`RelayQuery`] differ in any configuration such that no
-    /// consensus can be reached, the two [`RelayQuery`] are said to be
-    /// incompatible and `intersection` returns [`Option::None`].
+    /// * If two [`RelayQuery`]s differ such that no relay matches both, [`Option::None`] is returned:
+    /// ```rust
+    /// # use mullvad_relay_selector::query::builder::RelayQueryBuilder;
+    /// # use crate::mullvad_relay_selector::query::Intersection;
+    /// let query_a = RelayQueryBuilder::new().wireguard().build();
+    /// let query_b = RelayQueryBuilder::new().openvpn().build();
+    /// assert_eq!(query_a.intersection(query_b), None);
+    /// ```
     ///
     /// * Otherwise, a new [`RelayQuery`] is returned where each constraint is
     /// as specific as possible. See [`Constraint`] for further details.
+    /// ```rust
+    /// # use crate::mullvad_relay_selector::*;
+    /// # use crate::mullvad_relay_selector::query::*;
+    /// # use crate::mullvad_relay_selector::query::builder::*;
+    /// # use mullvad_types::relay_list::*;
+    /// # use talpid_types::net::wireguard::PublicKey;
+    ///
+    /// // The relay list used by `relay_selector` in this example
+    /// let relay_list = RelayList {
+    /// #   etag: None,
+    /// #   openvpn: OpenVpnEndpointData { ports: vec![] },
+    /// #   bridge: BridgeEndpointData {
+    /// #       shadowsocks: vec![],
+    /// #   },
+    /// #   wireguard: WireguardEndpointData {
+    /// #       port_ranges: vec![(53, 53), (4000, 33433), (33565, 51820), (52000, 60000)],
+    /// #       ipv4_gateway: "10.64.0.1".parse().unwrap(),
+    /// #       ipv6_gateway: "fc00:bbbb:bbbb:bb01::1".parse().unwrap(),
+    /// #       udp2tcp_ports: vec![],
+    /// #   },
+    ///     countries: vec![RelayListCountry {
+    ///         name: "Sweden".to_string(),
+    /// #       code: "Sweden".to_string(),
+    ///         cities: vec![RelayListCity {
+    ///             name: "Gothenburg".to_string(),
+    /// #           code: "Gothenburg".to_string(),
+    /// #           latitude: 57.70887,
+    /// #           longitude: 11.97456,
+    ///             relays: vec![Relay {
+    ///                 hostname: "se9-wireguard".to_string(),
+    ///                 ipv4_addr_in: "185.213.154.68".parse().unwrap(),
+    /// #               ipv6_addr_in: Some("2a03:1b20:5:f011::a09f".parse().unwrap()),
+    /// #               include_in_country: false,
+    /// #               active: true,
+    /// #               owned: true,
+    /// #               provider: "31173".to_string(),
+    /// #               weight: 1,
+    /// #               endpoint_data: RelayEndpointData::Wireguard(WireguardRelayEndpointData {
+    /// #                   public_key: PublicKey::from_base64(
+    /// #                       "BLNHNoGO88LjV/wDBa7CUUwUzPq/fO2UwcGLy56hKy4=",
+    /// #                   )
+    /// #                   .unwrap(),
+    /// #               }),
+    /// #               location: None,
+    ///             }],
+    ///         }],
+    ///     }],
+    /// };
+    ///
+    /// # let relay_selector = RelaySelector::from_list(SelectorConfig::default(), relay_list.clone());
+    /// # let city = |country, city| GeographicLocationConstraint::city(country, city);
+    ///
+    /// let query_a = RelayQueryBuilder::new().wireguard().build();
+    /// let query_b = RelayQueryBuilder::new().location(city("Sweden", "Gothenburg")).build();
+    ///
+    /// let result = relay_selector.get_relay_by_query(query_a.intersection(query_b).unwrap());
+    /// assert!(result.is_ok());
+    /// ```
+    ///
+    /// This way, if the mullvad app wants to check if the user's relay settings
+    /// are compatible with any other [`RelayQuery`], for examples those defined by
+    /// [`RETRY_ORDER`] , taking the intersection between them will never result in
+    /// a situation where the app can override the user's preferences.
     ///
-    /// This way, if the mullvad app wants to check if the user's configured
-    /// [`RelayQuery`] are compatible with any other [`RelayQuery`], taking the
-    /// intersection between them will never result in a situation where the app
-    /// can override the user's preferences.
+    /// [`RETRY_ORDER`]: crate::RETRY_ORDER
     fn intersection(self, other: Self) -> Option<Self>
     where
         Self: PartialEq,

mullvad-types/src/relay_list.rs

@@ -107,16 +107,50 @@ pub struct Relay {
 }
 
 impl PartialEq for Relay {
+    /// Hostnames are assumed to be unique per relay, i.e. a relay can be uniquely identified by its hostname.
+    ///
+    /// # Example
+    ///
+    /// ```rust
+    /// # use mullvad_types::{relay_list::Relay, relay_list::{RelayEndpointData, WireguardRelayEndpointData}};
+    /// # use talpid_types::net::wireguard::PublicKey;
+    ///
+    /// let relay = Relay {
+    ///     hostname: "se9-wireguard".to_string(),
+    ///     ipv4_addr_in: "185.213.154.68".parse().unwrap(),
+    ///     # ipv6_addr_in: None,
+    ///     # include_in_country: true,
+    ///     # active: true,
+    ///     # owned: true,
+    ///     # provider: "provider0".to_string(),
+    ///     # weight: 1,
+    ///     # endpoint_data: RelayEndpointData::Wireguard(WireguardRelayEndpointData {
+    ///     #   public_key: PublicKey::from_base64(
+    ///     #       "BLNHNoGO88LjV/wDBa7CUUwUzPq/fO2UwcGLy56hKy4=",
+    ///     #   )
+    ///     #   .unwrap(),
+    ///     # }),
+    ///     # location: None,
+    /// };
+    ///
+    /// let mut different_relay = relay.clone();
+    /// // Modify the relay's IPv4 address - should not matter for the equality check
+    /// different_relay.ipv4_addr_in = "1.3.3.7".parse().unwrap();
+    /// assert_eq!(relay, different_relay);
+    ///
+    /// // What matter's for the equality check is the hostname of the relay
+    /// different_relay.hostname = "dk-cph-wg-001".to_string();
+    /// assert_ne!(relay, different_relay);
+    /// ```
     fn eq(&self, other: &Self) -> bool {
-        // Hostnames are assumed to be unique per relay, i.e. a relay can be uniquely identified by its hostname.
         self.hostname == other.hostname
     }
 }
 
-// See uniqueness argument in the implementation of [`PartialEq`] for [`Relay`].
+/// Hostnames are assumed to be unique per relay, i.e. a relay can be uniquely identified by its hostname.
 impl Eq for Relay {}
 
-// See uniqueness argument in the implementation of [`PartialEq`] for [`Relay`].
+/// Hostnames are assumed to be unique per relay, i.e. a relay can be uniquely identified by its hostname.
 impl std::hash::Hash for Relay {
     fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
         self.hostname.hash(state)