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

Introduce mls-rs-uniffi #82

Merged
merged 2 commits into from
Feb 23, 2024
Merged

Introduce mls-rs-uniffi #82

merged 2 commits into from
Feb 23, 2024

Conversation

mgeisler
Copy link
Contributor

@mgeisler mgeisler commented Feb 21, 2024

Issues:

Addresses #81.

Description of changes:

This new create is a thin wrapper around mls-rs which exposes a subset of it’s interface using a UniFFI-compatible API.

We will expand this over the next few weeks to be a full API usable by messaging apps on different platforms.

Call-outs:

The code currently only works with mls_build_async, but this should be fixable in the future. From talking to @mulmarta, there should be a way to use traits to handle both cases.

Is the name mls-rs-uniffi good? I hope to use the same API for Wasm laber, see #82 (comment) below.

Testing:

I've mostly looked at the auto-generated Kotlin code to see that it has a useful shape. You can generate Kotlin code with:

cargo build -p mls-rs-uniffi
cargo run -p uniffi-bindgen generate --library target/debug/libmls_rs_uniffi.so --language kotlin --out-dir target

The generate file is in target/uniffi/mls_rs_uniffi/mls_rs_uniffi.kt. If you install dokka, then you can generate API docs using a dokka.json file that looks like this:

{
  "outputDir": "./target/dokka",
  "sourceSets": [
    {
      "sourceSetID": {
        "scopeId": "mls_rs_uniffi",
        "sourceSetName": "main"
      },
      "sourceRoots": [
        "/absolute/path/to/your/checkout/target/uniffi"
      ]
    }
  ],
  "pluginsClasspath": [
    "/absolute/path/to/your/dokka/installation/dokka-base-1.9.10.jar",
    "/absolute/path/to/your/dokka/installation/analysis-kotlin-descriptors-1.9.10.jar",
    "/absolute/path/to/your/dokka/installation/freemarker-2.3.31.jar",
    "/absolute/path/to/your/dokka/installation/kotlinx-html-jvm-0.8.0.jar"
  ]
}

I would love to check it in if someone knows how to get rid of the absolute paths 😄

I expect us to add/remove things from the API in the next few weeks.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT license.

@mulmarta
Copy link
Contributor

mulmarta commented Feb 21, 2024

To add more context to the sync build problems, the following code shows the issue and one not too satisfying solution. @stefunctional maybe you have ideas how to improve?

uniffi::setup_scaffolding!();

#[maybe_async::must_be_sync]
async fn bar() {}

#[derive(uniffi::Object)]
struct Foo;

/*
// This does not work because maybe_async must be called _before_ uniffi
// so that uniffi sees the sync function. On the other hand, uniffi must be called
// on the impl block which is higher up (and maybe_async can't be called on
// the impl block).

#[uniffi::export]
impl Foo {
    #[maybe_async::must_be_sync]
    async fn foo(&self) {
        bar().await
    }
}
*/

// This compiles
#[maybe_async::must_be_sync]
trait FooType {
    async fn foo(&self);
}

#[maybe_async::must_be_sync]
#[uniffi::export]
impl FooType for Foo {
    async fn foo(&self) {
        bar().await
    }
}

Copy link
Contributor

@mulmarta mulmarta left a comment

Choose a reason for hiding this comment

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

Thanks, Martin! This will be super helpful. I had a couple of small comments.

mls-rs-uniffi/src/lib.rs Outdated Show resolved Hide resolved
mls-rs-uniffi/src/lib.rs Outdated Show resolved Hide resolved
mls-rs-uniffi/src/lib.rs Outdated Show resolved Hide resolved
mls-rs-uniffi/src/lib.rs Outdated Show resolved Hide resolved
mls-rs-uniffi/src/lib.rs Outdated Show resolved Hide resolved
mls-rs-uniffi/src/lib.rs Show resolved Hide resolved
@stefunctional
Copy link
Contributor

fMeow/maybe-async-rs#24 should help with the interoperability between #[uniffi::export] and maybe_async macros.

@mgeisler
Copy link
Contributor Author

fMeow/maybe-async-rs#24 should help with the interoperability between #[uniffi::export] and maybe_async macros.

Thanks @stefunctional, that sounds great! I'll take a look!

@mgeisler
Copy link
Contributor Author

mgeisler commented Feb 22, 2024

This new create is a thin wrapper around mls-rs which exposes a subset of it’s interface using a UniFFI-compatible API.

One question that I had as I prepared this PR: the code here started out as a wrapper around mls-rs which was called mls-lite. The goal of mls-lite was to further simplify the mls-rs API to just the bits needed for a messaging app. That was the "-lite" part of the name.

At the same time, mls-lite was using UniFFI to expose an API for Kotlin/Swift/... apps. This is where the "-uniffi" part in this PR comes from 🙂 Are people here okay with mixing those two concerns?

One concern of mine is that I also would like to use the API here for Wasm somehow. There is an issue open for this: mozilla/uniffi-rs#1424, but I believe the state is still experimental at best. So "-uniffi" suffix might be wrong for my Wasm plans.

@mulmarta, you suggested moving the CipherSuite into SignatureKeypair (sounds great), but one could argue that it would be great to push such a change down to mls-rs as well?

Thoughts on the above?

@mgeisler
Copy link
Contributor Author

fMeow/maybe-async-rs#24 should help with the interoperability between #[uniffi::export] and maybe_async macros.

Thanks @stefunctional, that sounds great! I'll take a look!

This worked beautifully, I can now compile both with and without mls_build_async!

This version contains fMeow/maybe-async-rs#24,
which should make it possible to combine UniFFI and maybe-async.
mls-rs-uniffi/src/lib.rs Outdated Show resolved Hide resolved
mulmarta
mulmarta previously approved these changes Feb 22, 2024
mls-rs-uniffi/src/lib.rs Show resolved Hide resolved
mls-rs-uniffi/src/lib.rs Show resolved Hide resolved
mls-rs-uniffi/src/lib.rs Outdated Show resolved Hide resolved
mls-rs-uniffi/src/lib.rs Outdated Show resolved Hide resolved
mls-rs-uniffi/src/lib.rs Outdated Show resolved Hide resolved
@mulmarta
Copy link
Contributor

Another thing @mgeisler there seems to be no way to apply_pending_commit? Should process_incoming_message maybe take message or ack?

mls-rs-uniffi/src/lib.rs Outdated Show resolved Hide resolved
This new create is a thin wrapper around mls-rs which exposes a subset
of it’s interface using a [UniFFI-compatible][1] API.

We will expand this over the next few weeks to be a full API usable by
messaging apps on different platforms.

[1]: https://mozilla.github.io/uniffi-rs/
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 244 lines in your changes are missing coverage. Please review.

Project coverage is 90.73%. Comparing base (cdc826f) to head (8497736).

Files Patch % Lines
mls-rs-uniffi/src/lib.rs 0.00% 244 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
- Coverage   91.46%   90.73%   -0.74%     
==========================================
  Files         168      169       +1     
  Lines       30306    30548     +242     
==========================================
- Hits        27719    27717       -2     
- Misses       2587     2831     +244     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomleavy tomleavy merged commit 8110476 into awslabs:main Feb 23, 2024
12 checks passed
@mgeisler mgeisler deleted the mls-rs-uniffi branch February 26, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants