Add EVM Tests #170
Add EVM Tests #170
Conversation
WalkthroughThe pull request introduces several key updates to the project's CI workflow and codebase. It enhances the Rust CI configuration by adding steps to install Foundry and the Solidity compiler, while also changing the test execution command from a Rust-specific approach to a Yarn-based one. Additionally, modifications are made across various files to improve type safety and flexibility, particularly around provider types. New scripts and tests are introduced, including a Solidity contract for event logging, reflecting a comprehensive enhancement of both the testing framework and the underlying code architecture. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 9
🧹 Outside diff range and nitpick comments (17)
packages/ciphernode/scripts/test.sh (1)
1-7: Consider adding usage information and verbose mode.The script would benefit from having usage information and a verbose mode for debugging.
#!/usr/bin/env bash set -e + +# Usage information +show_usage() { + cat << EOF +Usage: $(basename "$0") [test arguments] + +This script builds EVM test fixtures and runs cargo tests. +Required dependencies: anvil, solc, cargo + +Environment variables: + VERBOSE Set to 1 for verbose output +EOF +} + +# Show usage if -h or --help is passed +if [[ "$1" == "-h" ]] || [[ "$1" == "--help" ]]; then + show_usage + exit 0 +fi + +# Enable verbose mode if requested +[[ "${VERBOSE:-0}" == "1" ]] && set -x pushd ./evm && ./scripts/build_fixtures.sh && popd cargo test -- "$@"🧰 Tools
🪛 Shellcheck
[error] 7-7: Double quote array expansions to avoid re-splitting elements.
(SC2068)
packages/ciphernode/evm/tests/fixtures/emit_logs.sol (1)
17-20: Consider adding input validation.While this is a test fixture, consider adding basic input validation to prevent potential issues during testing:
- Check for empty strings
- Validate string length if there are any constraints
function setValue(string memory value) public { + require(bytes(value).length > 0, "Value cannot be empty"); emit ValueChanged(msg.sender, _value, value); _value = value; }packages/ciphernode/evm/src/enclave_sol.rs (1)
14-15: LGTM! Good enhancement of type safety.The update to use
WithChainIdwrapper for both providers is a positive change that:
- Ensures proper chain ID awareness
- Maintains consistency with the broader codebase updates
- Enables generic Transport support as intended
This change improves the architecture by making chain ID handling more explicit and type-safe, which is crucial for multi-chain compatibility.
.github/workflows/rust-ci.yml (3)
22-24: LGTM with minor formatting fix needed.The Foundry installation using the official action is appropriate for the EVM testing requirements.
Remove trailing whitespace at the end of line 24:
- uses: foundry-rs/foundry-toolchain@v1 + uses: foundry-rs/foundry-toolchain@v1🧰 Tools
🪛 yamllint
[error] 24-24: trailing spaces
(trailing-spaces)
26-30: Consider improving the solc installation step.While the current implementation works, consider these improvements:
- Pin the solc version for reproducible builds
- Use YAML multi-line syntax instead of shell line continuation
- name: Install solc run: | - sudo add-apt-repository ppa:ethereum/ethereum \ - && sudo apt-get update -y \ - && sudo apt-get install -y solc + sudo add-apt-repository ppa:ethereum/ethereum + sudo apt-get update -y + sudo apt-get install -y solc=0.8.21 # Pin to specific version
Line range hint
21-54: Add Anvil availability check.Since Anvil is required for tests (as mentioned in PR objectives), consider adding a verification step after Foundry installation.
Add this step after Foundry installation:
- name: Verify Anvil installation run: | which anvil anvil --version🧰 Tools
🪛 actionlint
34-34: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
packages/ciphernode/Cargo.toml (1)
Line range hint
25-29: Consider aligning alloy dependency versions.The main alloy dependency is at 0.5.2 while alloy-primitives and alloy-sol-types are at 0.6. Consider aligning these versions for better maintainability.
-alloy = { version = "0.5.2", features = ["full", "node-bindings"] } -alloy-primitives = { version = "0.6", default-features = false, features = [ +alloy = { version = "0.6", features = ["full", "node-bindings"] } +alloy-primitives = { version = "0.6", default-features = false, features = [packages/ciphernode/evm/tests/evm_reader.rs (2)
22-30: Add error handling and configuration for provider setup.While the setup is functional, consider these improvements for robustness:
- Add explicit error handling for Anvil availability
- Consider making block time configurable
- Add timeout configuration for WebSocket connection
+ const BLOCK_TIME: u64 = 1; + const WS_TIMEOUT: Duration = Duration::from_secs(30); - let anvil = Anvil::new().block_time(1).try_spawn()?; + let anvil = Anvil::new() + .block_time(BLOCK_TIME) + .try_spawn() + .map_err(|e| anyhow::anyhow!("Failed to spawn Anvil. Is it installed? Error: {}", e))?; let ws = WsConnect::new(anvil.ws_endpoint()); - let provider = ProviderBuilder::new().on_ws(ws).await?; + let provider = ProviderBuilder::new() + .on_ws(ws) + .timeout(WS_TIMEOUT) + .await?;
31-62: Add timeout for transaction confirmation.The event reader setup and contract interactions look good, but consider adding a timeout for transaction watching to prevent indefinite hangs in case of network issues.
+ const TX_TIMEOUT: Duration = Duration::from_secs(60); contract .setValue("hello".to_string()) .send() .await? .watch() + .timeout(TX_TIMEOUT) .await?;packages/ciphernode/evm/src/enclave_sol_reader.rs (1)
87-89: LGTM! Type changes enhance safety and provider compatibility.The updated signature properly handles chain ID awareness and generic transport support while maintaining the method's intended passthrough behavior.
This change improves type safety by making the chain ID requirement explicit at the type level, which helps prevent runtime errors related to missing chain IDs.
packages/ciphernode/evm/src/enclave_sol_writer.rs (1)
24-24: LGTM: Enhanced type safety with WithChainId wrapper.The provider field type change improves type safety by ensuring chain ID information is always available. This is particularly important for cross-chain operations and event filtering.
Consider documenting the chain ID requirements and validation strategy in the struct-level documentation, especially for cases where chain IDs need to match between different components.
packages/ciphernode/evm/src/ciphernode_registry_sol.rs (1)
Line range hint
74-74: Improve error messages and fix typo.Consider these improvements:
- Add more context to error messages, such as including the raw log data.
- Fix the typo in trace message ("buut" -> "but").
- error!("Error parsing event CiphernodeAdded after topic was matched!"); + error!("Error parsing event CiphernodeAdded after topic was matched. Log data: {:?}", data); - "Unknown event was received by Enclave.sol parser buut was ignored" + "Unknown event was received by Enclave.sol parser but was ignored"Also applies to: 89-89
packages/ciphernode/evm/src/registry_filter_sol.rs (1)
Error handling should be consistent with other subscription patterns
Looking at the codebase patterns, I can see that error handling for subscriptions is consistently implemented in other components:
enclave_sol_writer.rsproperly handles subscription errors with.await?event_reader.rsproperly handles subscription errors with.await?- Only
registry_filter_sol.rsdiscards the error withlet _ =The review comment's suggestion to handle the subscription error is valid and aligns with the established patterns in the codebase.
🔗 Analysis chain
Line range hint
43-52: Consider handling subscription errors explicitly.The subscription setup uses
let _ =which discards any potential errors from theSubscribemessage. Consider handling these errors explicitly to ensure robust event handling.- let _ = bus - .send(Subscribe::new("PublicKeyAggregated", addr.clone().into())) - .await; + bus.send(Subscribe::new("PublicKeyAggregated", addr.clone().into())) + .await + .map_err(|e| anyhow::anyhow!("Failed to subscribe to PublicKeyAggregated: {}", e))?;
The previous command had a syntax error. Let's try these variations to gather more context about subscription handling patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are other places in the codebase where Subscribe errors are handled rg -A 3 "Subscribe::new.*send\(\)"Length of output: 133
Script:
#!/bin/bash # Let's try a different approach to find Subscribe usage patterns rg -A 3 "Subscribe::" # Also check for similar event subscription patterns rg -A 3 "\.send\(Subscribe" # Look for the specific event name rg -A 3 "PublicKeyAggregated"Length of output: 11355
packages/ciphernode/core/src/events.rs (3)
127-131: Enhance the test event documentation.The comment "This is a test event to use in testing" is redundant. Consider adding more specific documentation about the event's purpose in testing event ordering and transmission issues, as mentioned in the PR objectives.
- /// This is a test event to use in testing + /// Test event used to verify event ordering and transmission in EVM tests. TestEvent { id: EventId, data: TestEvent, },
332-339: Optimize cloning in From implementation.The current implementation clones the value twice. We can optimize this by cloning only once.
impl From<TestEvent> for EnclaveEvent { fn from(value: TestEvent) -> Self { + let cloned = value.clone(); EnclaveEvent::TestEvent { - id: EventId::from(value.clone()), - data: value.clone(), + id: EventId::from(cloned), + data: value, } } }
Line range hint
180-193: Add TestEvent to is_local_only check.TestEvent should be marked as local_only since it's used for testing purposes.
pub fn is_local_only(&self) -> bool { // Add a list of local events match self { EnclaveEvent::CiphernodeSelected { .. } => true, EnclaveEvent::E3Requested { .. } => true, EnclaveEvent::CiphernodeAdded { .. } => true, EnclaveEvent::CiphernodeRemoved { .. } => true, EnclaveEvent::E3RequestComplete { .. } => true, EnclaveEvent::Shutdown { .. } => true, + EnclaveEvent::TestEvent { .. } => true, _ => false, } }packages/ciphernode/evm/src/helpers.rs (1)
119-129: Complexity inSignerProviderType AliasThe
SignerProvidertype alias involves a deeply nested structure withFillProviderand multipleJoinFills, which can affect readability and maintainability.Consider refactoring the nested types by introducing intermediate type aliases or using type definitions to simplify and improve code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/ciphernode/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/rust-ci.yml(2 hunks)package.json(1 hunks)packages/ciphernode/Cargo.toml(1 hunks)packages/ciphernode/core/src/events.rs(5 hunks)packages/ciphernode/evm/scripts/build_fixtures.sh(1 hunks)packages/ciphernode/evm/src/ciphernode_registry_sol.rs(3 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(4 hunks)packages/ciphernode/evm/src/event_reader.rs(4 hunks)packages/ciphernode/evm/src/helpers.rs(4 hunks)packages/ciphernode/evm/src/lib.rs(1 hunks)packages/ciphernode/evm/src/registry_filter_sol.rs(5 hunks)packages/ciphernode/evm/tests/evm_reader.rs(1 hunks)packages/ciphernode/evm/tests/fixtures/.gitignore(1 hunks)packages/ciphernode/evm/tests/fixtures/emit_logs.sol(1 hunks)packages/ciphernode/scripts/test.sh(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/ciphernode/evm/scripts/build_fixtures.sh
- packages/ciphernode/evm/tests/fixtures/.gitignore
🧰 Additional context used
📓 Learnings (3)
packages/ciphernode/Cargo.toml (2)
Learnt from: ryardley
PR: gnosisguild/enclave#133
File: packages/ciphernode/Cargo.toml:19-63
Timestamp: 2024-10-04T00:33:53.250Z
Learning: The dependency versions specified are acceptable as per project standards.
Learnt from: ryardley
PR: gnosisguild/enclave#133
File: packages/ciphernode/Cargo.toml:19-63
Timestamp: 2024-10-08T19:45:18.209Z
Learning: The dependency versions specified are acceptable as per project standards.
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.
🪛 yamllint
.github/workflows/rust-ci.yml
[error] 24-24: trailing spaces
(trailing-spaces)
🪛 Shellcheck
packages/ciphernode/scripts/test.sh
[error] 7-7: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🔇 Additional comments (35)
packages/ciphernode/scripts/test.sh (1)
1-3: LGTM: Proper shell configuration.
The shebang and set -e ensure proper shell execution and immediate exit on errors.
packages/ciphernode/evm/src/lib.rs (2)
13-13: LGTM! Clean export addition using proper Rust syntax.
The grouped export syntax is correct and maintains good code organization.
13-13: Verify impact of exposing ExtractorFn type.
Since this change exposes a new type in the public API, let's verify its impact on existing consumers.
✅ Verification successful
ExtractorFn type exposure is safe and properly scoped
The type ExtractorFn is only used internally within the event_reader module and its direct consumers within the same crate. The exposure is appropriate as it's part of the EvmEventReader API contract, where both types are tightly coupled. No external crates are currently importing or using this type directly, making this change safe.
Key findings:
ExtractorFnis only used inevent_reader.rsas a type alias for the event extraction function- The type is used in conjunction with
EvmEventReaderin the same module - No external usage of the type was found outside the crate
- Test files show proper usage of the exposed types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing usage of ExtractorFn to ensure this change doesn't break existing code
# and identify places that might benefit from the newly exposed type.
# Search for any existing ExtractorFn usage that might be affected
echo "Checking for existing ExtractorFn usage..."
rg "ExtractorFn" --type rust
# Look for potential consumers (files importing from this module)
echo "Checking for potential consumers..."
rg "use.*ciphernode.*evm" --type rust
Length of output: 715
Script:
#!/bin/bash
# Let's check for any direct usage of the event_reader module or its types
echo "Checking for direct event_reader module usage..."
rg "use.*event_reader" --type rust
# Let's also check for any implementations or usage of EvmEventReader
# since ExtractorFn is closely related to it
echo "Checking for EvmEventReader usage..."
rg "EvmEventReader" --type rust -A 3
# Check for any test files that might be using these types
echo "Checking test files..."
fd "test.*\.rs$" --exec rg -l "ExtractorFn|EvmEventReader"
Length of output: 4294
packages/ciphernode/evm/tests/fixtures/emit_logs.sol (2)
5-5: LGTM! Well-structured event declaration.
The event follows best practices by:
- Using indexed parameter for the author address to enable efficient filtering
- Capturing both old and new values for proper state transition auditing
13-15: LGTM! Getter function is well-implemented.
The function correctly uses:
viewmodifier for read-only operations- Appropriate memory location for string return
packages/ciphernode/evm/src/enclave_sol.rs (1)
4-4: LGTM! Clean import addition.
The addition of WithChainId to the imports is well-organized and aligns with the new type requirements.
.github/workflows/rust-ci.yml (1)
54-54: Verify test script setup.
The change to use yarn ciphernode:test aligns with the new testing strategy, but let's ensure all dependencies are in place.
✅ Verification successful
Let me verify the content of the test script to ensure it's properly configured.
Test script setup is properly configured
The verification confirms that:
- The
ciphernode:testscript is correctly defined in package.json - The test script exists at the expected location
- The script properly handles both fixture building and test execution with:
- EVM fixture building via
./scripts/build_fixtures.sh - Rust test execution via
cargo test
- EVM fixture building via
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the test script and its dependencies exist
# Check if the test script exists in package.json
echo "Checking package.json for ciphernode:test script..."
jq '.scripts["ciphernode:test"]' package.json
# Check if the test shell script exists
echo "Checking for test shell script..."
fd -t f "test.sh" packages/ciphernode/scripts
Length of output: 365
Script:
#!/bin/bash
# Check the content of test.sh
cat packages/ciphernode/scripts/test.sh
Length of output: 139
packages/ciphernode/Cargo.toml (1)
25-25: Verify the alloy version upgrade impact.
The upgrade from alloy 0.3.3 to 0.5.2 is significant and could have breaking changes. The addition of "node-bindings" feature aligns with the PR objectives for EVM testing.
Let's verify the impact of this change:
✅ Verification successful
The alloy upgrade to 0.5.2 appears safe to proceed
The verification shows that the alloy usage in the codebase is compatible with the version upgrade:
- Primary usage is focused on stable primitives (Address, B256) and core functionality
- The new "node-bindings" feature is correctly utilized in tests with Anvil integration
- All alloy dependencies are consistently versioned (core at 0.5.2, primitives and sol-types at 0.6)
- Contract interactions and provider patterns follow standard alloy patterns that are stable across versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes in alloy usage
# Test 1: Look for alloy imports that might need updates
echo "Checking alloy imports..."
rg -l 'use alloy::'
# Test 2: Check if all alloy dependencies are consistently versioned
echo "Checking alloy dependency versions..."
rg 'alloy.*version.*' -g 'Cargo.toml'
Length of output: 1109
Script:
#!/bin/bash
# Let's check the actual usage patterns to verify compatibility
# Looking for specific alloy patterns that might be affected by the version change
# Check for alloy type usage patterns
echo "Checking alloy type patterns..."
rg 'alloy::(primitives|sol_types|json|providers|network|transport)' -A 2
# Check for potential breaking changes in test files
echo "Checking test files for alloy usage..."
rg 'use alloy::' -A 3 packages/ciphernode/tests/
rg 'use alloy::' -A 3 packages/ciphernode/evm/tests/
# Check for contract interactions that might be affected
echo "Checking contract interactions..."
rg 'Contract|Provider|Transaction' packages/ciphernode/evm/src/
Length of output: 6984
packages/ciphernode/evm/tests/evm_reader.rs (1)
1-13: LGTM! Imports are well-organized and necessary.
All required dependencies are properly imported for EVM testing functionality.
packages/ciphernode/evm/src/enclave_sol_reader.rs (1)
1-1: LGTM! Import changes align with generic transport support.
The addition of WithChainId and BoxTransport imports supports the PR's objective of adding generic Transport support for providers.
Also applies to: 5-5
packages/ciphernode/evm/src/enclave_sol_writer.rs (5)
2-2: LGTM: Import addition aligns with type changes.
The addition of WithChainId import is necessary and properly placed with related imports.
Line range hint 30-40: LGTM: Simplified new method signature.
The removal of async is appropriate as the method performs no asynchronous operations. The type changes are consistent with the struct definition.
Line range hint 65-73: LGTM: Proper chain ID validation in event handler.
The handler correctly filters events based on chain ID match, which is crucial for cross-chain security. The check self.provider.get_chain_id() == data.src_chain_id ensures events are only processed for the correct chain.
Line range hint 111-120: Review the proof generation implementation.
The proof generation appears to use a hardcoded value (vec![1]). This might be a placeholder implementation.
#!/bin/bash
# Search for other proof implementations to understand the expected format
rg -A 5 "proof.*=.*Bytes::from"Would you like assistance in implementing a proper proof generation mechanism?
Line range hint 43-54: Verify error handling in attach method.
While the changes look good, we should verify the error handling for chain ID mismatches.
packages/ciphernode/evm/src/ciphernode_registry_sol.rs (3)
1-4: LGTM: Import changes align with generic Transport support.
The addition of WithChainId import is consistent with the PR's objective to support generic Transport types.
115-115: LGTM: Consistent provider type usage.
The provider type change maintains consistency with CiphernodeRegistrySolReader::attach.
103-105: Verify provider usage in caller code.
The provider type change to WithChainId<ReadonlyProvider> is correct, but we should verify that all callers are updated to wrap their providers appropriately.
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information to verify the concern:
Provider type change is properly handled
All callers are correctly using the wrapped provider type. The create_readonly_provider helper function in helpers.rs already returns WithChainId<ReadonlyProvider>, and all usage sites obtain their provider through this helper:
- In
ciphernode.rs, the provider is created viacreate_readonly_providerbefore being passed toCiphernodeRegistrySol::attach - The helper function's return type signature explicitly shows
Result<WithChainId<ReadonlyProvider, BoxTransport>>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of CiphernodeRegistrySolReader::attach to ensure providers are properly wrapped
# Search for direct calls to attach
rg -A 5 "CiphernodeRegistrySolReader::attach"
# Search for potential provider creation/usage patterns
rg -A 5 "create_readonly_provider|ReadonlyProvider::new"
Length of output: 3071
packages/ciphernode/evm/src/registry_filter_sol.rs (4)
1-1: LGTM: Import changes align with generic transport support.
The addition of BoxTransport and use of WithChainId wrapper aligns with the PR's objective to support generic transports, particularly for compatibility with non-BoxTransport providers like Anvil.
Also applies to: 7-7
23-26: LGTM: Enhanced type safety with WithChainId wrapper.
The provider field now explicitly carries chain ID information at the type level, which is crucial for cross-chain operations and prevents potential chain ID mismatches.
Line range hint 54-107: LGTM: Well-implemented event handlers with proper chain ID validation.
The handlers demonstrate good practices:
- Chain ID validation before processing events
- Proper async handling with error propagation
- Clean shutdown handling
Line range hint 110-126: Consider logging or handling invalid node addresses explicitly.
The filter_map silently drops invalid addresses when parsing nodes. This could lead to unexpected committee compositions if addresses are malformed.
#!/bin/bash
# Check if there are tests covering invalid address scenarios
rg -A 5 "publish.*committee.*invalid" Consider either:
- Failing early if any address is invalid
- Logging warnings for dropped addresses
- let nodes: Vec<Address> = nodes
- .into_iter()
- .filter_map(|node| node.parse().ok())
- .collect();
+ let nodes: Vec<Address> = nodes
+ .into_iter()
+ .map(|node| node.parse().map_err(|e| {
+ tracing::warn!("Invalid node address {}: {}", node, e);
+ e
+ }))
+ .collect::<Result<_, _>>()?;packages/ciphernode/core/src/events.rs (1)
551-555: LGTM!
The TestEvent struct implementation is clean and follows the same pattern as other event types in the file.
packages/ciphernode/evm/src/event_reader.rs (7)
15-15: Generic ExtractorFn<E> enhances flexibility—good improvement.
The introduction of the generic type parameter E in ExtractorFn<E> increases flexibility by allowing different event types to be processed. This change enhances modularity and reusability.
18-26: Refactoring EvmEventReader with generics improves adaptability.
Making EvmEventReader generic over P and T allows it to work with various providers and transports, enhancing type safety and flexibility. This change aligns with Rust best practices for generic programming.
31-35: Consistent generic constraints in impl block ensure correctness.
The impl block for EvmEventReader<P, T> correctly mirrors the generic constraints defined in the struct, maintaining consistency and ensuring the methods operate as intended with the generic types.
70-74: Correct generic implementation of the Actor trait.
The Actor trait implementation for EvmEventReader<P, T> appropriately incorporates the generic parameters, ensuring that the actor functions correctly with different provider and transport types.
98-102: Proper handling in Handler<EnclaveEvent> implementation.
The Handler<EnclaveEvent> implementation for EvmEventReader<P, T> maintains consistency with the generic struct, and correctly handles EnclaveEvent messages, ensuring reliable messaging behavior.
36-47: Verify the change from asynchronous to synchronous new method.
The new method has been changed from asynchronous (async fn new) to synchronous (fn new). Ensure that this change does not affect any required asynchronous initialization processes.
To confirm that the change is safe, check if any existing code awaits the new method:
If any calls are found where new is awaited, you'll need to refactor those calls to accommodate the synchronous new method or reconsider keeping new asynchronous.
✅ Verification successful
Change from async to sync new method is safe
The change is safe because:
- The
newmethod only performs synchronous operations (creating a channel and cloning values) - The only usage found (
EvmEventReader::new) is immediately followed by.start()and is part of the asyncattachmethod - All async operations are properly handled in the
startedactor lifecycle method where the actual event streaming begins
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to `EvmEventReader::new` that are awaited.
# Test: Search for `await EvmEventReader::new` in the codebase.
rg 'await\s+EvmEventReader::new\('
Length of output: 36
Script:
#!/bin/bash
# Let's try a broader search to find any usage of EvmEventReader::new
rg "EvmEventReader::new" -A 2
# Also search for any potential test cases
rg "EvmEventReader::new" --type rust --glob "*test*.rs" -A 2
# Let's also check the complete implementation of EvmEventReader to understand if there are any async operations
ast-grep --pattern 'impl$_ EvmEventReader$_ {
$$$
}'
Length of output: 442
Script:
#!/bin/bash
# Let's check the implementation of start() method since it's called right after new()
ast-grep --pattern 'fn start($$$) {
$$$
}'
# Let's also check the complete file content to understand the context better
cat packages/ciphernode/evm/src/event_reader.rs
Length of output: 3635
55-60: Ensure attach method updates align with codebase usage.
The attach method now requires a provider of type &WithChainId<P, T>, reflecting the new generic parameters. Verify that all calls to attach in the codebase are updated to match the new signature.
Run the following script to find usages of attach and check for compatibility:
Ensure that each call passes the correct generic types and updated parameters as per the new method signature.
✅ Verification successful
Let me gather more context about the provider types being used in these calls.
Let me check the actual implementation of EvmEventReader::attach to compare with its usage.
Let me try a different approach to find the implementation.
Let me try one final search to find the implementation.
All attach method calls are correctly updated with WithChainId provider
Based on the verification results, all calls to EvmEventReader::attach are properly using the WithChainId wrapped provider:
- In
enclave_sol_reader.rs: UsesWithChainId<ReadonlyProvider, BoxTransport> - In
ciphernode_registry_sol.rs: UsesWithChainId<ReadonlyProvider> - In
tests/evm_reader.rs: Creates and usesWithChainIdwrapped provider viaWithChainId::new(provider)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to `EvmEventReader::attach` and review their parameters.
# Test: Search for `EvmEventReader::attach` usage.
rg 'EvmEventReader::attach\(' -A 3
Length of output: 980
Script:
#!/bin/bash
# Check the provider types in the calling files
rg -B 5 'EvmEventReader::attach' packages/ciphernode/evm/src/enclave_sol_reader.rs packages/ciphernode/evm/src/ciphernode_registry_sol.rs packages/ciphernode/evm/tests/evm_reader.rs
Length of output: 1946
Script:
#!/bin/bash
# Find the implementation of EvmEventReader::attach
ast-grep --pattern 'impl$_EvmEventReader$_{ $$$ fn attach($$$) { $$$ } $$$ }'
Length of output: 79
Script:
#!/bin/bash
# Find the implementation of EvmEventReader::attach using ripgrep
rg -B 5 -A 10 'impl.*EvmEventReader.*attach'
Length of output: 46
Script:
#!/bin/bash
# Search for attach method in event_reader.rs
rg -B 5 -A 10 'pub.*fn.*attach' packages/ciphernode/evm/src/event_reader.rs
Length of output: 587
packages/ciphernode/evm/src/helpers.rs (5)
27-33: Enhancement of stream_from_evm Function Signature
The stream_from_evm function has been updated to accept generic types P: Provider<T> and T: Transport + Clone. This change enhances flexibility by allowing different transport mechanisms and looks correct.
106-106: Confirmation of Updated ReadonlyProvider Type Alias
The ReadonlyProvider type alias has been updated to RootProvider<BoxTransport>. This change aligns with the modifications made to accommodate generic transport types and appears appropriate.
108-116: Consistency in Return Type of create_readonly_provider
The function create_readonly_provider now returns Result<WithChainId<ReadonlyProvider, BoxTransport>>, which is consistent with the updated WithChainId struct. This change ensures that the provider includes chain ID information alongside the transport type.
135-143: Adjustment of Return Type in create_provider_with_signer Function
The create_provider_with_signer function now returns Result<WithChainId<SignerProvider, BoxTransport>>. This modification is consistent with the changes to the WithChainId struct and ensures that the provider carries both the signer and chain ID information.
71-80:
Correct Initialization of PhantomData<T> in WithChainId Struct
In the WithChainId struct, the _t field is defined as PhantomData<T>, but in the new method, it is initialized as PhantomData without specifying the type T. To ensure type safety and clarity, initialize _t as PhantomData::<T>.
Apply the following diff to fix the initialization:
Ok(Self {
provider: Arc::new(provider),
chain_id,
- _t: PhantomData,
+ _t: PhantomData::<T>,
})Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/ciphernode/README.md (1)
1-25: Enhance documentation clarity and formatting.The prerequisites section is informative but could benefit from some improvements:
Apply these changes:
- Add language specifiers to code blocks for better syntax highlighting:
-``` +```bash $ rustc --version2. Improve readability with minor text adjustments: ```diff -We need foundry's `anvil` to test our evm facing rust code: +We need Foundry's `anvil` to test our EVM-facing Rust code: -Note some older versions of `anvil` are not compatible and can cause errors. +Note: Some older versions of `anvil` are incompatible and can cause errors.🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: You might be missing the article “a” here.
Context: ...+commit.d9974bed.Linux.g++ ``` We need foundry'sanvilto test our evm facing rust c...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...89 2024-10-30T00:22:24.181391729Z) ``` Note some older versions ofanvilare not ...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~25-~25: Consider using “incompatible” to avoid wordiness.
Context: ...Note some older versions ofanvilare not compatible and can cause errors. # E3 Requested ...(NOT_ABLE_PREMIUM)
🪛 Markdownlint
5-5: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
12-12: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
20-20: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/ciphernode/README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/ciphernode/README.md
[uncategorized] ~18-~18: You might be missing the article “a” here.
Context: ...+commit.d9974bed.Linux.g++ ``` We need foundry's anvil to test our evm facing rust c...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...89 2024-10-30T00:22:24.181391729Z) ``` Note some older versions of anvil are not ...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~25-~25: Consider using “incompatible” to avoid wordiness.
Context: ...Note some older versions of anvil are not compatible and can cause errors. # E3 Requested ...
(NOT_ABLE_PREMIUM)
🪛 Markdownlint
packages/ciphernode/README.md
5-5: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
12-12: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
20-20: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
packages/ciphernode/README.md (2)
38-39: LGTM: Sequence diagram formatting changes.
The formatting changes to the sequence diagrams improve readability while maintaining the correct flow of interactions.
Also applies to: 64-64
3-16: Clarify version requirements.
The documentation lists specific tool versions but doesn't indicate if these are minimum requirements or exact version requirements. This could lead to confusion for users with different versions.
Please specify whether these are minimum versions or exact version requirements. Additionally, consider adding version compatibility ranges.
🧰 Tools
🪛 Markdownlint
5-5: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
12-12: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/ciphernode/evm/tests/evm_reader.rs (2)
23-23: Enhance the requirement documentation.The comment about Anvil could be more descriptive to help developers set up their environment correctly.
- // NOTE: Anvil must be available on $PATH + // NOTE: Anvil (part of Foundry) must be available on $PATH + // Installation: https://book.getfoundry.sh/getting-started/installation
34-44: Improve error handling in the event decoder.The current implementation silently drops decoding errors. Consider logging these errors for better debugging.
|data, topic, _| match topic { Some(&EmitLogs::ValueChanged::SIGNATURE_HASH) => { - let Ok(event) = EmitLogs::ValueChanged::decode_log_data(data, true) else { - return None; - }; + match EmitLogs::ValueChanged::decode_log_data(data, true) { + Ok(event) => Some(EnclaveEvent::from(TestEvent { + msg: event.newValue, + })), + Err(e) => { + tracing::warn!("Failed to decode ValueChanged event: {}", e); + None + } + } - Some(EnclaveEvent::from(TestEvent { - msg: event.newValue, - })) } _ => None, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/ciphernode/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/ciphernode/Cargo.toml(1 hunks)packages/ciphernode/core/src/events.rs(5 hunks)packages/ciphernode/evm/tests/evm_reader.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ciphernode/Cargo.toml
- packages/ciphernode/core/src/events.rs
🧰 Additional context used
📓 Learnings (1)
packages/ciphernode/evm/tests/evm_reader.rs (1)
Learnt from: ryardley
PR: gnosisguild/enclave#170
File: packages/ciphernode/evm/tests/evm_reader.rs:14-18
Timestamp: 2024-11-03T11:00:21.089Z
Learning: The `emit_logs.json` fixture file in `packages/ciphernode/evm/tests/fixtures/` is generated during the build process and is not committed to the repository.
Closes: #169
This provides the ability to test evm event subscription.
anvilinstalled in the PATH in order to run the testsalloyto a modern version (5.2)EnclaveEvent::TestEventwe can use in testing to test event ordering and transmission issues.emit_logs.solas well as a build script to build the artifact used in the rust testWithChainIdwhich was required to support providers that didn't useBoxTransportsuch as the default anvil transports provided by alloy (ProviderBuilder::new().with_recommended_fillers().on_anvil_with_wallet())solcis available as a dependencyProviderBuilder::new().with_recommended_fillers().on_anvil_with_wallet()Summary by CodeRabbit
Summary by CodeRabbit
New Features
TestEventtype for enhanced event handling.build_fixtures.shscript to automate Ethereum smart contract fixture building.EmitLogsSolidity contract for event logging capabilities.Improvements
Bug Fixes
Tests
Chores
.gitignoreto exclude JSON files in test fixtures.test.shscript for streamlined testing processes.