feat: add missing rpc endpoints #744
Conversation
tomyrd
force-pushed
the
tomyrd-missing-rpc-endpoints
branch
3 times, most recently
from
c77bd5e to
083dcf4
Compare
tomyrd
force-pushed
the
tomyrd-missing-rpc-endpoints
branch
from
083dcf4 to
3b47265
Compare
|
Something else that I realized we should probably update is the feature list on |
crates/rust-client/src/mock.rs
Outdated
| &mut self, | ||
| _nullifiers: &[Nullifier], | ||
| ) -> Result<Vec<SmtProof>, RpcError> { | ||
| panic!("shouldn't be used for now") |
There was a problem hiding this comment.
Maybe this can be replaced with unimplemented. What do you think?
crates/rust-client/src/mock.rs
Outdated
| _from_block: BlockNumber, | ||
| _to_block: BlockNumber, | ||
| ) -> Result<AccountDelta, RpcError> { | ||
| panic!("shouldn't be used for now") |
| #[cfg(feature = "web-tonic")] | ||
| type InnerClient = tonic_web_wasm_client::Client; | ||
| #[cfg(feature = "tonic")] | ||
| type InnerClient = tonic::transport::Channel; | ||
|
|
There was a problem hiding this comment.
Should these be consistent with the TonicRpcClient::rpc_api() implementation? When instantiating the client, the tonic web wasm client is used based on the target architecture and not the features, but here we use the features.
I believe both should be configured in the same way: Because we only include the tonic_web_wasm_client crate based on the feature being enabled, we should probably base both off of that feature instead of target architecture. Additionally we should explore what happens if both features are set and maybe use compile_error!() to return a more specific error (because otherwise you'd get something related to InnerClient being defined twice. I think the same applies if no feature is set (InnerClient would not be defined then, I think). For this, maybe we want to only include the tonic_client mod if one of these features is enabled?
One other option that I think could work (although maybe I'm forgetting something): Could we make the actual TonicRpcClient::client (client: Option<ApiClient<T>>) generic over T? Then we could have 2 constructors: one for the web channel and one for the std-compatible one, and we could add these constructors based on the features. This way, if no features are set or both features are set, constructors get added or removed and there are no compilation errors. I think this could be cleaner but as I mentioned, maybe I'm missing something.
| #[cfg(feature = "web-tonic")] | ||
| type InnerClient = tonic_web_wasm_client::Client; | ||
| #[cfg(feature = "tonic")] | ||
| type InnerClient = tonic::transport::Channel; | ||
|
|
There was a problem hiding this comment.
Should these be consistent with the TonicRpcClient::rpc_api() implementation? When instantiating the client, the tonic web wasm client is used based on the target architecture and not the features, but here we use the features.
I believe both should be configured in the same way: Because we only include the tonic_web_wasm_client crate based on the feature being enabled, we should probably base both off of that feature instead of target architecture. Additionally we should explore what happens if both features are set and maybe use compile_error!() to return a more specific error (because otherwise you'd get something related to InnerClient being defined twice. I think the same applies if no feature is set (InnerClient would not be defined then, I think). For this, maybe we want to only include the tonic_client mod if one of these features is enabled?
One other option that I think could work (although maybe I'm forgetting something): Could we make the actual TonicRpcClient::client (client: Option<ApiClient<T>>) generic over T? Then we could have 2 constructors: one for the web channel and one for the std-compatible one, and we could add these constructors based on the features. This way, if no features are set or both features are set, constructors get added or removed and there are no compilation errors. I think this could be cleaner but as I mentioned, maybe I'm missing something.
tomyrd
force-pushed
the
tomyrd-missing-rpc-endpoints
branch
from
dd3b4e0 to
a293980
Compare
There was a problem hiding this comment.
LGTM! I still don't love the fact that we need to rely on feature flags this much in order to avoid falling into problems such as duplicate declarations, missing imports/exports, etc. However I do think that the deduplication is a net win here.
I briefly tried to generalize it without the feature flags but it becomes verbose too fast (hard to make generic over ApiClient because of the inner guards defined by tonic). So could we create an issue to keep this in the backlog as we merge this?
I also left a couple of minor comments.
crates/rust-client/README.md
Outdated
| @@ -18,7 +18,7 @@ miden-client = { version = "0.8" } | |||
| | `idxdb` | Includes `WebStore`, an IndexedDB implementation of the `Store` trait. **Disabled by default.** | | |||
| | `sqlite` | Includes `SqliteStore`, a SQLite implementation of the `Store` trait. This relies on the standard library. **Disabled by default.** | | |||
| | `tonic` | Includes `TonicRpcClient`, a Tonic client to communicate with Miden node. This relies on the standard library. **Disabled by default.** | | |||
| | `web-tonic` | Includes `WebTonicRpcClient`, a Tonic client to communicate with the Miden node in the browser. **Disabled by default.** | | |||
| | `web-tonic` | Includes a modified version of `TonicRpcClient`, a Tonic client to communicate with the Miden node updated so that it works in the browser. **Disabled by default.** | | |||
| | `testing` | Enables functions meant to be used in testing environments. **Disabled by default.** | | |||
|
|
|||
| Features `sqlite` and `idxdb` are mutually exclusive. | |||
There was a problem hiding this comment.
If we keep the current approach, I think we can add the fact that tonic and web-tonic are mutually exclusive as well.
crates/rust-client/README.md
Outdated
| @@ -18,7 +18,7 @@ miden-client = { version = "0.8" } | |||
| | `idxdb` | Includes `WebStore`, an IndexedDB implementation of the `Store` trait. **Disabled by default.** | | |||
| | `sqlite` | Includes `SqliteStore`, a SQLite implementation of the `Store` trait. This relies on the standard library. **Disabled by default.** | | |||
| | `tonic` | Includes `TonicRpcClient`, a Tonic client to communicate with Miden node. This relies on the standard library. **Disabled by default.** | | |||
| | `web-tonic` | Includes `WebTonicRpcClient`, a Tonic client to communicate with the Miden node in the browser. **Disabled by default.** | | |||
| | `web-tonic` | Includes a modified version of `TonicRpcClient`, a Tonic client to communicate with the Miden node updated so that it works in the browser. **Disabled by default.** | | |||
There was a problem hiding this comment.
nit: I would not mention that this is a "modified" version, since there is not a "standard" one. Rather, they are both different because they use different internal channels/transports (I don't know the actual tonic terminology). So maybe we can say that for the tonic feature flag we rely on an std-compatible transport, and for the web-tonic we rely on tonic_web_wasm_client as the inner transport.
closes #735