Refactor PacketTunnelActor to use a Reducer

[acb-mv]

This is a refactoring of the PacketTunnelActor state machine, which splits the state processing logic into two stages: a stateless function (a Reducer) which takes incoming events (Events) and existing state and produces updated state and zero or more Effect values, and stateful code within the actor that executes the Effect values, producing effects. This is an instance of the Reducer architecture, and has the effect of making the state transition logic purely stateless and allowing it to be tested and reasoned about without concern for side effects.

As part of this, the Command type has been renamed to Effect, for accuracy: a value of this type can represent not only a user command (such as .start, .stop or .reconnect) but non-user events such as key rotations and network condition changes. (Additionally, renaming it to Event keeps it consistent with common nomenclature in the Reducer pattern.)

There is also a suite of unit tests to validate state transitions in the Actor through its Reducer.

(There was an earlier, unnumbered, PR, but it fell by the wayside of extensive rebasing activity.)

---

This change is 

[linear]

IOS-692 Refactor PacketTunnelActor state-transition logic to use Reducer pattern, and add tests

[rablador]

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acb-mv)

---

ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 341 at r1 (raw file):

     - Returns: New connection state or `nil` if current state is at or past `.disconnecting` phase.
     */
    internal func makeConnectionState(

Swiftlint body length warning.

---

ios/PacketTunnelCore/Actor/PacketTunnelActorReducer.swift line 55 at r1 (raw file):

    }

    static func reducer(_ state: inout State, _ event: Event) -> [Effect] {

Perhaps this function could be broken down to smaller pieces to make it easier to follow.

[acb-mv]

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

---

ios/PacketTunnelCore/Actor/PacketTunnelActorReducer.swift line 55 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Perhaps this function could be broken down to smaller pieces to make it easier to follow.

Done.

[acb-mv]

ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 341 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Swiftlint body length warning.

That's not one of the functions touched in this PR

[rablador]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[buggmagnet]

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

---

ios/PacketTunnelCore/Actor/PacketTunnelActorReducer.swift line 12 at r2 (raw file):

import WireGuardKitTypes

extension WireGuardKey where Self: Equatable {}

That shouldn't be needed, WireGuardKey types already implement Equatable

---

ios/PacketTunnelCore/Actor/PacketTunnelActorReducer.swift line 55 at r2 (raw file):

    }

    static func reducer(_ state: inout State, _ event: Event) -> [Effect] {

Any specific reason why it's all static methods ?
Unless I'm mistaken, a struct should be able to do the same job and we can avoid having to depend on PacketTunnelActor inside the tests equally well.

I'm assuming this was done so we don't need to instantiate it for tests, and just use the static functions instead, but it will just make it harder to actually write logic tests for PacketTunnelActor now because with this, we can't avoid dealing with reduced states.

IMHO this shouldn't be an extension of the PacketTunnel at all, it should just consume it, and depend on it.

[rablador]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acb-mv)

[buggmagnet]

Reviewed 10 of 13 files at r1, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acb-mv)

---

ios/PacketTunnelCore/Actor/PacketTunnelActorReducer.swift line 55 at r2 (raw file):

Previously, buggmagnet wrote…

Any specific reason why it's all static methods ?
Unless I'm mistaken, a struct should be able to do the same job and we can avoid having to depend on PacketTunnelActor inside the tests equally well.

I'm assuming this was done so we don't need to instantiate it for tests, and just use the static functions instead, but it will just make it harder to actually write logic tests for PacketTunnelActor now because with this, we can't avoid dealing with reduced states.

IMHO this shouldn't be an extension of the PacketTunnel at all, it should just consume it, and depend on it.

This was discussed offline, we decided to take an approach with both the benefits of having a struct, and use static functions to infer that there shouldn't be any statefulness in there.

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.