Migrate to gRPC #6272
Migrate to gRPC #6272
Conversation
Rawa
force-pushed
the
android-grpc
branch
3 times, most recently
from
ee54fb2 to
d7f64c7
Compare
There was a problem hiding this comment.
Reviewed 1 of 537 files at r1.
Reviewable status: 0 of 537 files reviewed, all discussions resolved
Cargo.lock line 2249 at r2 (raw file):
"mullvad-api", "mullvad-daemon", "mullvad-management-interface",
Get's added when we run the build
There was a problem hiding this comment.
Reviewed 58 of 537 files at r1, 1 of 1 files at r3.
Reviewable status: 0 of 537 files reviewed, 2 unresolved discussions
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/WireguardCustomPortDialog.kt line 80 at r3 (raw file):
val port = remember { mutableStateOf(initialPort?.value?.toString() ?: "") } val isValidPort =
We should probably rework this dialog and do it closer to how we do it with MTU dialog, it is more clean and let's the ViewModel handle the logic (This one doesn't really have a ViewModel at this point at all though)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/payment/PaymentDialog.kt line 128 at r3 (raw file):
val state by vm.uiState.collectAsStateWithLifecycle() LaunchedEffectCollect(vm.uiSideEffect) { resultBackNavigator.navigateBack(result = false) }
We should not leave out the when statement here, it is dangerous if we decide to add a new sideEffect later.
There was a problem hiding this comment.
Reviewed 123 of 537 files at r1.
Reviewable status: 121 of 537 files reviewed, 7 unresolved discussions (waiting on @Rawa)
android/app/build.gradle.kts line 348 at r1 (raw file):
implementation(Dependencies.KotlinX.coroutinesAndroid) implementation(Dependencies.Arrow.core)
nit: move to after androidx deps
Code quote:
implementation(Dependencies.Arrow.core)android/lib/resource/src/main/res/drawable/ic_remove.xml line 1 at r1 (raw file):
<vector xmlns:android="http://schemas.android.com/apk/res/android"
nit: we should double check if the new format is correct. applies to other xml files as well.
android/lib/shared/build.gradle.kts line 9 at r1 (raw file):
android { namespace = "net.mullvad.mullvadvpn.lib.account"
should be shared?
Code quote:
accountandroid/lib/shared/build.gradle.kts line 34 at r1 (raw file):
implementation(project(Dependencies.Mullvad.modelLib)) implementation(Dependencies.Kotlin.stdlib)
nit: order alphabetically
android/service/build.gradle.kts line 60 at r1 (raw file):
implementation(project(Dependencies.Mullvad.talpidLib)) implementation(Dependencies.jodaTime)
nit: order alphabetically
There was a problem hiding this comment.
Reviewed 4 of 537 files at r1, 1 of 1 files at r4.
Reviewable status: 118 of 537 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad)
android/lib/shared/build.gradle.kts line 9 at r1 (raw file):
Previously, albin-mullvad wrote…
should be
shared?
Fixed
There was a problem hiding this comment.
Reviewed 16 of 537 files at r1, 1 of 1 files at r3, 1 of 1 files at r8, all commit messages.
Reviewable status: 136 of 537 files reviewed, 6 unresolved discussions (waiting on @Rawa)
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/ChannelId.kt line 3 at r8 (raw file):
package net.mullvad.mullvadvpn.lib.model @JvmInline value class ChannelId(val value: String)
nit: change to NotificationChannelId
Code quote:
ChannelIdandroid/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/CustomDnsOptions.kt line 7 at r8 (raw file):
@optics data class CustomDnsOptions(val addresses: List<InetAddress>) {
Might be good to represent addresses with something else than the magic InetAddress class? Perhaps a future improvement?
There was a problem hiding this comment.
Reviewed 28 of 537 files at r1.
Reviewable status: 164 of 537 files reviewed, 10 unresolved discussions (waiting on @Rawa)
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/Device.kt line 8 at r8 (raw file):
@Parcelize data class Device(val id: DeviceId, private val name: String, val created: DateTime) : Parcelable {
nit: would be nice to change created to something more descriptive, since it could mean both "creation date" or "is created"
Code quote:
createdandroid/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/Device.kt line 12 at r8 (raw file):
name.split(" ").joinToString(" ") { word -> word.replaceFirstChar { firstChar -> firstChar.uppercase() } }
Could this be worth breaking out to our own "capitalize" function?
Code quote:
name.split(" ").joinToString(" ") { word ->
word.replaceFirstChar { firstChar -> firstChar.uppercase() }
}android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/Endpoint.kt line 5 at r8 (raw file):
import java.net.InetSocketAddress data class Endpoint(val address: InetSocketAddress, val protocol: TransportProtocol)
Might be worth changing? Or perhaps later?
Code quote:
InetSocketAddressandroid/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/ErrorStateCause.kt line 18 at r8 (raw file):
data object Ipv6Unavailable : ErrorStateCause() data class SetFirewallPolicyError(val firewallPolicyError: FirewallPolicyError) :
nit: change to FirewallPolicyError since Set doesn't add much meaning? Same applies to SetDnsError
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/WwwAuthToken.kt line 3 at r8 (raw file):
package net.mullvad.mullvadvpn.lib.model @JvmInline value class WwwAuthToken(val value: String)
nit: the name of this class is quite ugly imo. can we change this to just AuthToken or perhaps WebsiteAuthToken?
Code quote:
WwwAuthToken
There was a problem hiding this comment.
Reviewed 421 of 537 files at r1, 1 of 1 files at r2, 4 of 4 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: 534 of 537 files reviewed, 13 unresolved discussions (waiting on @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/NewDeviceNotificationUseCase.kt line 16 at r2 (raw file):
fun notifications() = combine( deviceRepository.deviceState.mapNotNull { it?.displayName() },
Would this cause issues with notifications if we don't get a device? Compare to previous discussions about filterNotNull
mullvad-jni/src/lib.rs line 273 at r8 (raw file):
android_context: AndroidContext, ) -> Result<(), Error> { // let listener = JniEventListener::spawn(env, this).map_err(Error::SpawnJniEventListener)?;
Probably remove this
mullvad-paths/Cargo.toml line 16 at r8 (raw file):
thiserror = { workspace = true } [target.'cfg(not(target_os="android"))'.dependencies]
Should this be removed? Or what would be the consequences of this?
There was a problem hiding this comment.
Reviewed 61 of 537 files at r1.
Reviewable status: 533 of 537 files reviewed, 18 unresolved discussions (waiting on @Pururun and @Rawa)
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/NotificationChannel.kt line 7 at r8 (raw file):
data object TunnelUpdates : NotificationChannel { override val id: ChannelId = ChannelId("tunnel_state_notification")
nit: would be nice to define some type of constants for these channel ids
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/Udp2TcpObfuscationSettings.kt line 3 at r8 (raw file):
package net.mullvad.mullvadvpn.lib.model data class Udp2TcpObfuscationSettings(val port: Constraint<Port>)
port is not supported yet, but might be worth preparing for the implementation like we've done here
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/UpdateCustomListError.kt line 1 at r8 (raw file):
package net.mullvad.mullvadvpn.lib.model
When reading the code in this file it's a bit unclear what the differences between ModifyCustomListError and UpdateCustomListError are. Can that be clarified somehow?
mullvad-daemon/src/lib.rs line 334 at r8 (raw file):
ClearSplitTunnelProcesses(ResponseTx<(), split_tunnel::Error>), /// Exclude traffic of an application from the tunnel #[cfg(any(windows, target_os = "android", target_os = "macos"))]
explicit target_os not needed for windows? Asking since it was previously present and still there for android and macos
mullvad-daemon/src/lib.rs line 658 at r8 (raw file):
macos::bump_filehandle_limit(); log::warn!("##### mullvad-daemon/lib.rs#start:!!!");
nit: should this be cleaned up? 🤔
There was a problem hiding this comment.
Reviewed 26 of 537 files at r1, 1 of 1 files at r6.
Reviewable status: 532 of 537 files reviewed, 20 unresolved discussions (waiting on @Pururun and @Rawa)
mullvad-daemon/src/main.rs line 172 at r8 (raw file):
} log::warn!("##### mullvad-daemon/main.rs#run_standalone !!!");
Should of these new log statements be kept?
mullvad-daemon/src/management_interface.rs line 24 at r8 (raw file):
wireguard::{RotationInterval, RotationIntervalError}, }; #[cfg(any(target_os = "windows", target_os = "macos"))]
Should macos be removed from this line in this pr? 🤔
Code quote:
target_os = "macos"
There was a problem hiding this comment.
Reviewable status: 524 of 537 files reviewed, 18 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/WireguardCustomPortDialog.kt line 80 at r3 (raw file):
Previously, Rawa (David Göransson) wrote…
We should probably rework this dialog and do it closer to how we do it with MTU dialog, it is more clean and let's the ViewModel handle the logic (This one doesn't really have a ViewModel at this point at all though)
Yes I think we should we create an issue for it though, there is a lot of things to improve with this dialog.
Added to the post grpc project.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/NewDeviceNotificationUseCase.kt line 16 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Would this cause issues with notifications if we don't get a device? Compare to previous discussions about
filterNotNull
Fixed
There was a problem hiding this comment.
Reviewable status: 520 of 537 files reviewed, 17 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/NewDeviceNotificationUseCase.kt line 16 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Would this cause issues with notifications if we don't get a device? Compare to previous discussions about
filterNotNull
I believe we might hold onto a device too long, e.g if deviceRepository sends out deviceState with null, we will ignore it and keep the latest device instead. We should consider reverting this change.
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/CustomDnsOptions.kt line 7 at r8 (raw file):
Previously, albin-mullvad wrote…
Might be good to represent addresses with something else than the magic
InetAddressclass? Perhaps a future improvement?
After discussion, lets not 🙈, linear issue created.
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/Device.kt line 8 at r8 (raw file):
Previously, albin-mullvad wrote…
nit: would be nice to change
createdto something more descriptive, since it could mean both "creation date" or "is created"
Fixed
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/Endpoint.kt line 5 at r8 (raw file):
Previously, albin-mullvad wrote…
Might be worth changing? Or perhaps later?
Added to ticket
mullvad-daemon/src/lib.rs line 334 at r8 (raw file):
Previously, albin-mullvad wrote…
explicit
target_osnot needed forwindows? Asking since it was previously present and still there forandroidandmacos
Not needed technically, there are 2 ways of marking target os in the rust code base, but let's see what they want.
mullvad-daemon/src/main.rs line 218 at r8 (raw file):
} pub fn spawn_management_interface(
Does it need to be pub?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r9, 1 of 1 files at r10, 3 of 8 files at r11, 8 of 10 files at r12.
Reviewable status: 533 of 537 files reviewed, 17 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/payment/PaymentDialog.kt line 128 at r3 (raw file):
Previously, Rawa (David Göransson) wrote…
We should not leave out the
whenstatement here, it is dangerous if we decide to add a new sideEffect later.
Done
There was a problem hiding this comment.
Reviewed 15 of 537 files at r1, 1 of 10 files at r12.
Reviewable status: 532 of 537 files reviewed, 19 unresolved discussions (waiting on @Pururun and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/PreviewData.kt line 1 at r12 (raw file):
package net.mullvad.mullvadvpn.compose.util
Not introduced in this PR, but would be nice to wrap these functions in a Preview/PreviewData object to not clutter down the global scope. Optimally we should also find a way of making sure we don't include any of these functions in release builds.
An alternative that might be even better would be PreviewParameter:
https://developer.android.com/develop/ui/compose/tooling/previews#preview-data
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/PreviewData.kt line 87 at r12 (raw file):
fun generateDevice( id: DeviceId = DeviceId(UUID.randomUUID()),
Why do we generate the UUID rather than hard code it? Afaik it's preferred to keep the previews deterministic
Code quote:
UUID.randomUUID()
There was a problem hiding this comment.
Reviewed 1 of 10 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: 534 of 537 files reviewed, 19 unresolved discussions (waiting on @albin-mullvad and @Rawa)
There was a problem hiding this comment.
Reviewed 10 of 537 files at r1, all commit messages.
Reviewable status: 534 of 537 files reviewed, 19 unresolved discussions (waiting on @Pururun and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/payment/PaymentDialog.kt line 128 at r3 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Done
Can we assume that the side effect should always result in a back navigation? 🤔
There was a problem hiding this comment.
Reviewed 1 of 8 files at r11, 1 of 10 files at r12.
Reviewable status: 529 of 537 files reviewed, 18 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/ErrorStateCause.kt line 18 at r8 (raw file):
Previously, albin-mullvad wrote…
nit: change to
FirewallPolicyErrorsinceSetdoesn't add much meaning? Same applies toSetDnsError
Done
There was a problem hiding this comment.
Reviewable status: 529 of 537 files reviewed, 17 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/PreviewData.kt line 1 at r12 (raw file):
Previously, albin-mullvad wrote…
Not introduced in this PR, but would be nice to wrap these functions in a
Preview/PreviewDataobject to not clutter down the global scope. Optimally we should also find a way of making sure we don't include any of these functions in release builds.An alternative that might be even better would be
PreviewParameter:
https://developer.android.com/develop/ui/compose/tooling/previews#preview-data
I think the latter idea would be the best way to go. 🤩
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/ErrorStateCause.kt line 18 at r8 (raw file):
Previously, albin-mullvad wrote…
nit: change to
FirewallPolicyErrorsinceSetdoesn't add much meaning? Same applies toSetDnsError
This entire structure should be remade, do we want to it in this PR or a new one?
There was a problem hiding this comment.
Reviewable status: 529 of 537 files reviewed, 17 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/PreviewData.kt line 87 at r12 (raw file):
Previously, albin-mullvad wrote…
Why do we generate the UUID rather than hard code it? Afaik it's preferred to keep the previews deterministic
Yes, it would be, also to be more performant.
There was a problem hiding this comment.
Reviewable status: 524 of 537 files reviewed, 17 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/PreviewData.kt line 87 at r12 (raw file):
Previously, Rawa (David Göransson) wrote…
Yes, it would be, also to be more performant.
Done
There was a problem hiding this comment.
Reviewable status: 524 of 537 files reviewed, 17 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/ErrorStateCause.kt line 18 at r8 (raw file):
Previously, Rawa (David Göransson) wrote…
This entire structure should be remade, do we want to it in this PR or a new one?
New one please.
There was a problem hiding this comment.
Reviewed 1 of 539 files at r1, 3 of 22 files at r36, 2 of 23 files at r39, 1 of 10 files at r42, 24 of 36 files at r43, all commit messages.
Reviewable status: 539 of 551 files reviewed, 13 unresolved discussions (waiting on @Pururun and @Rawa)
android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/mapper/ToDomain.kt line 460 at r41 (raw file):
Previously, Rawa (David Göransson) wrote…
Now split into parts
Super nice improvement! 🙌
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/UpdateCustomListError.kt line 12 at r32 (raw file):
Previously, Rawa (David Göransson) wrote…
Fixed
Nice improvement 👍
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 1 at r43 (raw file):
package net.mullvad.mullvadvpn.service
Some really nice improvements in MullvadVpnService 🙌
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 98 at r43 (raw file):
override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { Log.d(TAG, "onStartCommand (intent=$intent, flags=$flags, startId=$startId)") Log.d(TAG, "intent action=${intent?.action}")
Should these be kept?
Code quote:
Log.d(TAG, "onStartCommand (intent=$intent, flags=$flags, startId=$startId)")
Log.d(TAG, "intent action=${intent?.action}")android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 104 at r43 (raw file):
// Always promote to foreground if connect/disconnect actions are provided to mitigate cases // where the service would potentially otherwise be too slow running `startForeground`. Log.d(TAG, "Intent Action: ${intent?.action}")
Should this be kept?
Code quote:
Log.d(TAG, "Intent Action: ${intent?.action}")android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 122 at r43 (raw file):
override fun onBind(intent: Intent?): IBinder { bindCount.incrementAndGet()
Can we clarify with a comment or something why we keep track of the bind count this way?
Code quote:
bindCount.incrementAndGet()android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 123 at r43 (raw file):
override fun onBind(intent: Intent?): IBinder { bindCount.incrementAndGet() Log.d(TAG, "onBind: $intent")
Keep?
Code quote:
Log.d(TAG, "onBind: $intent")android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 126 at r43 (raw file):
if (intent.isFromSystem()) { Log.d(TAG, "onBind from VPN_SERVICE_CLASS")
Keep?
Code quote:
Log.d(TAG, "onBind from VPN_SERVICE_CLASS")android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 134 at r43 (raw file):
} else { Binder() }
Can we add some comment/link regarding this?
Code quote:
return super.onBind(intent)
?: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
Binder(this.toString())
} else {
Binder()
}android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 179 at r43 (raw file):
} private fun Intent?.isFromSystem(): Boolean {
Can we clarify this with a comment? E.g. why does it matter?
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/ForegroundNotificationManager.kt line 27 at r43 (raw file):
// channel.apply{ lockscreenVisibility = // AndroidNotification.VISIBILITY_SECRET }
Cleanup?
Code quote:
// channel.apply{ lockscreenVisibility =
// AndroidNotification.VISIBILITY_SECRET }android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/ForegroundNotificationManager.kt line 53 at r43 (raw file):
val androidNotification = tunnelStateNotification.toNotification(vpnService) if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE) {
Please add a comment regarding this API specific behavior
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryNotificationProvider.kt line 44 at r43 (raw file):
if (!IS_PLAY_BUILD) accountRepository.getWebsiteAuthToken() else null, durationUntilExpiry = durationUntilExpiry, isPlayBuild = false
Mistake or is there some reason for us to assume it's a play build? 🤔
Code quote:
isPlayBuild = falseandroid/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryNotificationProvider.kt line 63 at r43 (raw file):
companion object { private val REMAINING_TIME_FOR_REMINDERS = Duration.standardDays(2)
I believe this should be 3 🤔
Code quote:
2android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/tunnelstate/TunnelStateNotificationAction.kt line 73 at r43 (raw file):
PendingIntent.getActivity(context, 1, intent, SdkUtils.getSupportedPendingIntentFlags()) } else { val intent = Intent(toKey()).setPackage(context.packageName)
When reading this I realized we should make sure to check that devmole/stagmole builds work as expected
Code quote:
context.packageNameandroid/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/tunnelstate/TunnelStateNotificationProvider.kt line 58 at r43 (raw file):
notificationId, Notification.Tunnel( channelId = channelId,
Regarding channels, we should make sure no notification settings are lost after upgrades from old app versions. Do we have a checklist for things to test as part of this major update? Otherwise it would be nice if we can start adding items to such a list
Code quote:
channelId = channelId,
There was a problem hiding this comment.
Reviewed 2 of 7 files at r44, all commit messages.
Reviewable status: 537 of 551 files reviewed, 14 unresolved discussions (waiting on @albin-mullvad, @Pururun, and @Rawa)
a discussion (no related file):
There are some unused dependencies still, eg in mullvad-jni.
| @@ -187,85 +173,65 @@ class CustomListsRepositoryTest { | |||
| @Test | |||
| fun `when delete custom lists is called a delete custom event should be sent`() = runTest { | |||
There was a problem hiding this comment.
Rename, no longer event, rather invokes the managmenet service.
There was a problem hiding this comment.
Reviewable status: 537 of 551 files reviewed, 15 unresolved discussions (waiting on @albin-mullvad and @Pururun)
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 98 at r43 (raw file):
Previously, albin-mullvad wrote…
Should these be kept?
Existing linear issue for this, marked as blocking for release. We want do be able to see this information during testing for now.
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 104 at r43 (raw file):
Previously, albin-mullvad wrote…
Should this be kept?
See above, should only result in Connect/Disconnect.
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 122 at r43 (raw file):
Previously, albin-mullvad wrote…
Can we clarify with a comment or something why we keep track of the bind count this way?
Clarified at the initialization of bindCount
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 123 at r43 (raw file):
Previously, albin-mullvad wrote…
Keep?
See above comment about logs
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 126 at r43 (raw file):
Previously, albin-mullvad wrote…
Keep?
See above
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 134 at r43 (raw file):
Previously, albin-mullvad wrote…
Can we add some comment/link regarding this?
Clarified with a comment.
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 179 at r43 (raw file):
Previously, albin-mullvad wrote…
Can we clarify this with a comment? E.g. why does it matter?
Clarified with a comment.
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/ForegroundNotificationManager.kt line 27 at r43 (raw file):
Previously, albin-mullvad wrote…
Cleanup?
Done. Removed, already handled
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryNotificationProvider.kt line 44 at r43 (raw file):
Previously, albin-mullvad wrote…
Mistake or is there some reason for us to assume it's a play build? 🤔
Removed isPlayBuild completely as it is not needed.
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryNotificationProvider.kt line 63 at r43 (raw file):
Previously, albin-mullvad wrote…
I believe this should be
3🤔
It was 2 standard days in the old code base. Should we change now or in separate PR?
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/tunnelstate/TunnelStateNotificationProvider.kt line 58 at r43 (raw file):
Previously, albin-mullvad wrote…
Regarding channels, we should make sure no notification settings are lost after upgrades from old app versions. Do we have a checklist for things to test as part of this major update? Otherwise it would be nice if we can start adding items to such a list
Word this should be done. Added a more detailed testing card to issue tracker
There was a problem hiding this comment.
Reviewable status: 530 of 551 files reviewed, 15 unresolved discussions (waiting on @albin-mullvad, @dlon, @MarkusPettersson98, and @Pururun)
a discussion (no related file):
Previously, dlon (David Lönnhager) wrote…
There are some unused dependencies still, eg in
mullvad-jni.
Would be nice if we can remove them! Not sure if @MarkusPettersson98 can help? He also has a couple of small changes to add to this PR still.
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/ForegroundNotificationManager.kt line 53 at r43 (raw file):
Previously, albin-mullvad wrote…
Please add a comment regarding this API specific behavior
Clarified behavior, by refactoring code.
There was a problem hiding this comment.
Reviewable status: 530 of 551 files reviewed, 15 unresolved discussions (waiting on @albin-mullvad, @dlon, @MarkusPettersson98, and @Pururun)
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/tunnelstate/TunnelStateNotificationAction.kt line 73 at r43 (raw file):
Previously, albin-mullvad wrote…
When reading this I realized we should make sure to check that
devmole/stagmolebuilds work as expected
Relevant, one issue is to be fixed by @MarkusPettersson98 whom is working on a fix for the daemon to support setting up the gRPC socket in accordance to each build variant.
There was a problem hiding this comment.
Reviewed 2 of 7 files at r44, 1 of 3 files at r45, 6 of 7 files at r46, 2 of 2 files at r47, all commit messages.
Reviewable status: 539 of 551 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98, @Pururun, and @Rawa)
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 98 at r43 (raw file):
Previously, Rawa (David Göransson) wrote…
Existing linear issue for this, marked as blocking for release. We want do be able to see this information during testing for now.
Sounds good 👍
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 126 at r43 (raw file):
Previously, Rawa (David Göransson) wrote…
See above
nit: maybe change to something like "Service bound by system"?
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 134 at r46 (raw file):
Good comment in general, I just suggest rephrasing it a little bit:
We always need to return a binder. If the system binds to our VPN service, VpnService will return a binder that shall be user, otherwise we return an empty dummy binder to keep connection service alive since the actual communication happens over gRPC.
Also, would be nice to clarify "that shall be user" a bit
Code quote:
// We always need to return a binder, if system binds to our VPN service, VpnService will
// return a binder that shall be user, otherwise we return a dummy binder to keep connection
// and service alive, but communication goes over gRPCandroid/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryNotificationProvider.kt line 63 at r43 (raw file):
Previously, Rawa (David Göransson) wrote…
It was 2 standard days in the old code base. Should we change now or in separate PR?
Alright 👍
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/tunnelstate/TunnelStateNotificationProvider.kt line 58 at r43 (raw file):
Previously, Rawa (David Göransson) wrote…
Word this should be done. Added a more detailed testing card to issue tracker
Nice
There was a problem hiding this comment.
Reviewed 5 of 36 files at r43, 2 of 7 files at r44, 3 of 3 files at r45, 3 of 7 files at r46, 2 of 2 files at r47, all commit messages.
Reviewable status: 547 of 551 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98 and @Rawa)
There was a problem hiding this comment.
Reviewable status: 547 of 551 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @MarkusPettersson98)
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 126 at r43 (raw file):
Previously, albin-mullvad wrote…
nit: maybe change to something like "Service bound by system"?
Clarified log.
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt line 134 at r46 (raw file):
Previously, albin-mullvad wrote…
Good comment in general, I just suggest rephrasing it a little bit:
We always need to return a binder. If the system binds to our VPN service, VpnService will return a binder that shall be user, otherwise we return an empty dummy binder to keep connection service alive since the actual communication happens over gRPC.
Also, would be nice to clarify "that shall be user" a bit
Good remark, updated! 👍
Rawa
force-pushed
the
android-grpc
branch
2 times, most recently
from
5ef8e86 to
ec4f0e9
Compare
| // Arrange | ||
| val tunnelEndpoint: TunnelEndpoint = mockk() | ||
| val location: GeoIpLocation = mockk() | ||
| val tunnelUiStateTestItem = TunnelState.Connected(tunnelEndpoint, location) |
There was a problem hiding this comment.
Since we don't talk about TunnelUiState anymore we should remove it from the tests and just call it TunnelState.
Rawa
force-pushed
the
android-grpc
branch
2 times, most recently
from
bd9d19b to
4c5b656
Compare
There was a problem hiding this comment.
Reviewed 1 of 10 files at r48, 1 of 1 files at r50, all commit messages.
Reviewable status: 538 of 551 files reviewed, 1 unresolved discussion (waiting on @Pururun and @Rawa)
a discussion (no related file):
Previously, Rawa (David Göransson) wrote…
Would be nice if we can remove them! Not sure if @MarkusPettersson98 can help? He also has a couple of small changes to add to this PR still.
As discussed we'll do some cleanup and fix the alternate flavors in a separate PR since this PR is quite unmanagable at this point.
android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/tunnelstate/TunnelStateNotificationAction.kt line 73 at r43 (raw file):
Previously, Rawa (David Göransson) wrote…
Relevant, one issue is to be fixed by @MarkusPettersson98 whom is working on a fix for the daemon to support setting up the gRPC socket in accordance to each build variant.
As discussed, that will be handled in a separate PR.
There was a problem hiding this comment.
Reviewed 8 of 10 files at r48, 1 of 1 files at r52.
Reviewable status: 547 of 551 files reviewed, 1 unresolved discussion (waiting on @Rawa)
There was a problem hiding this comment.
Reviewed 4 of 10 files at r48, 1 of 1 files at r49, 1 of 1 files at r51.
Reviewable status: 547 of 551 files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 547 of 551 files reviewed, all discussions resolved (waiting on @Rawa)
Co-authored-by: Jonatan Rhodin <[email protected]> Co-authored-by: Markus Pettersson <[email protected]> Co-authored-by: David Lönnhager <[email protected]>
There was a problem hiding this comment.
Reviewed 1 of 1 files at r54, all commit messages.
Reviewable status: 547 of 552 files reviewed, all discussions resolved (waiting on @Rawa)
There was a problem hiding this comment.
Reviewed 364 of 539 files at r1, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 1 of 8 files at r11, 1 of 10 files at r12, 1 of 1 files at r13, 2 of 2 files at r14, 1 of 5 files at r15, 1 of 5 files at r16, 1 of 1 files at r17, 2 of 17 files at r22, 1 of 2 files at r23, 2 of 11 files at r24, 6 of 6 files at r26, 2 of 13 files at r28, 1 of 2 files at r29, 2 of 3 files at r30, 13 of 31 files at r32, 4 of 5 files at r33, 4 of 8 files at r34, 1 of 1 files at r35, 9 of 22 files at r36, 2 of 6 files at r37, 1 of 2 files at r38, 6 of 23 files at r39, 1 of 5 files at r40, 13 of 14 files at r41, 8 of 10 files at r42, 10 of 36 files at r43, 3 of 7 files at r44, 3 of 3 files at r45, 4 of 7 files at r46, 1 of 2 files at r47, 6 of 10 files at r48, 1 of 1 files at r50, 1 of 1 files at r52.
Reviewable status: 547 of 552 files reviewed, all discussions resolved (waiting on @Rawa)
This PR performs a major overhaul of the communication between the Daemon and the Android Service layer. It migrates over most of the old legacy JNI and IPC Messenger implementation to use the same protocol as the desktop client, gRPC.
Most notable changes:
This change is