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

feat(crypto): Port to Wasm (in a JS host) and to NodeJS #675

Merged
merged 61 commits into from
May 31, 2022

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented May 16, 2022

The why and how

We want all the clients (as in client-server terminology) to use the same Rust code base for the Crypto part, namely: Element iOS, Element Android and Element Web.

Element iOS and Android are using (or soon) the matrix-sdk-crypto-ffi crate from this repository. What is important for us in this issue is: Element Web is relying on the matrix-js-sdk library.

This matrix-rust-sdk provides the Rust matrix-sdk-crypto crate, which already compiles to Wasm (short for WebAssembly, i.e. the wasm32-unknown-unknown target). However, no symbol is exported or imported, which means that it cannot be used. Note: I'm not minimising the crazy efforts that were done to reach this milestone, it implies writing Vodozemac etc. Kudos!

The idea of this PR is to push on the previous efforts by exposing Wasm functions and types to the host. Which host? Well, we want to target JavaScript for now so that the resulting Wasm module can be used by the matrix-js-sdk, and thus, by Element Web.

The plan is then to provide a JavaScript API through WebAssembly to the Rust matrix-sdk-crypto crate. This JavaScript API should ideally come with TypeScript definitions. To achieve that, we use the wasm-bindgen project. To facilitate the compilation, optimisation, and bundling, we use the wasm-pack project.

Edit: We need to support NodeJS too (see #699). We have started by adding NodeJS support within the same codebase, within the same matrix-sdk-crypto-js crate, but now, the code is split into 2 crates, please join me to welcome matrix-sdk-crypto-nodejs. Why? Because napi and wasm-bindgen are too different. At this step, the most notable bugs are the way napi is handling its proc macros. There is too much conflicts when used with #[cfg_attr]. It makes the code repetitive and harder to read and to understand (and also to compile, we must be very careful). But on the short-term, quickly, we will see more notable differences between wasm-bindgen and napi, e.g. with array (in wasm-bindgen, we can downcast array items into particular types, with napi it's going to be a very different code).

Instead of fighting the proc macros bugs now, and having to split the code later inside the same crate, we believe it's a good idea to split the code now into 2 crates. At first we will see obvious code duplications, but on the short-term, the code is likely to be more and
more different.

The timeline

At the time of writing, it's been 6 days I've started my work at Element/Matrix and I'm still not entirely familiar with the codebase.

It's not trivial to define a progression timeline, but since the OlmMachine is the main entry point of the Crypto API, I reckon focusing on its implementation progression would provide a good intuition of the overall status.

  • OlmMachine:
    • async new,
    • user_id,
    • device_id,
    • identity_keys,
    • async display_name,
    • tracked_users,
    • async outgoing_requests,
    • async mark_request_as_sent,
    • async bootstrap_cross_signing,
    • async get_missing_sessions,
    • async encrypt_room_event,
    • async encrypt_room_event_raw,
    • async invalidate_group_session,
    • async share_group_session,
    • async receive_unencrypted_verification_event,
    • get_verification,
    • get_verification_request,
    • get_verification_requests,
    • async receive_sync_changes,
    • async request_room_key,
    • async decrypt_room_event,
    • async update_tracked_users,
    • async get_device,
    • async get_identity,
    • async get_user_devices,
    • async import_keys,
    • async export_keys,
    • async cross_signing_status,
    • async export_cross_signing_keys,
    • async import_cross_signing_keys,
    • async sign.

How to test

$ cargo install wasm-pack
$ cd crates/matrix-sdk-crypto-js/js
$ make build
$ make test

pkg/{*.d.ts,*.wasm} files will be generated in crates/matrix-sdk-crypto-js/js/pkg/ for now.

Status

The focus of the company has shifted a little bit. The priority is on NodeJS now. The idea is to merge this PR with its current state, then iterate on NodeJS in another second PR, and finish the Wasm+JS project with a third PR.

See #1016 for the next PR

Hywan added 4 commits May 9, 2022 10:40
This patch updates to code to raise a compilation error if the `js`
feature is used for another architecture than `wasm32`.
Ask `rustc` to generate a dynamic system library, which will be useful
to generate a Wasm module.
@BillCarsonFr
Copy link
Member

Fixes #681

@Hywan
Copy link
Member Author

Hywan commented May 23, 2022

The code has moved into its own crate: matrix-sdk-crypto-js.

crates/matrix-sdk-crypto-js/Cargo.toml Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/lib.rs Show resolved Hide resolved
@Hywan
Copy link
Member Author

Hywan commented May 23, 2022

Yeah, it's two approaches for the same problem. I used Cargo features because it's just shorter to write cfg in the code. target = "wasm32" is not enough actually, we should write all(target_arch = "wasm32"), not(target_os = "wasi")) to exclude WASI.

