replication client #1174
replication client #1174
Conversation
WalkthroughThe changes in this pull request primarily enhance the functionality and structure of the XMTP API client across various modules. Key modifications include the introduction of asynchronous traits, the use of Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
insipx
force-pushed
the
insipx/replication-client
branch
4 times, most recently
from
24543d8 to
2b7c71e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (32)
xmtp_api_grpc/Cargo.toml (1)
17-17: Consider using a more specific version constraint.
While the dependency addition is appropriate, using a more specific version constraint (e.g., "0.1.74") would ensure better build reproducibility.
-async-trait = "0.1"
+async-trait = "0.1.74"xmtp_api_http/Cargo.toml (1)
19-19: Consider using workspace version management for async-trait.
While the addition of async-trait is appropriate for implementing async traits, consider managing its version through the workspace like other dependencies to ensure consistent versioning across the project.
-async-trait = "0.1"
+async-trait.workspace = truexmtp_mls/src/api/mod.rs (2)
Line range hint 43-48: Consider adding documentation for Arc usage
The constructor now accepts an Arc<ApiClient>, which implies shared ownership. Consider adding documentation to explain:
- Why
Arcis needed - Who is responsible for managing the lifecycle
- Any threading considerations
Would you like me to help draft the documentation comments?
34-37: Consider implementing Drop trait for cleanup
With shared ownership via Arc, consider implementing the Drop trait for ApiClientWrapper if any cleanup is needed when all references are dropped (e.g., closing connections, cleaning up resources).
xmtp_id/src/scw_verifier/remote_signature_verifier.rs (1)
20-22: LGTM: Constructor updated for Arc
The constructor correctly accepts Arc<C>, making thread-safety requirements explicit at the API level.
Consider adding a doc comment to explicitly state the thread-safety guarantees:
+/// Creates a new RemoteSignatureVerifier with thread-safe shared ownership of the identity client.
pub fn new(identity_client: Arc<C>) -> Self {
Self { identity_client }
}examples/cli/serializable.rs (1)
Line range hint 25-49: Consider improving error handling.
The implementation uses multiple expect calls which will panic on errors. Consider proper error handling:
- pub async fn from<ApiClient: XmtpApi>(group: &MlsGroup<xmtp_mls::Client<ApiClient>>) -> Self {
+ pub async fn from<ApiClient: XmtpApi>(
+ group: &MlsGroup<xmtp_mls::Client<ApiClient>>
+ ) -> Result<Self, Box<dyn std::error::Error>> {
let group_id = hex::encode(group.group_id.clone());
let members = group
.members()
.await
- .expect("could not load members")
+ .map_err(|e| format!("Failed to load members: {}", e))?
.into_iter()
.map(|m| m.inbox_id)
.collect::<Vec<String>>();
let metadata = group
.metadata(
group
.mls_provider()
- .expect("MLS Provider could not be created"),
+ .map_err(|e| format!("Failed to create MLS provider: {}", e))?,
)
- .expect("could not load metadata");
- let permissions = group.permissions().expect("could not load permissions");
+ .map_err(|e| format!("Failed to load metadata: {}", e))?;
+ let permissions = group.permissions()
+ .map_err(|e| format!("Failed to load permissions: {}", e))?;
- Self {
+ Ok(Self {
group_id,
members,
metadata: SerializableGroupMetadata {
creator_inbox_id: metadata.creator_inbox_id.clone(),
policy: permissions
.preconfigured_policy()
- .expect("could not get policy")
+ .map_err(|e| format!("Failed to get policy: {}", e))?
.to_string(),
},
- }
+ })
}xmtp_api_grpc/src/identity.rs (2)
Line range hint 12-70: Consider optimizing client cloning and error handling.
The implementation has a few areas that could be improved:
- Each method clones the identity client before use. Consider using Arc::clone instead of a full clone if possible.
- Error mapping with
Error::new(ErrorKind::IdentityError).with(err)loses the original error context. Consider adding more context about the specific operation that failed.
Example improvement for error handling:
- .map_err(|err| Error::new(ErrorKind::IdentityError).with(err))
+ .map_err(|err| Error::new(ErrorKind::IdentityError)
+ .with_context(format!("Failed to publish identity update: {}", err)))Security improvements needed for wallet signature verification
Based on the code analysis, several security concerns need to be addressed:
- The current implementation lacks input validation before forwarding requests to the identity client
- Error handling is generic and may not preserve enough context for security auditing
- Logging is minimal with only a basic debug trace
- No validation of signature format or length before processing
Required changes:
- Add input validation for signatures and account IDs before processing
- Enhance error handling to include specific failure reasons
- Implement comprehensive security logging including request metadata
- Add signature format validation before forwarding to the identity client
🔗 Analysis chain
Line range hint 54-70: Review security implications of wallet signature verification.
The verify_smart_contract_wallet_signatures method handles critical security functionality. Ensure:
- The request validation is thorough
- The error handling preserves security-relevant information
- The operation is properly logged for security auditing
Let's check for related security tests:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related security tests
rg -l "verify_smart_contract_wallet" --type rust | xargs rg "test.*verify"
Length of output: 74
Script:
#!/bin/bash
# Let's first check the implementation details of verify_smart_contract_wallet_signatures
ast-grep --pattern 'fn verify_smart_contract_wallet_signatures($$$)'
# Check for test files related to identity
fd "identity.*test.*\.rs$"
# Search for any security-related tests or validations
rg -i "test.*wallet.*signature" --type rust
# Look for request validation patterns
ast-grep --pattern 'impl $_ for VerifySmartContractWalletSignaturesRequest'
# Check error handling patterns
rg "ErrorKind::IdentityError" -A 2 --type rust
Length of output: 5303
Script:
#!/bin/bash
# Let's check the actual validation logic in the smart contract wallet verifier
rg -A 10 "verify_smart_contract_wallet" xmtp_id/src/scw_verifier/
# Check for any validation or security checks in the request builder
ast-grep --pattern 'fn build_request($$$)'
# Look for any security-related logging configurations
rg "tracing::(debug|info|warn|error).*wallet" --type rust
# Check for any input validation or sanitization
rg "validate.*signature" --type rust
Length of output: 1728
xmtp_api_http/src/util.rs (1)
Line range hint 128-129: Fix typo in error message.
There's a typo in the error message: "coult" should be "could".
- .expect("coult not create client")
+ .expect("could not create client")xmtp_id/src/scw_verifier/mod.rs (1)
67-69: Consider adding documentation for the Arc implementation.
Adding documentation would help explain the concurrent use case and why Arc wrapping is beneficial here.
+/// Implements signature verification for thread-safe shared references.
+/// This enables multiple components to share the same verifier instance
+/// in concurrent contexts without requiring multiple copies.
#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)]
#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))]
impl<T> SmartContractSignatureVerifier for Arc<T>xmtp_mls/src/utils/test/mod.rs (2)
157-158: Consider making the nonce configurable.
The nonce is hardcoded to 1, which might limit testing scenarios. Consider making it a parameter to allow testing different nonce values.
-async fn inner_build<A>(owner: impl InboxOwner, api_client: A) -> Client<A>
+async fn inner_build<A>(owner: impl InboxOwner, api_client: A, nonce: u32) -> Client<A>
where
A: XmtpApi,
{
- let nonce = 1;
let inbox_id = generate_inbox_id(&owner.get_address(), &nonce);Line range hint 179-205: Consider reducing code duplication with inner_build.
The function shares significant code with inner_build. Consider extracting common logic to reduce duplication.
async fn inner_build<A>(owner: impl InboxOwner, api_client: A) -> Client<A>
where
A: XmtpApi,
{
+ inner_build_common(owner, api_client, None).await
+ }
+
+async fn build_with_verifier<A, V>(
+ owner: impl InboxOwner,
+ api_client: A,
+ scw_verifier: V,
+) -> Client<A, V>
+where
+ A: XmtpApi,
+ V: SmartContractSignatureVerifier,
+{
+ inner_build_common(owner, api_client, Some(scw_verifier)).await
+}
+
+async fn inner_build_common<A, V>(
+ owner: impl InboxOwner,
+ api_client: A,
+ scw_verifier: Option<V>,
+) -> Client<A, V>
+where
+ A: XmtpApi,
+ V: SmartContractSignatureVerifier,
+{
let nonce = 1;
let inbox_id = generate_inbox_id(&owner.get_address(), &nonce);
-
- let client = Client::<A>::builder(IdentityStrategy::CreateIfNotFound(
+ let mut builder = Client::<A, V>::builder(IdentityStrategy::CreateIfNotFound(
inbox_id,
owner.get_address(),
nonce,
None,
));
- let client = client
- .temp_store()
- .await
- .api_client(api_client)
- .build()
- .await
- .unwrap();
- register_client(&client, owner).await;
+ builder = builder.temp_store().await.api_client(api_client);
+
+ if let Some(verifier) = scw_verifier {
+ builder = builder.scw_signature_verifier(verifier);
+ }
+
+ let client = builder.build().await.unwrap();
+ register_client(&client, owner).await;
client
}xmtp_mls/src/api/identity.rs (1)
Line range hint 190-266: Consider refactoring test setup to reduce duplication.
The test functions follow a similar pattern in setting up mocks and creating the wrapper. Consider extracting common setup into a test fixture or helper function to reduce boilerplate and make tests more maintainable.
Here's a suggested approach:
#[cfg(test)]
mod tests {
// ... existing imports ...
struct TestFixture {
wrapper: ApiClientWrapper<MockApiClient>,
inbox_id: String,
address: String,
}
impl TestFixture {
fn new() -> Self {
let inbox_id = rand_string();
let address = rand_string();
let mock_api = MockApiClient::new();
let wrapper = ApiClientWrapper::new(mock_api.into(), Retry::default());
Self {
wrapper,
inbox_id,
address,
}
}
fn setup_publish_expectation(&mut self) -> &mut Self {
// Setup publish expectations
self
}
fn setup_get_updates_expectation(&mut self) -> &mut Self {
// Setup get updates expectations
self
}
fn setup_get_inbox_ids_expectation(&mut self) -> &mut Self {
// Setup get inbox ids expectations
self
}
}
}xmtp_api_grpc/src/lib.rs (1)
Line range hint 16-27: Improve error handling and reduce duplication in test client implementation.
The implementation has several areas for improvement:
- Using
unwrap()in public API methods can cause panics - URLs are duplicated between constants and string literals
- Missing documentation for the trait implementation
Consider applying these changes:
#[async_trait::async_trait]
+/// Implementation of XmtpTestClient for creating test instances of the Client
+/// with predefined configurations for local and development environments.
impl XmtpTestClient for crate::Client {
async fn create_local() -> Self {
- crate::Client::create("http://localhost:5556".into(), false)
- .await
- .unwrap()
+ crate::Client::create(crate::LOCALHOST_ADDRESS.into(), false)
+ .await
+ .map_err(|e| panic!("Failed to create local test client: {}", e))
}
async fn create_dev() -> Self {
- crate::Client::create("https://grpc.dev.xmtp.network:443".into(), false)
- .await
- .unwrap()
+ crate::Client::create(crate::DEV_ADDRESS.into(), false)
+ .await
+ .map_err(|e| panic!("Failed to create dev test client: {}", e))
}
}Note: While we still use panic here, it's more informative and reuses the constant URLs. Since this is a test utility (under test-utils feature), panicking with a clear message is acceptable.
xmtp_mls/src/identity_updates.rs (1)
Line range hint 392-392: Consider implementing parallel processing as noted in TODO.
The TODO comment suggests parallelizing the installation diff computation. This would be a valuable optimization for performance, especially when dealing with multiple inbox IDs.
Would you like me to help implement the parallel processing using try_join_all or similar async utilities?
xmtp_mls/src/groups/message_history.rs (3)
Line range hint 40-82: Consider enhancing error messages for better debugging.
While the error handling is comprehensive, some error variants could provide more context:
Conversioncould specify what type conversion failedGenericcould include more details about the specific error scenario
Example improvement:
#[error("type conversion error")]
-Conversion,
+Conversion { from: String, to: String },
#[error("generic: {0}")]
-Generic(String),
+Generic { context: String, details: String },Line range hint 449-450: Document reasons for ignored tests.
Two tests are marked with #[ignore] but only one has a comment explaining why. Please add documentation for test_request_reply_roundtrip explaining why it's ignored and under what conditions it should be run.
Also applies to: 516-517
Based on the findings, I can now generate the final response:
Consider strengthening PIN security with additional measures
The current implementation uses a 4-digit numeric PIN (0-9999) generated using crypto_utils::rng(). While the random generation is cryptographically secure, the following security concerns should be addressed:
- The PIN space is limited to 10,000 combinations
- No rate limiting for PIN verification attempts is implemented
- No PIN expiration mechanism exists
Recommendations:
- Increase PIN length or use alphanumeric characters to expand the possible combinations
- Implement rate limiting for PIN verification attempts
- Add a time-based expiration for PINs
- Consider implementing a temporary lockout after multiple failed attempts
🔗 Analysis chain
Line range hint 673-677: Consider increasing PIN code length for better security.
The current 4-digit PIN (0-9999) provides only 10,000 possible combinations, which might be insufficient for sensitive operations. Consider:
- Increasing the PIN length
- Adding rate limiting for PIN verification attempts
- Using alphanumeric characters for higher entropy
Let me search for more context about the PIN implementation.
Let me search for the PIN generation implementation to understand how it's created.
Let me try a different search approach for the PIN implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if rate limiting is implemented elsewhere
rg -A 5 "verify_pin|PinNotFound|PinMismatch"
Length of output: 2584
Script:
#!/bin/bash
# Search for PIN generation and length constraints
rg -A 5 "pin.*code|generate.*pin"
# Search for any rate limiting implementations
rg -A 5 "rate.*limit|attempt.*count|max.*attempt"
Length of output: 19058
Script:
#!/bin/bash
# Search for new_pin implementation
ast-grep --pattern 'fn new_pin()' -A 10
Length of output: 41
Script:
#!/bin/bash
# Search for new_pin function with ripgrep
rg -A 10 "fn new_pin"
# Also search for any PIN-related constants or configurations
rg "PIN_|pin_length|PIN_LENGTH"
Length of output: 807
xmtp_id/src/utils/test.rs (1)
29-46: Ensure consistent platform-specific compilation attributes
The new method is annotated with #[cfg(not(target_arch = "wasm32"))], but the coinbase_smart_wallet_factory method is not. If the entire SmartContracts struct is not intended for wasm32 targets, consider applying the cfg attribute to the entire impl block or the struct itself for consistency.
xmtp_api_http/src/lib.rs (1)
Line range hint 128-212: Refactor repeated HTTP request handling to improve maintainability
The methods within the XmtpMlsClient implementation (upload_key_package, fetch_key_packages, send_group_messages, send_welcome_messages, query_group_messages, query_welcome_messages) contain repetitive code for constructing and handling HTTP POST requests.
Consider abstracting the common request logic into a helper function to reduce code duplication and enhance maintainability. For example:
async fn post_request<T: serde::Serialize, R: serde::de::DeserializeOwned>(
&self,
endpoint: &str,
request: T,
error_kind: ErrorKind,
) -> Result<R, Error> {
let res = self
.http_client
.post(self.endpoint(endpoint))
.json(&request)
.send()
.await
.map_err(|e| Error::new(error_kind).with(e))?
.bytes()
.await
.map_err(|e| Error::new(error_kind).with(e))?;
handle_error(&*res)
}Then, each method can call this helper function, passing the appropriate parameters, which simplifies the implementations.
xmtp_api_grpc/src/grpc_api_helper.rs (6)
Line range hint 10-10: Switch to tokio::sync::Mutex for asynchronous contexts
The code currently uses std::sync::Mutex, which is not suitable for asynchronous contexts and can lead to deadlocks or performance issues. Since the Subscription struct is used within asynchronous tasks, consider replacing std::sync::Mutex with tokio::sync::Mutex to ensure proper asynchronous locking.
Apply this diff to switch to async mutexes:
use std::pin::Pin;
-use std::sync::{Arc, Mutex}; // TODO switch to async mutexes
+use std::sync::Arc;
+use tokio::sync::Mutex; // Switched to async mutexes
use std::time::Duration;Line range hint 161-164: Update get_messages method to be asynchronous
After switching to tokio::sync::Mutex, the get_messages method needs to be asynchronous to await the mutex lock. Modify the XmtpApiSubscription trait and the get_messages method accordingly.
Update the method as follows:
#[async_trait::async_trait]
impl XmtpApiSubscription for Subscription {
fn is_closed(&self) -> bool {
self.closed.load(Ordering::SeqCst)
}
- fn get_messages(&self) -> Vec<Envelope> {
+ async fn get_messages(&self) -> Vec<Envelope> {
- let mut pending = self.pending.lock().unwrap();
+ let mut pending = self.pending.lock().await;
let items = pending.drain(..).collect::<Vec<Envelope>>();
items
}This change will also require updating any callers of get_messages to await the result.
Line range hint 138-209: Avoid unnecessary cloning of clients
In methods like publish, subscribe, and subscribe2, the client is cloned before use. Cloning the client may not be necessary and could have performance implications. Consider using a mutable reference instead.
Update the code as follows:
let client = &mut self.client.clone();
+ // Cloning may not be necessary; use a mutable reference instead
+ let client = &mut self.client;Line range hint 204-209: Simplify error handling using the ? operator
In the batch_query method, you manually match on the result to handle errors. You can simplify the code by using the ? operator to propagate errors more concisely.
Simplify the method as follows:
async fn batch_query(&self, request: BatchQueryRequest) -> Result<BatchQueryResponse, Error> {
let client = &mut self.client.clone();
- let res = client.batch_query(self.build_request(request)).await;
-
- match res {
- Ok(response) => Ok(response.into_inner()),
- Err(e) => Err(Error::new(ErrorKind::QueryError).with(e)),
- }
+ client
+ .batch_query(self.build_request(request))
+ .await
+ .map(|response| response.into_inner())
+ .map_err(|e| Error::new(ErrorKind::QueryError).with(e))
}Line range hint 271-283: Handle potential mutex poisoning
In the Subscription struct's methods, you use lock().unwrap() on the mutex. This could panic if the mutex is poisoned. Consider handling this case explicitly to improve robustness.
Update the code as follows:
fn get_messages(&self) -> Vec<Envelope> {
- let mut pending = self.pending.lock().unwrap();
+ let pending_lock = self.pending.lock();
+ let mut pending = match pending_lock {
+ Ok(guard) => guard,
+ Err(poisoned) => poisoned.into_inner(),
+ };
let items = pending.drain(..).collect::<Vec<Envelope>>();
items
}Alternatively, you can use expect with a meaningful error message.
Line range hint 343-393: Consistent error handling in MLS client methods
In the XmtpMlsClient implementation, ensure that error handling is consistent across all methods. For instance, in upload_key_package, you match on the result and handle errors differently than in methods like fetch_key_packages.
Standardize the error handling. For example, modify upload_key_package as follows:
async fn upload_key_package(&self, req: UploadKeyPackageRequest) -> Result<(), Error> {
let client = &mut self.mls_client.clone();
- let res = client.upload_key_package(self.build_request(req)).await;
- match res {
- Ok(_) => Ok(()),
- Err(e) => Err(Error::new(ErrorKind::MlsError).with(e)),
- }
+ client
+ .upload_key_package(self.build_request(req))
+ .await
+ .map_err(|e| Error::new(ErrorKind::MlsError).with(e))?;
+ Ok(())
}This approach uses the ? operator for cleaner code and consistent error handling.
xmtp_api_grpc/src/replication_client.rs (1)
5-5: Address the TODO: Switch to async mutexes
There's a TODO comment indicating the need to switch to asynchronous mutexes. Given the potential issues with blocking the async runtime, it's important to prioritize this change to improve performance and prevent deadlocks.
Would you like assistance in updating the mutexes to be asynchronous?
xmtp_mls/src/builder.rs (3)
127-128: Avoid Reassigning builder After Method Call
Instead of reassigning builder, you can chain the method call to improve code clarity:
let (mut builder, api_client) = inner_build_api_client(self)?;
- builder = builder.scw_signature_verifier(RemoteSignatureVerifier::new(api_client.clone()));
+ builder.scw_signature_verifier(RemoteSignatureVerifier::new(api_client.clone()));This eliminates unnecessary reassignment and makes the code more idiomatic.
133-135: Add Documentation for inner_build_api_client Function
Consider adding a doc comment to inner_build_api_client to explain its purpose and usage. Documenting helper functions enhances code readability and aids future maintenance.
Line range hint 670-672: Improve Error Matching Logic
In the test:
assert!(
matches!(err, IdentityError::InboxIdMismatch { id, stored } if id == inbox_id && stored == stored_inbox_id)
);Consider adding more informative error messages or logging to aid in debugging if the test fails. This can help quickly identify discrepancies in inbox_id and stored_inbox_id.
xmtp_mls/src/client.rs (1)
327-332: Suggest Using Arc::new for Clarity in Initialization
In the new method, consider using Arc::new instead of .into() when wrapping api_client and scw_verifier into Arc for better readability and explicitness.
Apply this diff to enhance clarity:
let (tx, _) = broadcast::channel(10);
Self {
- api_client: api_client.into(),
+ api_client: Arc::new(api_client),
context,
#[cfg(feature = "message-history")]
history_sync_url,
local_events: tx,
- scw_verifier: scw_verifier.into(),
+ scw_verifier: Arc::new(scw_verifier),
intents,
}bindings_ffi/src/mls.rs (1)
Line range hint 85-105: Refactor client builder to eliminate code duplication
Consider refactoring the creation of the xmtp_client to reduce code duplication and improve readability by initializing the ClientBuilder once and conditionally adding the history_sync_url if it is provided.
Apply this diff to implement the refactor:
let mut client_builder = ClientBuilder::new(identity_strategy)
.api_client(api_client)
.store(store);
if let Some(url) = history_sync_url {
client_builder = client_builder.history_sync_url(&url);
}
let xmtp_client = client_builder.build().await?;📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
- bindings_ffi/src/mls.rs (1 hunks)
- bindings_wasm/src/inbox_id.rs (1 hunks)
- examples/cli/cli-client.rs (7 hunks)
- examples/cli/serializable.rs (1 hunks)
- xmtp_api_grpc/Cargo.toml (1 hunks)
- xmtp_api_grpc/src/grpc_api_helper.rs (5 hunks)
- xmtp_api_grpc/src/identity.rs (1 hunks)
- xmtp_api_grpc/src/lib.rs (2 hunks)
- xmtp_api_grpc/src/replication_client.rs (1 hunks)
- xmtp_api_http/Cargo.toml (1 hunks)
- xmtp_api_http/src/lib.rs (4 hunks)
- xmtp_api_http/src/util.rs (1 hunks)
- xmtp_id/src/scw_verifier/mod.rs (2 hunks)
- xmtp_id/src/scw_verifier/remote_signature_verifier.rs (2 hunks)
- xmtp_id/src/utils/mod.rs (1 hunks)
- xmtp_id/src/utils/test.rs (1 hunks)
- xmtp_mls/src/api/identity.rs (3 hunks)
- xmtp_mls/src/api/mls.rs (9 hunks)
- xmtp_mls/src/api/mod.rs (2 hunks)
- xmtp_mls/src/api/test_utils.rs (1 hunks)
- xmtp_mls/src/builder.rs (8 hunks)
- xmtp_mls/src/client.rs (4 hunks)
- xmtp_mls/src/groups/message_history.rs (1 hunks)
- xmtp_mls/src/groups/scoped_client.rs (2 hunks)
- xmtp_mls/src/groups/subscriptions.rs (6 hunks)
- xmtp_mls/src/identity_updates.rs (1 hunks)
- xmtp_mls/src/subscriptions.rs (5 hunks)
- xmtp_mls/src/utils/test/mod.rs (7 hunks)
- xmtp_proto/Cargo.toml (2 hunks)
- xmtp_proto/src/api_client.rs (9 hunks)
✅ Files skipped from review due to trivial changes (1)
- xmtp_id/src/utils/mod.rs
🔇 Additional comments (64)
xmtp_mls/src/api/mod.rs (2)
6-6: LGTM: Arc import added for shared ownership support
The addition of Arc import is appropriate for implementing thread-safe shared ownership of the API client.
35-35: Verify thread-safety requirements for ApiClient
The change to use Arc<ApiClient> suggests concurrent access. Please ensure that ApiClient implements necessary thread-safety traits (Send + Sync).
✅ Verification successful
Based on the search results, I can now provide a definitive response about the thread-safety requirements for ApiClient:
Thread-safety requirements for ApiClient are properly enforced
The codebase shows that:
- ApiClient is used with Arc in concurrent contexts and has explicit Send + Sync bounds
- The trait implementations enforce thread-safety:
- XmtpApiClient requires Sync for Box implementations
- Client requires ApiClient: XmtpApi + Send + Sync + 'static
- The ApiClientWrapper struct safely wraps ApiClient in Arc for shared ownership
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ApiClient implements Send + Sync traits
# Expected: Find trait bounds or implementations ensuring thread safety
# Look for trait implementations or bounds
ast-grep --pattern 'impl<$_> Send for ApiClient<$_>'
ast-grep --pattern 'impl<$_> Sync for ApiClient<$_>'
ast-grep --pattern 'where ApiClient: Send'
ast-grep --pattern 'where ApiClient: Sync'
Length of output: 199
Script:
#!/bin/bash
# Let's check the ApiClient struct definition and its fields to understand thread-safety
ast-grep --pattern 'struct ApiClient {
$$$
}'
# Also check for any auto-derived traits that might include Send/Sync
ast-grep --pattern '#[derive($$$)]
struct ApiClient'
# Look for any unsafe impl blocks related to ApiClient
ast-grep --pattern 'unsafe impl $$ for ApiClient'
# Check if ApiClient is used in any async contexts or with Arc elsewhere
rg "ApiClient" -A 3 -B 3
Length of output: 90848
Script:
#!/bin/bash
# Let's check if ApiClient is used in any async/concurrent contexts
# and look for any Send/Sync trait implementations or auto-derive
# Look for async functions using ApiClient
rg "async.*ApiClient" -A 2 -B 2
# Look for any auto-derived traits on ApiClient implementations
ast-grep --pattern '#[derive($$$)]
$_ApiClient'
# Check for any Send/Sync bounds on ApiClient in trait definitions
rg "ApiClient.*Send|ApiClient.*Sync" -A 2 -B 2
# Look for any Arc/Mutex usage with ApiClient
rg "Arc<.*ApiClient|Mutex<.*ApiClient" -A 2 -B 2
Length of output: 7336
bindings_wasm/src/inbox_id.rs (1)
14-14: Verify the type conversion implementation.
The addition of .into() suggests a type conversion is now required for the API client. Let's verify this change is consistent across the codebase.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Type conversion implementation is correctly applied across the codebase
The .into() conversion is consistently used when creating ApiClientWrapper instances. This pattern is observed in:
- Test code in
xmtp_mls/src/api/mls.rsandxmtp_mls/src/api/identity.rs - Production code in
bindings_ffi,bindings_node, andbindings_wasm - Both HTTP and Tonic API client implementations follow this pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent API client type conversion patterns
# Test 1: Look for other instances of XmtpHttpApiClient creation
echo "Checking XmtpHttpApiClient creation patterns:"
rg -A 2 "XmtpHttpApiClient::new"
# Test 2: Look for ApiClientWrapper usage to understand the expected types
echo -e "\nChecking ApiClientWrapper usage patterns:"
ast-grep --pattern 'ApiClientWrapper::new($_, $_)'
Length of output: 3858
xmtp_proto/Cargo.toml (1)
43-43: Verify protoc marker formatting change.
The change from single to double '#' in the protoc insertion point marker might affect protobuf code generation.
Let's verify this is the correct format:
#!/bin/bash
# Description: Verify protoc marker format consistency
# Test: Check format in other proto-related files
echo "Checking protoc marker format in other files..."
rg "@@protoc.*point" --type tomlxmtp_id/src/scw_verifier/remote_signature_verifier.rs (2)
1-1: LGTM: Appropriate import for thread-safe shared ownership
The addition of Arc import aligns with the PR's objective of enhancing the concurrency model.
16-16: LGTM: Thread-safe field type change
The transition to Arc<C> appropriately enables shared ownership of the identity client across multiple threads while preserving the generic type flexibility.
Let's verify consistent Arc usage across related components:
✅ Verification successful
Let me analyze the Arc usage across the codebase with a more focused search to verify the consistency.
Let me try a different approach to verify Arc usage in client fields.
Arc usage is consistent across the codebase
The change to Arc<C> in RemoteSignatureVerifier aligns with the codebase's consistent pattern of using Arc for client fields that need shared ownership. This is evidenced by similar usage in:
- Client structs (
api_client: Arc<ApiClientWrapper<ApiClient>>) - Group implementations (
client: Arc<ScopedClient>) - Bindings (
inner_client: Arc<RustXmtpClient>)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent Arc usage in related client implementations
# Expect: All client fields should use Arc for thread-safety
# Search for other client field declarations
rg -U "client.*:.*C" --type rust
Length of output: 13231
Script:
#!/bin/bash
# Search for struct fields containing 'client' to verify Arc usage
ast-grep --pattern 'struct $_ {
$$$
client: $_,
$$$
}'
Length of output: 62
Script:
#!/bin/bash
# Search for struct field declarations with Arc<C> or Arc<Client> patterns
rg ":\s*Arc<.*Client" --type rust -B 2 -A 2
Length of output: 7802
examples/cli/serializable.rs (1)
25-25: LGTM! Removing unnecessary Clone bound.
The removal of the Clone trait bound is a good improvement as the implementation doesn't require cloning the ApiClient. This makes the API more flexible by not imposing unnecessary constraints on the client type.
xmtp_api_grpc/src/identity.rs (2)
12-13: LGTM! Trait implementation looks good.
The async trait implementation is correctly annotated and structured.
Line range hint 41-52: Method name mismatch in get_identity_updates_v2.
The method get_identity_updates_v2 internally calls get_identity_updates on the client. This inconsistency could lead to confusion. Either:
- Rename the method to match the gRPC call
- Add a comment explaining the version difference
Let's verify the gRPC service definition:
✅ Verification successful
Method name is correctly implemented, no changes needed
The implementation is correct. The gRPC service defines get_identity_updates as the underlying RPC method name, while get_identity_updates_v2 is a higher-level client method that provides a V2 interface by wrapping the underlying RPC call. This is a common pattern in API versioning where the transport layer method name remains stable while client interfaces evolve.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the service definition to confirm the method name
rg -A 10 "service.*Identity" --type proto
Length of output: 73
Script:
#!/bin/bash
# Let's try finding proto files without relying on file type
fd "\.proto$" | xargs rg -A 10 "service.*Identity"
# Also search for the actual method name in proto files
fd "\.proto$" | xargs rg "get_identity_updates"
Length of output: 9173
Script:
#!/bin/bash
# Let's look at the generated protobuf code to understand the service definition
fd "identity.api.v1.(rs|tonic.rs)$" | xargs rg "trait IdentityApi" -A 10
Length of output: 1067
xmtp_api_http/src/util.rs (1)
120-121: LGTM! Proper async trait configuration for cross-platform support.
The conditional attributes correctly handle the async trait implementation for both wasm32 and non-wasm32 targets, with appropriate Send bounds.
xmtp_id/src/scw_verifier/mod.rs (2)
9-9: LGTM: Arc import addition is appropriate.
The addition of Arc to the imports is well-placed and necessary for the new trait implementation.
67-84: LGTM: Arc implementation is well-structured.
The implementation correctly enables shared ownership of verifiers while maintaining all trait functionality. The async/await handling and target-specific attributes are properly configured.
xmtp_mls/src/utils/test/mod.rs (3)
108-113: LGTM! Clean separation of client building and verification.
The refactoring improves code organization by clearly separating the client building process from signature verification setup.
Also applies to: 122-126
135-135: LGTM! Simplified client creation without verification.
The changes appropriately use the simplified inner_build for cases where signature verification isn't needed.
Also applies to: 140-140
251-254: LGTM! Simplified type constraints.
Good cleanup removing unnecessary Clone bounds from the type parameters.
xmtp_mls/src/api/identity.rs (3)
220-220: LGTM! Consistent with the type conversion pattern.
The change maintains consistency with the type conversion pattern while preserving the thorough test coverage.
266-266: LGTM! Completes the consistent type conversion pattern.
This change completes the consistent application of type conversion across all test instantiations of ApiClientWrapper. The pattern enhances type safety and aligns with Rust's type system best practices.
190-190: Verify the type conversion implementation for MockApiClient.
The change to use into() follows Rust's type conversion best practices. However, let's verify the implementation of the conversion trait.
✅ Verification successful
Let me gather more information to verify the type conversion implementation.
Let me search for the specific implementation details of MockApiClient and its relationship with ApiClientWrapper.
Based on the search results, I can now provide a final response. The code shows that MockApiClient is used in tests and properly implements the required traits, with ApiClientWrapper being a generic wrapper that works with any type implementing XmtpApi. The use of into() is correct as it follows Rust's type conversion patterns.
The type conversion implementation is correctly used in the test context.
The change to use mock_api.into() is valid because:
ApiClientWrapperis designed to work with any type implementingXmtpApiMockApiClientis a test double that implements the required traits- The conversion is used consistently across all test cases
- The wrapper pattern with
ArcandRetryis properly maintained
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the From/Into trait implementation for MockApiClient
ast-grep --pattern 'impl $_ for MockApiClient {
$$$
}'
# Also check for any related type definitions
rg "type (Api|Client|Mock)" -A 3
Length of output: 1670
Script:
#!/bin/bash
# Search for MockApiClient definition and implementations
rg "struct MockApiClient|impl.*MockApiClient" -A 5
# Search for XmtpApi trait definition and implementations
rg "trait XmtpApi|impl XmtpApi" -A 5
# Look for Into/From implementations
ast-grep --pattern 'impl From<$_> for $_'
Length of output: 105311
Script:
#!/bin/bash
# Search for MockApiClient definition and its usage
rg "struct MockApiClient|impl.*MockApiClient" -A 10
# Search for ApiClientWrapper definition and its usage with mock_api
rg "struct ApiClientWrapper|impl.*ApiClientWrapper" -A 10
# Look for test files containing mock_api usage
rg "mock_api" --type rust -A 3
Length of output: 13274
xmtp_mls/src/groups/scoped_client.rs (1)
142-143: LGTM! Good simplification of trait bounds.
The removal of unnecessary Clone constraints while maintaining essential bounds (XmtpApi and SmartContractSignatureVerifier) improves flexibility without compromising functionality.
xmtp_api_grpc/src/lib.rs (2)
Line range hint 4-27: LGTM! The changes integrate well with existing code.
The addition of the replication client module and test client implementation:
- Maintains compatibility with existing code
- Follows established patterns
- Is well-covered by existing tests
4-4: Verify the replication_client module file exists.
The new public module declaration suggests a corresponding file should exist.
✅ Verification successful
The replication_client module file exists and is correctly placed
The verification confirms that replication_client.rs exists at the expected location xmtp_api_grpc/src/replication_client.rs, which is in the same directory as the lib.rs file that declares it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the replication_client module file exists
# Expected: Find replication_client.rs in the same directory as lib.rs
# Get the directory of lib.rs
fd --type f "lib.rs$" | while read -r lib_file; do
lib_dir=$(dirname "$lib_file")
echo "Checking for replication_client.rs in $lib_dir"
ls "$lib_dir/replication_client.rs" 2>/dev/null || echo "Missing: $lib_dir/replication_client.rs"
done
Length of output: 3828
xmtp_mls/src/api/mls.rs (4)
5-5: LGTM!
The addition of XmtpMlsStreams trait import is necessary for the new streaming functionality constraints.
280-283: LGTM!
The addition of where ApiClient: XmtpMlsStreams trait bound is consistent with the changes in subscribe_group_messages.
329-329: LGTM!
The test cases have been consistently updated to use .into() when creating ApiClientWrapper, properly handling the type conversion of MockApiClient.
Also applies to: 352-352, 390-390, 422-422, 473-473, 507-507
265-268: Verify implementations of ApiClient satisfy the new trait bound.
The addition of where ApiClient: XmtpMlsStreams is a breaking change that requires all implementations of ApiClient to support streaming functionality.
✅ Verification successful
Breaking change is properly implemented across the codebase
Based on the search results, the XmtpMlsStreams trait bound is correctly implemented for all relevant API clients:
-
The main GRPC client implementations have the trait:
ClientV4inxmtp_api_grpc/src/replication_client.rsClientinxmtp_api_grpc/src/grpc_api_helper.rs
-
The HTTP client implementation has it:
XmtpHttpApiClientinxmtp_api_http/src/lib.rs
-
The trait is also properly implemented for wrapper types:
Box<T>implementation inxmtp_proto/src/api_client.rs
-
Test utilities include mock implementations:
- Mock
ApiClientinxmtp_mls/src/api/test_utils.rs
- Mock
The breaking change is well-managed across the codebase with all necessary implementations in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all implementations of ApiClient implement XmtpMlsStreams
# Test: Search for ApiClient implementations
ast-grep --pattern 'impl$_ $ApiClient for $_'
# Test: Search for XmtpMlsStreams implementations
ast-grep --pattern 'impl$_ XmtpMlsStreams for $_'
Length of output: 99
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# Search for trait definitions first
echo "=== Searching for trait definitions ==="
rg "trait (ApiClient|XmtpMlsStreams)" -A 2
# Search for impl blocks
echo -e "\n=== Searching for implementations ==="
rg "impl.*?(ApiClient|XmtpMlsStreams)" -A 2
# Search for struct definitions that might implement these traits
echo -e "\n=== Searching for potential implementing types ==="
rg "struct.*?Client" -A 2
Length of output: 23505
xmtp_mls/src/identity_updates.rs (2)
92-93: LGTM! Type constraint simplification improves flexibility.
The removal of Clone trait bounds from ApiClient and V type parameters is a good improvement. This change makes the type constraints less restrictive while maintaining the essential functionality, following the principle of least constraints.
Line range hint 509-1000: LGTM! Comprehensive test coverage.
The test suite is thorough and well-structured, covering:
- Inbox creation and management
- Wallet association and revocation
- Cache behavior
- Identity updates
- Installation management
xmtp_mls/src/groups/message_history.rs (1)
109-110: LGTM: Simplified trait bounds.
The removal of unnecessary Clone bounds while retaining essential trait requirements improves the API's flexibility and follows Rust's best practices.
xmtp_api_http/src/lib.rs (4)
78-79: Consistent application of async_trait attribute
The conditional use of async_trait ensures compatibility with both wasm32 and non-wasm32 targets. This pattern is correctly implemented for the ClientWithMetadata trait.
128-129: Consistent application of async_trait attribute
Similarly, the async_trait attribute is appropriately applied to the XmtpMlsClient trait implementation, ensuring asynchronous functionality across different target architectures.
284-285: Consistent application of async_trait attribute
The async_trait attribute is correctly applied to the XmtpIdentityClient trait implementation, ensuring consistent asynchronous behavior across different targets.
240-241:
Typographical error in code comment
There's a minor typo in the comment. The word "ise" should be "use".
Apply this diff to correct the typo:
// 2.) ise `impl Stream` in return of `XmtpMlsStreams` but that
+// 2.) use `impl Stream` in return of `XmtpMlsStreams` but thatAdditionally, there's an extra colon in mockall::. It should be mockall.
- breaks the `mockall::` functionality
+ breaks the `mockall` functionalityLikely invalid or redundant comment.
xmtp_proto/src/api_client.rs (5)
59-67: Review Omission of Send + Sync Bounds in XmtpApi Trait for wasm32 Target
In the wasm32 target configuration, the XmtpApi trait omits the Send + Sync bounds:
pub trait XmtpApi
where
Self: XmtpMlsClient + XmtpIdentityClient + ClientWithMetadata,
{
}Given that Send and Sync are not automatically implemented for types in wasm32, this might be intentional. However, if any asynchronous operations or multithreading are introduced, the absence of these bounds could lead to runtime issues.
Please verify whether the omission is deliberate and whether it aligns with the intended concurrency model for wasm32.
189-190: Ensure Consistent Use of async_trait Attributes Across Traits
The #[cfg_attr] attributes are correctly applied to handle the async_trait macro differently for wasm32 and non-wasm32 targets. Ensure this pattern is consistently used across all traits that require it to prevent any asynchronous implementation issues on different platforms.
221-223: Confirm Necessity of Send Bound in XmtpApiClient for wasm32 Target
In the definition of XmtpApiClient, the async_trait attribute specifies (?Send) for the wasm32 target:
#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))]
pub trait XmtpApiClient {
// ...
}However, within the trait, methods like publish, subscribe, and query may rely on types or traits that require Send. Please ensure that all asynchronous methods and associated types are compatible with the ?Send specification on wasm32. If any method inherently requires Send, the ?Send may cause compilation errors or unexpected behavior.
247-281: Ensure Sync Bound on T is Sufficient in impl XmtpApiClient for Box<T>
In the implementation of XmtpApiClient for Box<T>, T is constrained with Sync:
impl<T> XmtpApiClient for Box<T>
where
T: XmtpApiClient + Sync + ?Sized,
{
// ...
}Considering that the methods are async and may involve concurrent operations, verify that the Sync bound is sufficient and that a Send bound on T is not required for thread safety, especially in non-wasm32 targets.
393-417: Consistency in Trait Bounds for XmtpMlsStreams Implementation
In the implementation of XmtpMlsStreams for Box<T>, T is constrained with Sync:
impl<T> XmtpMlsStreams for Box<T>
where
T: XmtpMlsStreams + Sync + ?Sized,
{
// ...
}Ensure that this constraint aligns with the requirements of your stream implementations. If the streams are sent across threads or require thread safety, consider whether a Send bound is necessary.
xmtp_mls/src/groups/subscriptions.rs (6)
6-6: Importing XmtpMlsStreams for streaming capabilities
The addition of use xmtp_proto::api_client::XmtpMlsStreams; is necessary to utilize the streaming functionalities provided by the XmtpMlsStreams trait.
138-139: Update trait bounds in stream_with_callback method
Adding 'static lifetime and the XmtpMlsStreams trait bound ensures that the ScopedClient and its ApiClient can support the streaming functionality required by stream_with_callback.
158-159: Ensure ApiClient trait bounds are consistent in stream_messages
The trait bounds <ScopedClient as ScopedGroupClient>::ApiClient: XmtpApi + XmtpMlsStreams + 'static ensure that the necessary API and streaming functionalities are available in the stream_messages function.
182-182: Creation of MlsGroup instance within stream processing
The initialization of MlsGroup using MlsGroup::new(client, group_id, stream_info.convo_created_at_ns) inside the asynchronous closure is appropriate. Ensure that the client can be safely used within the closure without ownership or lifetime issues.
206-207: Consistent trait bounds in stream_messages_with_callback
The addition of 'static lifetime and the trait bounds ensures that ScopedClient and its ApiClient support the necessary functionalities in stream_messages_with_callback.
119-119: Ensure ApiClient implements XmtpMlsStreams in stream method
By adding the trait bound <ScopedClient as ScopedGroupClient>::ApiClient: XmtpMlsStreams + 'static, you ensure that the ApiClient supports the required streaming methods in the stream function. Verify that all implementations of ScopedGroupClient have an ApiClient that implements XmtpMlsStreams.
Run the following script to check that all ApiClient types implement XmtpMlsStreams:
✅ Verification successful
Trait bound XmtpMlsStreams is correctly implemented by all API clients
Based on the verification results, the trait bound is correctly implemented:
- The main implementation of
ScopedGroupClientforClient<ApiClient, Verifier>requiresApiClient: XmtpApi XmtpMlsStreamsis implemented for all API client types:XmtpHttpApiClient(HTTP API)ClientV4(gRPC API)Client(gRPC helper)Box<T> where T: XmtpMlsStreams
The trait bound addition is safe and all implementations satisfy it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all `ApiClient` associated types of `ScopedGroupClient` implement `XmtpMlsStreams`.
# Expected result: All `ApiClient` types implement `XmtpMlsStreams`.
# Find all implementations of ScopedGroupClient and extract their ApiClient types
ast-grep --pattern 'impl ScopedGroupClient for $T where $$$' --lang rust | \
while read -r line; do
# Extract the ApiClient type
api_client_type=$(echo "$line" | grep -oP 'type ApiClient = \K[^;]+')
if [ -n "$api_client_type" ]; then
echo "Checking if $api_client_type implements XmtpMlsStreams"
# Search for the implementation of XmtpMlsStreams for the ApiClient type
ast-grep --pattern "impl XmtpMlsStreams for $api_client_type $$$" --lang rust
fi
done
Length of output: 434
Script:
#!/bin/bash
# Let's try a different approach to find implementations and trait bounds
# First, let's find all files that might contain the relevant code
fd -e rs | xargs rg -l "ScopedGroupClient|XmtpMlsStreams"
# Then let's examine the actual implementations and trait bounds
rg -A 5 "impl.*ScopedGroupClient"
# And also look for XmtpMlsStreams implementations
rg -A 5 "impl.*XmtpMlsStreams"
# Let's also check where these traits are defined
rg "trait (ScopedGroupClient|XmtpMlsStreams)"
Length of output: 8447
examples/cli/cli-client.rs (1)
71-72: Addition of testnet flag is correct
The testnet flag is added correctly to the Cli struct with the appropriate Clap attributes.
xmtp_api_grpc/src/grpc_api_helper.rs (1)
Line range hint 461-486: Specify lifetimes explicitly for associated types
In the XmtpMlsStreams implementation, you define associated types with explicit lifetimes. Ensure that these lifetimes are correctly specified and consistent with the rest of the code.
Confirm that the lifetimes 'a are necessary and used appropriately. If they are not required, you might simplify the associated types:
- type GroupMessageStream<'a> = GroupMessageStream;
- type WelcomeMessageStream<'a> = WelcomeMessageStream;
+ type GroupMessageStream = GroupMessageStream;
+ type WelcomeMessageStream = WelcomeMessageStream;Please verify if the lifetime parameters are essential.
xmtp_api_grpc/src/replication_client.rs (2)
154-171:
Implement the unimplemented methods or handle them appropriately
The methods publish, subscribe, subscribe2, query, and batch_query are currently marked with unimplemented!(). This can cause panics at runtime if called. Ensure these methods are implemented before production use, or handle their absence appropriately.
Please confirm if these methods are intended to be implemented imminently or if they should return meaningful errors instead.
218-237: 🛠️ Refactor suggestion
Adjust trait and implementation for asynchronous get_messages
Since get_messages is now asynchronous due to the use of tokio::sync::Mutex, the XmtpApiSubscription trait and its implementations need to be updated to reflect this change.
Update the trait to have an asynchronous get_messages method:
- fn get_messages(&self) -> Vec<Envelope>;
+ async fn get_messages(&self) -> Vec<Envelope>;And ensure all implementations are updated accordingly.
Likely invalid or redundant comment.
xmtp_mls/src/builder.rs (7)
1-1: Importing Arc for Shared Ownership
The addition of use std::sync::Arc; is appropriate as it facilitates shared ownership of the ApiClient across multiple components without violating Rust's ownership rules.
111-112: Adding Trait Bounds to Enforce Requirements
Including the trait bounds ApiClient: XmtpApi and V: SmartContractSignatureVerifier ensures that the types used meet the necessary interface contracts, enhancing type safety and clarity.
116-117: Refactoring with inner_build_api_client for Modularity
Utilizing the inner_build_api_client function to extract API client initialization enhances code modularity and reusability. This refactoring makes the build_with_verifier method cleaner and separates concerns effectively.
126-126: Removing Redundant mut Keyword from self
The mut keyword has been removed from self in the build method signature. Since self is consumed and not modified directly, this change simplifies the method signature without affecting functionality.
159-166: Ensure Comprehensive Error Handling in inner_build
Verify that all potential errors within inner_build are correctly propagated and handled. This includes checking the initialization of all components and ensuring that any failures are communicated back to the caller.
264-266: Generic register_client Function Enhances Flexibility
The register_client function has been updated to be generic over C and V, allowing it to work with any Client type that implements XmtpApi and SmartContractSignatureVerifier. This is a good practice that increases code reusability.
548-548: Consistent Instantiation of ApiClientWrapper
Using mock_api.into() when creating an ApiClientWrapper ensures that the mock API client is correctly transformed into the expected type. Consistency in this pattern aids readability and maintenance.
Also applies to: 590-590
xmtp_mls/src/subscriptions.rs (5)
7-7: Import Updated to Include XmtpMlsStreams
The import statement now includes XmtpMlsStreams, which is necessary for the streaming functionalities introduced in the updated methods.
51-51: Improved Variable Naming in Clone Implementation
Renamed the pattern match variable from c to group in the Clone implementation of LocalEvents to enhance code readability and clarity.
245-246: Added XmtpMlsStreams Trait Bound to ApiClient
In the Client implementation, ApiClient now includes the XmtpMlsStreams trait bound. Verify that this addition is compatible with the existing ApiClient implementations.
Refer to the previous verification script to ensure ApiClient implements XmtpMlsStreams.
192-195: Verify ApiClient Implements XmtpMlsStreams
The stream_conversations method now requires ApiClient to implement the XmtpMlsStreams trait. Ensure that the ApiClient in use satisfies this constraint to avoid any compilation errors.
Run the following script to confirm the implementation:
#!/bin/bash
# Description: Verify that `ApiClient` implements `XmtpMlsStreams`.
# Test: Search for implementations of `XmtpMlsStreams` for `ApiClient`. Expect: Implementation exists.
ast-grep 'impl XmtpMlsStreams for $_' --lang Rust130-131: Ensure Removal of Clone Trait Does Not Impact Functionality
The Clone trait bound has been removed from ApiClient. Please verify that this change does not affect any functionality that relies on cloning ApiClient.
Run the following script to check for instances where ApiClient is cloned:
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the concern about the Clone trait removal:
No Impact from Clone Trait Removal
The Clone trait removal from ApiClient is safe since the codebase uses Arc<ApiClient> for shared ownership where cloning is needed, rather than directly cloning the ApiClient itself. This is evident from:
ApiClientWrapperwraps the client inArc<ApiClient>- No direct usage of
api_client.clone()was found - All client sharing is done through
Arccloning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any occurrences of `self.api_client.clone()`.
# Test: Find all instances of `self.api_client.clone()`. Expect: No results.
rg 'self\.api_client\.clone\(\)'
Length of output: 175
Script:
#!/bin/bash
# Let's search for any usage of api_client in the codebase to understand its usage pattern
rg "api_client" -A 3 -B 3
# Also search for Clone trait implementations or derivations for types containing ApiClient
rg "#\[derive\(.*Clone.*\)\]" -A 3
Length of output: 254506
xmtp_mls/src/client.rs (4)
231-238: Wrapping Fields in Arc for Shared Ownership
The changes to wrap api_client and scw_verifier in Arc are appropriate for enabling shared ownership and thread-safe access across multiple instances of Client.
Line range hint 242-252: Updated Clone Implementation Aligns with Arc Usage
The Clone implementation correctly clones the Arc-wrapped fields, ensuring that shared resources are properly managed.
301-302: Removed Unnecessary Clone Trait Bounds
Since ApiClient and V are now wrapped in Arc, the explicit Clone trait bounds on these generic parameters are no longer needed, simplifying the type constraints.
344-345: Consistency in Generic Parameter Constraints
The removal of the Clone trait bounds on ApiClient and V here matches the changes made earlier and is appropriate due to the use of Arc.
bindings_ffi/src/mls.rs (1)
155-157: LGTM: Correct wrapping of api_client with ApiClientWrapper
The api_client is now appropriately wrapped with ApiClientWrapper::new to include default retry logic, ensuring consistent error handling and retry policies.
| @@ -14,6 +14,7 @@ tonic = { workspace = true, features = ["tls", "tls-native-roots", "tls-webpki-r | |||
| tracing.workspace = true | |||
| xmtp_proto = { path = "../xmtp_proto", features = ["proto_full"] } | |||
| xmtp_v2 = { path = "../xmtp_v2" } | |||
| async-trait = "0.1" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider managing async-trait version at workspace level.
For consistency with other dependencies in this file, consider moving the async-trait version specification to the workspace level and using .workspace = true here.
-async-trait = "0.1"
+async-trait.workspace = true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async-trait = "0.1" | |
| async-trait.workspace = true |
| @@ -10,7 +10,7 @@ pbjson.workspace = true | |||
| prost = { workspace = true, features = ["prost-derive"] } | |||
| serde = { workspace = true } | |||
| openmls = { workspace = true, optional = true } | |||
| trait-variant = "0.1.2" | |||
| async-trait = "0.1" | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration to async-trait needs attention in other crates
The dependency switch requires coordination across multiple crates:
trait-variantis still present in./xmtp_mls/Cargo.tomland rootCargo.tomltrait_variantattribute is still used inxmtp_mls/src/groups/scoped_client.rs- While
async-traitis widely adopted (present in 6 Cargo.toml files), this mixed usage could lead to compatibility issues
🔗 Analysis chain
LGTM: Good choice switching to async-trait!
The switch from trait-variant to async-trait is a solid improvement. async-trait is more widely used and maintained in the Rust ecosystem.
Let's verify the async-trait usage across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify async-trait usage and potential migration gaps
# Test 1: Check for any remaining trait-variant usage that needs migration
echo "Checking for remaining trait-variant usage..."
rg "trait-variant"
# Test 2: Verify async-trait attribute usage
echo "Verifying async-trait attribute usage..."
rg "#\[async_trait\]"
Length of output: 352
Script:
#!/bin/bash
# Let's try a more comprehensive search for both patterns
# Check for trait-variant in Cargo.toml files
echo "Checking Cargo.toml files for trait-variant..."
fd "Cargo.toml" --exec grep -l "trait-variant" {} \;
# Check for async-trait in Cargo.toml files
echo "Checking Cargo.toml files for async-trait..."
fd "Cargo.toml" --exec grep -l "async-trait" {} \;
# Look for async-trait attribute usage in Rust files
echo "Checking Rust files for async-trait attribute usage..."
rg --type rust "#\[async_trait\]" -l
# Look for trait-variant attribute usage in Rust files
echo "Checking Rust files for trait-variant attribute usage..."
rg --type rust "trait_variant" -l
Length of output: 1162
insipx
force-pushed
the
insipx/replication-client
branch
from
2b7c71e to
3cfb7ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
xmtp_id/src/utils/test.rs (1)
119-171: Add proper resource cleanup
The test harness functions should ensure proper cleanup of resources (Anvil instance, HTTP connections) after tests complete, especially in case of test failures. Consider implementing a cleanup mechanism using Drop trait or wrapping the test execution in a defer block.
Example approach:
struct TestHarness {
anvil: AnvilInstance,
provider: Provider<Http>,
// other fields...
}
impl Drop for TestHarness {
fn drop(&mut self) {
// Cleanup resources
}
}Also applies to: 176-215
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- bindings_node/src/inbox_id.rs (1 hunks)
- xmtp_id/src/utils/test.rs (1 hunks)
- xmtp_mls/src/api/test_utils.rs (1 hunks)
- xmtp_proto/src/api_client.rs (9 hunks)
🔇 Additional comments (27)
bindings_node/src/inbox_id.rs (3)
Line range hint 8-31: Implementation looks sound
The changes to error handling and type conversion in get_inbox_id_for_address appear well-structured and consistent with the codebase patterns. The function maintains clear separation of concerns between API client creation, inbox ID retrieval, and error handling.
Line range hint 34-39: Consider future extensibility of hardcoded nonce
While the hardcoded nonce of 1 is well-documented and currently aligned with create_client, this might limit future use cases. Consider whether this function should be more flexible by accepting a nonce parameter with a default value of 1.
19-20: Verify error propagation with the new type conversion
The addition of .into() after error mapping suggests a type conversion in the API client creation chain. Let's verify that errors are properly propagated to the Node.js side.
✅ Verification successful
Based on the comprehensive results, I can now provide the final response:
Error handling chain is correctly implemented
The .into() conversion after error mapping is correct and necessary. The code follows a consistent pattern across the codebase where:
TonicApiClient::create()result is mapped toErrorWrapper- The wrapped client is then converted into
ApiClientWrapperusing.into()
This pattern is used consistently in multiple places:
bindings_node/src/inbox_id.rsbindings_ffi/src/mls.rsbindings_wasm/src/inbox_id.rs
The ErrorWrapper type is properly implemented in lib.rs with all necessary conversions for error propagation to Node.js.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other similar error handling patterns in the codebase
rg -A 2 "TonicApiClient::create.*map_err.*into"
# Look for error type definitions and conversions
ast-grep --pattern 'impl From<$_> for ErrorWrapper { $$ }'
Length of output: 110
Script:
#!/bin/bash
# Let's first check the error wrapper implementation
rg -l "ErrorWrapper"
# Then look for error handling patterns in the codebase
rg "map_err\(ErrorWrapper" -A 2
# Check the TonicApiClient usage
rg "TonicApiClient::create" -A 3
# Look for From implementations
rg "impl From.*for ErrorWrapper"
Length of output: 13517
Script:
#!/bin/bash
# Let's check the ErrorWrapper definition and its implementations
rg -A 10 "struct ErrorWrapper"
# Check the specific file for error handling context
cat bindings_node/src/lib.rs | grep -A 10 "ErrorWrapper"
# Look for the ApiClientWrapper type and its usage
rg "ApiClientWrapper" -A 3
Length of output: 13560
xmtp_id/src/utils/test.rs (1)
24-46: LGTM!
The SmartContracts implementation is clean and well-structured, with appropriate WASM compilation guards.
xmtp_mls/src/api/test_utils.rs (3)
44-47: LGTM! Clean module structure with proper target-specific implementations.
The conditional compilation setup correctly handles different target architectures while maintaining a clean public API.
51-113: LGTM! Comprehensive mock implementation for non-WASM targets.
The implementation correctly:
- Uses async traits for async methods
- Handles different stream types based on features
- Provides all required trait methods with proper signatures
115-141: LGTM! Well-structured WASM-specific implementation.
The implementation correctly:
- Uses
async_trait(?Send)for WASM compatibility - Provides WASM-specific stream types
- Mirrors the non-WASM implementation while maintaining WASM constraints
Also applies to: 151-175
xmtp_proto/src/api_client.rs (20)
23-25: LGTM!
The XmtpTestClient trait is correctly defined with the necessary async attributes for both wasm32 and non-wasm32 targets.
35-35: LGTM!
The import of the XmtpTestClient trait is correct and consistent with the trait renaming.
41-41: LGTM!
The imports of the necessary traits for the XmtpApi trait are correct and consistent with the trait renamings.
45-45: LGTM!
The trait bounds for the XmtpApi trait are correctly defined, including the necessary Send and Sync bounds for non-wasm32 targets.
49-49: LGTM!
The trait bounds for the XmtpApi trait implementation are correctly defined, including the necessary Send and Sync bounds for non-wasm32 targets.
59-59: LGTM!
The imports of the necessary traits for the XmtpApi trait are correct and consistent with the trait renamings.
63-63: LGTM!
The trait bounds for the XmtpApi trait are correctly defined for wasm32 targets.
67-67: LGTM!
The trait bounds for the XmtpApi trait implementation are correctly defined for wasm32 targets.
73-73: LGTM!
The imports of the necessary traits for the XmtpApi trait are correct and consistent with the trait renamings.
77-77: LGTM!
The trait bounds for the XmtpApi trait are correctly defined, including the necessary Send and Sync bounds for non-wasm32 targets in test mode.
81-81: LGTM!
The trait bounds for the XmtpApi trait implementation are correctly defined, including the necessary Send and Sync bounds for non-wasm32 targets in test mode.
89-89: LGTM!
The imports of the necessary traits for the XmtpApi trait are correct and consistent with the trait renamings.
93-93: LGTM!
The trait bounds for the XmtpApi trait are correctly defined for wasm32 targets in test mode.
98-98: LGTM!
The trait bounds for the XmtpApi trait implementation are correctly defined for wasm32 targets in test mode.
189-190: LGTM!
The MutableApiSubscription trait is correctly defined with the necessary async attributes for both wasm32 and non-wasm32 targets.
221-223: LGTM!
The XmtpApiClient trait is correctly defined with the necessary async attributes for both wasm32 and non-wasm32 targets.
285-287: LGTM!
The XmtpMlsClient trait is correctly defined with the necessary async attributes for both wasm32 and non-wasm32 targets.
349-368: LGTM!
The XmtpMlsStreams trait is correctly defined for non-wasm32 targets, with the necessary Send bounds on the associated stream types.
Line range hint 370-389: LGTM!
The XmtpMlsStreams trait is correctly defined for wasm32 targets, without the Send bounds on the associated stream types.
420-422: LGTM!
The XmtpIdentityClient trait is correctly defined with the necessary async attributes for both wasm32 and non-wasm32 targets.
| #[cfg(all(not(feature = "http-api"), not(target_arch = "wasm32")))] | ||
| type GroupMessageStream<'a> = xmtp_api_grpc::GroupMessageStream; | ||
| #[cfg(all(not(feature = "http-api"), not(target_arch = "wasm32")))] | ||
| type WelcomeMessageStream<'a> = xmtp_api_grpc::WelcomeMessageStream; | ||
|
|
||
| #[cfg(all(feature = "http-api", not(target_arch = "wasm32")))] | ||
| type GroupMessageStream<'a> = futures::stream::BoxStream<'static, Result<GroupMessage, Error>>; | ||
| #[cfg(all(feature = "http-api", not(target_arch = "wasm32")))] | ||
| type WelcomeMessageStream<'a> = futures::stream::BoxStream<'static, Result<WelcomeMessage, Error>>; |
There was a problem hiding this comment.
Remove unreachable stream type definitions.
These stream type definitions are guarded by not(target_arch = "wasm32") but exist within a module that is only compiled for WASM targets. They can be safely removed.
Apply this diff to remove the unreachable code:
impl XmtpMlsStreams for ApiClient {
- #[cfg(all(not(feature = "http-api"), not(target_arch = "wasm32")))]
- type GroupMessageStream<'a> = xmtp_api_grpc::GroupMessageStream;
- #[cfg(all(not(feature = "http-api"), not(target_arch = "wasm32")))]
- type WelcomeMessageStream<'a> = xmtp_api_grpc::WelcomeMessageStream;
-
- #[cfg(all(feature = "http-api", not(target_arch = "wasm32")))]
- type GroupMessageStream<'a> = futures::stream::BoxStream<'static, Result<GroupMessage, Error>>;
- #[cfg(all(feature = "http-api", not(target_arch = "wasm32")))]
- type WelcomeMessageStream<'a> = futures::stream::BoxStream<'static, Result<WelcomeMessage, Error>>;
#[cfg(target_arch = "wasm32")]
type GroupMessageStream<'a> = futures::stream::LocalBoxStream<'static, Result<GroupMessage, Error>>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(all(not(feature = "http-api"), not(target_arch = "wasm32")))] | |
| type GroupMessageStream<'a> = xmtp_api_grpc::GroupMessageStream; | |
| #[cfg(all(not(feature = "http-api"), not(target_arch = "wasm32")))] | |
| type WelcomeMessageStream<'a> = xmtp_api_grpc::WelcomeMessageStream; | |
| #[cfg(all(feature = "http-api", not(target_arch = "wasm32")))] | |
| type GroupMessageStream<'a> = futures::stream::BoxStream<'static, Result<GroupMessage, Error>>; | |
| #[cfg(all(feature = "http-api", not(target_arch = "wasm32")))] | |
| type WelcomeMessageStream<'a> = futures::stream::BoxStream<'static, Result<WelcomeMessage, Error>>; | |
| #[cfg(target_arch = "wasm32")] | |
| type GroupMessageStream<'a> = futures::stream::LocalBoxStream<'static, Result<GroupMessage, Error>>; |
insipx
force-pushed
the
insipx/replication-client
branch
from
3cfb7ee to
14c5c85
Compare
insipx
force-pushed
the
insipx/replication-client
branch
from
14c5c85 to
cb6c0ae
Compare
Summary by CodeRabbit
Release Notes
New Features
testnetflag for connecting to test networks.replication_clientmodule for expanded API functionalities.Bug Fixes
Documentation
Refactor
Arcfor improved concurrency support.Chores