Refactor `crypto` and other WASM-based modules #341

cairomassimo · 2024-02-05T19:23:10Z

Description

Re-implement the functionality of @lit-protocol/crypto package as follows (WIP!):

create a new nx workspace (currently in the ng/ dir), with a simpler/standard build system,
create a unified Rust crate exposing an API to js,
merge all Rust crates, which expose functionality via wasm, into the unified one,
set up a mechanism to generate TypeScript types from Rust (beyond those created by wasm-bindgen, if needed),
add a thin (type-safe) JS layer on top of Rust, dealing with data conversion (the less, the better),
migrate all tests for @lit-protocol/crypto to the new implementation,
migrate all uses of/references to @lit-protocol/crypto to the new implementation.

Design choices

esbuild as bundler
- supports embedding binary files out-of-the box, useful for wasm
- fast
- supported by jest via esbuild-jest-transform
single javascript package (currently named @lit-protocol/ng, to be renamed)
- tree-shaking should remove unused functionalities,
- better split into modules, before splitting into packages,
- split into packages later, when useful/needed (e.g. to reduce build time, to support different build pipelines, what not).

Notes

The build does not generate .d.ts TypeScript declaration files yet.
Not adding this for now, since it could come for free if recent PR nrwl/nx#21084 is merged (or nrwl/nx#20688 fixed anyway).

CLAassistant · 2024-02-05T19:23:16Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

joshLong145 · 2024-02-05T20:36:46Z

@cairomassimo Would it be easier if we made a feature branch for this work? this would make it easier to review the work over time as comments will be cleared once you push newer commits. If we used a feature branch then we could review in a more manageable fashion. WDYT?

cairomassimo · 2024-02-09T19:08:50Z

To do:

make the exported wasm function handle multiple curves dynamically (the api is identical, less code, no/less need for macros)
see what happens if we bypass nx entirely
set up a long-lived feature branch and/or split this PR into smaller ones for easier reviews (@joshLong145)

mikelodder7 · 2024-02-12T19:12:26Z

ng/wasm/Cargo.toml

+serde_json = "1.0"
+serde_bare = "0.5"
+serde-wasm-bindgen = "0.5"
+bls12_381_plus = { version = "=0.8.9" }


Not needed since you're including blsful. Use blsful::inner_types and it gets you the same.

Agree.

I added this to fix the crate to an older version, which would fix a compile error. IIRC, I got the error when using the blsful crate from Git, but it doesn't occur at all using versions published to crates.io. Removing.

mikelodder7 · 2024-02-12T19:13:39Z

ng/wasm/src/bls.rs

+
+struct Bls<C>(C);
+
+// We have to create this trait because G1/G2 implementations do not use a common trait for all the methods we need.


They actually do. See https://github.com/mikelodder7/blsful/blob/main/src/traits/serdes.rs

Not sure if we can go Uint8Array -> Vec<u8> -> {struct} via serde and these traits. A TryFrom<Vec<u8>> would fit better, or we could change the overall approach to rely on serde for JS communication everywhere.
Happy to discuss this today!

mikelodder7 · 2024-02-12T19:14:48Z

ng/wasm/src/ecdsa.rs

+
+#[derive(Tsify, Deserialize)]
+#[tsify(from_wasm_abi)]
+pub enum EcdsaVariant {


Missing P384

mikelodder7 · 2024-02-12T19:16:02Z

ng/wasm/src/ecdsa.rs

+        Ok(k)
+    }
+
+    fn derive_key_inner(


Please use these traits instead https://github.com/LIT-Protocol/hd-keys-curves/blob/main/src/lib.rs#L23

Happily! See #341 (comment)

mikelodder7 · 2024-02-12T19:16:25Z

ng/wasm/src/ecdsa.rs

+        Ok(k)
+    }
+
+    fn sum_of_products_pippenger(


Why are we copying code here? All this is done for you in https://github.com/LIT-Protocol/hd-keys-curves/blob/main/src/lib.rs#L23

This code was moved*, not copied, from https://github.com/LIT-Protocol/lit-ecdsa-wasm-combine/blob/main/src/combiners/hd_ecdsa/mod.rs#L53, I didn't know it was also available in another crate.

[*] the idea is having only one crate that exposes the crypto functionality to JS via WASM, and have it be part of the SDK. Ideally, this crate would not actually implement any cryptography itself, but only delegate to other crates making sure the API is JS-friendly and type-safe.

So indeed this piece of code lives much better in a different crate, I'll happily work on delegating this to hd-keys-curves!

mikelodder7 · 2024-02-12T19:17:07Z

ng/wasm/src/frost.rs

+
+#[derive(Tsify, Deserialize)]
+#[tsify(from_wasm_abi)]
+pub enum FrostVariant {


Again why are we copying code from lit-frost? The point of a crate is that its reusable.

The `blstrs_plus` implementation of BLS curves would crash with a `panic_misaligned_pointer_dereference`. That implementation was wrongly selected over `bls12_381_plus` after adding dependency `hd-keys-curves`, which implied `blsful/default` -> `blsful/blst` -> `blsful/blstrs_plus`.

Create new nx workspace with round trip wasm test

e2b75cb

cairomassimo added 3 commits February 6, 2024 21:31

Add sev-snp to unified Rust crate

2c91511

Add bls, ecdsa, frost (4/8) to unified Rust crate

f0cc82a

Update .editorconfig for Rust files

06c9c80

This was referenced Feb 12, 2024

Feature/lit 2277 aggregate signature shares and detect bad ones LIT-Protocol/lit-ecdsa-wasm-combine#5

Closed

add experimental FROST/EdDSA signature combination #310

Closed

cairomassimo added 3 commits February 12, 2024 13:36

Make crypto wasm APIs parametric on the curve

b5b7078

Add missing tests for new wasm crypto impl

c1197e3

Return ECDSA signatures in Ethereum-like format

52e40a1

cairomassimo requested review from mikelodder7 and GTC6244 February 12, 2024 18:53

mikelodder7 reviewed Feb 12, 2024

View reviewed changes

ng/wasm/src/ecdsa.rs

#[derive(Tsify, Deserialize)]

#[tsify(from_wasm_abi)]

pub enum EcdsaVariant {

Copy link

Contributor

mikelodder7 Feb 12, 2024

Choose a reason for hiding this comment

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

Missing P384

mikelodder7 reviewed Feb 12, 2024

View reviewed changes

cairomassimo added 2 commits February 14, 2024 14:04

wasm: use TryFrom<&[u8]> from blsful

0d0c09c

wasm: use git dependencies for frost

6d99ccf

Ansonhkg mentioned this pull request Feb 14, 2024

[Release] 3.1.5 #357

Closed

5 tasks

cairomassimo added 2 commits February 14, 2024 17:46

wasm: delegate derive key to hd-keys-curves

194d7ba

wasm crypto: simpler conversion to/from Uint8Array

c5a9385

cairomassimo force-pushed the feature/lit-2382-js-sdk-refactor-crypto-and-other-wasm-based-modules branch from 54f422d to c5a9385 Compare February 15, 2024 11:27

Ansonhkg changed the base branch from master to staging/3.1.5 February 15, 2024 18:53

cairomassimo added 2 commits February 17, 2024 12:45

wasm: remove some dependency overrides

cf8e897

Ansonhkg mentioned this pull request Feb 19, 2024

[Release] 3.2.0 #370

Merged

5 tasks

Ansonhkg deleted the branch staging/3.1.5 February 19, 2024 16:09

Ansonhkg closed this Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `crypto` and other WASM-based modules #341

Refactor `crypto` and other WASM-based modules #341

cairomassimo commented Feb 5, 2024 •

edited

Loading

CLAassistant commented Feb 5, 2024

joshLong145 commented Feb 5, 2024

cairomassimo commented Feb 9, 2024 •

edited

Loading

mikelodder7 Feb 12, 2024

cairomassimo Feb 13, 2024

mikelodder7 Feb 12, 2024

cairomassimo Feb 13, 2024

mikelodder7 Feb 12, 2024

mikelodder7 Feb 12, 2024

cairomassimo Feb 13, 2024

mikelodder7 Feb 12, 2024

cairomassimo Feb 13, 2024

mikelodder7 Feb 12, 2024


		struct Bls<C>(C);

		// We have to create this trait because G1/G2 implementations do not use a common trait for all the methods we need.

Refactor crypto and other WASM-based modules #341

Refactor crypto and other WASM-based modules #341

Conversation

cairomassimo commented Feb 5, 2024 • edited Loading

Description

Design choices

Notes

CLAassistant commented Feb 5, 2024

joshLong145 commented Feb 5, 2024

cairomassimo commented Feb 9, 2024 • edited Loading

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

Refactor `crypto` and other WASM-based modules #341

Refactor `crypto` and other WASM-based modules #341

cairomassimo commented Feb 5, 2024 •

edited

Loading

cairomassimo commented Feb 9, 2024 •

edited

Loading