Adding Changes to Enable WASM-32 Support #378

dagarcia7 · 2024-06-07T09:24:20Z

Summary of Changes

Most of the changes needed to support a WASM build revolve around making the code async-friendly. As such, I added a wasm feature flag and added conditional rendering everywhere where async will be needed for WASM.
- Note: because the there is a lot of code duplication as a result of this conditional compilation, I am monitoring @phklive's PR here Implement async trait DataStore using maybe-async-await macro miden-base#725. When this PR is complete, I can change this approach to use the maybe-async-await macro as Paul-Henry has done in the miden-base repository.
Aside from using the flag to support the async changes needed, I also used it to avoid compiling certain things like CLI files or SQLite store files that aren't needed for WASM.
Finally, these changes trickled down to the Cargo.toml file where I tried to organize the dependencies based on whether the wasm feature flag and the target_arch is wasm32 or not.

Once this PR is accepted and merged, I will follow up with another PR to officially add the wasm crate. Once this is checked in, we can work together to publish the wasm code as an NPM package so others can start using it in their web applications.

Cargo.toml

src/store/note_record/mod.rs

src/store/mod.rs

src/main.rs

src/errors.rs

src/client/transactions/mod.rs

src/client/rpc/mod.rs

igamigo

Looks good so far! I didn't take a deep look at the duplicated code (though mostly it looked to be exactly duplicate), but everything else looks OK. Some of this work will probably clash with #367, but this seems like a higher priority and so we can look into merging this first.

src/client/mod.rs

igamigo · 2024-06-07T14:44:44Z

src/client/mod.rs

+pub mod chain_data;
+pub mod note_screener;
+pub mod notes;


If these are needed to be exported for the separate crate, we can remove the pub uses from below lines.

I will test this a bit further before merging. I may not need them to be pub after all. Will double check 👍

Thanks for pointing this out because I don't think I needed them after all 👍

src/client/rpc/mod.rs

bobbinth · 2024-06-07T21:23:47Z

Cargo.toml

+[target.'cfg(all(not(wasm), not(target_arch = "wasm32")))'.dependencies]
+clap = { version = "4.3", features = ["derive"] }
+comfy-table = "7.1.0"
+figment = { version = "0.10", features = ["toml", "env"] }
+lazy_static = "1.4.0"
+miden-node-proto = { version = "0.3", default-features = false }
+rusqlite = { version = "0.30.0", features = ["vtab", "array", "bundled"] }
+rusqlite_migration = { version = "1.0" }
+tokio = { version = "1.29", features = ["rt-multi-thread", "net", "macros"] }
+tonic = { version = "0.11" }
+toml = { version = "0.8" }


@mFragaBA we should do something similar in #367 - but instead of making this depend on wasm feature, I'd introduce the executable feature. It could look something like:

executable = ["clap", "comfy-table", "sqlite", "tonic", "etc"]

bobbinth · 2024-06-07T21:31:04Z

Looks great! Thank you! Overall, I agree with the plan:

First, we'll merge #725 in miden-base. For this, @phklive will update the current implementation of winter-maybe-async macro and I'll publish the new crate.
Then, we'll update this PR to remove code duplication. After that we'll merge this PR.
a. Here, would also be great to get rid of async_trait dependency.
Then, we'll merge #367 to clean up feature organization etc.
a. For example, rather than having dedicated wasm feature, we might just have an async feature and gate all CLI-related dependencies behind the executable feature.
After these are done, we can add the WASM crate in a separate PR.

bobbinth · 2024-06-11T16:26:27Z

First, we'll merge #725 in miden-base. For this, @phklive will update the current implementation of winter-maybe-async macro and I'll publish the new crate.

Then, we'll update this PR to remove code duplication. After that we'll merge this PR.
a. Here, would also be great to get rid of async_trait dependency.

Then, we'll merge #367 to clean up feature organization etc.
a. For example, rather than having dedicated wasm feature, we might just have an async feature and gate all CLI-related dependencies behind the executable feature.

After these are done, we can add the WASM crate in a separate PR.

The first item from this list is now complete. @dagarcia7 - could you start on the second item?

…elease

Cargo.toml

src/client/mod.rs

Cargo.toml

igamigo

Left a small comment, everything else looks good so far! We might want to decide to do something about the miden-node-proto errors and make them compatible, but it doesn't seem too pressing.

src/client/mod.rs

mFragaBA

Great work! Left some comments though mostly are questions

Cargo.toml

src/client/mod.rs

src/client/transactions/mod.rs

src/store/note_record/mod.rs

src/client/notes.rs

dagarcia7 · 2024-06-13T16:54:27Z

tests/integration/main.rs

@@ -1,3 +1,5 @@
+#![cfg(not(feature = "wasm"))]


