Add Web Client #437

dagarcia7 · 2024-07-12T15:20:10Z

Summary

This PR implements the WebClient, which is a WASM-compatible version of the miden-client. With #402 and #409 merged into the codebase, the interfaces for a WASM-compatible store and a WASM-compatible RPC, have already been implemented and this PR puts all the components together to enable the creation of a full WASM module. With the package.json and rollup.config.js file at the root of this new crate, this module can easily be published as an NPM package allowing seamless integration of Miden node interactions in web environments.

The WebClient is built using a WebStore and a WebTonicRpcClient, mirroring the approach taken by the CLI tool that uses a SQLiteStore and a TonicRpcClient. This cohesive integration ensures that all the necessary functionalities for interacting with the Miden node are available in a web-compatible format, paving the way for broader usage and easier access in web-based applications.

A Few Notes to the Reviewers

The biggest glaring issue with this PR right now is that the interface still needs some work. We started out making this interface mirror that of the CLI, which after talking to @bobbinth, we know isn't the correct strategy long term. After this code is merged, we hope to polish this interface and the methods it exposes to WASM to more correctly align with the general miden-client interface.
The second thing to discuss that can be handled in this PR is how we want to structure this new crate. For this first round of review, I added the web-client as a separate crate and attempted to clean up the workspace Cargo.toml file a bit as well as any redundancies in the rust-client's Cargo.toml file as well. However, I left the WebStore and WebTonicRpcClient implementations in the rust-client crate. This feels off and was planning on changing it before putting this up for review, but figured I'd get the OK first given that I think we did discuss the potential of that code remaining with the other sqlite and tonic client code. It shouldn't be a big effort take the WebStore and WebTonicRpcClient code and drop it all in the web-client crate so everything WASM-related lives together. This should also clean up some more of the Cargo files and make them a bit more distinct. Again, I think this is my preferred route, but maybe I'm missing an argument for the way it is currently structured in this first round of review.
Finally, I'll add the same addendum here that we are hoping to push this code through so that development of the miden-client also includes the necessary changes to the WebStore and WebTonicRpcClient so we don't get too behind. After this is done, we will work on refining the interfaces as mentioned above, but also add better testing infrastructure, and general polish.

bobbinth · 2024-07-12T17:12:00Z

Awesome work! Thank you! I'll start reviewing this early next week, but to make one comment:

The second thing to discuss that can be handled in this PR is how we want to structure this new crate. For this first round of review, I added the web-client as a separate crate and attempted to clean up the workspace Cargo.toml file a bit as well as any redundancies in the rust-client's Cargo.toml file as well. However, I left the WebStore and WebTonicRpcClient implementations in the rust-client crate. This feels off and was planning on changing it before putting this up for review, but figured I'd get the OK first given that I think we did discuss the potential of that code remaining with the other sqlite and tonic client code. It shouldn't be a big effort take the WebStore and WebTonicRpcClient code and drop it all in the web-client crate so everything WASM-related lives together. This should also clean up some more of the Cargo files and make them a bit more distinct. Again, I think this is my preferred route, but maybe I'm missing an argument for the way it is currently structured in this first round of review.

I actually think it is OK to keep WebStore and WebTonicRpcClient in the rust-client crate. I view them as parallel to SqliteStore and TonicRpcClient - and if one set is in rust-client, then I think the other set can be in rust-client too.

tomyrd

Good Job with this implementation! Left a few comments.

tomyrd · 2024-07-12T19:50:43Z

crates/rust-client/src/lib.rs

@@ -150,7 +150,7 @@ impl<N: NodeRpcClient, R: FeltRng, S: Store, A: TransactionAuthenticator> Client
        &mut self.rpc_api
    }

-    #[cfg(any(test, feature = "testing"))]
+    #[cfg(any(test, feature = "testing", feature = "idxdb"))]


I feel like exposing the store (even just for idxdb) may let the user access store methods which are only intended to be used by the client.

