0xPolygonMiden / miden-client

Client library that facilitates interaction with the Miden rollup
MIT License
25 stars 22 forks source link

Add WebTonicClient to miden-client #409

Open dagarcia7 opened 4 days ago

dagarcia7 commented 4 days ago

Summary

This PR adds the WebRpcClient, a WASM-compatible implementation of the miden-client NodeRpcClient trait.

The WebRpcClient is designed for web environments. The existing tonic_client builds an auto-generated ApiClient using tonic_build with the default transport feature enabled. As a result, this auto-generated implementation relies heavily on tokio, which is not compatible with WASM. This poses a challenge for web-based applications that need to interact with the Miden node.

To address this, the WebRpcClient has been developed to provide the same functionality but in a web-compatible manner. The key difference is that WebRpcClient also generates an ApiClient via tonic_build, but with the transport feature disabled. When used in combination with the tonic-web-wasm-client crate, you can wrap the newly generated ApiClient and create an ApiClient that is compatible with grpc-web.

Considerations

I'm sure the biggest thing that will stand out to reviewers is the fact that I had to basically duplicate the tonic_client/domain folder to web_tonic_client/domain as well. As well as the fact that the auto-generated code to come from both tonic_build configurations is the same with the exception of the rpc.rs file. I took a while to see I could manipulate the build.rs file to dump all auto-generated files with the exception of rpc.rs to /src/client/rpc/generated_common, and dump the respective rpc.rs copies needed (one build with transport, the other without) to src/client/rpc/tonic_client/generated and src/client/rpc/web_tonic_client/generated, respectively. From here, I wanted to also include a step in the build.rs file to apply some post-processing to each rpc.rs file so the imports within the auto-generated code could properly point to the right objects in the generated_common folder.

This proved to be a bit difficult, however. Couldn't get it working after a few hours, so decided to put this PR up with the duplication. Happy to discuss alternate approaches, or to see if we can get this working in the follow-up PR.

dagarcia7 commented 18 hours ago

Great work! I don't mind the duplication that much for now. Also, if the idea still is to make a wasm crate, we could consider moving the webstore and webtonicclient implementations to that crate if it makes sense (I think that might also remove the need for a wasm feature flag). Left some minor comments/nits

@mFragaBA great point! I think for now it would be best to keep this PR as is and then my next PR planned is the one that will be adding the web-client code that references both the WebStore and WebTonicClient. I think we can have the discussion of it living as its own crate or not there, and I can copy this folder and the WebStore code accordingly should we decide to move forward with that route then. 👍

dagarcia7 commented 17 hours ago

I tried running the make doc command locally and getting these types of errors

error[E0252]: the name `ProtoAccountId` is defined multiple times
 --> crates/rust-client/src/rpc/domain/accounts.rs:9:5
  |
7 | use crate::rpc::tonic_client::generated::account::AccountId as ProtoAccountId;
  |     ------------------------------------------------------------------------- previous import of the type `ProtoAccountId` here
8 | #[cfg(feature = "web-tonic")]
9 | use crate::rpc::web_tonic_client::generated::account::AccountId as ProtoAccountId;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `ProtoAccountId` reimported here
  |
  = note: `ProtoAccountId` must be defined only once in the type namespace of this module
help: you can use `as` to change the binding name of the import
  |
9 | use crate::rpc::web_tonic_client::generated::account::AccountId as OtherProtoAccountId;

Is this because the underlying command runs all-features? cargo doc --all-features --keep-going --release -p miden-client. I could address all of these issues, but then would have to gate all the code in domain under different feature flags so don't think we want that. Any suggestions?

igamigo commented 13 hours ago

Is this because the underlying command runs all-features? cargo doc --all-features --keep-going --release -p miden-client. I could address all of these issues, but then would have to gate all the code in domain under different feature flags so don't think we want that. Any suggestions?

Yeah, this seems to be the case. As for solutions, I'm not sure what would be the best way to mitigate this right now. I can check tomorrow but some things that come to mind:

dagarcia7 commented 2 hours ago

LGTM! @dagarcia7 Lint failed because of fmt.

Fixed!

Oh, I also forgot to mention we should update CHANGELOG with this change (I think we also missed it for the WebStore)

Added a message retroactively for WebStore and added one for the WebTonicClient