Part of the Github Actions workflow run against this branch is to run the tests against all features, but these integration tests aren't relevant for wasm (and lots of the code breaks because of conditional compilation on the "wasm" feature throughout the codebase) because they require usage of the Sqlite Store. So I added this conditional compile flag at the top level here so none of the downstream test modules will compile.

Does this mean the clippy check with --all-features will not go through this file? If so, we might want to change the CI to not include the wasm feature (and maybe include another step for wasm-exclusive code)

same thing for fix

I think once we remove wasm feature (in a follow-up PR) this shouldn't be an issue. Though, we might have other similar situations (e.g., with conditional async) and we could handle them similar to how we did it in miden-base.

So is the resolution to leave this for now? To my understanding, the clippy and fix checks should still reach this code for all other features. Agreed that there needs to be an improvement here, though. Reading @bobbinth's message, it sounds like this might be tackled in a follow-up PR.

Yeah - doing this in a follow-up PR is fine.

dagarcia7 · 2024-06-13T16:55:58Z

src/store/note_record/mod.rs

@@ -14,6 +14,7 @@ mod output_note_record;
 pub use input_note_record::InputNoteRecord;
 pub use output_note_record::OutputNoteRecord;

+#[allow(rustdoc::broken_intra_doc_links)]


Because [SqliteStore](crate::store::sqlite_store::SqliteStore) is compiled out with the "wasm" feature, this code fails the cargo make doc command in the github actions workflows. Added this to suppress the error. Happy to change this is a different route is preferred!

Should we do it like this?

#![cfg_attr(feature = "wasm", allow(rustdoc::broken_intra_doc_links))]

Yep, this is much better. Thank you!

dagarcia7 · 2024-06-13T16:56:45Z

src/client/sync.rs

@@ -574,13 +574,13 @@ impl<N: NodeRpcClient, R: FeltRng, S: Store, A: TransactionAuthenticator> Client
        Ok(PartialMmr::from_parts(current_peaks, tracked_nodes, track_latest))
    }

+    #[allow(rustdoc::broken_intra_doc_links)]


Same story here with the cargo make doc command. In this case, [SyncStateResponse] is not compiled under the "wasm" feature

igamigo

LGTM! Left some final comments but we should be good to merge after addressing them.

igamigo · 2024-06-13T18:12:53Z

src/store/note_record/mod.rs

@@ -14,6 +14,7 @@ mod output_note_record;
 pub use input_note_record::InputNoteRecord;
 pub use output_note_record::OutputNoteRecord;

+#[allow(rustdoc::broken_intra_doc_links)]


Should we do it like this?

#![cfg_attr(feature = "wasm", allow(rustdoc::broken_intra_doc_links))]

igamigo · 2024-06-13T18:16:06Z

tests/integration/main.rs

@@ -1,3 +1,5 @@
+#![cfg(not(feature = "wasm"))]


Does this mean the clippy check with --all-features will not go through this file? If so, we might want to change the CI to not include the wasm feature (and maybe include another step for wasm-exclusive code)

igamigo · 2024-06-13T18:19:16Z

src/errors.rs

