Consolidate providers #168
Conversation
WalkthroughThe changes in this pull request involve a significant refactoring of the provider management and attachment logic within various components of the codebase. The Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
packages/ciphernode/evm/src/enclave_sol.rs (1)
17-18: LGTM! Good separation of read and write providers.The signature change from using a single RPC URL to separate read and write providers is a positive architectural improvement that:
- Enforces clear separation between read and write operations
- Prevents potential issues with upstream services by controlling access patterns
- Aligns perfectly with the PR objective of consolidating providers
This change establishes a solid foundation for implementing the principle of least privilege, where components only get the access level they need.
packages/ciphernode/enclave_node/src/ciphernode.rs (1)
48-51: LGTM: Clean provider consolidation implementation.The implementation correctly uses a single read-only provider for both contracts, aligning with the PR's objective. Consider extracting the provider creation to a separate line for better readability:
- let read_provider = create_readonly_provider(&ensure_ws_rpc(rpc_url)).await?; + let ws_rpc = ensure_ws_rpc(rpc_url); + let read_provider = create_readonly_provider(&ws_rpc).await?;packages/ciphernode/enclave_node/src/aggregator.rs (1)
42-62: Excellent provider consolidation strategy!The implementation effectively prevents upstream service issues through:
- Clear separation between read-only (WebSocket) and writer (HTTP) providers
- Single provider instance per role per chain
- Appropriate provider usage based on contract requirements
This architecture will help maintain consistent connections and prevent potential race conditions or conflicts with upstream services.
packages/ciphernode/evm/src/ciphernode_registry_sol.rs (1)
100-115: Excellent provider consolidation implementation.The transition from raw RPC URLs to typed providers brings several architectural benefits:
- Type-safe access control through
ReadonlyProvider- Centralized provider management
- Better encapsulation of blockchain interaction details
- Reduced risk of upstream service issues through controlled access
This implementation serves as a good example for similar refactoring in other parts of the system.
packages/ciphernode/evm/src/enclave_sol_writer.rs (3)
37-45: LGTM! Provider-based approach aligns with consolidation goals.The change from RPC URL to SignerProvider improves the architecture by centralizing provider management.
Consider removing the redundant
signerparameter since theSignerProvidershould already contain the necessary signer:pub async fn new( bus: &Addr<EventBus>, provider: &SignerProvider, contract_address: Address, - signer: &Arc<PrivateKeySigner>, ) -> Result<Self> {
Line range hint
49-63: Enhance input validation and remove redundancy.The attach method changes align well with the provider consolidation, but there are two improvements to consider:
- Remove the redundant
signerparameter as it's already part of the SignerProvider:pub async fn attach( bus: &Addr<EventBus>, provider: &SignerProvider, contract_address: &str, - signer: &Arc<PrivateKeySigner>, ) -> Result<Addr<EnclaveSolWriter>> { - let addr = EnclaveSolWriter::new(bus, provider, contract_address.parse()?, signer) + let addr = EnclaveSolWriter::new(bus, provider, contract_address.parse()?)
- Add validation for the contract address before parsing:
let contract_address = Address::from_str(contract_address) .map_err(|e| anyhow::anyhow!("Invalid contract address: {}", e))?;
Line range hint
1-145: Architecture aligns well with provider consolidation goals.The refactoring to use
SignerProviderinstead of raw RPC URLs is a solid architectural improvement that:
- Centralizes provider management
- Maintains clean separation between reading and writing operations
- Preserves existing event handling and error management
This implementation supports the PR's goal of preventing upstream service issues through provider consolidation.
Consider documenting the provider lifecycle and ownership model in the module-level documentation to help future maintainers understand the design decisions.
packages/ciphernode/evm/src/registry_filter_sol.rs (2)
133-138: Consider enhancing error context.While the implementation is correct, consider adding context to errors for better debugging:
pub async fn attach( bus: &Addr<EventBus>, provider: &SignerProvider, contract_address: &str, ) -> Result<()> { - RegistryFilterSolWriter::attach(bus, provider, contract_address).await?; + RegistryFilterSolWriter::attach(bus, provider, contract_address) + .await + .context("Failed to attach RegistryFilterSolWriter")?; Ok(()) }
Chain ID validation is correctly implemented, but logging can be improved
The chain ID validation is properly implemented across the codebase:
- Chain ID is securely captured during provider initialization via
WithChainIdwrapper- Consistent validation pattern is used in both
registry_filter_sol.rsandenclave_sol_writer.rs- The validation ensures events are only processed for matching source chains
However, there is no logging when events are skipped due to chain ID mismatch, which could make debugging harder. Consider adding trace/debug level logging in the else branch of the chain ID checks.
Example locations:
packages/ciphernode/evm/src/registry_filter_sol.rs:68packages/ciphernode/evm/src/enclave_sol_writer.rs:77🔗 Analysis chain
Line range hint
66-73: Consider strengthening chain ID validation.The chain ID validation in the event handler is crucial for ensuring messages are processed by the correct chain. Consider adding explicit validation when the provider is first attached, and logging when messages are skipped due to chain ID mismatch.
Let's verify chain ID validation patterns across the codebase:
Let's also check the provider initialization:
And let's examine the event handling patterns:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent chain ID validation patterns # Expect: Chain ID validations should be consistent across provider usage rg -A 3 'get_chain_id|chain_id'Length of output: 12429
Script:
#!/bin/bash # Let's check for chain ID validation patterns in the provider initialization and usage ast-grep --pattern 'provider.get_chain_id()' # Let's also check for any logging related to chain ID mismatches rg -B 2 -A 2 'chain.*mismatch|skip.*chain'Length of output: 188
Script:
#!/bin/bash # Check provider initialization patterns ast-grep --pattern 'WithChainId::new'Length of output: 37
Script:
#!/bin/bash # Check event handling patterns for chain ID validation ast-grep --pattern $'if $_.get_chain_id() == $_ { $$$ }'Length of output: 634
packages/ciphernode/evm/src/helpers.rs (2)
69-70: Enhance documentation for WithChainId struct.While the current documentation explains the "what", it would be beneficial to add:
- Why chain ID caching is important (performance, consistency)
- Example usage
- Error conditions for the async constructor
Add comprehensive documentation like this:
-/// We need to cache the chainId so we can easily use it in a non-async situation -/// This wrapper just stores the chain_id with the Provider +/// A wrapper that caches the chain ID alongside a Provider to avoid repeated async calls. +/// +/// # Purpose +/// - Caches chain ID for synchronous access in performance-critical paths +/// - Ensures consistency by using the same chain ID throughout the provider's lifetime +/// +/// # Example +/// ```rust +/// let provider = create_readonly_provider("http://...").await?; +/// let with_chain = WithChainId::new(provider).await?; +/// let chain_id = with_chain.get_chain_id(); // Synchronous access +/// ``` +/// +/// # Errors +/// The constructor will fail if: +/// - The provider fails to fetch the chain ID +/// - The connection to the node fails
Line range hint
126-146: Enhance error handling in signer management functions.While the functions work correctly, consider improving error handling by:
- Using custom error types instead of generic anyhow::Error
- Adding specific error messages for different failure scenarios
Consider implementing custom errors like this:
#[derive(Debug, thiserror::Error)] pub enum SignerError { #[error("Environment variable {0} not found")] EnvVarNotFound(String), #[error("Invalid private key format")] InvalidPrivateKey(#[source] alloy::SignerError), #[error("Repository error")] RepositoryError(#[from] RepositoryError), // ... other specific errors } pub async fn pull_eth_signer_from_env(var: &str) -> Result<Arc<PrivateKeySigner>, SignerError> { let private_key = env::var(var) .map_err(|_| SignerError::EnvVarNotFound(var.to_string()))?; // ... rest of the implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
packages/ciphernode/enclave_node/src/aggregator.rs(2 hunks)packages/ciphernode/enclave_node/src/ciphernode.rs(2 hunks)packages/ciphernode/evm/src/ciphernode_registry_sol.rs(2 hunks)packages/ciphernode/evm/src/enclave_sol.rs(2 hunks)packages/ciphernode/evm/src/enclave_sol_reader.rs(2 hunks)packages/ciphernode/evm/src/enclave_sol_writer.rs(1 hunks)packages/ciphernode/evm/src/event_reader.rs(2 hunks)packages/ciphernode/evm/src/helpers.rs(1 hunks)packages/ciphernode/evm/src/registry_filter_sol.rs(2 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/ciphernode/evm/src/enclave_sol.rs (1)
Learnt from: ryardley
PR: gnosisguild/enclave#145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
packages/ciphernode/evm/src/enclave_sol_reader.rs (1)
Learnt from: ryardley
PR: gnosisguild/enclave#145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
🔇 Additional comments (15)
packages/ciphernode/evm/src/enclave_sol.rs (2)
3-7: LGTM! Clean import organization.
The imports are well-organized, clearly separating internal module imports from external crate dependencies. The addition of ReadonlyProvider and SignerProvider from the helpers module aligns with the PR's objective of consolidating providers.
22-23: LGTM! Clean implementation of the new provider pattern.
The implementation correctly delegates to the respective reader and writer components using the new provider parameters. This maintains the existing separation of concerns while upgrading to the new provider-based approach.
Let's verify that all related components have been updated to use the new provider pattern:
✅ Verification successful
Provider pattern migration is complete and consistent across the codebase
The verification confirms that all attach methods across the codebase have been properly updated to use the new provider pattern:
- All instances use either
ReadonlyProviderorSignerProviderparameters - No direct RPC URL parameters in attach methods
- The only remaining
rpc_urlreferences are in configuration files and helper functions that create providers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all attach methods in related components use the new provider pattern
# Expected: No instances of rpc_url in attach methods
# Search for any remaining rpc_url parameters in attach methods
ast-grep --pattern 'fn attach($$$, $rpc:rpc_url, $$$)'
# Search for any direct URL string parameters in attach methods
rg -p 'fn attach.*String.*\)'
Length of output: 88
Script:
#!/bin/bash
# Let's try a broader search to find all attach methods and their implementations
# This will help us verify the provider pattern adoption
# Search for attach method signatures and their implementations
ast-grep --pattern 'pub async fn attach($$$) -> Result<$_> { $$$}'
# Also search for any remaining references to rpc_url in the codebase
rg -p "rpc_url"
# And look for provider-related patterns
rg -p "provider.*attach|attach.*provider" -A 2
Length of output: 11203
packages/ciphernode/enclave_node/src/ciphernode.rs (2)
8-11: LGTM: Clean import organization for provider consolidation.
The new imports appropriately support the transition to consolidated providers with clear helper functions for provider creation and WebSocket RPC handling.
48-51: Verify consistent provider consolidation pattern.
Let's verify that this provider consolidation pattern is consistently applied across other contract attachments in the codebase.
✅ Verification successful
Let me gather more information about the contract attachment patterns to verify consistency.
Provider usage pattern is consistently applied across contract attachments
The verification shows that the provider consolidation pattern is consistently implemented:
- In
ciphernode.rs, the read-only provider is created once and reused for bothEnclaveSolReaderandCiphernodeRegistrySolattachments - In
aggregator.rs, the same pattern is followed where a single read-only provider is created and reused across multiple contract attachments - The
create_readonly_providerfunction is consistently used withensure_ws_rpcfor websocket connections - No instances found of direct RPC URL usage in contract attachments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent provider usage pattern across contract attachments
# Test 1: Look for direct RPC URL usage in attach methods
echo "Checking for direct RPC URL usage in attach methods..."
rg -A 2 "attach.*rpc.*url"
# Test 2: Verify provider usage pattern
echo "Checking for consolidated provider usage pattern..."
ast-grep --pattern 'create_readonly_provider($$$)'
Length of output: 593
Script:
#!/bin/bash
# Look for all contract attachments and their provider usage
echo "Checking contract attachment patterns..."
rg -B 3 -A 1 "::attach\("
# Look for create_readonly_provider usage context
echo "Checking provider creation context..."
rg -B 2 -A 2 "create_readonly_provider"
Length of output: 10567
packages/ciphernode/enclave_node/src/aggregator.rs (2)
7-11: LGTM! Well-structured imports for provider management.
The new imports clearly reflect the separation between read-only and writer providers, supporting the PR's objective of provider consolidation.
49-62: LGTM! Well-structured provider consolidation.
The implementation correctly separates concerns between read and write operations:
- Read-only operations use WebSocket RPC
- Write operations use HTTP RPC with signer
- Contract attachments appropriately use the respective providers based on their needs
Let's verify the WebSocket connection stability configuration:
✅ Verification successful
Let me search for provider creation and connection handling implementation to verify the WebSocket stability configuration.
Let me try a different approach to find the provider implementation.
Based on the code analysis, I can now provide the final response:
WebSocket provider implementation is robust and well-configured
The implementation shows proper WebSocket connection handling:
- Uses
ProviderBuilderwith built-in connection management - Includes error context for connection failures
- Properly converts between HTTP/HTTPS and WS/WSS URLs
- Separates read-only operations to dedicated WebSocket connections
- Has proper error propagation with
Resulttypes
The architecture ensures stable WebSocket connections through the provider's built-in connection management, making the original concern about connection stability unnecessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for WebSocket connection handling and retry mechanisms
# Test: Look for WebSocket-related configuration and retry logic
rg -A 5 'ws|websocket|retry|reconnect' --type rust
Length of output: 7990
Script:
#!/bin/bash
# Check provider creation implementation
ast-grep --pattern 'fn create_readonly_provider($$$)'
Length of output: 55
Script:
#!/bin/bash
# Search for provider creation and connection handling
rg -A 10 "create_readonly_provider" --type rust
Length of output: 5725
packages/ciphernode/evm/src/event_reader.rs (2)
26-33: LGTM: Constructor changes align with provider consolidation.
The change from URL-based provider creation to using a pre-configured ReadonlyProvider aligns well with the PR's objective of consolidating providers. The implementation maintains proper ownership semantics through cloning.
43-47: Verify WebSocket support in the provided ReadonlyProvider.
While the provider consolidation looks good, we should verify that the provided ReadonlyProvider supports WebSocket connections, which was previously enforced by ensure_ws_rpc. This is crucial for proper event streaming.
packages/ciphernode/evm/src/enclave_sol_reader.rs (2)
1-1: LGTM: Import aligns with provider consolidation strategy.
The addition of ReadonlyProvider import aligns with the PR's objective of consolidating providers and moving away from direct RPC URLs.
86-89: LGTM: Provider abstraction properly implemented.
The change from rpc_url to ReadonlyProvider in the method signature is consistent with the PR's goal of consolidating providers. The implementation maintains its simplicity while providing better type safety and encapsulation through the provider abstraction.
packages/ciphernode/evm/src/ciphernode_registry_sol.rs (2)
1-1: LGTM! Import aligns with provider consolidation strategy.
The addition of ReadonlyProvider import supports the PR's objective of consolidating providers and enforcing read-only access where appropriate.
110-115: LGTM! Consistent implementation with reader.
The changes maintain proper delegation to the reader while adopting the new provider-based approach.
packages/ciphernode/evm/src/registry_filter_sol.rs (2)
32-38: LGTM! Provider consolidation implemented correctly.
The change from accepting raw RPC URL and signer to using a SignerProvider simplifies the implementation while maintaining thread safety through proper cloning.
Line range hint 44-54: LGTM! Verify event subscription remains functional.
The provider consolidation is implemented correctly. The event bus subscription is preserved, maintaining the actor system's message handling capabilities.
Let's verify the event subscription pattern is consistent across the codebase:
✅ Verification successful
Event subscription pattern is consistent and correctly implemented
The verification shows that:
- The
Subscribe::new("PublicKeyAggregated", ...)pattern inregistry_filter_sol.rsis consistent with the event handling architecture - The subscription is properly integrated with the event bus system alongside other event types like "PlaintextAggregated" and "Shutdown"
- The
PublicKeyAggregatedevent is correctly defined and used across the codebase in the aggregator, registry filter, and tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent event subscription patterns
# Expect: All Subscribe::new calls for "PublicKeyAggregated" follow similar pattern
rg -A 2 'Subscribe::new.*"PublicKeyAggregated"'
Length of output: 12429
Script:
#!/bin/bash
# Let's try a different search pattern to find Subscribe::new calls
rg -A 3 'Subscribe::new\('
# Also search for event handling related to PublicKeyAggregated
ast-grep --pattern 'PublicKeyAggregated'
Length of output: 2982
packages/ciphernode/evm/src/helpers.rs (1)
Line range hint 71-124: LGTM! Well-structured provider abstraction.
The implementation effectively consolidates provider types and provides clear factory methods, aligning with the PR objectives of having distinct read-only and writer providers.
Let's verify the adoption of these new provider types across the codebase:
✅ Verification successful
Provider types are consistently used across the codebase
The verification shows that:
- No direct RPC URL provider instantiation found outside helpers.rs
- Both
ReadonlyProviderandSignerProviderare properly imported and used across multiple components:enclave_sol.rs: Uses both providers for read/write operationsregistry_filter_sol.rs: Uses SignerProvider for write operationsenclave_sol_reader.rs: Uses ReadonlyProvider for read operationsevent_reader.rs: Uses ReadonlyProvider for event readingciphernode_registry_sol.rs: Uses ReadonlyProvider for registry operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that old provider patterns are fully migrated to new types
# Test 1: Check for direct RPC URL usage that should use ReadonlyProvider
echo "Checking for direct RPC URL usage..."
rg -t rust "Provider.*::new.*http" --glob '!src/helpers.rs'
# Test 2: Check adoption of new provider types
echo "Checking adoption of new provider types..."
rg -t rust "ReadonlyProvider|SignerProvider"
Length of output: 2912
closes: #167
This creates one readonly websocket provider and one http write provider per instance and passes them down the stack instead of creating multiple providers or multiple websocket connections.
Summary by CodeRabbit
New Features
ReadonlyProviderandSignerProvider) for improved handling of blockchain interactions.Bug Fixes
Refactor
Documentation