show outgoing connection on map

[mojganii]

this PR introduces a valuable feature that enhances the user experience by showing outgoing addresses in the connection view. This information is especially useful when access to exit IPs is available, and the implementation ensures the accuracy and completeness of the displayed data by making two network requests and waiting for their successful completion.these actions are taken to achieve the result :

1.Network Requests for IP Addresses: To gather the necessary information, two network requests are initiated, one for IPv4 and the other for IPv6 addresses. These requests are essential for determining the exit IP address associated with the established tunnel.
2. Wait for Completion: The system intelligently waits for both of these requests to be completed successfully, ensuring that we have comprehensive data for both IPv4 and IPv6 addresses if they are available .
3. Exit Addresses Display: Once both IPv4 and IPv6 address data are done, the connection view is updated to prominently display the exit addresses.

Before	After

---

This change is 

[linear]

IOS-301 Show outgoing address in connection view

We should show outgoing address in connection view when we have access to one. Functionality is mostly prepared in ConnectionPanelView.

See the Zeplin screenshot for more details. You should also look at the connection details dropdown in the desktop app.

The exit addresses can be obtained via a GET request to https://ipv4.am.i.mullvad.net/json and https://ipv6.am.i.mullvad.net/json

The exit IP is contained in the ip field.

These HTTP requests shouldn't be issued via our REST framework - they will be using a completely different host and don't need any of our TLS certificate pinning and they must not be routed through the packet tunnel.

https://app.zeplin.io/project/5f928a32fdc9962af9018d70/screen/63c028f3b8b705029ef6dfc7

[rablador]