+#[cfg(feature = "wasm")]
+pub trait MissingFieldHelper {
+    fn missing_field(field_name: &'static str) -> ConversionError;
+}
+
+#[cfg(feature = "wasm")]
+impl<T: prost::Message> MissingFieldHelper for T {
+    fn missing_field(field_name: &'static str) -> ConversionError {
+        ConversionError::MissingFieldInProtobufRepresentation {
+            entity: type_name::<T>(),
+            field_name,
+        }
+    }
+}


Could these be safely removed? It doesn't seem like they would be used but I could be missing something.

Cross-linking the reply in a different comment here #378 (comment) for visibility

I meant the helper trait and the impl, are those used/needed?

Ah, yeah. To get the RPC working in WASM, because I couldn't pull the miden-node-proto crate, I also had to copy the domain files here https://github.com/0xPolygonMiden/miden-node/tree/main/crates/proto/src/domain and the proto definitions here https://github.com/0xPolygonMiden/miden-node/tree/main/crates/proto/proto to my WASM code.

The files in the domain folder are the ones that require the MissingFieldHelper https://github.com/0xPolygonMiden/miden-node/blob/37f2b59bc11b1333334094aa575b7bef7bcc5770/crates/proto/src/domain/accounts.rs#L12. When I create an issue to the miden-node repo to allow me to pull the ConversionError code directly in to WASM, I'll make sure that this is also part of it.

If you're curious as to why I need the domain & proto files in the first place, it's because the ApiClient that is auto-generated via prost-build here https://github.com/0xPolygonMiden/miden-node/blob/37f2b59bc11b1333334094aa575b7bef7bcc5770/crates/proto/src/generated/rpc.rs#L3 isn't compatible with WASM due to its dependencies on tokio. In an effort to get the RPC working for WASM, I copied the proto definitions along with the domain files, and used https://crates.io/crates/tonic-web-wasm-client to generate the ApiClient in a way that is compatible with WASM.

I think this could be either part of the same issue we're discussing above (to make changes to miden-node-proto such that it can be pulled in to WASM) or a separate issue on top of that to see if we can get a WASM-compatible ApiClient generated directly through the miden-node repo that I can pull from WASM.

igamigo · 2024-06-13T18:20:24Z

src/errors.rs

+pub enum ConversionError {
+    #[error("Hex error: {0}")]
+    HexError(#[from] hex::FromHexError),
+    #[error("SMT leaf error: {0}")]
+    SmtLeafError(#[from] SmtLeafError),
+    #[error("SMT proof error: {0}")]
+    SmtProofError(#[from] SmtProofError),
+    #[error("Too much data, expected {expected}, got {got}")]
+    TooMuchData { expected: usize, got: usize },
+    #[error("Not enough data, expected {expected}, got {got}")]
+    InsufficientData { expected: usize, got: usize },
+    #[error("Value is not in the range 0..MODULUS")]
+    NotAValidFelt,
+    #[error("Invalid note type value: {0}")]
+    NoteTypeError(#[from] NoteError),
+    #[error("Field `{field_name}` required to be filled in protobuf representation of {entity}")]
+    MissingFieldInProtobufRepresentation {
+        entity: &'static str,
+        field_name: &'static str,
+    },
+}


This should be fine for now, but we likely want to tackle this better from the node, we should add a TODO or file an issue to track this.

Yeah - let's create an issue to address this.

Yeah, I tried explaining here #378 (comment), but basically this error is brought in from miden-node-proto which currently brings in some dependencies that aren't compatible with the WASM code. And I need the error for the RPC work in the WASM. Completely agree that we can make a fix on the miden-node-proto repo to be able to import that error directly in WASM and remove the code here. I can file an issue against the miden-node-proto crate 👍

Cargo.toml

src/client/mod.rs

bobbinth

Looks good! Thank you! This is not a super deep review - but I did leave a few comments inline (some of which are for future PRs).

bobbinth · 2024-06-13T21:20:46Z

Cargo.toml

 integration = ["testing", "concurrent", "uuid", "assert_cmd"]
 std = ["miden-objects/std"]
 testing = ["miden-objects/testing", "miden-lib/testing"]
 test_utils = ["miden-objects/testing"]
+wasm = ["std", "getrandom", "hex", "thiserror", "prost"]


Not for this PR: but WASM compilation shouldn't depend on std. What functionality specifically is needed from miden-objects/std for WASM?

@bobbinth great point. This is the error I get if I don't include it

error[E0599]: the method `as_dyn_error` exists for reference `&SmtLeafError`, but its trait bounds were not satisfied --> src/errors.rs:26:20 | 26 | SmtLeafError(#[from] SmtLeafError), | ^^^^ method cannot be called on `&SmtLeafError` due to unsatisfied trait bounds | ::: /Users/dennisgarcia/.cargo/registry/src/index.crates.io-6f17d22bba15001f/miden-crypto-0.9.3/src/merkle/smt/full/error.rs:14:1 | 14 | pub enum SmtLeafError { | --------------------- doesn't satisfy `SmtLeafError: AsDynError<'_>` or `SmtLeafError: StdError` | = note: the following trait bounds were not satisfied: `SmtLeafError: StdError` which is required by `SmtLeafError: AsDynError<'_>` `&SmtLeafError: StdError` which is required by `&SmtLeafError: AsDynError<'_>`

which is due to that pesky code in src/errors.rs that I'm duplicating from the miden-node-proto module. All the more reason to do a quick follow up to remove that code 👍

bobbinth · 2024-06-13T21:28:49Z

src/client/notes.rs

+    #[cfg(not(feature = "wasm"))]
    pub fn get_input_notes(&self, filter: NoteFilter) -> Result<Vec<InputNoteRecord>, ClientError> {
        self.store.get_input_notes(filter).map_err(|err| err.into())
    }

+    #[cfg(feature = "wasm")]
+    pub async fn get_input_notes(
+        &self,
+        filter: NoteFilter<'_>,
+    ) -> Result<Vec<InputNoteRecord>, ClientError> {
+        self.store.get_input_notes(filter).await.map_err(|err| err.into())
+    }


I'm assuming we need to maintain 2 separate versions because of the lifetime? Could we have just a single function which looks like this:

#[maybe_async] pub fn get_input_notes( &self, filter: NoteFilter<'_>, ) -> Result<Vec<InputNoteRecord>, ClientError> { ... }

If not, I'd get rid of the lifetime requirement in NoteFilter (I think cloning of Note IDs is fine). But this can also be done in a follow-up PR.

I've just checked and clippy doesn't complain about having the lifetime in the non-async version. So I agree, let's keep one implementation for get_input_notes and get_output_notes. Also, you might get an error at get_input_note_with_id_prefix if you do so, but it's related to this comment

Good call. Tried keeping the lifetime requirement with the maybe-async/maybe-await pattern and it looks like it's compiling and working fine 🎉

bobbinth · 2024-06-13T21:29:09Z

src/client/notes.rs

+    #[cfg(not(feature = "wasm"))]
    pub fn get_output_notes(
        &self,
        filter: NoteFilter,
    ) -> Result<Vec<OutputNoteRecord>, ClientError> {
        self.store.get_output_notes(filter).map_err(|err| err.into())
    }

+    #[cfg(feature = "wasm")]
+    pub async fn get_output_notes(
+        &self,
+        filter: NoteFilter<'_>,
+    ) -> Result<Vec<OutputNoteRecord>, ClientError> {
+        self.store.get_output_notes(filter).await.map_err(|err| err.into())
+    }


Similar comment as the one above.

bobbinth · 2024-06-13T21:29:59Z

src/main.rs

+#[cfg(feature = "wasm")]
+fn main() {}


Why do we need this?

I think Dennis mentioned it here. I'm not sure, but would splitting (either two independent crates or both in the same workspace) the CLI from the client library might solve the issue, but I think we can tackle it in a separate refactor

Since I basically compiled out the main function that runs the CLI

#[cfg(not(feature = "wasm"))] #[tokio::main] async fn main() -> Result<(), String> { tracing_subscriber::fmt::init(); // read command-line args let cli = Cli::parse(); // execute cli action cli.execute().await }

I needed to replace the main function. So I just added one that does nothing 😅. @mFragaBA's suggestion is a lot cleaner. I'll jot it down as the list of things to follow-up on after this PR 👍

bobbinth · 2024-06-13T21:33:30Z

tests/integration/main.rs

@@ -1,3 +1,5 @@
+#![cfg(not(feature = "wasm"))]


I think once we remove wasm feature (in a follow-up PR) this shouldn't be an issue. Though, we might have other similar situations (e.g., with conditional async) and we could handle them similar to how we did it in miden-base.

bobbinth · 2024-06-13T21:34:16Z

src/client/rpc/mod.rs

@@ -1,19 +1,22 @@
+#![allow(async_fn_in_trait)]


Is this attribute still needed?

I checked and If we remove this clippy starts complaining:

Interesting! I'm surprised we didn't run into this issue in miden-base. Could it be due to Rust compiler version differences?

hmm, we're both using 1.78 (at least as per the rust-toolchain file) so I don't think so.

Yeah I was also curious about this. Removing the async-trait dependency on the traits works, but yields these warning messages. I suppressed them with this line of code, but also unsure why rust yells at us to begin with.

bobbinth · 2024-06-13T21:36:37Z

src/errors.rs

+pub enum ConversionError {
+    #[error("Hex error: {0}")]
+    HexError(#[from] hex::FromHexError),
+    #[error("SMT leaf error: {0}")]
+    SmtLeafError(#[from] SmtLeafError),
+    #[error("SMT proof error: {0}")]
+    SmtProofError(#[from] SmtProofError),
+    #[error("Too much data, expected {expected}, got {got}")]
+    TooMuchData { expected: usize, got: usize },
+    #[error("Not enough data, expected {expected}, got {got}")]
+    InsufficientData { expected: usize, got: usize },
+    #[error("Value is not in the range 0..MODULUS")]
+    NotAValidFelt,
+    #[error("Invalid note type value: {0}")]
+    NoteTypeError(#[from] NoteError),
+    #[error("Field `{field_name}` required to be filled in protobuf representation of {entity}")]
+    MissingFieldInProtobufRepresentation {
+        entity: &'static str,
+        field_name: &'static str,
+    },
+}


Yeah - let's create an issue to address this.

mFragaBA

LGTM!

Dominik1999 · 2024-06-14T16:16:08Z

merge?

* Adding Changes to Enable WASM-32 Support * Fixed all conflicts * Minor cleanup * Incorporate maybe-async/maybe-await macros * Feedback * Added async feature with no dependencies yet. Need miden-base 0.3.1 release * Reverting miden-client.toml changes * Feedback * Cleanup * Lint * Fixed more github action errors * Feedback --------- Co-authored-by: Dennis Garcia <dennis@demoxlabs.xyz>

Dennis Garcia added 3 commits June 7, 2024 03:15

Adding Changes to Enable WASM-32 Support

beb1971

Up to date with next branch

94d01bd

Fixed all conflicts

06e51ec