Update data structure to support new obfuscation selection

[rablador]

---

This change is 

[linear]

IOS-890 Update data structure to support new obfuscation selection

[mojganii]

Reviewable status: 0 of 21 files reviewed, 1 unresolved discussion (waiting on @rablador)

---

ios/MullvadSettings/WireGuardObfuscationSettings.swift line 128 at r3 (raw file):

    public let state: WireGuardObfuscationState
    public let udpOverTcpPort: WireGuardObfuscationUdpOverTcpPort

are we going to keep the previous state when user is changing obfuscation method? in this architecture, if we add another state into WireGuardObfuscationState that has its own custom configuration then we need to add another property for that here, as well. is it the thing we will?

[buggmagnet]

Reviewed all commit messages.
Reviewable status: 0 of 21 files reviewed, 3 unresolved discussions (waiting on @rablador)

---

ios/MullvadSettings/WireGuardObfuscationSettings.swift line 56 at r3 (raw file):

    case port5001

    public var portValue: UInt16? {

This should not be Optional, we always know the real value of the port.

---

ios/PacketTunnelCore/Actor/ProtocolObfuscator.swift line 61 at r3 (raw file):

        let obfuscator = Obfuscator(
            remoteAddress: obfuscatedEndpoint.ipv4Relay.ip,
            tcpPort: tcpPort.portValue ?? 0

This is the reason why portValue shouldn't be nullable. 0 is an invalid port number in this case, and it should be impossible
to have this condition here since if the obfuscation is set to automatic it would either be 80 or 5001

[rablador]

Reviewable status: 0 of 21 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadSettings/WireGuardObfuscationSettings.swift line 56 at r3 (raw file):

Previously, buggmagnet wrote…

This should not be Optional, we always know the real value of the port.

We don't, since .automatic isn't really equal to eg. 0 we should properly represent that a port has not been explicity set.

---

ios/MullvadSettings/WireGuardObfuscationSettings.swift line 128 at r3 (raw file):

Previously, mojganii wrote…

are we going to keep the previous state when user is changing obfuscation method? in this architecture, if we add another state into WireGuardObfuscationState that has its own custom configuration then we need to add another property for that here, as well. is it the thing we will?

I think so. Doing it this way we keep each port setting no matter what other settings the user changes, so that's according to spec I think. Unfortunately we could not - in a good way - combine state and port into a single enum. The reason for that being that we set state and port in different places at different times, which makes it really bothersome to keep track of in a viewmodel.

---

ios/PacketTunnelCore/Actor/ProtocolObfuscator.swift line 61 at r3 (raw file):

Previously, buggmagnet wrote…

This is the reason why portValue shouldn't be nullable. 0 is an invalid port number in this case, and it should be impossible
to have this condition here since if the obfuscation is set to automatic it would either be 80 or 5001

That won't be true for long. This code will change when the relay selector PR goes up, and this part will handle the custom port for shadowsocks as well, which can be anything. I don't think we should be relying on an Int that's implicitly wrong if it happens to be a 0 but correct if it's a 1.

[rablador]

Reviewable status: 0 of 21 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/PacketTunnelCore/Actor/ProtocolObfuscator.swift line 61 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

That won't be true for long. This code will change when the relay selector PR goes up, and this part will handle the custom port for shadowsocks as well, which can be anything. I don't think we should be relying on an Int that's implicitly wrong if it happens to be a 0 but correct if it's a 1.

Just to clarify, in the new code there won't be any fallbacks like this.

[buggmagnet]

Reviewable status: 0 of 21 files reviewed, 2 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadSettings/WireGuardObfuscationSettings.swift line 56 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

We don't, since .automatic isn't really equal to eg. 0 we should properly represent that a port has not been explicity set.

This was discussed offline.

---

ios/PacketTunnelCore/Actor/ProtocolObfuscator.swift line 61 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

Just to clarify, in the new code there won't be any fallbacks like this.

Sounds good then, a temporary solution is fine.

[buggmagnet]

Reviewable status: 0 of 21 files reviewed, 1 unresolved discussion (waiting on @mojganii)

[mojganii]

Reviewable status: 0 of 21 files reviewed, all discussions resolved