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 toggle to enable smart routing #6919

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Oct 3, 2024

A toggle to disable smart routing should be placed below the DAITA toggle. The toggle should be named Direct only. It should also have a footer explaining how, by default, enabling DAITA overrides multihop and an info icon that leads to an explanation for how smart routing works. When Direct only is toggled, the relay selector must honor user location constraints, allowing the user to end up in the blocked state. When turning on Direct only, if the user will end up in the blocked state, the user should be shown a pop-up explaining so - as it should already be working when toggling DAITA.

When DAITA is turned off, the Direct only toggle state shouldn't be affected.


This change is Reviewable

Copy link

linear bot commented Oct 3, 2024

@rablador rablador force-pushed the add-toggle-to-enable-smart-routing-ios-831 branch from 3d35036 to 0fd5d0d Compare October 3, 2024 15:01
@rablador rablador force-pushed the fix-smart-routing-algorithm-to-select-closest-relay-ios-830 branch from 6263436 to ebccf37 Compare October 4, 2024 09:05
@rablador rablador force-pushed the add-toggle-to-enable-smart-routing-ios-831 branch 3 times, most recently from 73ff74c to 8c4f947 Compare October 4, 2024 13:19
@rablador rablador force-pushed the fix-smart-routing-algorithm-to-select-closest-relay-ios-830 branch from ebccf37 to c7e0ff9 Compare October 7, 2024 13:30
@rablador rablador force-pushed the add-toggle-to-enable-smart-routing-ios-831 branch from 8c4f947 to 84e0c9c Compare October 7, 2024 13:41
@rablador rablador force-pushed the fix-smart-routing-algorithm-to-select-closest-relay-ios-830 branch from c7e0ff9 to 3ce1b39 Compare October 8, 2024 08:29
@buggmagnet buggmagnet force-pushed the fix-smart-routing-algorithm-to-select-closest-relay-ios-830 branch from 3ce1b39 to a7b8c35 Compare October 8, 2024 09:14
Base automatically changed from fix-smart-routing-algorithm-to-select-closest-relay-ios-830 to main October 8, 2024 09:22
@rablador rablador force-pushed the add-toggle-to-enable-smart-routing-ios-831 branch from 84e0c9c to 4baf132 Compare October 8, 2024 11:46
@rablador rablador requested review from mojganii and buggmagnet and removed request for mojganii October 8, 2024 11:46
@rablador rablador self-assigned this Oct 8, 2024
@rablador rablador added the iOS Issues related to iOS label Oct 8, 2024
@rablador rablador marked this pull request as ready for review October 8, 2024 11:46
@rablador rablador force-pushed the add-toggle-to-enable-smart-routing-ios-831 branch from 4baf132 to 77536e1 Compare October 8, 2024 11:54
Copy link
Contributor Author

@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.

Note: Waiting for a text in the "Direct only" info dialog.

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

@rablador rablador force-pushed the add-toggle-to-enable-smart-routing-ios-831 branch from 77536e1 to ca7ee82 Compare October 10, 2024 13:43
Copy link
Contributor Author

@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.

Done.

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

@rablador rablador force-pushed the add-toggle-to-enable-smart-routing-ios-831 branch 4 times, most recently from 5378eaa to 189959d Compare October 11, 2024 07:45
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 13 of 21 files at r1, 8 of 10 files at r2.
Reviewable status: 19 of 23 files reviewed, 4 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/View controllers/Settings/SettingsInfoButtonItem.swift line 22 at r2 (raw file):

                tableName: "DAITA",
                value: """
                DAITA (Defense against AI-guided Traffic Analysis) hides patterns in your encrypted VPN traffic. \

We should use the british english spelling of "Defence"


ios/MullvadVPN/View controllers/Settings/SettingsInfoButtonItem.swift line 29 at r2 (raw file):

                with any server.
                Attention: Be cautious if you have a limited data plan as this feature will increase your network \
                traffic. This feature can only be used with WireGuard.

That last line is irrelevant on iOS, we only support WireGuard, we should remove it.


ios/MullvadVPN/View controllers/Settings/SettingsInteractor.swift line 55 at r2 (raw file):

            selectedRelays?.entry == nil ? .multihop : nil
        } else {
            selectedRelays?.exit == nil ? .singlehop : nil

I'm not sure how I feel about this false dichotomy where the exit can be nil, since selectRelay cannot return a nil exit.

What do you think about this ?

        guard let selectedRelays = try? tunnelManager.selectRelays(tunnelSettings: tunnelSettings) else {
            return multihopEnabled ? .multihop : .singlehop
        }

        /// `selectRelays` *cannot* return a `nil` exit, therefore, either the entry is `nil`, or there is no error.
        return selectedRelays.entry == nil ? .multihop : nil

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsInfoButtonItem.swift line 105 at r2 (raw file):

                tableName: "DAITA",
                value: """
                DAITA (Defense against AI-guided Traffic Analysis) hides patterns in your encrypted VPN traffic. \

Please revert this

Copy link
Contributor Author

@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: 19 of 23 files reviewed, 4 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/View controllers/Settings/SettingsInfoButtonItem.swift line 22 at r2 (raw file):

Previously, buggmagnet wrote…

We should use the british english spelling of "Defence"

Done.


ios/MullvadVPN/View controllers/Settings/SettingsInfoButtonItem.swift line 29 at r2 (raw file):

Previously, buggmagnet wrote…

That last line is irrelevant on iOS, we only support WireGuard, we should remove it.

Done.

Copy link
Contributor Author

@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: 19 of 23 files reviewed, 4 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/View controllers/Settings/SettingsInteractor.swift line 55 at r2 (raw file):

Previously, buggmagnet wrote…

I'm not sure how I feel about this false dichotomy where the exit can be nil, since selectRelay cannot return a nil exit.

What do you think about this ?

        guard let selectedRelays = try? tunnelManager.selectRelays(tunnelSettings: tunnelSettings) else {
            return multihopEnabled ? .multihop : .singlehop
        }

        /// `selectRelays` *cannot* return a `nil` exit, therefore, either the entry is `nil`, or there is no error.
        return selectedRelays.entry == nil ? .multihop : nil

You're right, but we don't even need look at entry. Either selectedRelays is nil or it's correct.


ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsInfoButtonItem.swift line 105 at r2 (raw file):

Previously, buggmagnet wrote…

Please revert this

Done.

@rablador rablador force-pushed the add-toggle-to-enable-smart-routing-ios-831 branch from 189959d to 5ec2f77 Compare October 11, 2024 11:07
buggmagnet
buggmagnet previously approved these changes Oct 11, 2024
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.

:lgtm:

Reviewed 1 of 10 files at r2, 1 of 1 files at r3, 2 of 3 files at r4, all commit messages.
Reviewable status: 21 of 23 files reviewed, 1 unresolved discussion

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.

:lgtm:

Reviewed 1 of 1 files at r5.
Reviewable status: 22 of 23 files reviewed, all discussions resolved

@buggmagnet buggmagnet merged commit c28b222 into main Oct 11, 2024
10 of 11 checks passed
@buggmagnet buggmagnet deleted the add-toggle-to-enable-smart-routing-ios-831 branch October 11, 2024 12:49
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
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants