Restructure, UniFFI, and document the fxa-client crate. #3876

rfk · 2021-02-18T09:22:43Z

This is a substantial refactor of the fxa-client crate, intended to bring it
up to speed with our latest best-practices around developing cross-platform
Rust components, in order to ease ongoing maintenance.

It's still very much a work in progress, but the setup does seem to be working,
and feedback is always welcome.

[Edit: It's ready for review]

This introduces backwards-incompatible API changes, which I'm tracking in PRs to our downstream consumers here:

Android Components: Update to Application Services v73.0.1 mozilla-mobile/android-components#9731
Firefox for iOS: Update to application-services v73.0.0 mozilla-mobile/firefox-ios#8044

rfk · 2021-02-18T09:25:56Z

/cc @grigoryk for visibility into this work. I currently have a small diff to a-c to ensure that service-firefox-account compiles against this restructured version of fxaclient, my next steps are to try it out in a full Fenix build and see how it goes.

codecov-io · 2021-02-24T02:07:50Z

Codecov Report

Merging #3876 (e26cd01) into main (ee6cb4d) will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##            main   #3876    +/-   ##
======================================
  Coverage   0.00%   0.00%            
======================================
  Files        174     174            
  Lines      12257   12081   -176     
======================================
+ Misses     12257   12081   -176

Impacted Files	Coverage Δ
components/fxa-client/src/internal/auth.rs	`0.00% <ø> (ø)`
components/fxa-client/src/internal/commands/mod.rs	`0.00% <0.00%> (ø)`
...nents/fxa-client/src/internal/commands/send_tab.rs	`0.00% <0.00%> (ø)`
components/fxa-client/src/internal/config.rs	`0.00% <ø> (ø)`
components/fxa-client/src/internal/device.rs	`0.00% <0.00%> (ø)`
components/fxa-client/src/internal/error.rs	`0.00% <0.00%> (ø)`
components/fxa-client/src/internal/http_client.rs	`0.00% <0.00%> (ø)`
components/fxa-client/src/internal/migrator.rs	`0.00% <0.00%> (ø)`
components/fxa-client/src/internal/mod.rs	`0.00% <0.00%> (ø)`
components/fxa-client/src/internal/oauth.rs	`0.00% <0.00%> (ø)`
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee6cb4d...e26cd01. Read the comment docs.

rfk · 2021-02-26T10:48:01Z

components/fxa-client/android/build.gradle

+        commandLine 'cargo', 'uniffi-bindgen', 'generate', "${project.projectDir}/../src/fxa_client.udl", '--language', 'kotlin', '--out-dir', "${buildDir}/generated/source/uniffi/${variant.name}/java"
+
+        inputs.file '../src/fxa_client.udl'
+        outputs.dir "${buildDir}/generated/source/uniffi/${variant.name}/java/"


The inputs/outputs thing here helps gradle know when to re-run this command, and may be worth applying to other uniffied crates; /cc @lougeniaC64 if you want to try it out for autofil

megazords/ios/MozillaAppServices.xcodeproj/project.pbxproj

rfk · 2021-02-28T22:50:08Z

@grigoryk I've pinged you for review here for the Kotlin-side changes, if you're happy to take a look. (Your feedback on the whole thing is of course most welcome! But don't feel obliged to dig in to all the Rust changes)

mhammond

Wow, this is looking great and is a huge improvement! It's a huge diff and I'm only 1/2 way through and need to leave for the day, so might as well submit what I have. I'll look more tomorrow.

mhammond · 2021-03-03T05:06:58Z

components/fxa-client/README.md

+identity service.
+
+* [Features](#features)
+* [Using the Firefox Accounts client](#using-the-logins-component)


nit: a number of 'logins' references here.

mhammond · 2021-03-03T05:07:35Z

components/fxa-client/README.md

+   or a QR-code-based pairing flow.
+1. Accessing basic profile information about the signed-in user,
+   such as email address and display name.
+2. Obtaining OAuth access tokens and client-side encryption keys.


nit - use 1 consistently?

mhammond · 2021-03-03T05:08:58Z

components/fxa-client/README.md

+### Before using this component
+
+This component does not currently integrate with the [Glean SDK](https://mozilla.github.io/glean/book/index.html)
+and does not generate any telemetry, so you do not need to request a data-review before using this component.


s/generate/submit/ here? IIUC it does generate some via telemetry.rs

mhammond · 2021-03-03T05:11:24Z

components/fxa-client/README.md

+public API surface in Rust, and then generate bindings for Kotlin and Swift. The
+code is organized as follows:
+
+* The public API surface is implemented in [`./src/lib.rs`](./src/lib/rs), with matching


While this is great, very little is specific to FxA, so I wonder if there's an opportunity to move it to some other doc, just to avoid other components doing copy-pasta of it and it diverging over time? Or we just do that when we do that next component.

Indeed. I think it would be good to pull it out into a common doc at some point, but IMHO that's premature until we get most of the components in a similar shape.

mhammond · 2021-03-03T05:27:45Z

components/fxa-client/ios/FxAClient/PersistedFirefoxAccount.swift

+/// - Some convenience higher-level datatypes, such as URLs rather than plain Strings.
+///
+/// Eventually we'd like to move all of this into the underlying Rust code, once UniFFI
+/// grows support for these extra features.


Is it worth capturing what enhancements uniffi needs here (ie, links to issues?)

mhammond · 2021-03-03T05:33:25Z

components/fxa-client/ios/FxAClient/PersistedFirefoxAccount.swift

+        }
+    }
+
+    // TODO: not sure why we switched to a bool API for the Swift wrapper here...


nit: it's not quite clear what this is asking to actually be done.

Heh, yeah, I guess it's not really a TODO so much as a XXX I do not understand the point of this

mhammond · 2021-03-03T05:34:00Z

components/fxa-client/ios/FxAClient/PersistedFirefoxAccount.swift

+
+    // TODO: not sure why we switched to a bool API for the Swift wrapper here...
+
+    public func migrateFromSessionToken(


I wonder if we can kill the migration stuff soon? Regardless, I support not doing it as part of this PR.

I hope to one day need it for Desktop :-)

mhammond · 2021-03-03T05:36:16Z

components/fxa-client/README.md

+  declarations in [`./src/fxa_client.udl`](./src/fxa_client.udl) to define how it gets
+  exposed to other languages.
+    * All the implementation details are written in Rust and can be found under
+      [`./src/internal/`](./src/internal).


Do you think a convention of internal makes sense anywhere else? We don't have that with nimbus or autofill, where IIUC, the rule is, roughly, "if it's not exposed via the URL, it's internal". Why is it used here?

I'm trying it out.

Broadly, I like having the contents of src/lib.rs mirror the public API exposed to foreign-language consumers, I think it makes it easier to form a consistent mental model of the component. Pushing all the non-public stuff into an internal crate makes that easy, but it might end up feeling a little forced.

For a crate like this, I like internal as a refactoring mechanism, basically push all the existing stuff in there and then we can incrementally clean it up. For crates built from the start to work with UniFFI, maybe it would be a bit pointless.

What do you think?

For a crate like this, I like internal as a refactoring mechanism, basically push all the existing stuff in there and then we can incrementally clean it up. For crates built from the start to work with UniFFI, maybe it would be a bit pointless.

It's a good idea in that context, but TBH I don't think it makes sense longer term (ie, it should be temporary) and that rust visibility is the right tool, particularly if fully leveraged (eg, pub(super) is probably underused by us).

In particular, I'm mildly concerned that pragmatic compromises might mean moving lots of code from internal just to expose it - or worse, marking code in internal as pub. For example, there are a number of things in places that's only pub because places-utils.rs wants to use it (and ditto autofill), which is unfortunate, but I tend to suck that up and just allow it to be marked pub)

That's weakly held though, particularly as it's not a "battle tested" opinion (although I'm trying to be better at it in autofill)

Yep, I think I broadly agree with your point here - Rust has good mechanisms to manage what's public API and what's private, and we should lean on them rather than hack around them.

Apart from the contents of internal/mod.rs, most of what's in internal could quite happily live at the top level of the crate with limited visibility to prevent it from leaking out.

The goal is "don't have a bunch of pub stuff that's visible to Rust callers, but not available to foreign-language consumers" and I'm not too fussed about the implementation details. Which actually, that goal is pretty aligned with the recent email thread about Public API surface and mental models for our components...

mhammond · 2021-03-03T05:41:06Z

components/fxa-client/src/fxa_client.udl

+  // any potentially-state-changing operation.
+  //
+  // **⚠️ Warning:** the serialized state may contain encryption keys and access
+  // tokens that let anyone holding them access the user's data in Firefox Sync.


maybe add "and/or other FxA services" or similar - eg, the send-tab keys will be there too?

mhammond · 2021-03-03T05:57:20Z

components/fxa-client/src/fxa_client.udl

+  AuthorizationInfo check_authorization_status();
+
+
+  // Disconenct from the user's account.


nit: typo in Disconenct

mhammond

This is a massive patch but with even more massive value - I admit I skipped over some of it, so not formally approving, but will happily spend even more time to give it a formal r+ whenever you ask.

mhammond · 2021-03-04T06:12:22Z

components/fxa-client/src/internal/commands/mod.rs

+use super::error::*;
+
+#[derive(Clone, Debug)]
+pub enum IncomingDeviceCommand {


hrm - so after writing that comment about visibility, I'm a little confused by seeing pub here? Maybe pub(crate) better reflects what's going on?

This is one of the things that's currently used by some example crates.

mhammond · 2021-03-04T06:15:18Z

components/fxa-client/src/internal/commands/send_tab.rs

@@ -37,7 +39,7 @@ impl EncryptedSendTabPayload {
    }
 }

-#[derive(Debug, Serialize, Deserialize)]
+#[derive(Clone, Debug, Serialize, Deserialize)]


Yes, this makes perfect sense to me and I expect will be a common pattern for our components - eg, what we serialize to/from the sync servers is rarely going to match what we expose over the api.

Re my previous comment - an internal dir that really only had internal structs used for things like deserializing server responses does make more sense, but it makes less sense to see many impl blocks there other than the Into etc impls

mhammond · 2021-03-04T06:16:35Z

components/fxa-client/src/internal/device.rs

+
+use serde_derive::*;
+
+pub use super::http_client::{


as per above comment, 100% agree

mhammond · 2021-03-04T06:19:03Z

components/fxa-client/src/internal/device.rs

@@ -295,11 +293,7 @@ impl FirefoxAccount {
    // for the device because the server does not have a `PATCH commands`
    // endpoint yet.
    #[allow(dead_code)]


I suspect these dead_code annotations are now stale, so we should probably get an issue on file to remove them, as IMO they have value in general. I understand they are often necessary during dev, but generally shouldn't be necessary longer term.

I've removed a couple, and put a couple back that appeared to still be necessary.

mhammond · 2021-03-04T06:27:18Z

components/fxa-client/src/internal/mod.rs

@@ -0,0 +1,560 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */


mhammond · 2021-03-04T06:30:21Z

components/fxa-client/src/lib.rs

+// Aspirationally, I'd like to make it private, so that the public API of the crate
+// is entirely the same as the API exposed to consumers via UniFFI. That's not
+// possible right now because some of our tests/example use features that we do
+// not currently expose to consumers. But we should figure out how to expose them!


That's a great aspiration. As mentioned above, the URL/Guid <-> String type things might require some uniffi changes, or maybe it already has what we need there - but yeah, not sure how much we should try and insist on that.

mhammond · 2021-03-04T06:33:46Z

examples/fxa-client/src/devices-api.rs

@@ -4,7 +4,7 @@

 use cli_support::prompt::prompt_string;
 use dialoguer::Select;
-use fxa_client::{device, Config, FirefoxAccount, IncomingDeviceCommand};
+use fxa_client::internal::{device, Config, FirefoxAccount, IncomingDeviceCommand};


yeah - some tension I've found here is probably phrased better as "sometimes we want an example to be more like a diagnotic/test utility than an example of real-world usage" - ie, I explicitly want to call internal stuff in my example while not making it public". ie2, sometimes I want my examples to have crate visibility.

rfk · 2021-03-04T06:54:35Z

I think there's ongoing work to be done around managing the visibility of things in this crate, but just to ensure we're on the same page: @mhammond do you think it's worth pursuing more as part of this PR, or doing it in followups over time?

mhammond · 2021-03-04T21:45:55Z

or doing it in followups over time?

IMO followups is fine (and TBH I'm not too bothered even then - they were more off-the-cuff comments)

rfk · 2021-03-04T21:58:21Z

@mhammond sounds good, thanks! I'll take some time to rebase this and address your comments so far, and then let's please consider this ready for official review. It would be good to get it out into Fenix nightly while we're early in the cycle for maximal bake time.

rfk · 2021-03-05T06:20:32Z

Just noting for completeness, I've pushed some tweaks and this is ready for formal r?.

rfk · 2021-03-08T00:40:52Z

The failing tests here are the sync integration tests, which are failing due to 502 server errors from the FxA dev server.

mhammond

🚢 this is great, future me thanks you!

mhammond · 2021-03-09T01:02:55Z

components/fxa-client/README.md

+   or a QR-code-based pairing flow.
+1. Accessing basic profile information about the signed-in user,
+   such as email address and display name.
+1. Obtaining OAuth access tokens and client-side encryption keys.


nit: stray trailing period.

mhammond · 2021-03-09T01:03:43Z

components/fxa-client/README.md

+1. Listing and managing other applications connected to the
+   user's account.
+     1. Sending tabs to other applications that are capable
+      of receiving them.


indentation looks odd here (not clear if it matters when rendered, so feel free to ignore if not)

mhammond · 2021-03-09T01:10:46Z

...ts/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/PersistedFirefoxAccount.kt

-        )
-    }, persistCallback) {
+    constructor(config: Config, persistCallback: PersistCallback? = null) : this(FirefoxAccount(
+        config.contentUrl,


Maybe add the essence of this as comment?

mhammond · 2021-03-09T01:19:04Z

components/fxa-client/ios/FxAClient/PersistedFirefoxAccount.swift

+    }
+
+    public func completeOAuthFlow(code: String, state: String) throws {
+        defer { tryPersistState() }


I noticed above that the kotlin bindings previously did tryPersistState() before the call and now do it after - which I thought a little strange, but assumed doing it after was easier to rationalize about, so shrugged. However, here we are doing it before, which seems an unfortunate inconsistency.

The use of defer here means that we'll always do it at the end of the function call, like a weird finally block.

mhammond · 2021-03-09T02:39:33Z

components/fxa-client/src/internal/state_persistence.rs

-// If you find yourself modifying this code, you're almost certainly creating a potential data-migration
-// problem and should reconsider.
-
+/// `StateV1` was a previous state schema.


Does this need to exist still? It looks like it's very old and we will never hit this case in the wild. (OTOH, no need to block - maybe another ticket which could be good for onboarding?)

Sounds good, filed as #3912

mhammond · 2021-03-09T02:42:13Z

megazords/ios/MozillaAppServicesTests/FxAccountManagerTests.swift

@@ -173,7 +173,7 @@ class FxAccountManagerTests: XCTestCase {
    func testAccountRestorationEnsureCapabilitiesNonAuthError() {
        class MockAccount: MockFxAccount {
            override func ensureCapabilities(supportedCapabilities _: [DeviceCapability]) throws {
-                throw FirefoxAccountError.network(message: "The WiFi cable is detached.")
+                throw FxaError.Network(message: "The WiFi cable is detached.")


lol @ this error message - I've never seen it before

mhammond · 2021-03-09T02:46:14Z

components/fxa-client/src/fxa_client.udl

+// calling code should respond.
+//
+//
+//  `Authentication`:


Any reason to not put the comments for each error next to the definition?

TBH I don't recall, but the answer is almost certainly "I could not convince the automagical UDL generator to do that for me"...

Aha, it was because we currently represent error enum variants in the ComponentInterface by just their string names, rather than as a Variant object, so there was nowhere for me to stash the docs on each individual variant. I'll fix it up by hand.

This is a substantial refactor of the fxa-client crate, intended to bring it up to speed with our latest best-practices around developing cross-platform Rust components, in order to ease ongoing maintenance. THe core change here is that I've deleted the hand-written Kotlin and Swift bindings, replacing them with autogenerated bindings thanks to UniFFI. There is still a little bit of hand-written Kotlin, since we have a layer that automatically manages persistence via a callback. There is still a nontrivial amount of hand-written Swift, since we have a higher-level state machine built atop the lower-level fxa-client functionality (such state-machine also exists for Kotlin, but lives in the android-components repo). If UniFFI works out then we should look into moving more of that logic into shared Rust code over time. To support the introduction of UniFFI, I have restructured to Rust crate so that its public interface deliberately parallels the interface offered to Kotlin and Swift, and have moved the implementation details into a submodule named `internal`. It's my opinionated belief that structuring the crate this way will help us focus on producing a nice consumer API, which is not always the same thing as producing a nice Rust crate. On top of that, I've also revamped the documentation in the crate, leaning in to the use of `cargo doc` as the source of truth for both developer- and consumer-facing documentation. Let's consider that an experiment and see how we like it in practice. Unfortunately, this is a big PR, but I don't think I could have made it too much smaller. Hopefully it will be easier to review than its size suggests since it's mostly additions and deletions rather than complex changes.

rfk force-pushed the fxa-client-cleanups branch from 53191a5 to 397f96d Compare February 18, 2021 09:24

This was referenced Feb 19, 2021

Update to Application Services v73.0.1 mozilla-mobile/android-components#9731

Merged

How to handle acronyms when normalizing names? mozilla/uniffi-rs#400

Open

rfk force-pushed the fxa-client-cleanups branch 5 times, most recently from a663055 to e64915e Compare February 24, 2021 02:00

rfk force-pushed the fxa-client-cleanups branch 6 times, most recently from fb5d407 to 9f5dfaf Compare February 25, 2021 04:29

rfk mentioned this pull request Feb 25, 2021

Flesh out the docs on consuming Swift bindings. mozilla/uniffi-rs#405

Merged

rfk force-pushed the fxa-client-cleanups branch 2 times, most recently from 7cd10f0 to ccaf1f3 Compare February 25, 2021 06:39

rfk mentioned this pull request Feb 25, 2021

Update to application-services v73.0.0 mozilla-mobile/firefox-ios#8044

Closed

rfk force-pushed the fxa-client-cleanups branch 6 times, most recently from 0274c82 to e22c368 Compare February 26, 2021 10:45

rfk marked this pull request as ready for review February 26, 2021 10:46

rfk requested review from mhammond and grigoryk February 26, 2021 10:46

rfk commented Feb 26, 2021

View reviewed changes

jhugman reviewed Feb 26, 2021

View reviewed changes

megazords/ios/MozillaAppServices.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved

mhammond reviewed Mar 3, 2021

View reviewed changes

rfk mentioned this pull request Mar 3, 2021

Experiment: replace protobuf with uniffi #3900

Closed

jhugman mentioned this pull request Mar 3, 2021

Adding Nimbus to the iOS Megazord #3901

Merged

4 tasks

mhammond reviewed Mar 4, 2021

View reviewed changes

rfk force-pushed the fxa-client-cleanups branch from 1bcffee to c144361 Compare March 5, 2021 06:20

rfk requested a review from mhammond March 5, 2021 06:20

rfk force-pushed the fxa-client-cleanups branch 2 times, most recently from 63559bb to 945eb0e Compare March 5, 2021 06:31

rfk mentioned this pull request Mar 8, 2021

Experiment: Use Rust code directly as the interface definition. mozilla/uniffi-rs#416

Closed

mhammond approved these changes Mar 9, 2021

View reviewed changes

This was referenced Mar 9, 2021

[discuss] next steps for interface definition language? mozilla/uniffi-rs#385

Closed

Remove the old StateV1 code from fxa-client #3912

Closed

rfk force-pushed the fxa-client-cleanups branch 3 times, most recently from 3229719 to e26cd01 Compare March 9, 2021 10:54

rfk force-pushed the fxa-client-cleanups branch from e26cd01 to 5272313 Compare March 9, 2021 11:13

rfk merged commit d77600e into main Mar 9, 2021

rfk deleted the fxa-client-cleanups branch March 9, 2021 11:34

rfk mentioned this pull request Mar 9, 2021

Adding Nimbus to the iOS Megazord #3913

Merged

data-sync-user mentioned this pull request Mar 17, 2021

Write up a "how to uniffi a component" doc #3940

Closed

This was referenced Jun 23, 2021

How to handle acronyms when normalizing names? #4247

Closed

[discuss] next steps for interface definition language? #4293

Closed


		// TODO: not sure why we switched to a bool API for the Swift wrapper here...

		public func migrateFromSessionToken(

		AuthorizationInfo check_authorization_status();


		// Disconenct from the user's account.

Restructure, UniFFI, and document the fxa-client crate. #3876

Restructure, UniFFI, and document the fxa-client crate. #3876

Conversation

rfk commented Feb 18, 2021 • edited Loading

rfk commented Feb 18, 2021

codecov-io commented Feb 24, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

rfk commented Feb 28, 2021

mhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Mar 4, 2021

mhammond commented Mar 4, 2021

rfk commented Mar 4, 2021

rfk commented Mar 5, 2021

rfk commented Mar 8, 2021

mhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Feb 18, 2021 •

edited

Loading

codecov-io commented Feb 24, 2021 •

edited

Loading