Clarify naming of variables in Socks5Local datastructures

[MarkusPettersson98]

This change aims to make the intent of the different fields of the Socks5Local datastructures more clear

---

This change is 

[dlon]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)

[MarkusPettersson98]

Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @dlon and @faern)

---

mullvad-management-interface/src/types/conversions/access_method.rs line 223 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think peer here should also be renamed remote_peer. It's otherwise extremely easy to think that's where you connect to talk to the socks5, which is not actually the case. And port -> local_port, since we use that in other places. As soon as you have two of anything, you need to describe which one is for what. If Socks5Local only described how to connect to the socks5 server it would be fine to call the field port. But since it also has to contain more data, also containing ports, we should try to be more explicit IMO

I've attempted an exhaustive search+replace where Socks5Local pop up in order to apply the <remote|local>_-prefix naming scheme consistently. Please have a look 😊

[dlon]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faern and @MarkusPettersson98)

---

mullvad-types/src/access_method.rs line 271 at r2 (raw file):

    /// [`std::net::SocketAddr`] for you. If `remote_ip` or `remote_port` are
    /// valid [`Some(Socks5Local)`] is returned, otherwise [`None`].
    pub fn from_args(remote_ip: String, remote_port: u16, local_port: u16) -> Option<Self> {

Nit: I actually think this parsing doesn't belong here. This probably belongs in mullvad-management-interface

[MarkusPettersson98]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dlon and @faern)

---

mullvad-types/src/access_method.rs line 271 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: I actually think this parsing doesn't belong here. This probably belongs in mullvad-management-interface

I'll challenge this Nit by saying that this pattern shows up elsewhere, for example in talpid-core

mullvadvpn-app/talpid-core/src/firewall/macos.rs

Lines 19 to 47 in 1de3f4e

	pub struct Firewall {
	pf: pfctl::PfCtl,
	pf_was_enabled: Option<bool>,
	rule_logging: RuleLogging,
	}
	impl Firewall {
	pub fn from_args(_args: FirewallArguments) -> Result<Self> {
	Self::new()
	}
	pub fn new() -> Result<Self> {
	// Allows controlling whether firewall rules should log to pflog0. Useful for debugging the
	// rules.
	let firewall_debugging = env::var("TALPID_FIREWALL_DEBUG");
	let rule_logging = match firewall_debugging.as_ref().map(String::as_str) {
	Ok("pass") => RuleLogging::Pass,
	Ok("drop") => RuleLogging::Drop,
	Ok("all") => RuleLogging::All,
	Ok(_) | Err(_) => RuleLogging::None,
	};
	log::trace!("Firewall debug log policy: {:?}", rule_logging);
	Ok(Firewall {
	pf: pfctl::PfCtl::new()?,
	pf_was_enabled: None,
	rule_logging,
	})
	}

Except that in the example, the semantics of new and from_args are swapped. In general, I think it is natural to keep constructors of types close to the type definition itself 😊

In the case of Socks5Local::from_args we are constructing the type Socks5Local from its parts, whereas I see it as conversions between equivalent (but different) types belong in mullvad-management-interface.

[dlon]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)

[dlon]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)