Reviewed 2 of 15 files at r1, all commit messages.
Reviewable status: 2 of 15 files reviewed, 3 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 94 at r1 (raw file):

        let exitIP: Bool

        enum CodingKeys: String, CodingKey {

This struct already conforms to Codable, so no need to encode or decode manually. This enum, along with init(from decoder: Decoder) and encode(to encoder: Encoder), can be removed.

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 98 at r1 (raw file):

        }

        init(ip: T, exitIP: Bool) {

This init is never used and should be ok to remove.

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 116 at r1 (raw file):

        static func == (
            lhs: OutgoingConnectionProxy.OutgoingConnectionData<T>,

Nit: OutgoingConnectionProxy.OutgoingConnectionData<T> can be substituted with Self instead to increase readability.

[rablador]

Reviewable status: 2 of 15 files reviewed, 3 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 94 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

This struct already conforms to Codable, so no need to encode or decode manually. This enum, along with init(from decoder: Decoder) and encode(to encoder: Encoder), can be removed.

Update: We actually need the mapping enum here, because of exitIP = "mullvad_exit_ip", but the rest is ok to remove.

[mojganii]

Reviewable status: 1 of 15 files reviewed, 3 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 94 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Update: We actually need the mapping enum here, because of exitIP = "mullvad_exit_ip", but the rest is ok to remove.

Done.

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 98 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

This init is never used and should be ok to remove.

Done.

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 116 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: OutgoingConnectionProxy.OutgoingConnectionData<T> can be substituted with Self instead to increase readability.

indeed!

[rablador]

Reviewed 13 of 15 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 67 at r1 (raw file):

    var outAddress: String? {
        let v4 = ipv4.exitIP ? ipv4.ip : nil

This can be compressed a bit, also only adding the line break if there are two items to separate them:

Code snippet:

        let v4: IPAddress? = ipv4.exitIP ? ipv4.ip : nil
        let v6: IPAddress? = ipv6.flatMap { $0.exitIP ? $0.ip : nil }

        let string = [v4, v6].compactMap { $0 }.map { $0.debugDescription }.joined(separator: "\n")
        return string.isEmpty ? nil : string

---

ios/MullvadVPNTests/OutgoingConnectionServiceTests.swift line 35 at r1 (raw file):

        let result = try await outgoingConnectionService.getOutgoingConnectionInfo()
        if result.ipv4 == .mock,
           let ipv6 = result.ipv6,

Just result.ipv6 == .mock works too.

[pronebird]

Reviewed 14 of 15 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 91 at r2 (raw file):

    struct OutgoingConnectionData<T: IPAddressType>: Codable, Equatable {
        let ip: T?

Can ip really be nil?

---

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 67 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

This can be compressed a bit, also only adding the line break if there are two items to separate them:

@rablador transforming IP to string can be done using "\(ipAddress)", no need to call debugDescription directly.

[mojganii]

Reviewable status: 11 of 15 files reviewed, 3 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 91 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Can ip really be nil?

After checking the server response I figured out it cannot be optional.

---

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 67 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

This can be compressed a bit, also only adding the line break if there are two items to separate them:

Done.

---

ios/MullvadVPNTests/OutgoingConnectionServiceTests.swift line 35 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Just result.ipv6 == .mock works too.

Done.

[buggmagnet]

That's really good work !

Reviewed 7 of 15 files at r1.
Reviewable status: 11 of 15 files reviewed, 13 unresolved discussions (waiting on @mojganii, @pronebird, and @rablador)

---

ios/MullvadVPN/SceneDelegate.swift line 73 at r2 (raw file):

            accountsProxy: appDelegate.accountsProxy,
            outgoingConnectionService: OutgoingConnectionService(
                outgoingConnectionProxy: OutgoingConnectionProxy(urlSession: .shared)

We don't want to cache the result of calls to am.i.mullvad.net, therefore we don't want to use the .shared sessions.

Please use URLSessionConfiguration.ephemeral instead for getting a URLSession object

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 83 at r2 (raw file):

}

extension OutgoingConnectionProxy {

IMHO OutgoingConnectionData should be in its own file and not part of OutgoingConnectionProxy

The protocol signature would look simpler as well

protocol OutgoingConnectionHandling {
    func getIPV6(retryStrategy: REST.RetryStrategy) async throws -> IPV6ConnectionData
    func getIPV4(retryStrategy: REST.RetryStrategy) async throws -> IPV4ConnectionData
}

---

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 30 at r2 (raw file):

    func getOutgoingConnectionInfo() async throws -> OutgoingConnectionInfo {
        try await withThrowingTaskGroup(of: OutgoingConnectionResult.self) { taskGroup -> OutgoingConnectionInfo in

We can simplify this.
However, technically we are not running the calls in parallel but that shouldn't matter in practice. Also we're shaving 40 lines. IMHO it's worth it.

func getOutgoingConnectionInfo() async throws -> OutgoingConnectionInfo {
    let ipv4 = try await outgoingConnectionProxy.getIPV4(retryStrategy: .default)
    let ipv6 = try await outgoingConnectionProxy.getIPV6(retryStrategy: .noRetry)

    return OutgoingConnectionInfo(ipv4: ipv4, ipv6: ipv6)
}

I don't think we should be touching the task priority until we have good reason to believe that this would be significantly better for the UX.
If we absolutely need to, then we can change the call site instead (which I still think we shouldn't)

outgoingConnectionTask = Task(priority: .userInitiated) { [weak self] in
    guard let outgoingConnectionInfo = try await self?.outgoingConnectionService
        .getOutgoingConnectionInfo() else {
        return
    }
    await self?.didGetOutGoingAddress?(outgoingConnectionInfo)
}

---

ios/MullvadVPN/View controllers/Tunnel/TunnelControlView.swift line 153 at r2 (raw file):

        if let tunnelRelay = tunnelState.relay {
            connectionPanel.dataSource = ConnectionPanelData(
                inAddress: "\(tunnelRelay.endpoint.ipv4Relay) UDP",

We shouldn't be hardcoding UDP here since it's depending on whether we're using the UDP-over-TCP obfuscation.

Maybe let's add a TODO comment for now that we will be solving this soon.

---

ios/MullvadVPN/View controllers/Tunnel/TunnelViewControllerInteractor.swift line 42 at r2 (raw file):

        self.outgoingConnectionService = outgoingConnectionService

        let tunnelObserver = TunnelBlockObserver(

Is there anyway we can do this when we start the tunnel instead of the init ?

---

ios/MullvadVPNTests/MockURLProtocol.swift line 12 at r2 (raw file):

class MockURLProtocol: URLProtocol {
    static var error: Error?

Same remark as in OutgoingConnectionProxyStub, this should not be static

---

ios/MullvadVPNTests/OutgoingConnectionProxy+Stub.swift line 12 at r2 (raw file):

struct OutgoingConnectionProxyStub: OutgoingConnectionHandling {
    static var hasError = false

Please let's not use static unless we absolutely have to. It always leads to hard to test designs and unwanted shared states.

Here in particular, it makes it so that all the OutgoingConnectionProxyStub have the same error state which is definitely not what we want when running multiple tests in parallel.

---

ios/MullvadVPNTests/OutgoingConnectionProxyTests.swift line 18 at r2 (raw file):

    private let encoder: JSONEncoder = REST.Coding.makeJSONEncoder()

    override func setUp() {

You can use setUpWithError() throws and remove the force try as well as the swiftlint:disable commands instead

Remember to also use tearDownWithError() throws as well in that case

---

ios/MullvadVPNTests/OutgoingConnectionProxyTests.swift line 34 at r2 (raw file):

    func testNoInternetConnection() async throws {
        let failureExpectation = expectation(description: "should fail")
        let successExpectation = expectation(description: "should not succeed")

There are a couple of things we can improve in this test.

• Expectations :
  Because we are using inverted expectations here, when this fails, the message produced will be
  Fulfilled inverted expectation "should not succeed".
  Which is a double negation.

In essence, we should keep the same kind messaging so that the error is easy to read and understand.
In this case, it would make sense to read Fulfilled inverted expectation "Received IPv4".

• Expectation descriptions :
  A message such as "should fail" is not very descriptive. What are we expecting here ? We shouldn't have to read what the code does when a test fail to know what failed.
  A much better message would be "Did not receive IPv4".
  That way, if the test fails, this is what we would see "Asynchronous wait failed [...] with unfulfilled expectations: "Did not receive IPv4".
  Now we know that we expected to not receive IPV4, and that the expectation was not fullfilled, therefore, we did receive an IPv4. We probably know how to fix this.

• Expectation names :
  As important as the message, usually, we try to name the expectation according to what's being expected.
  In this case, a good name would be let noIPv4Expectation = expectation(description: "Did not receive IPv4") for example.

• Catching the error :

Unfortunately, the XCTest API is not well suited for async results, otherwise, I would have said to use XCTAssertThrowsError which is suited for that.

So I wrote a helper method to help with that

extension XCTest {
    func XCTAssertThrowsErrorAsync<T: Sendable>(
        _ expression: @autoclosure () async throws -> T,
        _ message: @autoclosure () -> String = "",
        file: StaticString = #filePath,
        line: UInt = #line,
        _ errorHandler: (_ error: Error) -> Void = { _ in }
    ) async {
        do {
            _ = try await expression()
            XCTFail(message(), file: file, line: line)
        } catch {
            errorHandler(error)
        }
    }
}

Here's what the rewritten test looks like now

func testGetIPv4FailsWhenNotConnected() async throws {
    let noIPv4Expectation = expectation(description: "Did not receive IPv4")
    MockURLProtocol.error = URLError(URLError.notConnectedToInternet)
    MockURLProtocol.requestHandler = nil

    await XCTAssertThrowsErrorAsync(try await outgoingConnectionProxy.getIPV4(retryStrategy: .noRetry)) { error in
        noIPv4Expectation.fulfill()
        XCTAssertEqual((error as? URLError)?.code, .notConnectedToInternet)
    }
    await fulfillment(of: [noIPv4Expectation], timeout: 1)
}

I suggest rewriting the other tests in this file with these improvements in mind

---

ios/MullvadVPNTests/OutgoingConnectionServiceTests.swift line 54 at r2 (raw file):

}

enum MockNetworkError: Error {

nit
Technically, this is a stub, not a mock

[mojganii]

Reviewable status: 11 of 15 files reviewed, 12 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

---

ios/MullvadVPN/SceneDelegate.swift line 73 at r2 (raw file):

Previously, buggmagnet wrote…

We don't want to cache the result of calls to am.i.mullvad.net, therefore we don't want to use the .shared sessions.

Please use URLSessionConfiguration.ephemeral instead for getting a URLSession object

Done.

---

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 30 at r2 (raw file):

Previously, buggmagnet wrote…

We can simplify this.
However, technically we are not running the calls in parallel but that shouldn't matter in practice. Also we're shaving 40 lines. IMHO it's worth it.

func getOutgoingConnectionInfo() async throws -> OutgoingConnectionInfo {
    let ipv4 = try await outgoingConnectionProxy.getIPV4(retryStrategy: .default)
    let ipv6 = try await outgoingConnectionProxy.getIPV6(retryStrategy: .noRetry)

    return OutgoingConnectionInfo(ipv4: ipv4, ipv6: ipv6)
}

I don't think we should be touching the task priority until we have good reason to believe that this would be significantly better for the UX.
If we absolutely need to, then we can change the call site instead (which I still think we shouldn't)

outgoingConnectionTask = Task(priority: .userInitiated) { [weak self] in
    guard let outgoingConnectionInfo = try await self?.outgoingConnectionService
        .getOutgoingConnectionInfo() else {
        return
    }
    await self?.didGetOutGoingAddress?(outgoingConnectionInfo)
}

but task group does it parallely.regarding the task priority I think we need to change it for better UX

---

ios/MullvadVPN/View controllers/Tunnel/TunnelControlView.swift line 153 at r2 (raw file):

Previously, buggmagnet wrote…

We shouldn't be hardcoding UDP here since it's depending on whether we're using the UDP-over-TCP obfuscation.

Maybe let's add a TODO comment for now that we will be solving this soon.

I'll put todo for that

---

ios/MullvadVPN/View controllers/Tunnel/TunnelViewControllerInteractor.swift line 42 at r2 (raw file):

Previously, buggmagnet wrote…

Is there anyway we can do this when we start the tunnel instead of the init ?

is it the responsibility of tunnel to fetch exit ip.I don't think so.

---

ios/MullvadVPNTests/MockURLProtocol.swift line 12 at r2 (raw file):

Previously, buggmagnet wrote…

Same remark as in OutgoingConnectionProxyStub, this should not be static

if I'm gonna eliminate static variable in this particular part I need to go protocol-oriented for url-session and data task and so on that takes much efforts and bring more complexity.do you have any solution for this part?

---

ios/MullvadVPNTests/OutgoingConnectionProxy+Stub.swift line 12 at r2 (raw file):

Previously, buggmagnet wrote…

Please let's not use static unless we absolutely have to. It always leads to hard to test designs and unwanted shared states.

Here in particular, it makes it so that all the OutgoingConnectionProxyStub have the same error state which is definitely not what we want when running multiple tests in parallel.

fair enough

---

ios/MullvadVPNTests/OutgoingConnectionProxyTests.swift line 18 at r2 (raw file):

Previously, buggmagnet wrote…

You can use setUpWithError() throws and remove the force try as well as the swiftlint:disable commands instead

Remember to also use tearDownWithError() throws as well in that case

Done.

---

ios/MullvadVPNTests/OutgoingConnectionServiceTests.swift line 54 at r2 (raw file):

Previously, buggmagnet wrote…

nit
Technically, this is a stub, not a mock

Done

[buggmagnet]

Reviewable status: 5 of 16 files reviewed, 8 unresolved discussions (waiting on @mojganii, @pronebird, and @rablador)

---

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 30 at r2 (raw file):

but task group does it parallely.
Yes but there are a couple of considerations at play here:

• We are only running 2 tasks
• We are doing network calls
• 3 lines of code is vastly easier to understand than 40 with a custom built enum

There's very likely more overhead to try to run 2 tasks in parallel rather than running them one after the other given how simple the calls are (and how fast iPhone processors are nowadays)
And since it's networking code, on a bad connection, it won't make any difference whether the tasks are running in parallel rather than in sequence.

I think we need to change it for better UX
Doing this will in the best case, not do anything, and in the worst case, make the whole system perform worse.
This is a good example of the Priority Inversion problem.
However, the swift async runtime should help us in those situations, hopefully. Happy to talk more about why that's a problematic design, but not in this comment.

---

ios/MullvadVPNTests/MockURLProtocol.swift line 12 at r2 (raw file):

Previously, mojganii wrote…

if I'm gonna eliminate static variable in this particular part I need to go protocol-oriented for url-session and data task and so on that takes much efforts and bring more complexity.do you have any solution for this part?

Technically, the only part where we use that URLSession object is in OutgoingConnectionProxy where we do
let (data, response) = try await urlSession.data(from: url)

Ideally, we should have extracted that call to a single interface, then we wouldn't need all this mocking of URLProtocol and the complicated setup of tests.
That's a bit unfortunate, but it's not the end of the world.

I will create a cleanup janitor task instead, it's not critical.

[pronebird]

Reviewed 1 of 4 files at r3.
Reviewable status: 6 of 16 files reviewed, 7 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

---

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 30 at r2 (raw file):

Previously, buggmagnet wrote…

but task group does it parallely.
Yes but there are a couple of considerations at play here:

• We are only running 2 tasks
• We are doing network calls
• 3 lines of code is vastly easier to understand than 40 with a custom built enum

There's very likely more overhead to try to run 2 tasks in parallel rather than running them one after the other given how simple the calls are (and how fast iPhone processors are nowadays)
And since it's networking code, on a bad connection, it won't make any difference whether the tasks are running in parallel rather than in sequence.

I think we need to change it for better UX
Doing this will in the best case, not do anything, and in the worst case, make the whole system perform worse.
This is a good example of the Priority Inversion problem.
However, the swift async runtime should help us in those situations, hopefully. Happy to talk more about why that's a problematic design, but not in this comment.

Let's not get into micro optimizations.

Parallelization for IP lookup requests makes sense. This part looks right to me.

However I think that using async let binding to run two tasks in parallel and then collect the result would be simpler way to approach this.

async let ipv4Result = getIPv4(..)
async let ipv6Result = getIPv6(..)

OutgoingConnectionInfo(
  ipv4: try await ipv4Result, 
  ipv6: try? await ipv6Result
)

[mojganii]

Reviewable status: 6 of 16 files reviewed, 7 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

---

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 30 at r2 (raw file):

Previously, buggmagnet wrote…

but task group does it parallely.
Yes but there are a couple of considerations at play here:

• We are only running 2 tasks
• We are doing network calls
• 3 lines of code is vastly easier to understand than 40 with a custom built enum

There's very likely more overhead to try to run 2 tasks in parallel rather than running them one after the other given how simple the calls are (and how fast iPhone processors are nowadays)
And since it's networking code, on a bad connection, it won't make any difference whether the tasks are running in parallel rather than in sequence.

I think we need to change it for better UX
Doing this will in the best case, not do anything, and in the worst case, make the whole system perform worse.
This is a good example of the Priority Inversion problem.
However, the swift async runtime should help us in those situations, hopefully. Happy to talk more about why that's a problematic design, but not in this comment.

accepted

---

ios/MullvadVPNTests/OutgoingConnectionProxyTests.swift line 34 at r2 (raw file):

Previously, buggmagnet wrote…

There are a couple of things we can improve in this test.

• Expectations :
  Because we are using inverted expectations here, when this fails, the message produced will be
  Fulfilled inverted expectation "should not succeed".
  Which is a double negation.

In essence, we should keep the same kind messaging so that the error is easy to read and understand.
In this case, it would make sense to read Fulfilled inverted expectation "Received IPv4".

• Expectation descriptions :
  A message such as "should fail" is not very descriptive. What are we expecting here ? We shouldn't have to read what the code does when a test fail to know what failed.
  A much better message would be "Did not receive IPv4".
  That way, if the test fails, this is what we would see "Asynchronous wait failed [...] with unfulfilled expectations: "Did not receive IPv4".
  Now we know that we expected to not receive IPV4, and that the expectation was not fullfilled, therefore, we did receive an IPv4. We probably know how to fix this.

• Expectation names :
  As important as the message, usually, we try to name the expectation according to what's being expected.
  In this case, a good name would be let noIPv4Expectation = expectation(description: "Did not receive IPv4") for example.

• Catching the error :

Unfortunately, the XCTest API is not well suited for async results, otherwise, I would have said to use XCTAssertThrowsError which is suited for that.

So I wrote a helper method to help with that

extension XCTest {
    func XCTAssertThrowsErrorAsync<T: Sendable>(
        _ expression: @autoclosure () async throws -> T,
        _ message: @autoclosure () -> String = "",
        file: StaticString = #filePath,
        line: UInt = #line,
        _ errorHandler: (_ error: Error) -> Void = { _ in }
    ) async {
        do {
            _ = try await expression()
            XCTFail(message(), file: file, line: line)
        } catch {
            errorHandler(error)
        }
    }
}

Here's what the rewritten test looks like now

func testGetIPv4FailsWhenNotConnected() async throws {
    let noIPv4Expectation = expectation(description: "Did not receive IPv4")
    MockURLProtocol.error = URLError(URLError.notConnectedToInternet)
    MockURLProtocol.requestHandler = nil

    await XCTAssertThrowsErrorAsync(try await outgoingConnectionProxy.getIPV4(retryStrategy: .noRetry)) { error in
        noIPv4Expectation.fulfill()
        XCTAssertEqual((error as? URLError)?.code, .notConnectedToInternet)
    }
    await fulfillment(of: [noIPv4Expectation], timeout: 1)
}

I suggest rewriting the other tests in this file with these improvements in mind

Done.

[mojganii]

Reviewable status: 6 of 16 files reviewed, 7 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 83 at r2 (raw file):

Previously, buggmagnet wrote…

IMHO OutgoingConnectionData should be in its own file and not part of OutgoingConnectionProxy

The protocol signature would look simpler as well

protocol OutgoingConnectionHandling {
    func getIPV6(retryStrategy: REST.RetryStrategy) async throws -> IPV6ConnectionData
    func getIPV4(retryStrategy: REST.RetryStrategy) async throws -> IPV4ConnectionData
}

It's a personal preferences.I'm fan of keeping protocols in individual files.let's know other's opinion.
@pronebird @rablador

[pronebird]

Reviewed 5 of 9 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mojganii and @rablador)

[rablador]

Reviewed 2 of 4 files at r3, 8 of 9 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet, @mojganii, and @pronebird)

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 83 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

I don't mind it either way (in separate file or in here). You could however prefer a separate file if you want to mock the actual implementation, for example when you only need to include the protocol definition in the tests bundle.

I think it's fine to combine them if they're tightly coupled/associated with each other.

---

ios/MullvadVPN/View controllers/Tunnel/TunnelViewControllerInteractor.swift line 46 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Fallthrough and default case are somewhat considered enemies unless used with care, akin to goto in the good old days. Since you're only interested in connected state, you could rewrite it as a simple if case .connected = .. and then you eliminate both fallthrough and default case this way.

+1

[mojganii]

Reviewable status: 13 of 19 files reviewed, 5 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

---

ios/MullvadVPN/View controllers/Tunnel/TunnelViewControllerInteractor.swift line 46 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

+1

Done.

[rablador]

Reviewed 6 of 6 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @pronebird)

[buggmagnet]

Reviewed 1 of 4 files at r3, 3 of 6 files at r8, 2 of 3 files at r9, all commit messages.
Reviewable status: 18 of 19 files reviewed, 5 unresolved discussions (waiting on @mojganii, @pronebird, and @rablador)

[pronebird]

Reviewed 1 of 1 files at r5, 1 of 6 files at r8, 1 of 3 files at r9.
Reviewable status: 18 of 19 files reviewed, 5 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

---

ios/MullvadREST/RESTDefaults.swift line 17 at r9 (raw file):

    /// Exit ipV4  API hostname.
    public static let ipV4APIHostname = "ipv4.am.i.mullvad.net"

Nit: Maybe we should call it exitIPv4Hostname and exitIPv6Hostname instead because IPv4/6 API hostname sounds like the way we reach to api.mullvad.net. These are two different services however.

[mojganii]

Reviewable status: 18 of 19 files reviewed, 4 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

---

ios/MullvadREST/RESTDefaults.swift line 17 at r9 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: Maybe we should call it exitIPv4Hostname and exitIPv6Hostname instead because IPv4/6 API hostname sounds like the way we reach to api.mullvad.net. These are two different services however.

Done

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 15 at r6 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

The protocol should not reference a type from a concrete implementation.

Consider moving the IPV*ConnectionData outside of concrete implementation, perhaps somewhere next to protocol.

Alternatively use a protocol to describe the return type and conform the concrete implementation to it, i.e:

protocol OutgoingConnectionnDataProtocol<IPAddressType> {
   associated type IPAddressType: IPAddress

   var ip: IPAddressType { get }
   var isMullvadExit: Bool { get }
}

this approach adds more complexity. Apple is using the same format for Codable

Code snippet:

public typealias Codable = Decodable & Encodable

[pronebird]

Reviewable status: 18 of 19 files reviewed, 5 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 15 at r6 (raw file):

Previously, mojganii wrote…

this approach adds more complexity. Apple is using the same format for Codable

Extract the concrete value types

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 88 at r9 (raw file):

        } catch {
            throw error

do-catch is superficial as it doesn't do anything but rethrow an error. You can eliminate it to streamline the code.

[rablador]

Reviewed 3 of 3 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)

[buggmagnet]

Reviewed 1 of 9 files at r4, 1 of 6 files at r8, 1 of 3 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

[pronebird]

Reviewed 1 of 9 files at r4, 1 of 2 files at r10.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

[pronebird]

Reviewed 2 of 9 files at r4, 2 of 3 files at r9, 1 of 2 files at r10.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

[pronebird]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 15 at r6 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Extract the concrete value types

To follow up on that. If the IPv{4|6}ConnectionData is a requirement of the OutgoingConnectionHandling protocol, then it's better to keep the connection data type around the protocol and not embed it into the concrete implementation to avoid cross-dependencies.

[rablador]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

[mojganii]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 15 at r6 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Extract the concrete value types

Done.

---

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 88 at r9 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

do-catch is superficial as it doesn't do anything but rethrow an error. You can eliminate it to streamline the code.

Done.

---

ios/MullvadVPNTests/OutgoingConnectionProxy+Stub.swift line 12 at r6 (raw file):

Previously, buggmagnet wrote…

nit
Technically we don't need this hasError
We could have just made error an Optional, and then in the get function do

guard let error else { return ipVXXX }
throw error

But not a big deal

done

[pronebird]

Reviewed 7 of 7 files at r11.
Reviewable status: complete! all files reviewed, all discussions resolved

[buggmagnet]

Reviewed 6 of 7 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved