Initial Web Client Web Workers Implementation #720

dagarcia7 · 2025-02-06T07:35:45Z

Summary

This PR introduces an initial web worker infrastructure for the web client. Early adopters have observed that some of the computationally heavy tasks—such as creating accounts, executing transactions, and submitting transactions—cause the UI to freeze in the browser because these computations are performed on the main thread. With this new infrastructure, those heavy tasks are offloaded to a dedicated web worker, alleviating the UI freeze issue.

You might ask, "Why aren't all functions of the web client executed within a web worker?" The primary challenge is that data passed between the main thread and the web worker via messages cannot be complex objects like those returned from the WASM module. As a result, any parameters or return types that involve these complex WASM objects must be serialized when sent and then deserialized and reconstructed on the receiving end. Since the WebClient exported from the WASM module contains a large number of callable functions, I decided to start by offloading only the most computationally intensive ones, gather feedback, and then address the others in a similar fashion if necessary.

On a related note, the current approach is quite hard-coded. I spent about a week exploring ways to generalize the solution so that, from the user's perspective, the web client API remains unchanged while every call to the WebClient is executed in a web worker. I experimented with dynamically parsing the inputs for functions and even tried instantiating the WASM module in both the main thread and the web worker using shared memory. The idea behind the shared memory approach was to pass pointers to complex WASM objects between threads and reconstruct them on the other side by accessing the same memory location. However, while I managed to share memory between the two WASM instances, keeping them synchronized proved too challenging, and I repeatedly encountered "memory access out of bounds" errors.

I believe a more generalizable approach exists—one that would avoid the need to explicitly handle the serialization and deserialization of each function's inputs separately—but this will require further investigation. For now, this PR should resolve most of the community's performance concerns and offers a neat, easy-to-follow starting point should we decide to build off of this work.

Finally, the way the users consume the web client should largely remain the same thanks to the use of a Proxy that forwards calls not designated to be computed in a web worker to the underlying WASM WebClient. The only "breaking" change should be the way users instantiate the WebClient which I will flag in the PR.

igamigo · 2025-02-06T20:02:56Z

crates/web-client/src/models/transaction_result.rs