@Hywan Hywan changed the title feat(crypto): Port to Wasm (in a JS host) feat(crypto): Port to Wasm (in a JS host) and to NodeJS May 24, 2022
@Hywan Hywan self-assigned this May 24, 2022
@Hywan Hywan marked this pull request as ready for review May 30, 2022 06:46
@Hywan
Copy link
Member Author

Hywan commented May 30, 2022

The priorities of the company have shifted a little bit. For the next days, I'll focus on NodeJS bindings. I'll come back to Wasm+JS bindings after that. With the team, we have agreed to review and to merge this PR, then I'll open a new PR for NodeJS only, and another one to finish Wasm+JS only.

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Overall this looks good, I left some nits here and there. The biggest problem is how we're dealing with custom enum variants and events, I would like to see this fixed before we merge, just as a precaution so we don't forget about it.

Cargo.toml Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto-js/js/Makefile Show resolved Hide resolved
crates/matrix-sdk-crypto-js/src/events.rs Show resolved Hide resolved
crates/matrix-sdk-crypto-js/.cargo/config Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto-js/src/machine.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto-js/src/machine.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto-js/src/machine.rs Show resolved Hide resolved
crates/matrix-sdk-crypto-js/src/machine.rs Show resolved Hide resolved
crates/matrix-sdk-crypto-js/src/machine.rs Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #675 (bc7034f) into main (2ff6497) will decrease coverage by 1.50%.
The diff coverage is 0.00%.

❗ Current head bc7034f differs from pull request most recent head 7931c4a. Consider uploading reports for the commit 7931c4a to get more accurate results

@@            Coverage Diff             @@
##             main     #675      +/-   ##
==========================================
- Coverage   72.72%   71.21%   -1.51%     
==========================================
  Files         109      116       +7     
  Lines       15974    16291     +317     
==========================================
- Hits        11617    11602      -15     
- Misses       4357     4689     +332     
Impacted Files Coverage Δ
crates/matrix-sdk-crypto-js/src/future.rs 0.00% <0.00%> (ø)
crates/matrix-sdk-crypto-js/src/lib.rs 0.00% <0.00%> (ø)
crates/matrix-sdk-crypto-js/src/responses.rs 0.00% <0.00%> (ø)
crates/matrix-sdk-crypto-nodejs/src/errors.rs 0.00% <0.00%> (ø)
crates/matrix-sdk-crypto/src/lib.rs 100.00% <ø> (ø)
crates/matrix-sdk-crypto/src/machine.rs 73.76% <ø> (ø)
crates/matrix-sdk-crypto/src/requests.rs 70.42% <ø> (ø)
xtask/src/ci.rs 0.00% <0.00%> (ø)
crates/matrix-sdk-crypto/src/olm/utility.rs 96.42% <0.00%> (-1.08%) ⬇️
crates/matrix-sdk-crypto/src/olm/account.rs 79.51% <0.00%> (-0.20%) ⬇️
... and 7 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 2ff6497...7931c4a. Read the comment docs.

@Hywan Hywan enabled auto-merge May 30, 2022 13:48
@Hywan Hywan disabled auto-merge May 30, 2022 14:35
Hywan added 12 commits May 31, 2022 08:35
…y code.

`wasm_bindgen_future::future_to_promise` expects a `Future<Output =
Result<JsValue, JsValue>>`. We reimplement this function by expecting
a `Future<Output = Result<T, anyhow::Error>>` where `T:
Into<JsValue>`. That way, we apply the type conversions to `JsValue`
inside this helper rather than in the call site. Additionally, all
errors are managed automatically without having to deal with `JsError`
or `JsValue`. It makes the code simpler to read and easier to write
from my point of view.
Hywan added 23 commits May 31, 2022 08:38
Why? Because `napi` and `wasm-bindgen` are too different. At this
step, the most notable bugs are the way `napi` is handling its proc
macros. There is too much conflicts when used with `#[cfg_attr]`. It
makes the code repetitive and harder to read and to understand (and
also to compile, we must be very careful). But on the short-term,
quickly, we will see more notable differences between `wasm-bindgen`
and `napi`, e.g. with array (in `wasm-bindgen`, we can downcast array
items into particular types, with `napi` it's going to be a very
different code).

Instead of fighting the proc macros bugs now, and having to split the
code later inside the same crate, we believe it's a good idea to split
the code now into 2 crates. At first we will see obvious code
duplications, but on the short-term, the code is likely to be more and
more different.
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Looks good, there are still missing newlines at the end of two files. Please add those back and feel free to merge after that's done.

Cargo.toml Show resolved Hide resolved
crates/matrix-sdk-crypto/Cargo.toml Show resolved Hide resolved
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.

WASM bindings for the matrix-sdk-crypto crate
4 participants