Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DAITA into TunnelSettings #6605

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

mojganii
Copy link
Collaborator

@mojganii mojganii commented Aug 12, 2024

This PR integrates DAITA settings into the tunnel settings, enhancing configuration flexibility and ensuring seamless DAITA support within the tunnel environment.


This change is Reviewable

@mojganii mojganii added good first issue iOS Issues related to iOS labels Aug 12, 2024
@mojganii mojganii self-assigned this Aug 12, 2024
Copy link

linear bot commented Aug 12, 2024

@mojganii mojganii requested review from rablador and buggmagnet and removed request for rablador August 12, 2024 15:54
@mojganii mojganii force-pushed the add-daita-settings-into-tunnel-ios-791 branch 2 times, most recently from 9d16e71 to f449127 Compare August 13, 2024 14:22
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 47 at r2 (raw file):

                didUpdateSettings: { [weak self] _, latestTunnelSettings in
                    guard let self else { return }
                    if settingsStrategy.shouldReconnectToNewRelay(

Isn't this used more as a general check for changes in constraints? At least this seems to be the only application at this point. Perhaps shouldReconnectToNewRelay should be renamed to constraintsHaveChangedor something similar?


ios/MullvadSettings/TunnelSettingsV6.swift line 28 at r2 (raw file):

    public var tunnelMultihopState: MultihopState

    /// Whether Multi-hop is enabled.

"DAITA" instead of multihop

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 21 of 32 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mojganii)


ios/MullvadSettings/DAITASettings.swift line 16 at r2 (raw file):

    case off

    public var isEnabled: Bool {

Do we really need this for an enum that is a bool value ?


ios/MullvadSettings/TunnelSettingsPropagator.swift line 21 at r2 (raw file):

public class SettingsObserverBlock: SettingsObserver {
    public typealias DidUpdateSettingsHandler = (SettingsPropagation, LatestTunnelSettings) -> Void

This looks like a delegate pattern except it's called observer instead.
Why do we pass a settings handler which you can also subscribe to, when the object (SettingsObserverBlock) we are listening to, already gives us an update on the settings ?

From the use I can see, we don't use that argument anywhere, we should remove this.

Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @mojganii)


ios/MullvadSettings/DAITASettings.swift line 16 at r2 (raw file):

Previously, buggmagnet wrote…

Do we really need this for an enum that is a bool value ?

Could be that we'll have .auto or something in the future as well. Also, it's consistent with multihopState.

@mojganii mojganii force-pushed the add-daita-settings-into-tunnel-ios-791 branch 2 times, most recently from f7ca84f to 2c63c73 Compare August 14, 2024 12:03
Copy link
Collaborator Author

@mojganii mojganii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 25 of 33 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)


ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 47 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Isn't this used more as a general check for changes in constraints? At least this seems to be the only application at this point. Perhaps shouldReconnectToNewRelay should be renamed to constraintsHaveChangedor something similar?

it's not about the constraints, it's all about whether reconnecting to the new relays should be done by some part changing in settings or not. the major responsibility for this part is that forces relaySelector to pick new relay if needed, regardless if it's ShadowSocks or Direct.


ios/MullvadSettings/DAITASettings.swift line 16 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Could be that we'll have .auto or something in the future as well. Also, it's consistent with multihopState.

yes, it's for consistency.


ios/MullvadSettings/TunnelSettingsPropagator.swift line 21 at r2 (raw file):

Previously, buggmagnet wrote…

This looks like a delegate pattern except it's called observer instead.
Why do we pass a settings handler which you can also subscribe to, when the object (SettingsObserverBlock) we are listening to, already gives us an update on the settings ?

From the use I can see, we don't use that argument anywhere, we should remove this.

Done.


ios/MullvadSettings/TunnelSettingsV6.swift line 28 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

"DAITA" instead of multihop

Good catch!

Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

@buggmagnet buggmagnet requested a review from rablador August 15, 2024 07:37
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 32 files at r1, 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii and @rablador)


ios/MullvadSettings/DAITASettings.swift line 16 at r2 (raw file):
But that doesn't mean it's a good idea 😭

Could be that we'll have .auto or something in the future as well.
YAGNI

Anyhow, it's not a big deal.

@buggmagnet buggmagnet force-pushed the add-daita-settings-into-tunnel-ios-791 branch from 2c63c73 to bdfd397 Compare August 15, 2024 07:38
@buggmagnet buggmagnet merged commit ad713fe into main Aug 15, 2024
8 of 9 checks passed
@buggmagnet buggmagnet deleted the add-daita-settings-into-tunnel-ios-791 branch August 15, 2024 07:40
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants