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

Improve Purchase api verification #5512

Merged

Conversation

Pururun
Copy link
Contributor

@Pururun Pururun commented Nov 28, 2023

  1. Add exponential back off for purchase verification
  2. Add purchase verification after a purchase flow has failed

This change is Reviewable

@Pururun Pururun added the Android Issues related to Android label Nov 28, 2023
@Pururun Pururun requested review from Rawa and sabercodic November 28, 2023 13:10
@Pururun Pururun self-assigned this Nov 28, 2023
Copy link

linear bot commented Nov 28, 2023

DROID-534 Add exponential back off to payment verification

If we get failed verification we should try again with exponential back off.

@Pururun Pururun force-pushed the add-exponential-back-off-to-payment-verification-droid-534 branch from 8db3a07 to 94b35d5 Compare November 28, 2023 13:14
Copy link
Contributor

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r1, all commit messages.
Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/PaymentUseCase.kt line 57 at r1 (raw file):

                backOffDelayFactor = VERIFICATION_BACK_OFF_FACTOR
            ) {
                it is VerificationResult.Error

Does it work on both VerificationError and BillingError?

Copy link
Contributor Author

@Pururun Pururun 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: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/PaymentUseCase.kt line 57 at r1 (raw file):

Previously, sabercodic wrote…

Does it work on both VerificationError and BillingError?

The idea is that it will retry on both billing error (unable to retrieve any purchase from Google) or VerificationError (unable to verify with our api).

Copy link
Contributor

@sabercodic sabercodic 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: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/FlowUtils.kt line 138 at r1 (raw file):

@Suppress("UNCHECKED_CAST")
suspend inline fun <T> Flow<T>.retryWhen(

Can we go with better naming here?

@Pururun Pururun force-pushed the add-exponential-back-off-to-payment-verification-droid-534 branch from 94b35d5 to 65993f5 Compare December 1, 2023 10:08
Copy link
Contributor Author

@Pururun Pururun 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: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/FlowUtils.kt line 138 at r1 (raw file):

Previously, sabercodic wrote…

Can we go with better naming here?

Done.

Copy link
Contributor

@Rawa Rawa 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: 1 of 6 files reviewed, 3 unresolved discussions (waiting on @Pururun and @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/FlowUtils.kt line 152 at r2 (raw file):

        }
        .retryWhen { cause, attempt ->
            if (attempt < maxAttempts) {

Small detail that I think makes it a bit more readable, we should early terminate if it is possible. E.g in this case:

if(attempt >= maxAttemps) {
   return false
}

android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/FlowUtils.kt line 154 at r2 (raw file):

            if (attempt < maxAttempts) {
                delay(backOffDelay)
                backOffDelay *= backOffDelayFactor

Suggestion: We simply calculate this value every single time, then we don't have to bother modifying the var.

fun Long.pow(exponent: Int): Long = toDouble().pow(exponent).toLong()
initialBackOffDelay * backOffDelayFactor.pow(attempt)

Start of the function would be more clear as well:

): Flow<T> = map {

Copy link
Contributor

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@Pururun Pururun force-pushed the add-exponential-back-off-to-payment-verification-droid-534 branch from 65993f5 to 082578a Compare December 4, 2023 07:52
Copy link
Contributor

@Rawa Rawa 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: 5 of 7 files reviewed, 3 unresolved discussions (waiting on @Pururun and @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/FlowUtils.kt line 154 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Suggestion: We simply calculate this value every single time, then we don't have to bother modifying the var.

fun Long.pow(exponent: Int): Long = toDouble().pow(exponent).toLong()
initialBackOffDelay * backOffDelayFactor.pow(attempt)

Start of the function would be more clear as well:

): Flow<T> = map {

nice 👍


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/FlowUtils.kt line 144 at r3 (raw file):

    crossinline predicate: (T) -> Boolean,
): Flow<T> =
    this.map {

Do we need this? Shouldn't it be possible to remove?

Copy link
Contributor Author

@Pururun Pururun 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: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @Rawa and @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/FlowUtils.kt line 144 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Do we need this? Shouldn't it be possible to remove?

Not sure I follow what should be removed?

Copy link
Contributor

@Rawa Rawa 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: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @Pururun and @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/FlowUtils.kt line 144 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Not sure I follow what should be removed?

this.map -> map, sorry it wasn't very clear that i meant the keyword this ^^

@Pururun Pururun force-pushed the add-exponential-back-off-to-payment-verification-droid-534 branch from 082578a to 7cd1472 Compare December 4, 2023 12:53
Copy link
Contributor Author

@Pururun Pururun 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: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @Rawa and @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/FlowUtils.kt line 144 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

this.map -> map, sorry it wasn't very clear that i meant the keyword this ^^

Ah yes, that could be removed. Done

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 5 of 7 files reviewed, all discussions resolved (waiting on @sabercodic)

@Pururun Pururun force-pushed the add-exponential-back-off-to-payment-verification-droid-534 branch from 7cd1472 to 9967b12 Compare December 6, 2023 22:35
@Pururun Pururun merged commit 74340e9 into main Dec 6, 2023
14 checks passed
@Pururun Pururun deleted the add-exponential-back-off-to-payment-verification-droid-534 branch December 6, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants