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

ZK-559: Add wasm implementation for CryptoClient #80

Merged
merged 21 commits into from
Jan 10, 2025
Merged

ZK-559: Add wasm implementation for CryptoClient #80

merged 21 commits into from
Jan 10, 2025

Conversation

kroist
Copy link
Collaborator

@kroist kroist commented Jan 8, 2025

This pull request includes a new package shielder-sdk-crypto-wasm which is basically a standalone /crypto module from shielder-sdk, implementing CryptoClient interface for WASM web target.

Main Changes

Most of the code is similar to /crypto module, however:

  • Introduced modular structure, where each module has single responsibilty on wrapping its functionality from WASM.
  • Added src/utils/wasmModuleLoader.ts base classes to reduce a lot of code duplications
  • Removed duplicate code/wrappers from /src/wasmClientWorker.ts and /src/wasmClient.ts

Vite Patch

During the extension build with WXT framework (which uses vite under the hood) it was found out that shielder-sdk didn't properly bundle:

  • code inside WebWorkers wasn't included
  • WASM code wasn't included

To adress this issue I added ./vite, ./wasm_singlethreaded and ./wasm_multithreaded entrypoints in package, which would allow to use the vite distribution, which is basically the original distribution with vite-suggested WebWorkers declaration (as currently used in extension). The usage is documented in README.md

@kroist kroist requested review from guspiel and jalooc January 8, 2025 18:15
Copy link

github-actions bot commented Jan 9, 2025

Transaction NameMainCurrentDifference (%)
NewAccountNative20002392000287+0.00240%
DepositNative18279661827882-0.00460%
WithdrawNative18980061898150+0.00759%

Copy link
Contributor

@guspiel guspiel left a comment

Choose a reason for hiding this comment

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

Well, I have to say in Polish "kawał dobrej roboty", somehow similar phrases in English don't sound the same. Just a few small comments to clarify.

ts/shielder-sdk-crypto-wasm/src/wasmClientWorker.ts Outdated Show resolved Hide resolved
ts/shielder-sdk-crypto-wasm/src/utils/wasmModuleLoader.ts Outdated Show resolved Hide resolved
ts/shielder-sdk-crypto-wasm/src/utils/wasmModuleLoader.ts Outdated Show resolved Hide resolved
// 1. Our proxy returns the hasher object directly from the worker
// 2. Comlink then wraps hasher's methods for cross-thread communication
// This way, only method calls are transferred between threads, not entire objects.
const exposed = new Proxy(wasmClientWorker, {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, this improves the worker interface a lot, we're not forced to do everything through worker methods anymore, awesome!

ts/shielder-sdk-crypto-wasm/src/noteTreeConfig.ts Outdated Show resolved Hide resolved
ts/shielder-sdk-crypto-wasm/src/secretGenerator.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,39 @@
import * as singleThreadedWasm from "shielder-wasm/web-singlethreaded";
Copy link
Contributor

Choose a reason for hiding this comment

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

Really happy to get rid of all these blocks

  constructor(caller: Caller) {
    this.caller = caller;
    if (caller == "web_singlethreaded") {
      this.wasmModule = singleThreadedWasm;
    } else if (caller == "web_multithreaded") {
      this.wasmModule = multiThreadedWasm;
    } else {
      throw new Error("Invalid caller");
    }
  }

!

ts/shielder-sdk-crypto-wasm/package.json Outdated Show resolved Hide resolved
ts/package.json Show resolved Hide resolved
ts/shielder-sdk-crypto-wasm/package.json Outdated Show resolved Hide resolved
@kroist kroist merged commit 6b49575 into main Jan 10, 2025
14 checks passed
@kroist kroist deleted the crypto-wasm branch January 10, 2025 14:31
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.

2 participants