Adding client builder for simple initialization of the Rust client #741

partylikeits1983 · 2025-02-18T19:44:24Z

Adding client builder for simple initialization of the Rust client

Overview

This PR introduces a new client builder to the Rust client, streamlining the initialization process and reduces the amount of boilerplate code when getting started with a Miden dApp. With the new client builder, setting up a Miden dApp in Rust becomes significantly easier. Personally, this was one of the features I really wanted to have as I was building the tutorial code examples.

By integrating this feature, we lower the barrier to entry and reduce the lines of code needed to deploy and interact with smart contracts and notes. For context, our Miden dApp tutorials currently showcase the older, more verbose approach where the initialize_client() function is used in each code example.

This improvement was discussed internally in this issue.

What This Solves

Reduced Boilerplate: Eliminates the need for the repetitive initialize_client() function in the tutorial docs.
Simpler Onboarding: New users can now instantiate the client with minimal configuration.
Enhanced Flexibility: The builder allows for easy configuration of RPC endpoints, timeouts, store paths, and debug modes.
Reduces Lines of Code: The builder reduces the lines of code it takes to build a simple dApp on Miden using the rust client.

Usage

Initializing the client is now as simple as:

let mut client = Client::initialize()
    .with_rpc("https://rpc.testnet.miden.io:443")
    .in_debug_mode(true)
    .build()
    .await?;

Tests

I added a simple test of instantiating the client using the new client builder. I also tested the new client builder in a separate repository.

Future Work

There may be further improvements in the instantiation process. However, the primary goal here is to reduce the number of lines and the complexity required to get started with building on Miden.

Please let me know if I can make any improvements.

partylikeits1983 · 2025-02-18T19:49:27Z

crates/rust-client/src/tests.rs

Is it ok to use #[tokio::test] here? And is it ok that this test requires an internet connection to ping the testnet?

An integration test is probably better suited for tests that require syncing against the node. Integration tests are executed in the CI against a locally running node running the next branch. The builder itself may also probably be successfully tested with unit tests as well.

igamigo

Thanks for the PR! I think a builder could be useful as the client's instantiation API becomes more complex. I had made some comments a while ago about potentially introducing one, but it's probably a very stale comment.

In general we might not be able to provide "good" defaults (for the context of the tutorials, at least), because we need to be generic enough that the Client can be compiled for no_std environments, so not entirely sure if this would provide a ton of value for your use-case, but I still think it probably makes for a good addition.

igamigo · 2025-02-18T20:39:30Z

crates/rust-client/src/builder.rs

+use miden_objects::crypto::rand::RpoRandomCoin;
+use rand::Rng;
+
+pub struct ClientBuilder {


We should add doc comments to the struct and the fields.

With this, we might have to update library.md to reflect these changes as well.

Added doc comments. Let me know if this is the correct format.

igamigo · 2025-02-18T20:41:25Z

crates/rust-client/src/builder.rs

+    }
+
+    /// Sets the RPC endpoint via a URL.
+    pub fn with_rpc(mut self, url: &str) -> Self {


Could this not provide a generic dyn NodeRpcClient + Send? Because otheriwse if we hardcode it to a TonicRpcClient, then the std feature becomes a requirement and this means we cannot compile the client for wasm32

What about putting it behind a tonic feature flag? This would mean we could still compile for wasm32.

#[cfg(feature = "tonic")] pub fn with_rpc(mut self, url: &str) -> Self { ... }

I think we could add a ClientBuilder block within the TonicRpcClient that contains this method and we could avoid having the #[cfg(feature="std")] guard for this mod.
Could look something like this:

// (on rpc/tonic_client/mod.rs): impl ClientBuilder { #[must_use] pub fn with_tonic_rpc(mut self, url:&Endpoint, timeout: u64) -> Self { let tonic_client = TonicRpcClient::new(..,); self.with_rpc_client(Box::new(tonic_client)) } }

Maybe the same could be done for SQLite store. I think this might be a bit of an antipattern in Rust but it should work well here for making the builder available but also making it more ergonomic if certain features are turned on.

igamigo · 2025-02-18T20:42:35Z

crates/rust-client/src/builder.rs

+    }
+
+    /// Optionally set a custom store path.
+    pub fn with_store_path(mut self, path: &str) -> Self {


Similar to the previous comment, we should provide the store generically instead of always building a SqliteStore

We probably want to do the same for all components that were not defined here as well (like the RNG component)

I think the more generic method is with_store() right? I think one of the intentions of this method was it to make it configuring sqlite store as easy as possible. So, maybe the way to go there is to rename this into something like with_sqlite_store() so that it is clear that this method is intended to work specifically with sqlite store.

That makes sense. Will modify.

Ah yes, true. Agree with the rename. I'd also move it to the SQLite mod but maybe we could refactor it separately

crates/rust-client/src/lib.rs

igamigo · 2025-02-18T20:57:58Z

crates/rust-client/src/tests.rs

An integration test is probably better suited for tests that require syncing against the node. Integration tests are executed in the CI against a locally running node running the next branch. The builder itself may also probably be successfully tested with unit tests as well.

crates/rust-client/src/builder.rs

partylikeits1983 · 2025-02-19T01:47:19Z

Refactored the src/builder.rs to not use std.
Added the builder test to the integration test directory.
Fixed small nits regarding comments.

After adding tonic and Sqlite feature flags to certain parts of the code, the builder.rs can be compiled for wasm32

Compiled successfully when I ran this:

cargo build --manifest-path=crates/rust-client/Cargo.toml --target wasm32-unknown-unknown --no-default-features

partylikeits1983 · 2025-02-19T01:56:56Z

Testing again locally with my miden dApp testing repo, this PR makes client instantiation much cleaner!

Now it is possible to do this:

let mut client = Client::initialize()
    .with_rpc("https://rpc.testnet.miden.io:443")
    .in_debug_mode(true)
    .build()
    .await?;

Instead of this: https://github.com/partylikeits1983/miden-tx/blob/ad0e378687ce950078b0bccd62ca030ac0ff09fb/src/bin/ephemeral_notes.rs#L26-L53

SantiagoPittella · 2025-02-19T13:45:21Z

crates/rust-client/src/builder.rs

+        // Determine the scheme and strip it from the URL.
+        let (scheme, rest) = if let Some(stripped) = url.strip_prefix("https://") {
+            ("https", stripped)
+        } else if let Some(stripped) = url.strip_prefix("http://") {
+            ("http", stripped)
+        } else {
+            ("https", url)
+        };
+
+        // Attempt to find a colon indicating a port.
+        let (host, port) = if let Some(colon_index) = rest.find(':') {
+            // Split the host and port.
+            let host = &rest[..colon_index];
+            let port_str = &rest[colon_index + 1..];
+            // Try parsing the port. If it fails, use None.
+            let port = port_str.parse::<u16>().ok();
+            (host.to_string(), port)
+        } else {
+            // No colon found, so use the entire string as the host and no port.
+            (rest.to_string(), None)
+        };
+
+        // Create the endpoint using the parsed scheme, host, and port.
+        let endpoint = Endpoint::new(scheme.to_string(), host, port);


It might be possible to replace this with Endpoint::try_from(&str).

I think so too, this function could even receive a built Endpoint object. This way the user can use the known endpoints (like Endpoint::testnet()).

Yes it does! Will fix.

tomyrd · 2025-02-19T15:07:04Z

crates/rust-client/src/builder.rs

+        // Determine the scheme and strip it from the URL.
+        let (scheme, rest) = if let Some(stripped) = url.strip_prefix("https://") {
+            ("https", stripped)
+        } else if let Some(stripped) = url.strip_prefix("http://") {
+            ("http", stripped)
+        } else {
+            ("https", url)
+        };
+
+        // Attempt to find a colon indicating a port.
+        let (host, port) = if let Some(colon_index) = rest.find(':') {
+            // Split the host and port.
+            let host = &rest[..colon_index];
+            let port_str = &rest[colon_index + 1..];
+            // Try parsing the port. If it fails, use None.
+            let port = port_str.parse::<u16>().ok();
+            (host.to_string(), port)
+        } else {
+            // No colon found, so use the entire string as the host and no port.
+            (rest.to_string(), None)
+        };
+
+        // Create the endpoint using the parsed scheme, host, and port.
+        let endpoint = Endpoint::new(scheme.to_string(), host, port);


I think so too, this function could even receive a built Endpoint object. This way the user can use the known endpoints (like Endpoint::testnet()).

tomyrd · 2025-02-19T15:07:58Z

crates/rust-client/src/builder.rs

+        self.rpc_api = Some(Box::new(TonicRpcClient::new(endpoint, self.timeout_ms)));
+        self


nit

Suggested change

self.rpc_api = Some(Box::new(TonicRpcClient::new(endpoint, self.timeout_ms)));

self

self.with_rpc_api(Box::new(TonicRpcClient::new(endpoint, self.timeout_ms))));

I think so too, this function could even receive a built Endpoint object. This way the user can use the known endpoints (like Endpoint::testnet()).

The only thing about doing this is that it might make initializing the client take one extra line of code in a Miden app.

Would it mean doing this?

let endpoint = Endpoint::testnet(); let mut client = Client::initialize() .with_rpc(endpoint) .in_debug_mode(true) .build() .await?;

You could create the endpoint inside the function call. Like this:

let mut client = Client::initialize() .with_rpc(Endpoint::testnet()) .in_debug_mode(true) .build() .await?;

And if the user wants a custom endpoint they can build it from a string like the old interface:

let mut client = Client::initialize() .with_rpc(Endpoint::try_from("https://test:123")?) .in_debug_mode(true) .build() .await?;

Ok this makes sense. Will change.

tomyrd · 2025-02-19T15:12:32Z

crates/rust-client/src/builder.rs

+    /// Optionally set a custom timeout (in ms).
+    pub fn with_timeout(mut self, timeout_ms: u64) -> Self {
+        self.timeout_ms = timeout_ms;
+        self
+    }


We would need to specify that this function should be called before with_rpc (if not, the parameter would be ignored). I think it would be even better if we move the timeout_ms parameter to the with_rpc function. The parameter could be optional if we want to allow for a default timeout.

tomyrd · 2025-02-19T15:18:16Z

crates/rust-client/src/builder.rs

+        // Set up the store.
+        // If the user provided a store, use it.
+        // Otherwise, if the `sqlite` feature is enabled, build one from the store_path.
+        // If not, return an error.
+        let arc_store: Arc<dyn Store> = {
+            #[cfg(feature = "sqlite")]
+            {
+                if let Some(store) = self.store {
+                    store
+                } else {
+                    let store = SqliteStore::new(self.store_path.into())
+                        .await
+                        .map_err(ClientError::StoreError)?;
+                    Arc::new(store)
+                }
+            }
+            #[cfg(not(feature = "sqlite"))]
+            {
+                self.store.ok_or_else(|| {
+                    ClientError::ClientInitializationError(
+                        "No store provided and the sqlite feature is disabled.".into(),
+                    )
+                })?
+            }
+        };


I think the store could be instantiated in with_store_path. Similar to what we do with the rpc.

tomyrd · 2025-02-19T15:22:36Z

crates/rust-client/src/builder.rs

+        };
+
+        // Create the authenticator.
+        let authenticator = StoreAuthenticator::new_with_rng(arc_store.clone(), rng.clone());


After merging #718, the authenticator now needs a KeyStore to be built, this keystore would need to be created externally and not by the builder (because the user of the client needs the keystore to add new keys).

We can probably add a with_keystore function to the builder and use the provided keystore here. It should fail if no keystore is defined as there's no clear default implementation (one is std and the other no-std and they don't share parameters).

Nevermind, this PR is going to main so those changes aren't relevant here. We will need to merge these changes to next when this gets merged so we can add this.

@tomyrd I can rebase this to the next branch if it makes things easier. Would be useful to have this feature on 0.7, but if it's too complicated can rebase to next

Thanks! But I also think it's best for 0.7, it won't be difficult to bring these changes to next once they get merged. I was just mentioning it so we don't forget.

Just saw this. @igamigo told me to rebase to next. Not an urgent change or anything. Rebasing to next means the PR can be ironed out a bit more.

Perfect! Let me know if you have questions about the new authenticator

tomyrd · 2025-02-19T15:28:18Z

crates/rust-client/src/lib.rs