@@ -36,6 +41,22 @@ impl TransactionResult {
    pub fn consumed_notes(&self) -> InputNotes {
        self.0.consumed_notes().into()
    }
+
+    pub fn serialize(&self) -> Uint8Array {


This would mean that we can close #711, right?

Yes! When I created the issue I wasn't sure if it had been done or not, but I was able to rebase, get the changes, and use them here in the web client interface for TransactionResult 👍

dagarcia7 · 2025-02-07T05:39:42Z

crates/web-client/test/mocha.global.setup.mjs

-      const client = new WebClient();
-      await client.create_client(rpc_url, prover_url);


This should be the "breaking" change end users see when this work is released. The instantiation of the WebClient now is done via the static method exposed of the wrapped WebClient class, but the rest of the web client API should remain the same.

dagarcia7 · 2025-02-07T05:40:54Z

crates/web-client/src/models/provers.rs

+            "local" => TransactionProver::new_local_prover(),
+            "remote" => {
+                // Use as_deref() to convert Option<String> to Option<&str>
+                let ep = endpoint.as_deref().unwrap_or("http://localhost:50051"); // TODO: is this right?


What should be the default endpoint here for a remote prover? I can get rid of the TODO comment, but wanted to ask here because I wasn't sure!

@bobbinth it's there going to be a canonical remote prover deployed by Miden that can be used here? I know that we have one already deployed and working but I'm not sure if it is correct to use it here.

Yes, we should have a remote prover running. @Dominik1999 @Mirko-von-Leipzig - do we have one set up already?

Thanks for following up! I can throw an error for now if the prover_type is "remote" and no endpoint is passed in and when we have the info I can add it as a default quickly 👍

@bobbinth we're waiting on devops to upgrade and give it CNAME; I was late with creating the ticket.

julian-demox · 2025-02-07T16:12:47Z

crates/web-client/src/models/account_storage_mode.rs

+
+    pub fn serialize(&self) -> String {
+        match self.0 {
+            NativeAccountStorageMode::Private => "private".to_string(),


Is there a way to expose the strings used for serde on the impl? I see a slight concern in the future since we're manually tracking these strings in the tests here. Clients would have to do the same if they try to leverage the serde for their uses

Could also be a follow up

Hm good point 🤔. So I changed this code to the following interface:

pub fn from_str(s: &str) -> Result<AccountStorageMode, JsValue> { let mode = NativeAccountStorageMode::from_str(s) .map_err(|e| JsValue::from_str(&format!("Invalid AccountStorageMode string: {:?}", e)))?; Ok(AccountStorageMode(mode)) } pub fn as_str(&self) -> String { self.0.to_string() }

which just relies on the native implementations here

impl fmt::Display for AccountStorageMode { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { AccountStorageMode::Public => write!(f, "public"), AccountStorageMode::Private => write!(f, "private"), } } } impl TryFrom<&str> for AccountStorageMode { type Error = AccountIdError; fn try_from(value: &str) -> Result<Self, AccountIdError> { match value.to_lowercase().as_str() { "public" => Ok(AccountStorageMode::Public), "private" => Ok(AccountStorageMode::Private), _ => Err(AccountIdError::UnknownAccountStorageMode(value.into())), } } }

I think this is better than what I currently have because it at least has the source of truth for the string mapping at the rust client level. In the test you linked, I'm not sure we can do much more about it. The reason is that outside of the evaluate function, we can't call into anything WASM-related, so it requires the user to at least know what type of AccountStorageModes are available and how to test them. I'm passing a string knowing that I can call from_str to get the object back, but someone can also do a switch statement as well with custom strings to create the objects from scratch and setup the test that way. I think with this change it's now more of a testing issue because of the different contexts inside and outside the puppetteer evaluate function.

dagarcia7 · 2025-02-10T04:21:30Z

crates/web-client/js/workers/web-client-methods-worker.js

+    );
+  },
+  [MethodName.SYNC_STATE]: async () => {
+    await wasmWebClient.sync_state();


I just realized we may need SyncSummary to be serializable/deserializable. Didn't catch it the first time around because I don't think my integration tests make use of the information returned by sync, but it's definitely something that should be returned. Is this something we can potentially look this week @igamigo or @SantiagoPittella ?

@TomasArrachea implemented this on #725 (merged to main!), so you should be able to rebase and use this as needed.

tomyrd

Looks good! The integration tests work correctly with the web workers. Left some non blocking comments

tomyrd · 2025-02-10T20:13:51Z

crates/web-client/src/models/note_type.rs

+
+    pub fn serialize(&self) -> String {
+        match self.0 {
+            NativeNoteType::Private => "private".to_string(),
+            NativeNoteType::Public => "public".to_string(),
+            NativeNoteType::Encrypted => "encrypted".to_string(),
+        }
+    }
+
+    pub fn deserialize(mode: &str) -> Result<NoteType, JsValue> {
+        match mode {
+            "private" => Ok(NoteType::private()),
+            "public" => Ok(NoteType::public()),
+            "encrypted" => Ok(NoteType::encrypted()),
+            _ => Err(JsValue::from_str("Invalid NoteType string")),
+        }
+    }


Here we could use the NativeNoteType serialize and deserialize functions to avoid having hardcoded values.

tomyrd · 2025-02-10T20:16:56Z

crates/web-client/src/models/transaction_result.rs

+        let native_transaction_result = &self.0;
+        let mut buffer = Vec::new();
+        native_transaction_result.write_into(&mut buffer);
+        Uint8Array::from(&buffer[..])


This part and the deserialize part are really similar in every type that already have serialize and deserialize implemented. Could we move them to shared helper functions?

tomyrd · 2025-02-10T20:21:38Z

crates/web-client/js/types/index.d.ts

+   *
+   * @param rpcUrl - The RPC URL (optional).
+   * @param proverUrl - The prover URL (optional).
+   * @param seed - The seed for the account (optional).


I think it's strange that we need an account seed to initialize the client but after some investigating it seems that we also use it in the current version so we can tackle it in the future.

SantiagoPittella · 2025-02-10T20:52:58Z

Should the base of the PR be main instead of next?

crates/web-client/src/models/transaction_request.rs

igamigo

Leaving some comments! I reviewed most of the code but not very familiar with the approach.

Can users still instantiate the client the older way? That is, with the generated constructor instead of the new static one. I wonder if there could be ways in which there could be race conditions or things like that where the worker is processing some stuff and the store gets modified by something else. Seems like the older methods (like WebClient::submit_transaction() still have wasm bindings being generated so I wonder if we should disable that way of instantiating the client completely. But maybe this falls out of the scope of the PR.

igamigo · 2025-02-10T21:03:19Z

crates/web-client/js/index.js

+    const serializedProver = prover.serialize();
+    try {
+      await this.callMethodWithWorker(
+        "submit_transaction_with_prover",


Could this use the MethodName constants (maybe it has to be moved to a different file or something) or would that not be possible?

igamigo · 2025-02-10T21:05:05Z

crates/web-client/js/workers/web-client-methods-worker.js

+  [MethodName.SUBMIT_TRANSACTION]: async (args) => {
+    const [serializedTransactionResult] = args;
+    const transactionResult = wasm.TransactionResult.deserialize(
+      new Uint8Array(serializedTransactionResult)
+    );
+    await wasmWebClient.submit_transaction(transactionResult);
+    return;
+  },
+  [MethodName.SUBMIT_TRANSACTION_WITH_PROVER]: async (args) => {


Maybe you explored this already, but would it not be possible to generalize this to only have a "submit transaction with prover" and then have the "native" WASM client pass the correct prover to the worker? So as to simplify the worker API.

Ah, I see yeah I think I understand what you're saying. There are some weird things downstream that are confusing me a bit, though. For example, I see that when the WebClient is instantiated, in the constructor there is this line creates a default remote prover. It was added in this PR. However, the underlying rust object does this and sets the default tx_prover to be a local prover. As a result, submit_transaction and submit_transaction_with_prover feel a bit weird to me.

Should we look at reworking these in general? Or should I just do as you're saying and keep SUBMIT_TRANSACTION_WITH_PROVER here in the worker and accept an optional prover? If a prover is present, then call the ...with_prover version, and if no prover is present, then call submit_transaction?

I think the web-client's instantiation might be the older API we had, where we could set the prover at the moment of building the client. As you mentioned, the default is now to locally prove unless it's set to something else at the moment of proving and submitting the proof. Probably would be good to align these to work as similarly as possible, unless there's something else in play that I'm missing.
For the worker, sounds like it might be good to have few operations to make it easier to maintain, so my intuition would be to remove one of the calls and resolve that internally, depending on whether there is a specific prover in the operation. But I'm not super familiar with this architecture so fine for me either way!

dagarcia7 · 2025-02-11T07:46:53Z

@igamigo

Can users still instantiate the client the older way? That is, with the generated constructor instead of the new static one. I wonder if there could be ways in which there could be race conditions or things like that where the worker is processing some stuff and the store gets modified by something else. Seems like the older methods (like WebClient::submit_transaction() still have wasm bindings being generated so I wonder if we should disable that way of instantiating the client completely. But maybe this falls out of the scope of the PR.

That's a good question and I believe consumers of the SDK should not be able to instantiate the client the other way. Just via the static method which I have exposed and is a breaking change in this PR. The reason is that in my index.js file, I have this code to import some of the objects from the built WASM module, and then export them as the public API of the npm package.

import wasm from "../dist/wasm.js";
import { MethodName, WorkerAction } from "./constants.js";

const {
  Account,
  AccountHeader,
  ...
  Word,
  WebClient: WasmWebClient, // Alias the WASM-exported WebClient
} = wasm;

export {
  Account,
  AccountHeader,
  ...
  Word,
};

When importing, I'm importing the WebClient as it is defined by rust and wasm_bindgen, but when I export, I omit this object completely. I replace it with this wrapped version of the WebClient

export class WebClient {}

that overrides specific functions (the account creation methods, sync, and all the transaction calls) so it ends up calling those functions defined for the wrapped object and computes them in a web worker, and then anything else that is called against the web client that doesn't match one of the functions that has been overridden, it uses a Proxy to get the function name and arguments, and pass them to the WASM version of the WebClient.

So in theory, the problem you've identified shouldn't be able to happen. That being said, I am, however, worried about users calling functions that aren't computed in web-workers (say, get_account) and having race conditions with functions that are called via web workers. But I think my plan to test that was just to ship this work and let those bugs surface in the wild 😅

dagarcia7 · 2025-02-11T08:01:42Z

Feedback mostly addressed. TODOs still:

Resolve submit_prover vs. submit_prover_with_transaction conversation
Base the PR off main instead of next
Resolve merge conflicts
Return SyncSummary in sync_state from web worker

bobbinth · 2025-02-12T01:02:00Z

@dagarcia7 - are we planning to merge this into next or into main? If main, then I'll be able to make a patch release - but for next, we probably won't be able to release until early March.

dagarcia7 · 2025-02-12T08:01:46Z

@dagarcia7 - are we planning to merge this into next or into main? If main, then I'll be able to make a patch release - but for next, we probably won't be able to release until early March.

@bobbinth yes I think the plan is to merge to main! I rebased off of main and changed the target branch in the PR.
@igamigo I think all the feedback has been addressed maybe with the exception of #720 (comment). Do we have a default prover url I can put? I think that's the only open question I have. Otherwise, let me know if you think there's anything else to address 👍

I also wanted to try and consume the SDK in the wallet to try and surface anymore hidden bugs, but I think that will be a separate effort on our end this week.

igamigo

LGTM!

tomyrd

LGTM, left a small suggestion and a question

tomyrd · 2025-02-13T18:38:16Z

crates/web-client/js/index.js

+ * Additionally, the wrapper provides a static create_client function. This static method
+ * instantiates the WebClient object and ensures that the necessary create_client calls are
+ * performed both in the main thread and within the worker thread. This dual initialization
+ * correctly passes user parameters (RPC URL, prover URL, and seed) to both the main-thread


Suggested change

* correctly passes user parameters (RPC URL, prover URL, and seed) to both the main-thread

* correctly passes user parameters (RPC URL and seed) to both the main-thread

tomyrd · 2025-02-13T18:45:04Z

crates/web-client/js/index.js

+   * @param  {...any} args - Arguments for the method.
+   * @returns {Promise<any>}
+   */
+  async callMethodDirectly(methodName, ...args) {


I think we are not using this function. Should this function be used in the Proxy to call the wasm webclient methods?

Whoops! No, sorry the proxy used to call this function, but I reworked it so I don't have to call this and forgot to remove. Thanks for catching!

bobbinth · 2025-02-14T15:45:43Z

@dagarcia7 @igamigo - should I make a v0.7.1 release today from the main branch today? Or are we waiting for something else to lend there?

igamigo · 2025-02-14T17:36:19Z

@dagarcia7 @igamigo - should I make a v0.7.1 release today from the main branch today? Or are we waiting for something else to lend there?

I think we should unless @dagarcia7 had a different idea (not sure what the plans were in relation to the NPM package). Although there's #736 which we might want to address.

dagarcia7 · 2025-02-14T17:50:09Z

@bobbinth I may ask to hold off until the top of next week if possible! Consuming the updated SDK work in the wallet, I realized that I may need to add a change where if you are consuming the SDK in a context where the browser Worker is unavailable, it should default to using the SDK as usual without web workers. Bumped into this issue in the chrome extension as much of the code runs in a service worker where you're not allowed to spawn nested workers. Don't think it should be a tough change, but I can follow up in a separate PR.

bobbinth · 2025-02-14T17:53:44Z

@dagarcia7 - no rush! sounds good!

igamigo reviewed Feb 6, 2025

View reviewed changes

dagarcia7 commented Feb 7, 2025

View reviewed changes

julian-demox reviewed Feb 7, 2025

View reviewed changes

dagarcia7 commented Feb 10, 2025

View reviewed changes

TomasArrachea mentioned this pull request Feb 10, 2025

feat: implement Serializable and Deserializable for SyncSummary #725

Merged

dagarcia7 marked this pull request as ready for review February 10, 2025 18:21

igamigo linked an issue Feb 10, 2025 that may be closed by this pull request

Make TransactionResult Serializable and Deserializable #711

Closed

tomyrd approved these changes Feb 10, 2025

View reviewed changes

igamigo reviewed Feb 10, 2025

View reviewed changes

crates/web-client/src/models/transaction_request.rs Show resolved Hide resolved

igamigo reviewed Feb 10, 2025

View reviewed changes

dagarcia7 changed the base branch from next to main February 11, 2025 08:00

dagarcia7 changed the base branch from main to next February 11, 2025 08:00

dagarcia7 force-pushed the web-workers branch 2 times, most recently from 6a0e4b9 to 0ef6ac1 Compare February 12, 2025 03:31

dagarcia7 changed the base branch from next to main February 12, 2025 03:31

igamigo approved these changes Feb 13, 2025

View reviewed changes

tomyrd approved these changes Feb 13, 2025

View reviewed changes

dagarcia7 force-pushed the web-workers branch from cde1685 to 7119b1f Compare February 14, 2025 03:26

Web Workers for Account Creation and All Transaction Types

40d9327

dagarcia7 force-pushed the web-workers branch from 7119b1f to 40d9327 Compare February 14, 2025 03:30

dagarcia7 merged commit 0e5bbba into 0xPolygonMiden:main Feb 14, 2025
13 checks passed

dagarcia7 deleted the web-workers branch February 14, 2025 04:04

bobbinth mentioned this pull request Feb 14, 2025

Make new_mint_transaction(), new_send_transaction(), and new_consume_transaction() automatically use delegated proving if available #736

Open

This was referenced Feb 19, 2025

Web Client Fix: Handle Case Where Web Workers Not Available #743

Merged

Web Client Submit Transaction Fix #760

Merged

		const client = new WebClient();
		await client.create_client(rpc_url, prover_url);

	* correctly passes user parameters (RPC URL, prover URL, and seed) to both the main-thread
	* correctly passes user parameters (RPC URL and seed) to both the main-thread

Initial Web Client Web Workers Implementation #720

Initial Web Client Web Workers Implementation #720

Conversation

dagarcia7 commented Feb 6, 2025 • edited Loading

Summary

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

SantiagoPittella Feb 7, 2025 • 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

julian-demox Feb 7, 2025 • 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

tomyrd 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

SantiagoPittella commented Feb 10, 2025

igamigo 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

dagarcia7 commented Feb 11, 2025 • edited Loading

dagarcia7 commented Feb 11, 2025 • edited Loading

bobbinth commented Feb 12, 2025

dagarcia7 commented Feb 12, 2025 • edited Loading

igamigo left a comment

Choose a reason for hiding this comment

tomyrd 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

bobbinth commented Feb 14, 2025

igamigo commented Feb 14, 2025 • edited Loading

dagarcia7 commented Feb 14, 2025

bobbinth commented Feb 14, 2025

dagarcia7 commented Feb 6, 2025 •

edited

Loading

SantiagoPittella Feb 7, 2025 •

edited

Loading

julian-demox Feb 7, 2025 •

edited

Loading

dagarcia7 commented Feb 11, 2025 •

edited

Loading

dagarcia7 commented Feb 11, 2025 •

edited

Loading

dagarcia7 commented Feb 12, 2025 •

edited

Loading

igamigo commented Feb 14, 2025 •

edited

Loading