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 piping for google play payment API requests #5230

Merged
merged 5 commits into from
Oct 16, 2023

Conversation

Jontified
Copy link
Contributor

@Jontified Jontified commented Oct 4, 2023

This commit adds all of the basic piping in order to let Android use the JNI interface in order to make requests to our API pertaining to google play payment initialization and status.


This change is Reviewable

@Jontified Jontified added WIP Android Issues related to Android labels Oct 4, 2023
@Jontified Jontified requested review from Pururun and dlon October 4, 2023 12:41
@Jontified Jontified force-pushed the feature-api-android-inapp-purchases branch 5 times, most recently from 887f1d9 to 02b91ed Compare October 4, 2023 13:53
Copy link
Member

@dlon dlon 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 r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Jontified)


mullvad-api/src/lib.rs line 504 at r1 (raw file):

        }

        let submission = PlayPurchaseSubmission {

Nit: Could we not just derive Serialize on PlayPurchase?


mullvad-jni/src/lib.rs line 196 at r1 (raw file):

#[derive(IntoJava)]
#[jnix(package = "net.mullvad.mullvadvpn.model")]
pub enum PlayPurchaseInitResult {

Should we create the actual type in Kotlin for this?


mullvad-jni/src/lib.rs line 202 at r1 (raw file):

#[derive(IntoJava)]
#[jnix(package = "net.mullvad.mullvadvpn.model")]

Should we create the actual type in Kotlin for this?


mullvad-types/src/account.rs line 15 at r1 (raw file):

/// The payment token returned by initiating a google play purchase.
/// In the API this is called the `obfuscated_id`.
pub type PlayPurchaseInitResult = String;

Could we maybe rename this to PlayPurchasePaymentToken or something else less generic?

Copy link
Contributor

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

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon and @Jontified)


mullvad-jni/src/lib.rs line 196 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should we create the actual type in Kotlin for this?

They are added in a separate PR


mullvad-jni/src/lib.rs line 202 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should we create the actual type in Kotlin for this?

They are added in a seperate PR

Copy link
Member

@dlon dlon 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, 3 unresolved discussions (waiting on @Jontified)


mullvad-jni/src/lib.rs line 204 at r1 (raw file):

#[jnix(package = "net.mullvad.mullvadvpn.model")]
pub enum PlayPurchaseInitError {
    OtherError,

Is this generic error good enough? We might want to differentiate between timeouts, the service being unavailable, etc.

Copy link
Contributor Author

@Jontified Jontified 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, 3 unresolved discussions (waiting on @dlon)


mullvad-api/src/lib.rs line 504 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: Could we not just derive Serialize on PlayPurchase?

Yeah, why not


mullvad-types/src/account.rs line 15 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Could we maybe rename this to PlayPurchasePaymentToken or something else less generic?

Yes we can!

@Pururun Pururun force-pushed the feature-api-android-inapp-purchases branch from 23e27a5 to cd92b92 Compare October 9, 2023 15:06
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 6 files at r2, 2 of 8 files at r3, all commit messages.
Reviewable status: 9 of 15 files reviewed, 1 unresolved discussion (waiting on @Jontified)

Copy link
Contributor

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

Reviewed 5 of 6 files at r2, 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)


mullvad-jni/src/lib.rs line 204 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Is this generic erro enough? We might want to differentiate between timeouts, the service being unavailable, etc.

Yeah it would be nice to differentiate between generic errors and the mullvad service being unavailable for some reason.

Copy link
Contributor

@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: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@dlon dlon 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: :shipit: complete! all files reviewed, all discussions resolved

@Jontified Jontified force-pushed the feature-api-android-inapp-purchases branch from cd92b92 to 9ea4e9a Compare October 16, 2023 13:34
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 8 files at r5, 7 of 7 files at r6, 7 of 7 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Jontified)


mullvad-api/src/rest.rs line 404 at r6 (raw file):

const DEFAULT_ERROR_TYPE: &str = "about:blank";
#[derive(serde::Deserialize)]
pub struct NewErrorResponse {

Can we make this struct and OldErrorResponse private?


mullvad-api/src/rest.rs line 407 at r6 (raw file):

    pub status: i32, 
    pub title: String,
    pub r#type: Option<String>,

Could we out the fields we don't actually use? That is, everything except r#type.


mullvad-daemon/src/device/mod.rs line 908 at r6 (raw file):

        match &response {
            Ok(_) => (),
            Err(Error::InvalidAccount) => {

Do we still get the old style error codes from the API when this occurs? If not, they'll probably not have been parsed correctly here.


mullvad-types/src/account.rs line 54 at r6 (raw file):

pub struct PlayPurchase {
    pub product_id: String,
    pub purchase_token: String,

Nit: Shouldn't this be a PlayPurchasePaymentToken?

Copy link
Member

@dlon dlon 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, 3 unresolved discussions (waiting on @Jontified)

Copy link
Contributor Author

@Jontified Jontified 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: 14 of 16 files reviewed, all discussions resolved (waiting on @dlon)


mullvad-daemon/src/device/mod.rs line 908 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Do we still get the old style error codes from the API when this occurs? If not, they'll probably not have been parsed correctly here.

Old codes will (likely) be sent for this.


mullvad-jni/src/lib.rs line 204 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Yeah it would be nice to differentiate between generic errors and the mullvad service being unavailable for some reason.

Update: we've agreed to do this in a separate PR when we've worked out how exactly we want to do errors.

Copy link
Member

@dlon dlon 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 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Jontified Jontified force-pushed the feature-api-android-inapp-purchases branch from f938aea to 133d6f1 Compare October 16, 2023 16:33
Jontified and others added 4 commits October 16, 2023 18:34
This commit adds all of the basic piping in order to let Android use the
JNI interface in order to make requests to our API pertaining to google
play payment initialization and status.
Add conditional compilation for google pay API access for only android.
Also allow new error type to be parsed.
Additionally fix review comments, formatting and warnings.
@Jontified Jontified force-pushed the feature-api-android-inapp-purchases branch from 133d6f1 to 3d5cbe6 Compare October 16, 2023 16:34
@Jontified Jontified merged commit 9ea790f into main Oct 16, 2023
30 checks passed
@Jontified Jontified deleted the feature-api-android-inapp-purchases branch October 16, 2023 17:45
@Jontified Jontified removed the WIP label Oct 30, 2023
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.

4 participants