Skip to content

Conversation

@sergerad
Copy link
Contributor

@sergerad sergerad commented Jul 2, 2025

Context

Relates to #849.

We have various trait objects in our stack that cause Send/Sync problems downstream.

One such trait object is the Box<dyn FeltRng> used by Client and ClientBuilder.

We also hand roll ClientRng(Box<dyn FeltRng>) for a reason that eludes me at the moment. In any case its not required when moving to generics AFAICT.

Changes

  • Replace Box<dyn FeltRng> with generics.
  • Delete ClientRng.

Future Work

Consider moving the remaining trait objects of the Client struct to generics.

  • store: Arc<dyn Store>
  • rpc_api: Arc<dyn NodeRpcClient + Send>
  • authenticator: Option<Arc<dyn TransactionAuthenticator>>

@sergerad sergerad force-pushed the sergerad-felt-generic branch from 1dc9185 to c91abb1 Compare July 2, 2025 23:07
@sergerad sergerad added the no docs This PR does not require an update to documentation files label Jul 2, 2025
@sergerad
Copy link
Contributor Author

sergerad commented Jul 2, 2025

Oddly the docs already had an example without Box<>. So did not need to change docs/src/library.md for example.

@sergerad sergerad force-pushed the sergerad-felt-generic branch from db690d3 to 04e94da Compare July 2, 2025 23:35
/// RPC endpoint, store, RNG, and keystore. It is generic over the keystore type. By default, it
/// uses `FilesystemKeyStore<rand::rngs::StdRng>`.
pub struct ClientBuilder {
pub struct ClientBuilder<R: FeltRng = RpoRandomCoin> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to do away with this by requiring the R to be supplied during the build fn. But wasn't sure people would prefer those ergonomics.

@sergerad sergerad requested review from bobbinth, igamigo and tomyrd and removed request for igamigo July 3, 2025 02:30
@tomyrd
Copy link
Contributor

tomyrd commented Jul 4, 2025

We also hand roll ClientRng(Box) for a reason that eludes me at the moment. In any case its not required when moving to generics AFAICT.

Generics was the old way we used to do it but it was removed here.

I think I prefer the trait object approach but it's totally subjective.
CCing @igamigo @bobbinth for their opinion.

In any case we should be consistent, so if we want to go with the generics route we should also change the other trait objects. If we wanted to keep trait objects could we add + Sync to the ClientRng?

@igamigo
Copy link
Collaborator

igamigo commented Jul 4, 2025

We briefly discussed deferring this to the next release, mainly for making these changes cohesively and see how the requirements change with the upcoming VM API refactors.

@vakochetkov
Copy link
Contributor

vakochetkov commented Sep 28, 2025

Hi, @igamigo! Are there any plans to merge this PR? It would be much easier if we had the ability to use MidenClient in async Send + Sync contexts or at least Send + Mutex, e.g., with the Tokio runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no docs This PR does not require an update to documentation files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants