-
Notifications
You must be signed in to change notification settings - Fork 356
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
Replace retry with exponential backoff with arrow schedule #6362
Replace retry with exponential backoff with arrow schedule #6362
Conversation
02b948d
to
4432dbe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 14 of 15 files at r1, all commit messages.
Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/PaymentUseCase.kt
line 61 at r1 (raw file):
} override suspend fun verifyPurchases() =
It's a bit difficult to interpret what this boolean means in respect to verifyPurchases
. true
could for example potentially mean that "everything has already been verified".
Also, I'm a bit curious to hear your thoughts on whether or not we could expose the result as an either
🤔
Code quote:
verifyPurchases
android/lib/payment/build.gradle.kts
line 44 at r1 (raw file):
implementation(Dependencies.Kotlin.stdlib) implementation(Dependencies.KotlinX.coroutinesAndroid) implementation(Dependencies.Arrow.core)
nit: move to the top of the dependency list
Code quote:
implementation(Dependencies.Arrow.core)
3373677
to
14b4273
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 8 of 15 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/PaymentUseCase.kt
line 61 at r1 (raw file):
Previously, albin-mullvad wrote…
It's a bit difficult to interpret what this boolean means in respect to
verifyPurchases
.true
could for example potentially mean that "everything has already been verified".Also, I'm a bit curious to hear your thoughts on whether or not we could expose the result as an
either
🤔
I guess it depends if you want to add getOrNull() == VerificationResult.Success
to all viewmodels or not.
Updated.
android/lib/payment/build.gradle.kts
line 44 at r1 (raw file):
Previously, albin-mullvad wrote…
nit: move to the top of the dependency list
Done
fb77b72
to
7a5bc62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 15 files at r1, 5 of 6 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/PaymentUseCase.kt
line 61 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
I guess it depends if you want to add
getOrNull() == VerificationResult.Success
to all viewmodels or not.
Updated.
Yeah, I think it's best to prioritze keeping the main API clear even though it might require the rest of the code to be a bit more verbose.
Maybe there's some arrow/either API that can be used to make the VMs prettier? Otherwise we can always consider using some extension function such as:
fun Either<VerificationError, VerificationResult>.isSuccess() =
this.getOrNull() == VerificationResult.Success
Which would let the VMs use:
paymentUseCase.verifyPurchases().isSuccess()
android/lib/billing/src/main/kotlin/net/mullvad/mullvadvpn/lib/billing/BillingPaymentRepository.kt
line 136 at r3 (raw file):
} override suspend fun verifyPurchases(): Either<VerificationError, VerificationResult> = either {
How will the lack of emitting FetchingUnfinishedPurchases
and VerificationStarted
here impact the code and app behavior? 🤔
There was a problem hiding this 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, 2 unresolved discussions (waiting on @albin-mullvad)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/PaymentUseCase.kt
line 61 at r1 (raw file):
Previously, albin-mullvad wrote…
Yeah, I think it's best to prioritze keeping the main API clear even though it might require the rest of the code to be a bit more verbose.
Maybe there's some arrow/either API that can be used to make the VMs prettier? Otherwise we can always consider using some extension function such as:
fun Either<VerificationError, VerificationResult>.isSuccess() = this.getOrNull() == VerificationResult.Success
Which would let the VMs use:
paymentUseCase.verifyPurchases().isSuccess()
Yes that would look better.
android/lib/billing/src/main/kotlin/net/mullvad/mullvadvpn/lib/billing/BillingPaymentRepository.kt
line 136 at r3 (raw file):
Previously, albin-mullvad wrote…
How will the lack of emitting
FetchingUnfinishedPurchases
andVerificationStarted
here impact the code and app behavior? 🤔
No impact at all. The fact that it was a flow that emitted states before was kind of redundant since we only checked for success state and error states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/PaymentUseCase.kt
line 61 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Yes that would look better.
So that means you'll update the PR? I'm fine with either keeping as-is or updating 🙂
android/lib/billing/src/main/kotlin/net/mullvad/mullvadvpn/lib/billing/BillingPaymentRepository.kt
line 136 at r3 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
No impact at all. The fact that it was a flow that emitted states before was kind of redundant since we only checked for success state and error states.
Alright 👍
There was a problem hiding this 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, 1 unresolved discussion (waiting on @albin-mullvad)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/PaymentUseCase.kt
line 61 at r1 (raw file):
Previously, albin-mullvad wrote…
So that means you'll update the PR? I'm fine with either keeping as-is or updating 🙂
Yes sorry, will add it.
7a5bc62
to
6acbeb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 10 of 16 files reviewed, all discussions resolved (waiting on @albin-mullvad)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/PaymentUseCase.kt
line 61 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Yes sorry, will add it.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
6acbeb8
to
93c9ff3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
Also clean up the code related to play purchase verification
93c9ff3
to
8934d4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
Also clean up the code related to play purchase verification
This change is