+
+// BUILDER ENTRY POINT (OPTIONAL HELPER)
+// --------------------------------------------------------------------------------------------
+
+impl Client<RpoRandomCoin> {
+    pub fn initialize() -> ClientBuilder {
+        ClientBuilder::new()
+    }
+}


Do we want this helper? I think it may hide the fact that we are using an underlying ClientBuilder and not a Client to initialize. We may want them to be separate (like the TransactionRequest and TransactionRequestBuilder).

I agree, is there any benefit to this helper?

I see now, definitely makes sense to remove it.

partylikeits1983 · 2025-02-19T18:00:22Z

Rebased this PR from main to next.

Also I made the entire client builder abstraction to only compile when std is available.

crates/rust-client/src/builder.rs

bobbinth

Looks great! Thank you for doing this. I left one small comment above.

Also, what needs to be done to make the CI green?

igamigo

Thanks! Leaving some more comments for now. As @bobbinth mentioned, the CI checks should be reviewed. Upon a quick look, errors seem quite simple. You can check them locally using the make targets (make clippy, make clippy-wasm, make fix for applying superficial fixes, etc.). Feel free to let me know if you have any questions about any of this

igamigo · 2025-02-20T01:56:55Z

crates/rust-client/src/builder.rs

+    /// An optional store provided by the user.
+    store: Option<Arc<dyn Store>>,
+    /// An optional RNG provided by the user.
+    rng: Option<RpoRandomCoin>,


Could this be generic as well? There might not be a lot of value in forcing an RpoRandomCoin, but I could be wrong.

Would this mean that when initializing the client the user will have to specify type annotations for the client builder?

I implemented a basic version making RpoRandomCoin generic, but it requires that I specify type annotations when initializing the client.

This is how it looks:

let mut client = ClientBuilder::<RpoRandomCoin>::new() .with_rpc(Endpoint::try_from("https://rpc.testnet.miden.io:443").unwrap()) .with_timeout(10_000) .with_sqlite_store("store.sqlite3") .in_debug_mode(true) .build() .await?;

Maybe there is a way to make RpoRandomCoin generic and not have to specify the type annotation?

igamigo · 2025-02-20T01:57:30Z

crates/rust-client/src/builder.rs

@@ -0,0 +1,155 @@
+#![cfg(feature = "std")]


Usually this would be in the mod declaration (ie, in lib.rs).

EDIT: Just saw you added this there, so this can be removed.

igamigo · 2025-02-20T01:58:02Z

crates/rust-client/src/builder.rs

