From afaa32ff2e36c4eeff9bb4ec1380b1b0f9274c94 Mon Sep 17 00:00:00 2001 From: tomyrd Date: Mon, 26 May 2025 18:23:36 -0300 Subject: [PATCH 1/4] feat: add `Send+Sync` markers and tests to dyn traits --- crates/rust-client/src/rpc/mod.rs | 6 +-- .../rust-client/src/rpc/tonic_client/mod.rs | 45 ++++++++++++++++--- crates/rust-client/src/store/mod.rs | 4 +- .../rust-client/src/store/sqlite_store/mod.rs | 28 +++++++++++- crates/rust-client/src/test_utils/mock.rs | 4 +- 5 files changed, 71 insertions(+), 16 deletions(-) diff --git a/crates/rust-client/src/rpc/mod.rs b/crates/rust-client/src/rpc/mod.rs index c94549ab56..2c80f5c9cf 100644 --- a/crates/rust-client/src/rpc/mod.rs +++ b/crates/rust-client/src/rpc/mod.rs @@ -42,7 +42,6 @@ use alloc::{boxed::Box, collections::BTreeSet, string::String, vec::Vec}; use core::fmt; -use async_trait::async_trait; use domain::{ account::{AccountDetails, AccountProofs}, note::{NetworkNote, NoteSyncInfo}, @@ -93,8 +92,9 @@ use crate::{ /// The implementers are responsible for connecting to the Miden node, handling endpoint /// requests/responses, and translating responses into domain objects relevant for each of the /// endpoints. -#[async_trait(?Send)] -pub trait NodeRpcClient { +#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] +#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] +pub trait NodeRpcClient: Send + Sync { /// Given a Proven Transaction, send it to the node for it to be included in a future block /// using the `/SubmitProvenTransaction` RPC endpoint. async fn submit_proven_transaction( diff --git a/crates/rust-client/src/rpc/tonic_client/mod.rs b/crates/rust-client/src/rpc/tonic_client/mod.rs index 07c2b2fa5c..3fae9bfaae 100644 --- a/crates/rust-client/src/rpc/tonic_client/mod.rs +++ b/crates/rust-client/src/rpc/tonic_client/mod.rs @@ -1,4 +1,3 @@ -#![allow(clippy::await_holding_lock)] use alloc::{ boxed::Box, collections::{BTreeMap, BTreeSet}, @@ -6,7 +5,6 @@ use alloc::{ vec::Vec, }; -use async_trait::async_trait; use miden_objects::{ Digest, account::{Account, AccountCode, AccountDelta, AccountId}, @@ -72,16 +70,18 @@ impl TonicRpcClient { /// Takes care of establishing the RPC connection if not connected yet. It ensures that the /// `rpc_api` field is initialized and returns a write guard to it. async fn ensure_connected(&self) -> Result { - let mut client = self.client.write(); - if client.is_none() { - client.replace(ApiClient::new_client(self.endpoint.clone(), self.timeout_ms).await?); + if self.client.read().is_none() { + let new_client = ApiClient::new_client(self.endpoint.clone(), self.timeout_ms).await?; + let mut client = self.client.write(); + client.replace(new_client); } - Ok(client.as_ref().expect("rpc_api should be initialized").clone()) + Ok(self.client.read().as_ref().expect("rpc_api should be initialized").clone()) } } -#[async_trait(?Send)] +#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] +#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] impl NodeRpcClient for TonicRpcClient { async fn submit_proven_transaction( &self, @@ -488,3 +488,34 @@ impl NodeRpcClient for TonicRpcClient { Ok(block) } } + +#[cfg(test)] +mod tests { + use std::boxed::Box; + + use super::TonicRpcClient; + use crate::rpc::{Endpoint, NodeRpcClient}; + + fn assert_send_sync() {} + #[test] + fn send_sync() { + assert_send_sync::(); + assert_send_sync::>(); + } + + async fn fn_dyn_trait(client: Box) { + // This wouldn't compile if `get_block_header_by_number` doesn't return a `Send+Sync` + // future. This only tests one method but that might still be enough to prove that + // it's possible + let res = client.get_block_header_by_number(None, false).await; + assert!(res.is_ok()); + } + + #[tokio::test] + async fn send_it() { + let endpoint = &Endpoint::devnet(); + let client = TonicRpcClient::new(endpoint, 10000); + let client: Box = client.into(); + tokio::task::spawn(async move { fn_dyn_trait(client).await }); + } +} diff --git a/crates/rust-client/src/store/mod.rs b/crates/rust-client/src/store/mod.rs index d5aead901e..5357273e2e 100644 --- a/crates/rust-client/src/store/mod.rs +++ b/crates/rust-client/src/store/mod.rs @@ -27,7 +27,6 @@ use alloc::{ }; use core::fmt::Debug; -use async_trait::async_trait; use miden_objects::{ Digest, Word, account::{Account, AccountCode, AccountHeader, AccountId}, @@ -83,7 +82,8 @@ pub use note_record::{ /// Because the [`Store`]'s ownership is shared between the executor and the client, interior /// mutability is expected to be implemented, which is why all methods receive `&self` and /// not `&mut self`. -#[async_trait(?Send)] +#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] +#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] pub trait Store: Send + Sync { /// Returns the current timestamp tracked by the store, measured in non-leap seconds since /// Unix epoch. If the store implementation is incapable of tracking time, it should return diff --git a/crates/rust-client/src/store/sqlite_store/mod.rs b/crates/rust-client/src/store/sqlite_store/mod.rs index 7495d20ffe..dcf20ef693 100644 --- a/crates/rust-client/src/store/sqlite_store/mod.rs +++ b/crates/rust-client/src/store/sqlite_store/mod.rs @@ -100,7 +100,7 @@ impl SqliteStore { // // To simplify, all implementations rely on inner SqliteStore functions that map 1:1 by name // This way, the actual implementations are grouped by entity types in their own sub-modules -#[async_trait(?Send)] +#[async_trait] impl Store for SqliteStore { fn get_current_timestamp(&self) -> Option { let now = chrono::Utc::now(); @@ -358,8 +358,32 @@ pub fn u64_to_value(v: u64) -> Value { #[cfg(test)] pub mod tests { + use std::boxed::Box; + use super::SqliteStore; - use crate::tests::create_test_store_path; + use crate::{store::Store, tests::create_test_store_path}; + + fn assert_send_sync() {} + #[test] + fn send_sync() { + assert_send_sync::(); + assert_send_sync::>(); + } + + async fn fn_dyn_trait(store: Box) { + // This wouldn't compile if `get_tracked_block_headers` doesn't return a `Send+Sync` + // future. This only tests one method but that might still be enough to prove that + // it's possible + let res = store.get_tracked_block_headers().await; + assert!(res.is_ok()); + } + + #[tokio::test] + async fn send_it() { + let client = SqliteStore::new(create_test_store_path()).await.unwrap(); + let client: Box = client.into(); + tokio::task::spawn(async move { fn_dyn_trait(client).await }); + } pub(crate) async fn create_test_store() -> SqliteStore { SqliteStore::new(create_test_store_path()).await.unwrap() diff --git a/crates/rust-client/src/test_utils/mock.rs b/crates/rust-client/src/test_utils/mock.rs index 34d422c152..3d22b3a2af 100644 --- a/crates/rust-client/src/test_utils/mock.rs +++ b/crates/rust-client/src/test_utils/mock.rs @@ -1,6 +1,5 @@ use alloc::{collections::BTreeSet, sync::Arc, vec::Vec}; -use async_trait::async_trait; use miden_lib::transaction::TransactionKernel; use miden_objects::{ Digest, Felt, @@ -210,7 +209,8 @@ impl MockRpcApi { } } use alloc::boxed::Box; -#[async_trait(?Send)] +#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] +#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] impl NodeRpcClient for MockRpcApi { async fn sync_notes( &self, From 9a0f5e9d86416264b7819e303a702d69343c5ba3 Mon Sep 17 00:00:00 2001 From: sergerad Date: Wed, 28 May 2025 06:33:40 +1200 Subject: [PATCH 2/4] Refactor tests and remove explicit Send/Sync bounds from trait objects --- .../rust-client/src/rpc/tonic_client/mod.rs | 19 +++++++++---------- .../rust-client/src/store/sqlite_store/mod.rs | 19 +++++++++---------- 2 files changed, 18 insertions(+), 20 deletions(-) diff --git a/crates/rust-client/src/rpc/tonic_client/mod.rs b/crates/rust-client/src/rpc/tonic_client/mod.rs index 3fae9bfaae..dc7b88bc11 100644 --- a/crates/rust-client/src/rpc/tonic_client/mod.rs +++ b/crates/rust-client/src/rpc/tonic_client/mod.rs @@ -491,31 +491,30 @@ impl NodeRpcClient for TonicRpcClient { #[cfg(test)] mod tests { - use std::boxed::Box; - use super::TonicRpcClient; use crate::rpc::{Endpoint, NodeRpcClient}; + use std::boxed::Box; fn assert_send_sync() {} + #[test] - fn send_sync() { + fn is_send_sync() { assert_send_sync::(); - assert_send_sync::>(); + assert_send_sync::>(); } - async fn fn_dyn_trait(client: Box) { - // This wouldn't compile if `get_block_header_by_number` doesn't return a `Send+Sync` - // future. This only tests one method but that might still be enough to prove that - // it's possible + // Function that returns a `Send` future from a dynamic trait that must be `Sync`. + async fn dyn_trait_send_fut(client: Box) { + // This won't compile if `get_block_header_by_number` doesn't return a `Send+Sync` future. let res = client.get_block_header_by_number(None, false).await; assert!(res.is_ok()); } #[tokio::test] - async fn send_it() { + async fn future_is_send() { let endpoint = &Endpoint::devnet(); let client = TonicRpcClient::new(endpoint, 10000); let client: Box = client.into(); - tokio::task::spawn(async move { fn_dyn_trait(client).await }); + tokio::task::spawn(async move { dyn_trait_send_fut(client).await }); } } diff --git a/crates/rust-client/src/store/sqlite_store/mod.rs b/crates/rust-client/src/store/sqlite_store/mod.rs index dcf20ef693..3c1f47fac3 100644 --- a/crates/rust-client/src/store/sqlite_store/mod.rs +++ b/crates/rust-client/src/store/sqlite_store/mod.rs @@ -358,31 +358,30 @@ pub fn u64_to_value(v: u64) -> Value { #[cfg(test)] pub mod tests { - use std::boxed::Box; - use super::SqliteStore; use crate::{store::Store, tests::create_test_store_path}; + use std::boxed::Box; fn assert_send_sync() {} + #[test] - fn send_sync() { + fn is_send_sync() { assert_send_sync::(); - assert_send_sync::>(); + assert_send_sync::>(); } - async fn fn_dyn_trait(store: Box) { - // This wouldn't compile if `get_tracked_block_headers` doesn't return a `Send+Sync` - // future. This only tests one method but that might still be enough to prove that - // it's possible + // Function that returns a `Send` future from a dynamic trait that must be `Sync`. + async fn dyn_trait_send_fut(store: Box) { + // This wouldn't compile if `get_tracked_block_headers` doesn't return a `Send` future. let res = store.get_tracked_block_headers().await; assert!(res.is_ok()); } #[tokio::test] - async fn send_it() { + async fn future_is_send() { let client = SqliteStore::new(create_test_store_path()).await.unwrap(); let client: Box = client.into(); - tokio::task::spawn(async move { fn_dyn_trait(client).await }); + tokio::task::spawn(async move { dyn_trait_send_fut(client).await }); } pub(crate) async fn create_test_store() -> SqliteStore { From cdccdaa20cb5411a2259bc620b24db61e96da86e Mon Sep 17 00:00:00 2001 From: sergerad Date: Wed, 28 May 2025 06:35:47 +1200 Subject: [PATCH 3/4] Lint --- crates/rust-client/src/rpc/tonic_client/mod.rs | 3 ++- crates/rust-client/src/store/sqlite_store/mod.rs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/rust-client/src/rpc/tonic_client/mod.rs b/crates/rust-client/src/rpc/tonic_client/mod.rs index dc7b88bc11..1bc9beb2ee 100644 --- a/crates/rust-client/src/rpc/tonic_client/mod.rs +++ b/crates/rust-client/src/rpc/tonic_client/mod.rs @@ -491,9 +491,10 @@ impl NodeRpcClient for TonicRpcClient { #[cfg(test)] mod tests { + use std::boxed::Box; + use super::TonicRpcClient; use crate::rpc::{Endpoint, NodeRpcClient}; - use std::boxed::Box; fn assert_send_sync() {} diff --git a/crates/rust-client/src/store/sqlite_store/mod.rs b/crates/rust-client/src/store/sqlite_store/mod.rs index 3c1f47fac3..6753e36947 100644 --- a/crates/rust-client/src/store/sqlite_store/mod.rs +++ b/crates/rust-client/src/store/sqlite_store/mod.rs @@ -358,9 +358,10 @@ pub fn u64_to_value(v: u64) -> Value { #[cfg(test)] pub mod tests { + use std::boxed::Box; + use super::SqliteStore; use crate::{store::Store, tests::create_test_store_path}; - use std::boxed::Box; fn assert_send_sync() {} From 87df3e2d9e0ca2c346b8218dafa1c3998406690d Mon Sep 17 00:00:00 2001 From: sergerad Date: Wed, 28 May 2025 12:54:44 +1200 Subject: [PATCH 4/4] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ff5a1ebca..68143e5933 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ * Added support for RPC client/server version matching through HTTP ACCEPT header (#912). * Added a way to ignore invalid input notes when consuming them in a transaction (#898). * Added `NoteUpdate` type to the note update tracker to distinguish between different types of updates (#821). +* Updated `TonicRpcClient` and `Store` traits to be subtraits of `Send` and `Sync` (#926). +* Updated `TonicRpcClient` and `Store` trait functions to return futures which are `Send` (#926). ### Changes