I agree; I don't like this either. It currently is an unfortunate consequence of the get_signature method of the StoreAuthenticator being synchronous. This is a problem because in the context of indexedDB, the store method method this function calls should be asynchronously called. Because it's not (and very difficult to change), we had to expose this method on the store that is asynchronous, and basically serves the purpose of fetching the account auth info in an async manner, and caches that info in memory. Then, when get_signature is called during a transaction request later downstream, the store call made to get_account_auth_by_pub_key simply retrieves the auth info from this cached object in a synchronous manner.

In any case, for now, the quick solution is to allow my web-client to access the indexedDB store directly so that I may be able to call the function that caches the auth object before a transaction call. This change is found in this PR in this file crates/web-client/src/account.rs.

In the future, the hope is that we can collaborate to find a better solution in general to the get_signature method relying on the store in general, especially in a synchronous manner. My current idea is that beyond keeping a WebClient IndexedDB store that the client needs to run its operations smoothly, the application (in this case the wallet), can keep a separate store and organize data as they see fit. This could include storing auth or private info in an encrypted manner for the application, and then passing the auth directly to the client. This would eliminate the need to access the store directly and then we could keep the store private to the client for all cases.

Sorry for the long explanation, but this one required some context! 😅

tomyrd · 2024-07-12T20:11:30Z

crates/rust-client/src/store/web_store/js/chainData.js

@@ -15,7 +15,7 @@ export async function insertBlockHeader(
            blockNum: blockNum,
            header: header,
            chainMmrPeaks: chainMmrPeaks,
-            hasClientNotes: hasClientNotes
+            hasClientNotes: hasClientNotes.toString()


Small question, why were booleans changed to strings? Did they not work with idxdb?

Great question and something I stumbled into by accident. Whenever you have a query for indexedDB like this

const allMatchingRecords = await blockHeaders .where('hasClientNotes') .equals("true") .toArray();

where you need to query a store via a specific field, you must add that field as an index when creating the schema. As such, indexedDB does not recognize certain data types like booleans (or blobs, I believe), as valid keys. So in this case, I decided to store the booleans instead as strings and wherever I found myself needing to index something that the SQLite implementation stores as a blob, I used a base64 encoded string for those.

tomyrd · 2024-07-12T21:42:03Z

crates/web-client/rollup.config.js

+        plugins: [
+            rust({
+                cargoArgs: [
+                    "--features", "testing",


Do we want this feature always on for the web client?

No, we don't. For context, my colleague, John, and I are planning on publishing a "test" package and a "production" package for now for this reason. The reason we currently are using this one is because when creating a new account without the testing feature, the proof of work takes an incredibly long time to finish on a web browser. I think @bobbinth and I have discussed this briefly, but haven't found a good solution to it yet. As such, we think it'll be most helpful for teams to use the test version of the npm package first, and we can publish a production version separately so teams get a feel for the current state of the "real" times needed to create accounts, execute proofs, etc. in the browser.

tomyrd · 2024-07-15T14:45:28Z

crates/web-client/src/sync.rs

+
+#[wasm_bindgen]
+impl WebClient {
+    pub async fn sync_state(&mut self) -> Result<JsValue, JsValue> {


With the current interface we don't have the ability to update ignored notes in the WebClient. Maybe we could add a boolean parameter stating whether we want to update them or not (like the cli does) or add a separate method (like the one in the client).

tomyrd · 2024-07-15T14:48:39Z

crates/web-client/src/tags.rs

Maybe we could add methods for getting the tracked tags/removing them.

tomyrd · 2024-07-15T14:56:08Z

crates/rust-client/src/lib.rs

+///
+/// TODO: we should make the function in base public and once that gets released use that one and
+/// delete this implementation.
+pub fn build_swap_tag(


I feel like this function and get_input_note_with_id_prefix don't belong here.

Maybe a better place for the former is crates/rust-client/src/transactions/mod.rs or crates/rust-client/src/sync/tags.rs and the latter could go in crates/rust-client/src/notes/mod.rs.

Makes sense to me. Went ahead and moved them and updated the paths to anything that imports them 👍

igamigo

Awesome work. Left some comments. Some more general things to discuss:

I don't have a strong opinion on leaving the web store and RPC clients within rust-client vs the new web crate. I think ideally if we were to leave them where they are, if both feature flags are turned on, it wouldn't fail to compile (which I think is not the case right now, due to conflicting names)
I didn't take a deep look yet at some sections of the code (specifically the client's API). Something that caught my eye are the New* struct names which we probably want to change, but as you mentioned these refinements can come later.
The build seems to be broken. After a quick look I wasn't able to figure out why precisely but I can investigate further if you need help.

igamigo · 2024-07-15T14:20:46Z

crates/rust-client/src/lib.rs

@@ -12,7 +12,7 @@ mod errors;
 pub mod notes;
 pub mod rpc;
 pub mod store;
-mod store_authenticator;
+pub mod store_authenticator;


Is this needed? I think it might not be (StoreAuthenticator is re-exported)

igamigo · 2024-07-15T14:22:42Z

crates/rust-client/src/lib.rs

@@ -150,7 +150,7 @@ impl<N: NodeRpcClient, R: FeltRng, S: Store, A: TransactionAuthenticator> Client
        &mut self.rpc_api
    }

-    #[cfg(any(test, feature = "testing"))]
+    #[cfg(any(test, feature = "testing", feature = "idxdb"))]


Why is this needed? It might not be ideal if users can access the store (especially if it's based on the idxdb flag

Great question! This is something that @tomyrd also brought up. I replied here #437 (comment). If there is a different workaround solution I'm happy to change this, but this was my bandaid for now 🤔

igamigo · 2024-07-15T14:31:00Z

crates/rust-client/src/store/web_store/js/chainData.js

@@ -15,7 +15,7 @@ export async function insertBlockHeader(
            blockNum: blockNum,
            header: header,
            chainMmrPeaks: chainMmrPeaks,
-            hasClientNotes: hasClientNotes
+            hasClientNotes: hasClientNotes.toString()


Out of curiosity, why was this changed?

@tomyrd also had the same question. Gave a reply here as well! #437 (comment)

igamigo · 2024-07-15T14:34:41Z

crates/rust-client/src/lib.rs

+///
+/// TODO: we should make the function in base public and once that gets released use that one and
+/// delete this implementation.
+pub fn build_swap_tag(


Maybe not for this PR, but if this was moved up to the library level, should the CLI version be deleted?

Went ahead and deleted this and the get_input_note_with_id_prefix from the bin crate and updated imports to point to the implementations defined in the rust-client 👍

igamigo · 2024-07-15T14:35:37Z

crates/web-client/README.md

Nice readme! I think there are some stylistic nits ("miden vm" -> "Miden VM"); tagging @Dominik1999 in case he wants to take a look

Dominik1999

Looks good to me. I just checked the README.md and added some nit comments. Really cool work!

Dominik1999 · 2024-07-16T07:52:37Z

crates/web-client/README.md

+```typescript
+import { WebClient } from "@demox-labs/miden-sdk";
+
+const webClient = new WebClient();


Does the WebClient automatically run with --features testing? The WebClient should have the same defaults as the Rust client.

I explained a bit here #437 (comment), but when it comes to building the web-client crate, you can build with any combination of the features specified in the Cargo.toml file. However, when it comes to publishing an actual NPM package, I think it may have to be based on a specific set of features. This is because we have to build the crate, create a WASM, and publish this code as a package for others to consume directly via a package.json. I think for this reason, the easiest route for now may be to publish a "test" version of the miden-client which is build with the "testing" feature, and a "production" version of the miden-client which is built with the default features. Another idea would be to publish a single NPM package, but include multiple build artifacts that reflect the different ways in which the crate could be built. Let me sync up with John and we will create a plan for this ASAP!

Thanks a lot! I think a production mode for the WebClient wouldn't work right now. The PoW in the browser takes too long. So maybe we should default to testing? What do you think @bobbinth ?

Yeah - testing should be the default for now. This will change after we refactor how account ID generation works.

Dominik1999 · 2024-07-16T07:53:19Z

crates/web-client/README.md

+const webClient = new WebClient();
+
+// Creates the internal client of a previously instantiated WebClient.
+// Can provide `node_url` as an optional parameter. Defaults to "http://localhost:57291".


Can you default the WebClient to our remote testnet Node? 18.203.155.106 It should have the same defaults as the Rust client

Dominik1999 · 2024-07-16T07:54:21Z

crates/web-client/README.md

+
+// Creates the internal client of a previously instantiated WebClient.
+// Can provide `node_url` as an optional parameter. Defaults to "http://localhost:57291".
+// See https://github.com/0xPolygonMiden/miden-node for setting up and running your own node locally


It would be better not to ask ppl to run a Miden node locally. We can do that for testing, but let's try to simplify it as much as we can for users

Dominik1999 · 2024-07-16T07:55:15Z

crates/web-client/README.md

+/**
+ * Creates a new wallet account.
+ * 
+ * @param storage_mode String. Either "OffChain" or "OnChain".


Should we consistently rename those to Private and Public? @bobbinth ?

I can open an issue to make the changes everywhere.

Yes, we already made these renamings in miden-base and should propagate them everywhere.

Dominik1999 · 2024-07-16T07:58:06Z

crates/web-client/README.md

+```
+
+### Transactions
+You can use the webClient to facilitate transactions between accounts.


In line 51 you capitalize WebClient, here you use lower case. Can you make it consistent?

Dominik1999 · 2024-07-16T08:03:30Z

crates/web-client/README.md

+ * @param {string} amount
+ * @returns {Promise<NewTransactionResult>}
+ * 
+ * Example of NewTransactionResult object:


Dominik1999 · 2024-07-16T08:03:39Z

crates/web-client/README.md

+ * @param {string | undefined} [recall_height]
+ * @returns {Promise<NewTransactionResult>}
+ * 
+ * Example of NewTransactionResult object:


Dominik1999 · 2024-07-16T08:03:49Z

crates/web-client/README.md

+ * @param {(string)[]} list_of_notes
+ * @returns {Promise<NewTransactionResult>}
+ * 
+ * Example of NewTransactionResult object:


Dominik1999 · 2024-07-16T08:03:57Z

crates/web-client/README.md

+ * @param {string} note_type
+ * @returns {Promise<NewSwapTransactionResult>}
+ * 
+ * Example of NewSwapTransactionResult object:


Dominik1999 · 2024-07-16T08:04:37Z

crates/web-client/README.md

This is so cool, to see that working! Congrats

dagarcia7 · 2024-07-18T12:34:58Z

@igamigo, @tomyrd, @Dominik1999, @bobbinth first round of feedback has been addressed! Will work now on trying to get these builds passing. Can also address any more feedback that is generated!

tomyrd · 2024-07-18T14:46:13Z

I've been checking the build errors for the workspace and the underlying problem is that the tonic and web-tonic features are mutually exclusive which is a problem in the Cargo feature philosophy.

When Cargo tries to build the whole workspace it adds all the features together assuming they are additive and it breaks. An example of these problems can be seen here where activating both features causes a collision in the name.

I don't know what the correct solution would be in this case, some things that come to mind are:

Having the web client in a separate crate, this would mean that it is built separately and Cargo wouldn't join the features.
If the only problem when building is the name collisions maybe we can try to unify the generated files and remove the conditional use statment for the features entirely. The generated files are really similar in both cases. The only difference is that the web-tonic has the transport features disabled, maybe this isn't really necesary and we can unify them.

dagarcia7 · 2024-07-18T16:45:37Z

I've been checking the build errors for the workspace and the underlying problem is that the tonic and web-tonic features are mutually exclusive which is a problem in the Cargo feature philosophy.

When Cargo tries to build the whole workspace it adds all the features together assuming they are additive and it breaks. An example of these problems can be seen here where activating both features causes a collision in the name.

I don't know what the correct solution would be in this case, some things that come to mind are:

Having the web client in a separate crate, this would mean that it is built separately and Cargo wouldn't join the features.

If the only problem when building is the name collisions maybe we can try to unify the generated files and remove the conditional use statment for the features entirely. The generated files are really similar in both cases. The only difference is that the web-tonic has the transport features disabled, maybe this isn't really necesary and we can unify them.

Hey @tomyrd thanks for investigating! I think messing with the generation of the two sets of files is tricky.

I actually gave this a shot in a different PR where I tried basically answering the question "can I generate the files for everything except the actual client (rpc.rs) in one way (i.e. same features whether it's transport or no transport), and drop them in one location, and then generate the actual rpc client in the manner we're currently doing and drop them in a different location?". I tried for a while, but couldn't get anything working well. But I agree that for everything except the rpc.rs file built from the rpc.proto file, that the transport feature doesn't matter.

If we want to keep the current crate structure (which I think seems to be the preference checking the comments through the PR), then another easy solution would be to duplicate the domain files over to the web_tonic_client module. This code duplication wouldn't be great, but I think should solve our issue.

I think putting everything web-related in one crate is a similar approach to previous approach and would involve duplicating the domain files.

igamigo · 2024-07-18T18:04:28Z

If we want to keep the current crate structure (which I think seems to be the preference checking the comments through the PR), then another easy solution would be to duplicate the domain files over to the web_tonic_client module. This code duplication wouldn't be great, but I think should solve our issue.

Yeah, I think we might have to do this. Most alternatives are hacky (macros, build scripts, etc.), and it does not seem like we could improve this with features.
The other thing we could do is fail to compile with both features on, and then have proper make targets and defaults to build each part more cleanly (replacing the CI actions accordingly)

bobbinth · 2024-07-19T06:02:30Z

Yeah, I think we might have to do this. Most alternatives are hacky (macros, build scripts, etc.), and it does not seem like we could improve this with features.
The other thing we could do is fail to compile with both features on, and then have proper make targets and defaults to build each part more cleanly (replacing the CI actions accordingly)

I'm fine with doing w/e is easier at this point, but in the longer run, we should probably not allow building with both features enabled (and update the Makefile commands accordingly). One way to make this explicit is something like this:

#[cfg(all(feature = "tonic", feature = "web-tonic"))]
compile_error!("features `tonic` and `web-tonic` are mutually exclusive");

#[cfg(all(feature = "sqlite", feature = "idxdb"))]
compile_error!("features `sqlite` and `idxdb` are mutually exclusive");

dagarcia7 · 2024-07-19T14:38:40Z

Ok I just duplicated the domain files to the web-client crate for now. Maybe we can a follow up issue to move forward with @bobbinth and @igamigo's approach? That sounds like the more correct approach and I would do it here, but unsure what that solution looks like myself. Let me know if there's any other feedback! I think I have some rebasing to do so will focus on that next.

dagarcia7 · 2024-07-22T10:19:27Z

Ok small update. After taking a look at the github actions that were failing, the main reason they were failing is because now that this repo has a workspace structure, commands like these in the Makefile

build: ## Builds the CLI binary and client library in release mode
	cargo build --release --features $(FEATURES_CLI)

were causing all the crates to build. The result of this (I think) is that there is a conflict of stores and clients built with and without async. The reason I think this is due to errors like this

error[E0277]: the `?` operator can only be applied to values that implement `Try`
   --> bin/miden-cli/src/commands/account.rs:122:24
    |
122 |     let (account, _) = client.get_account(account_id)?;
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `?` operator cannot be applied to type `impl Future<Output = Result<(Account, std::option::Option<[Felt; miden_crypto::::Word::{constant#0}]>), ClientError>>`
    |
    = help: the trait `Try` is not implemented for `impl Future<Output = Result<(Account, std::option::Option<[Felt; miden_crypto::::Word::{constant#0}]>), ClientError>>`

which signals to me that for some reason, the miden-cli is expecting an async version of that method. So to fix this, I took a look at the failing commands in the Makefile and just excluded the miden-client-web crate for commands meant to build just the miden-client and miden-cli and created separate commands (if applicable) for wasm-related activities.

A downstream effect of modifying the Makefile commands is that now I had no reason to not go with @bobbinth's solution mentioned here #437 (comment). So I reverted the changes to the rpc domain folder and unified them once again. With the Makefile commands separated properly now, this doesn't pop up in the build errors anymore.

igamigo · 2024-07-22T13:54:50Z

Yeah, I think as @tomyrd mentioned it adds up all the features and so the CLI is built with web versions of the store and tonic client which means it enables async, which causes the CLI to fail to compile because all store calls are presumed to be sync.

Docs are failing to build due to a similar cause (happened with the web tonic client PR as well), it seems like. I think we should try to find a way to make the build less fragmented in the future.

igamigo

LGTM! I left some comments but in general I believe we could merge this and address work in future PRs. We should track this work on an issue. Some of the things that I can think of off the top of my head that we might want to tackle next:

Adding CI checks for the web client (if not done on this PR)
Cleaning up (and removing unwraps, etc.) of the API to expose methods more similar to the client library
Removing the need of exposing the client store (might be better to open an issue on miden-base for this, since it's related to TransactionAuthenticator)
Finding a way of running existing integration tests with the web client

Ideally something else that would be great to have is to not fail to compile when std and wasm versions of the components are included at the same time, but this might not be easily achievable, or maybe not without changing the project structure.

igamigo · 2024-07-22T15:08:35Z

Makefile

+.PHONY: clippy-wasm
+ clippy-wasm: ## Runs Clippy with configs
+	cargo +nightly clippy --workspace --exclude miden-client --exclude miden-cli --exclude miden-client-tests --all-targets --features $(FEATURES_CLI) -- -D warnings


Can we add these to the CI checks? We can do it in a follow up or in this PR, as you want

Additionally, should it run clippy just for web-client instead of excluding the other projects or is there any reason why this is preferrable?

igamigo · 2024-07-22T15:10:07Z

Makefile


 build-wasm: ## Builds the client library for wasm32
-	cargo build --target wasm32-unknown-unknown --features idxdb,web-tonic --no-default-features --package miden-client
+	cargo build --workspace --exclude miden-client --exclude miden-cli --features $(FEATURES_WEB_CLIENT)


I think we could just build the web client here instead of building the workspace and excluding the other projects, no?

igamigo · 2024-07-22T15:10:56Z

Makefile

+	cargo +nightly clippy --workspace --exclude miden-client-web --all-targets --features $(FEATURES_CLI) -- -D warnings
+
+.PHONY: clippy-wasm
+ clippy-wasm: ## Runs Clippy with configs


nit: The comment could be updated to reflect the fact that it's mainly for web-client

tomyrd

LGTM! Left a couple of comments/questions.

tomyrd · 2024-07-22T16:11:13Z

Makefile

+
+.PHONY: clippy-wasm
+ clippy-wasm: ## Runs Clippy with configs
+	cargo +nightly clippy --workspace --exclude miden-client --exclude miden-cli --exclude miden-client-tests --all-targets --features $(FEATURES_CLI) -- -D warnings


We should use FEATURES_WEB_CLIENT here instead of FEATURES_CLI

tomyrd · 2024-07-22T16:22:42Z

Makefile


 build-wasm: ## Builds the client library for wasm32
-	cargo build --target wasm32-unknown-unknown --features idxdb,web-tonic --no-default-features --package miden-client
+	cargo build --workspace --exclude miden-client --exclude miden-cli --features $(FEATURES_WEB_CLIENT)


The flag --target wasm32-unknown-unknown was removed is this ok?

dagarcia7 · 2024-07-23T18:44:56Z

LGTM! I left some comments but in general I believe we could merge this and address work in future PRs. We should track this work on an issue. Some of the things that I can think of off the top of my head that we might want to tackle next:

Adding CI checks for the web client (if not done on this PR)

Cleaning up (and removing unwraps, etc.) of the API to expose methods more similar to the client library

Removing the need of exposing the client store (might be better to open an issue on miden-base for this, since it's related to TransactionAuthenticator)

Finding a way of running existing integration tests with the web client

Ideally something else that would be great to have is to not fail to compile when std and wasm versions of the components are included at the same time, but this might not be easily achievable, or maybe not without changing the project structure.

Thanks so much for the feedback!

CI checks (build-wasm and clippy-wasm) have been added in this PR ✅
make doc now only builds the miden-client package (as it did before), but without both the web-tonic and idxdb features. ✅

The rest of your points are top of mind for me for sure! I will make issues out of them and start working on them ASAP. Specifically, improving the testing infrastructure and error handling I think will be the first to come. In parallel, maybe I can revisit the conversation surrounding the TransactionAuthenticator with the team.

igamigo · 2024-07-24T11:09:14Z

Looks like the target is not correctly being installed? Perhaps it should be rustup target add wasm32-unknown-unknown --toolchain nightly?

igamigo · 2024-07-24T11:27:02Z

Looks like the check passed this time! I think we are good to merge after the other checks are green

igamigo · 2024-07-24T13:09:28Z

I think we are ready to merge this unless @bobbinth wants to take a final look.

bobbinth

Great work! Thank you! This is a relatively superficial review from me - but I did leave a some comments/questions inline. The main ones are about the need of concurrent and std features for the web-client crate.

Makefile

crates/rust-client/src/lib.rs

crates/rust-client/src/transactions/mod.rs

crates/web-client/Cargo.toml

crates/rust-client/src/store/web_store/accounts/mod.rs

dagarcia7 · 2024-07-29T14:09:12Z

@igamigo @bobbinth feedback addressed!

bobbinth

Looks good! Thank you! I left just one nit inline. Other than that, we should be good to merge.

Makefile

bobbinth · 2024-07-30T18:12:00Z

@dagarcia7 - seems like some tests are failing? This is probably due to some recent changes merged into next?

igamigo · 2024-07-30T18:18:45Z

Yeah, this is because we merged #445 (didn't think about it breaking this PR). I contacted Dennis earlier as well; we should have an update soon (I can fix the build in any case)

tomyrd reviewed Jul 15, 2024

View reviewed changes

igamigo reviewed Jul 15, 2024

View reviewed changes

Dominik1999 reviewed Jul 16, 2024

View reviewed changes

dagarcia7 force-pushed the add-web-client-clean branch 2 times, most recently from fab4ec7 to 9837ded Compare July 19, 2024 15:23

igamigo approved these changes Jul 22, 2024

View reviewed changes

tomyrd approved these changes Jul 22, 2024

View reviewed changes

dagarcia7 force-pushed the add-web-client-clean branch from 6c59bb5 to 0923256 Compare July 23, 2024 18:38

dagarcia7 force-pushed the add-web-client-clean branch from 0923256 to ac9f86f Compare July 23, 2024 19:31

bobbinth approved these changes Jul 25, 2024

View reviewed changes

igamigo mentioned this pull request Jul 29, 2024

Make miden_lib::notes::build_swap_tag() public 0xPolygonMiden/miden-base#806

Closed

dagarcia7 mentioned this pull request Jul 29, 2024

Add 'check' to Makefile and CI checks for miden-client (regular and WASM) #450

Closed

dagarcia7 force-pushed the add-web-client-clean branch 2 times, most recently from 3228bb4 to 6926529 Compare July 29, 2024 14:07

bobbinth approved these changes Jul 29, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

dagarcia7 force-pushed the add-web-client-clean branch 2 times, most recently from 1ebed6e to 10d71d2 Compare August 1, 2024 18:33

Add Web Client

cdec1e0

dagarcia7 force-pushed the add-web-client-clean branch from 10d71d2 to cdec1e0 Compare August 1, 2024 19:15

Modified changelog

75acd2c

igamigo merged commit 5eb2725 into 0xPolygonMiden:next Aug 1, 2024
13 checks passed

Add Web Client #437

Add Web Client #437

Conversation

dagarcia7 commented Jul 12, 2024

Summary

A Few Notes to the Reviewers

bobbinth commented Jul 12, 2024

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

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

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

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

Dominik1999 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

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

dagarcia7 commented Jul 18, 2024

tomyrd commented Jul 18, 2024

dagarcia7 commented Jul 18, 2024

igamigo commented Jul 18, 2024

bobbinth commented Jul 19, 2024

dagarcia7 commented Jul 19, 2024 • edited Loading

dagarcia7 commented Jul 22, 2024

igamigo commented Jul 22, 2024

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

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

dagarcia7 commented Jul 23, 2024

igamigo commented Jul 24, 2024 • edited Loading

igamigo commented Jul 24, 2024

igamigo commented Jul 24, 2024

bobbinth left a comment

Choose a reason for hiding this comment

dagarcia7 commented Jul 29, 2024

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth commented Jul 30, 2024

igamigo commented Jul 30, 2024

dagarcia7 commented Jul 19, 2024 •

edited

Loading

igamigo commented Jul 24, 2024 •

edited

Loading