+    }
+
+    /// Sets the RPC client directly (for custom NodeRpcClient implementations).
+    pub fn with_rpc_client(mut self, client: Box<dyn NodeRpcClient + Send>) -> Self {


nit: We could add #[must_use] to these functions.

igamigo · 2025-02-20T02:01:16Z

crates/rust-client/src/lib.rs

+
+// BUILDER ENTRY POINT (OPTIONAL HELPER)
+// --------------------------------------------------------------------------------------------
+
+impl Client<RpoRandomCoin> {
+    pub fn initialize() -> ClientBuilder {
+        ClientBuilder::new()
+    }
+}


I agree, is there any benefit to this helper?

igamigo · 2025-02-20T02:05:51Z

crates/rust-client/src/builder.rs

+    }
+
+    /// Sets the RPC endpoint via a URL.
+    pub fn with_rpc(mut self, url: &str) -> Self {


I think we could add a ClientBuilder block within the TonicRpcClient that contains this method and we could avoid having the #[cfg(feature="std")] guard for this mod.
Could look something like this:

// (on rpc/tonic_client/mod.rs): impl ClientBuilder { #[must_use] pub fn with_tonic_rpc(mut self, url:&Endpoint, timeout: u64) -> Self { let tonic_client = TonicRpcClient::new(..,); self.with_rpc_client(Box::new(tonic_client)) } }

igamigo · 2025-02-20T02:08:08Z

crates/rust-client/src/builder.rs

+    }
+
+    /// Sets the RPC endpoint via a URL.
+    pub fn with_rpc(mut self, url: &str) -> Self {


Maybe the same could be done for SQLite store. I think this might be a bit of an antipattern in Rust but it should work well here for making the builder available but also making it more ergonomic if certain features are turned on.

partylikeits1983 · 2025-02-20T15:35:04Z

Addressed several comments from above.

Remove the helper from lib.rs
Added method to set the keystore
Addressed clippy & CI warnings
.with_rpc() now takes an Endpoint type
.with_timeout() can be called in any order before .build()

bobbinth

Looks good! Thank you! It seems like the integration tests are not passing - but maybe that's something can be fixed rebasing from next?

igamigo · 2025-02-20T19:16:30Z

Looks good! Thank you! It seems like the integration tests are not passing - but maybe that's something can be fixed rebasing from next?

Yes, I think this may be the case.

I'd also see if this could be addressed before merging.

tomyrd

LGTM! Left a small comment to be addressed before merging.

tomyrd · 2025-02-20T21:37:17Z

crates/rust-client/src/builder.rs

+            None => FilesystemKeyStore::new(temp_dir())
+                .map_err(|err| ClientError::ClientInitializationError(err.to_string()))?,


Should we fail if the keystore is not set? If not, I wouldn't use the temp_dir to store the keys, it might get cleared up.

Ah yes that is true. What would be a good reasonable default? I think I was following the cli tests as a guide when writing this, that's why I use temp_dir().

In the CLI we create a "keystore" dir on the current directory and use that.

But in this case, I feel like we should have it so that the user always provides the keystore themselves. My reasoning is that if we create the keystore in the builder, then the user won't have a reference to it. This means that when adding new accounts, the user won't be able to add the authentication keys to the keystore.

Would this make sense as a standard use example? This creates a /keystore directory.

let keystore = FilesystemKeyStore::new("keystore".into()).unwrap(); let mut client = ClientBuilder::new() .with_rpc(Endpoint::try_from("https://rpc.testnet.miden.io:443").unwrap()) .with_timeout(10_000) .with_keystore(keystore) .with_sqlite_store("store.sqlite3") .in_debug_mode(true) .build() .await?;

This way the user has reference to the keystore.

@tomyrd @igamigo Or could the .with_keystore() method take a string that is the name of the directory? Just trying to reduce the # lines it takes and make it as visually simple as possible to set up the client. What do y'all think?

If we make it so that the with_keystore() method takes a string, then we would have to change it so that the builder also returns the keystore it created internally, something like:

let (mut client, mut keystore) = ClientBuilder::new() .with_rpc(Endpoint::try_from("https://rpc.testnet.miden.io:443").unwrap()) .with_timeout(10_000) .with_keystore("keystore") .with_sqlite_store("store.sqlite3") .in_debug_mode(true) .build() .await?;

I'm open to either one but I think I prefer the first one.

I think one of the goals here is to make assumptions about a reasonable set of defaults while also keeping some flexibility. So, I'd probably go with more specialized methods that make ergonomics a bit better. For example:

let mut client = ClientBuilder::new() .with_tonic_rpc("https://rpc.testnet.miden.io:443") .with_timeout(10_000) .with_filesystem_keystore("keystore") .with_sqlite_store("store.sqlite3") .in_debug_mode(true) .build() .await?;

Ff the user wanted to customize things beyond these defaults, they could also do something like:

let mut client = ClientBuilder::new() .with_rpc(my_rpc) .with_keystore(my_keystore) .with_store(my_store) .in_debug_mode(true) .build() .await?;

The only thing which I think doesn't fit this pattern would be the random coin. But maybe we keep it as is for now and make it more flexible in another PR.

SantiagoPittella

Overall looks good, I think that after addressing https://github.com/0xPolygonMiden/miden-client/pull/741/files#r1962738199 and https://github.com/0xPolygonMiden/miden-client/pull/741/files#r1960564185 this can be ready to merge.

partylikeits1983 · 2025-02-21T20:35:25Z

Addressed several comments from above.

keystore must be specified during build phase.
ClientBuilder now has 2 methods for setting the keystore: .with_keystore() and .with_filesystem_keystore()

The following is possible:

let mut client = ClientBuilder::new()
    .with_tonic_rpc("https://rpc.testnet.miden.io:443")
    .with_timeout(10_000)
    .with_filesystem_keystore("keystore")
    .with_sqlite_store("store.sqlite3")
    .in_debug_mode(true)
    .build()
    .await?;

The .with_keystore() method allows the user to directly set the keystore.

partylikeits1983 · 2025-02-21T20:46:23Z

Force pushed to keep everything in a single commit & to fix CI/CD issues.

partylikeits1983 · 2025-02-21T20:49:01Z

@igamigo Regarding this: https://github.com/0xPolygonMiden/miden-client/pull/741/files#r1962738199

Would it be possible to address this in a separate PR? I tried implementing it but I believe it could get a bit complicated. Also not 100% sure how to make RpoRandomCoin generic without having to specify the type in the ClientBuilder, which means my initial implementation made things look like this:

let mut client = ClientBuilder::<RpoRandomCoin>::new();

bobbinth

Looks good! Thank you! I left a coupe of optional comments inline.

Beyond these, I think we can make a couple of improvements in the future:

Make handling of KeyStore more generic (so that we could use non-FilesystemKeyStores as well).
Make handling of Rng more generic (so that we could use non-RpoRandomCoin as well).

But I would do it in a separate PR - so, let's create an issue for this.

crates/rust-client/src/builder.rs

bobbinth · 2025-02-22T21:42:27Z

crates/rust-client/src/builder.rs

+    pub fn with_rpc(mut self, endpoint: Endpoint) -> Self {
+        self.rpc_endpoint = Some(endpoint);
+        self
+    }
+
+    /// Sets a custom RPC client directly.
+    ///
+    /// This method overrides any previously set RPC endpoint.
+    #[must_use]
+    pub fn with_rpc_client(mut self, client: Box<dyn NodeRpcClient + Send>) -> Self {


nit: I would maybe rename the current with_rpc_client() into just with_rpc(), and the current with_rpc() into with_tonic_rpc().

Also, with_tonic_rpc() could probably take a string rather than Endpoint as an argument - or is there a reason not to do this?

Also, with_tonic_rpc() could probably take a string rather than Endpoint as an argument - or is there a reason not to do this?

The reason why the with_rpc() function takes an Endpoint input was because I was following @tomyrd suggestion here:

I think so too, this function could even receive a built Endpoint object. This way the user can use the known endpoints (like Endpoint::testnet())
#741 (comment)

I do like the idea of just passing in the RPC url string as it is cleaner, but maybe it is useful to be able to initialize the endpoing like: Endpoint::testnet()

Yep, makes sense. I'm fine with either approach.

btw, another option is to use something like AsRef<str> or maybe Into<String> - e.g.:

pub fn with_tonic_rpc(mut self, url: impl Into<String>) -> Self { ... }

Then, if we have From<Endpoint> for String implemented, we'd be able to pass it into the method as well. We would need to change the internal field type of rpc_endpoint to String, but I don't think this should be a problem.

igamigo · 2025-02-24T15:09:11Z

@igamigo Regarding this: https://github.com/0xPolygonMiden/miden-client/pull/741/files#r1962738199

Would it be possible to address this in a separate PR? I tried implementing it but I believe it could get a bit complicated. Also not 100% sure how to make RpoRandomCoin generic without having to specify the type in the ClientBuilder, which means my initial implementation made things look like this:
let mut client = ClientBuilder::<RpoRandomCoin>::new();

Sure! I think we could make the RNG a trait object. In any case, because the RNG component is necessary, it could be an argument of the new() function. It could look something like ClientBuilder::new(rng: R) and then the type can be inferred and you wouldn't need to specify the type within brackets.

BTW the std feature can be enabled without (for example) the tonic feature. In this case the build will probably fail because the tonic client will not be present but the builder will. We should tacke it as explained here. Let's add this to the issue alongside @bobbinth 's points.

EDIT: Also, seems like you should rebase from the latest next

partylikeits1983 · 2025-02-24T18:24:49Z

Changed the name of the function with_rpc_client() to with_tonic_client() as suggested by @bobbinth
Added a test for using the ClientBuilder with with_tonic_client()
Rebased to latest next branch updates.

There are a few outstanding issues that I can address in a separate PR:

Modifying rpc/tonic_client/mod.rs to avoid having the #[cfg(feature="std")] guard for the ClientBuilder mod
Make handling of KeyStore more generic (so that we could use non-FilesystemKeyStores as well).
Make handling of Rng more generic (so that we could use non-RpoRandomCoin as well).

partylikeits1983 · 2025-02-24T19:25:48Z

Opened an issue for a separate PR to improve the ClientBuilder implementation. #759

bobbinth · 2025-02-24T22:10:04Z

crates/rust-client/src/builder.rs

+    pub fn with_rpc(mut self, endpoint: Endpoint) -> Self {
+        self.rpc_endpoint = Some(endpoint);
+        self
+    }
+
+    /// Sets a custom RPC client directly.
+    ///
+    /// This method overrides any previously set RPC endpoint.
+    #[must_use]
+    pub fn with_tonic_rpc(mut self, client: Box<dyn NodeRpcClient + Send>) -> Self {


I think the naming here is reversed. It should be:

pub fn with_rpc(mut self, client: Box<dyn NodeRpcClient + Send>) -> Self { ... } pub fn with_tonic_rpc(mut self, endpoint: Endpoint) -> Self { ... }

The idea is that with_tonic_rpc() is more specialized - i.e., we know that we'll be using the TonicRpcClient.

That makes sense. I changed the naming of the functions.

igamigo · 2025-02-25T16:49:48Z

Unused dependencies check was fixed on #761 so we can merge this safely

bobbinth requested a review from igamigo February 18, 2025 19:47

partylikeits1983 commented Feb 18, 2025

View reviewed changes

igamigo reviewed Feb 18, 2025

View reviewed changes

SantiagoPittella reviewed Feb 19, 2025

View reviewed changes

tomyrd reviewed Feb 19, 2025

View reviewed changes

partylikeits1983 changed the base branch from main to next February 19, 2025 17:54

partylikeits1983 force-pushed the main branch from 8fe9f66 to 7c9ae64 Compare February 19, 2025 17:59

bobbinth reviewed Feb 20, 2025

View reviewed changes

crates/rust-client/src/builder.rs Show resolved Hide resolved

bobbinth reviewed Feb 20, 2025

View reviewed changes

igamigo reviewed Feb 20, 2025

View reviewed changes

partylikeits1983 force-pushed the main branch from 5a97e01 to 025efcf Compare February 20, 2025 16:00

bobbinth approved these changes Feb 20, 2025

View reviewed changes

tomyrd approved these changes Feb 20, 2025

View reviewed changes

SantiagoPittella reviewed Feb 21, 2025

View reviewed changes

partylikeits1983 force-pushed the main branch from 025efcf to 98ab566 Compare February 21, 2025 20:31

partylikeits1983 force-pushed the main branch 3 times, most recently from 5eb3859 to f686789 Compare February 21, 2025 20:44

bobbinth approved these changes Feb 22, 2025

View reviewed changes

partylikeits1983 force-pushed the main branch from f686789 to 8752e9b Compare February 24, 2025 18:21

partylikeits1983 mentioned this pull request Feb 24, 2025

Refactor & improve ClientBuilder #759

Open

igamigo approved these changes Feb 24, 2025

View reviewed changes

bobbinth reviewed Feb 24, 2025

View reviewed changes

feat: add client builder

2784861

partylikeits1983 force-pushed the main branch from 8752e9b to 2784861 Compare February 24, 2025 22:39

tomyrd approved these changes Feb 25, 2025

View reviewed changes

Merge branch 'next' into main

e16c6d2

igamigo merged commit d957e87 into 0xPolygonMiden:next Feb 25, 2025
12 of 13 checks passed

		self.rpc_api = Some(Box::new(TonicRpcClient::new(endpoint, self.timeout_ms)));
		self

	self.rpc_api = Some(Box::new(TonicRpcClient::new(endpoint, self.timeout_ms)));
	self
	self.with_rpc_api(Box::new(TonicRpcClient::new(endpoint, self.timeout_ms))));

		None => FilesystemKeyStore::new(temp_dir())
		.map_err(\|err\| ClientError::ClientInitializationError(err.to_string()))?,

Adding client builder for simple initialization of the Rust client #741

Adding client builder for simple initialization of the Rust client #741

Conversation

partylikeits1983 commented Feb 18, 2025 • edited Loading