feat(user-ops-indexer): rewrite to alloy #1165
Conversation
WalkthroughThis pull request introduces a comprehensive migration from the Possibly related PRs
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 (
|
![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
🧹 Nitpick comments (26)
user-ops-indexer/user-ops-indexer-proto/swagger/user-ops-indexer.swagger.yaml (1)
Line range hint
1-624: Consider documenting address format expectationsGiven the migration from ethers to alloy, it would be helpful to document the expected format for address fields (e.g., checksum format, case sensitivity) in the schema descriptions. This will help API consumers handle addresses correctly with the new implementation.
Example enhancement for address fields:
address: type: string + description: "Ethereum address in checksummed format (0x...)" + pattern: "^0x[0-9a-fA-F]{40}$"user-ops-indexer/user-ops-indexer-server/src/indexer.rs (4)
55-55: Handle potential provider initialization failures
The new asynchronous provider creation is correct, but verify that any transient RPC failures are retried.
58-59: Config-driven restart delay
Reading therestart_delayfrom settings fosters configurability. Ensure that defaults or zero values are properly handled.
86-86: Unexpected end logging
Explicit logging of an unexpected end is helpful; you might consider further distinguishing error categories if these happen often.
96-99: Reconnect logic
The logic re-establishing the RPC client is solid. Consider centralizing or factoring out the repeated provider creation to avoid duplication.Also applies to: 103-103
user-ops-indexer/user-ops-indexer-logic/src/indexer/common.rs (3)
Line range hint
12-31: Dynamically parsing multiple ABI function signatures
Storing them inEXECUTE_SELECTORSwithJsonAbi::parseand collecting the selectors is a neat approach for easy matching. Handle parse errors carefully (currently usingunwrap()).
96-96: Selector matching
call_data.starts_with(sig.as_slice())is a simple approach. If partial collisions of signatures ever occur, consider more robust checks.
117-120: Test imports
Importing test helpers fromalloy::{primitives::..., rpc::types::Log, ...}is good. Just verify you only import what's necessary.user-ops-indexer/user-ops-indexer-logic/src/indexer/v06/mod.rs (2)
7-7: ImportingIEntrypointV06CallsandUserOperation
Ensure these items are indeed used or remove them to maintain clarity.
147-150: Filtering forDepositedlogs
Filtering the entire logs array to buildtx_depositsis correct. Confirm performance if logs are large.user-ops-indexer/user-ops-indexer-logic/src/indexer/base_indexer.rs (6)
65-70: Constants for event signatures and version.
Replacing old method returns withconstfields is straightforward and likely more performant. Avoid duplicating these constants elsewhere in the code to ensure a single source of truth.
102-102: Filter usage withevent_signature().
AttachingSelf::BEFORE_EXECUTION_SIGNATUREto the filter ensures targeted log retrieval. Keep an eye on performance if additional events are appended in the future.
231-233: WS disconnection handling.
Skipping transactions after multiple retries is a valid approach to keep the indexer moving. The transport-levelBackendGonedetection helps. Consider logging data to correlate missed ops if they remain relevant.
364-364: Diagnostic logs.
Including mismatch details"{} != {}"forcalldatasvs.log_bundlesis good for debugging.
405-423: Local integration test scaffolding.
Using a real mainnet RPC is a practical approach for verifying real data. Long-term, switching to a mocked or ephemeral dev net can be more stable for CI.
429-430: TODO for mocked connections.
Consider removing or fulfilling these TODOs to speed up deterministic tests.Do you want me to open a new issue or provide a PR for a mock-based approach?
user-ops-indexer/user-ops-indexer-logic/src/indexer/rpc_utils.rs (1)
25-34:impl From<String>forTraceClient.
Detecting “Geth/” inclientVersionis a simple heuristic that works well. Watch out for future node name changes (“Erigon/” or “Nethermind/”). Might eventually need a more robust approach, but good for now.user-ops-indexer/user-ops-indexer-logic/src/types/factory.rs (1)
22-22: Changed tov.factory.to_string().
Eliminating theto_checksumcall is simpler, though it loses the EIP-55 checksummed format. Decide if the plain hex representation is acceptable.user-ops-indexer/user-ops-indexer-logic/src/types/paymaster.rs (1)
22-22:v.paymaster.to_string()
As with factories, we lose checksummed addresses. If checksummed strings are desired in user-facing data, consider reintroducing a library function or your own checksumming method.user-ops-indexer/user-ops-indexer-logic/src/repository/bundle.rs (1)
64-64: Inspect the usage ofU160carefully.
You’re usingU160for constructing addresses, which is valid. Just ensure it’s consistent with other address creation patterns in your codebase so you have a uniform approach.user-ops-indexer/user-ops-indexer-logic/src/repository/paymaster.rs (2)
55-55: Double-check the tuple comparison approach.
The usage of tuple-based comparison(count(), paymaster)for the HAVING clause could be vulnerable to ordering issues if you later inject more columns. Keep an eye on query expansions.
81-81: Consistent numeric conversions for addresses.
UsingU160is an effective approach, but ensure it’s the same approach used in other test modules to avoid confusion.user-ops-indexer/user-ops-indexer-logic/src/indexer/settings.rs (1)
1-2: Introduce specialized traces behind a feature toggle if necessary.
AddingTraceClientis a good step. If performance overhead or partial usage is a concern, consider gating it behind an optional feature.user-ops-indexer/user-ops-indexer-logic/src/repository/bundler.rs (2)
81-81: Uniform numeric conversions for addresses.
As with other modules,U160is valid. Just ensure the entire test suite remains consistent using the same approach for address creation from integers.
103-103: Additional scenario coverage indicated.
Testing different numeric addresses helps guarantee correctness across varying address ranges. Consider also testing the zero address or a very large address.user-ops-indexer/README.md (1)
74-74: Documentation looks good with minor suggestions for clarity.The new environment variable documentation is well-structured and aligns with the PR's objective of migrating to alloy-rs. The purpose, options, and default behavior are clearly explained.
Consider these minor improvements for clarity:
-| `USER_OPS_INDEXER__INDEXER__TRACE_CLIENT` | | Node client variant to use for tracing transactions, `Geth` for using `debug_traceTransaction`, `Parity` for using `trace_transaction` | Depends on `web3_clientVersion` | +| `USER_OPS_INDEXER__INDEXER__TRACE_CLIENT` | | Node client variant for transaction tracing. Options: `Geth` (uses `debug_traceTransaction`) or `Parity` (uses `trace_transaction`). Auto-detected from `web3_clientVersion` if not set. | Auto-detected |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
user-ops-indexer/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
user-ops-indexer/README.md(1 hunks)user-ops-indexer/user-ops-indexer-logic/Cargo.toml(1 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/base_indexer.rs(19 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/common.rs(7 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/common_transport.rs(0 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/mod.rs(0 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/rpc_utils.rs(4 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/settings.rs(3 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/v06/mod.rs(6 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/v07/mod.rs(7 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/account.rs(6 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/bundle.rs(5 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/bundler.rs(6 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/factory.rs(5 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/paymaster.rs(5 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/user_op.rs(12 hunks)user-ops-indexer/user-ops-indexer-logic/src/types/account.rs(3 hunks)user-ops-indexer/user-ops-indexer-logic/src/types/bundle.rs(3 hunks)user-ops-indexer/user-ops-indexer-logic/src/types/bundler.rs(2 hunks)user-ops-indexer/user-ops-indexer-logic/src/types/common.rs(1 hunks)user-ops-indexer/user-ops-indexer-logic/src/types/factory.rs(2 hunks)user-ops-indexer/user-ops-indexer-logic/src/types/paymaster.rs(2 hunks)user-ops-indexer/user-ops-indexer-logic/src/types/user_op.rs(14 hunks)user-ops-indexer/user-ops-indexer-proto/src/lib.rs(1 hunks)user-ops-indexer/user-ops-indexer-proto/swagger/user-ops-indexer.swagger.yaml(7 hunks)user-ops-indexer/user-ops-indexer-server/Cargo.toml(1 hunks)user-ops-indexer/user-ops-indexer-server/src/indexer.rs(3 hunks)user-ops-indexer/user-ops-indexer-server/src/services/user_ops.rs(9 hunks)
💤 Files with no reviewable changes (2)
- user-ops-indexer/user-ops-indexer-logic/src/indexer/mod.rs
- user-ops-indexer/user-ops-indexer-logic/src/indexer/common_transport.rs
✅ Files skipped from review due to trivial changes (1)
- user-ops-indexer/user-ops-indexer-proto/src/lib.rs
🔇 Additional comments (167)
user-ops-indexer/user-ops-indexer-proto/swagger/user-ops-indexer.swagger.yaml (3)
347-347: Schema enhancement: Explicit type definition for rpcStatus.details items
The addition of explicit type definition for array items improves schema clarity and type safety.
413-413: LGTM: Explicit type definition for ListAccountsResponse items
The schema properly defines the array items type and maintains compatibility with the alloy migration.
423-423: LGTM: Consistent schema improvements across list responses
The changes systematically improve type definitions across all list response schemas:
- ListBundlersResponse (v1Bundler)
- ListBundlesResponse (v1Bundle)
- ListFactoriesResponse (v1Factory)
- ListPaymastersResponse (v1Paymaster)
- ListUserOpsResponse (v1ListUserOp)
These modifications enhance schema clarity while maintaining compatibility with the alloy migration.
Also applies to: 433-433, 443-443, 453-453, 485-485
user-ops-indexer/user-ops-indexer-server/Cargo.toml (2)
24-24: 🛠️ Refactor suggestion
Pin the sea-orm version in dev-dependencies
Using a wildcard version (*) in dependencies can lead to inconsistent builds across different environments. Consider pinning to a specific version for better reproducibility.
-sea-orm = { version = "*", features = ["sqlx-sqlite"] }
+sea-orm = { version = "0.12.2", features = ["sqlx-sqlite"] }Likely invalid or redundant comment.
22-22: Verify alloy version and features
The alloy dependency looks appropriate for the migration from ethers-rs. Let's verify the version and any potential security advisories.
user-ops-indexer/user-ops-indexer-server/src/indexer.rs (6)
2-2: Transition to alloy provider looks good
Switching from the ethers provider to the alloy::providers::ProviderBuilder is a core part of the PR. This import aligns with the rest of the changes.
6-6: Check for unused imports
Importing Indexer, v06, v07, etc. from user_ops_indexer_logic appears correct, but ensure all listed items are indeed used.
49-50: Consistent definition of version and entry_point
Using constants (like L::VERSION) for consistent logging is clear and maintainable.
61-66: Indexer instantiation in the retry loop
Reinstantiating Indexer each time is a sound design choice so that each retry starts fresh.
72-72: Error logging clarity
Error logging includes all key context such as version, delay, and the error object. This is good for debugging.
80-80: Exiting on clean stream termination
Clean exit logic for non-realtime indexers is clearly stated.
user-ops-indexer/user-ops-indexer-logic/src/indexer/common.rs (8)
1-6: Import adjustments for alloy types
Transitioning to alloy for JsonAbi, Selector, etc. is consistent with dependency changes.
8-8: Ensure no duplication
use entity::sea_orm_active_enums::SponsorType; is used by some functions (e.g. extract_sponsor_type). If it's unused, consider removing it.
22-22: Check function signature accuracy
execFromEntryPointWithFee modifies the call data. Confirm the signature is correct to avoid decode errors.
64-66: Optimizing boundary detection
The sliding window approach (while l < r && condition ...) is straightforward. Double-check edge cases if logs array is empty or has unexpected addresses.
Also applies to: 69-69, 73-73
80-81: Use of from_be_slice
Switching to U256::from_be_slice is correct for big-endian representations. Confirm that the slice lengths (16 bytes vs. remainder) align with intended usage.
98-101: Decoding call data
The usage of SolValue::abi_decode_params with (Address, U256, Bytes) is well structured. Ensure no panic can occur if call data is too short.
125-127: Explicit test addresses
Using separate addresses for entry_point, paymaster, etc. is more readable.
139-142: Log creation for boundary tests
Creating logs with different addresses and indices to test extract_user_logs_boundaries is thorough. Great test coverage.
Also applies to: 145-145, 148-149
user-ops-indexer/user-ops-indexer-logic/src/indexer/v06/mod.rs (12)
11-16: Transition to alloy
Using alloy::primitives::{Address, B256, ...} is consistent with the rest of the rewrite. This set of imports looks correct.
21-21: s ol! macro usage
Declaring the ABI from ./src/indexer/v06/abi.json is fine. Make sure the ABI path is correct.
36-40: New constant definitions
Declaring VERSION, USER_OPERATION_EVENT_SIGNATURE, and BEFORE_EXECUTION_SIGNATURE supports the new event checks.
42-43: entry_point method
Straightforward getter. Fine as is.
47-48: Calldata matching
Checking handleOpsCall or handleAggregatedOpsCall for v0.6. This ensures the logic is correct for recognized calls.
58-61: Aggregated ops decoding
Properly decoding opsPerAggregator and iterating over .userOps. This is consistent with the aggregator logic.
Also applies to: 65-65, 75-75
108-108: Logging build failures
Tracking the start index of logs is helpful for debugging.
138-138: Optional event parse
Chaining .and_then / .find_map with match_and_parse is a good pattern to handle missing events gracefully.
Also applies to: 143-143
155-156: Extracting optional addresses
Using extract_address for factory and paymaster maintains uniform approach.
161-170: Building the UserOp with B256 conversions
Mapping userOpHash, nonce, fee fields, etc. to B256 or U256 is consistent with the new library usage.
178-179: Unwrapping block number/hash
Defaulting to 0 or BlockHash::ZERO if missing is a design choice. Ensure these sentinel values won’t confuse downstream logic.
186-187: Gas, fee, and revert reasoning
Combining callGasLimit, verificationGasLimit, and preVerificationGas is correct. Reporting gas_price as the ratio is also valid. This is a well-structured user operation representation.
Also applies to: 191-192, 196-196
user-ops-indexer/user-ops-indexer-logic/src/indexer/v07/mod.rs (16)
8-8: Importing PackedUserOperation
Aligns with aggregator logic changes for v0.7. Ensure the code references this structure properly.
12-17: alloy transition
Imports for B256, etc. are consistent with the rest of v0.7 changes.
22-22: Check ABI path
"./src/indexer/v07/abi.json" must match the actual location.
37-41: Constant definitions for v0.7
Matches the approach in v0.6. This emphasizes clarity and consistency across multiple versions.
43-44: entry_point method
Simple getter, consistent with v0.6 logic.
48-49: Calldata validation
Mirrors the logic from v0.6 for aggregated vs. non-aggregated calls.
59-62: Parsing aggregated ops
Collecting aggregator props and passing them to ExtendedUserOperation is correct.
Also applies to: 66-66, 76-76
109-109: Error logging
Capturing logs_start_index and logs_count helps debug partial user op parsing.
139-139: match_and_parse usage
Same pattern as v0.6. This ensures uniform event parsing.
Also applies to: 144-144
148-151: Processing Deposited events
Same approach as v0.6. The code is consistent.
157-158: Unpacking custom fields
unpack_uints usage to retrieve verification / call / paymaster limits is well-structured. Double-check array slicing offsets (20..52) for correctness.
Also applies to: 160-161, 163-163
172-172: Extracting gas fees
Again, using unpack_uints for max_fee_per_gas and max_priority_fee_per_gas is consistent with the aggregator logic.
174-175: Optional addresses
factory and paymaster extraction remains consistent.
180-183: Constructing UserOp
Appropriately mapping the new fields. Ensure data is validated if paymaster is missing.
Also applies to: 190-190
197-198: Default block references
As in v0.6, using zero or BlockHash::ZERO if absent.
205-205: Gas price, usage, and fee
Setting the user op’s gas_price, gas_used, and fee from the event is consistent. Good approach for linking actual usage to the final record.
Also applies to: 207-208, 212-212
user-ops-indexer/user-ops-indexer-logic/src/types/user_op.rs (9)
5-6: Replacing ethers with alloy
Good switch. Multiple new types (TxHash, BlockHash, etc.) keep the code consistent.
16-18: Struct fields
Storing hash, nonce, transaction_hash, block_number, block_hash as B256 or specialized types ensures strong type safety.
Also applies to: 32-34
56-56: ListUserOp enhancements
Similar shift to B256 and TxHash, consistent with UserOp.
Also applies to: 61-61
70-72: From for Model
Storing fields as byte arrays in the DB is a natural approach. The distinction between optional addresses is handled well with map(|a| a.to_vec()).
Also applies to: 82-84, 86-86, 88-89, 92-93
111-113: From for UserOp
Converting DB byte arrays back into B256, TxHash, etc. ensures strong typed usage in code.
Also applies to: 127-129
157-158: Proto raw conversions
Using B128 to concatenate the gas values is a clever approach. Confirm the offsets if the fields change.
Also applies to: 176-177, 180-186, 188-194
207-209: Proto output fields
Preserving all user op data for external usage is thorough. The code is consistent with the typed fields.
Also applies to: 218-220, 222-224, 228-229, 243-243
252-252: From for ListUserOp
Same pattern of array-to-typed conversions as above, aligning with the DB. Good consistency.
Also applies to: 257-257
271-272: From for proto
Stringifying B256, TxHash, etc. is standard for client-facing or RPC usage.
Also applies to: 275-276
user-ops-indexer/user-ops-indexer-logic/src/indexer/base_indexer.rs (19)
3-4: Switch to TraceClient and TraceType imports looks good.
The move away from NodeClient references aligns with the new transport design in rpc_utils.rs.
9-16: Using alloy crate primitives and providers.
Adopting TxHash, BlockHash, B256, and Provider from alloy significantly streamlines the type references. Ensure the rest of the codebase is updated to avoid mismatched types, especially in any leftover calls referencing Ethers-based types.
31-32: Job struct updated to use TxHash and BlockHash.
This change cleanly distinguishes transaction hashes from block hashes. The impl From<TxHash> sets block_hash to BlockHash::ZERO, which is useful for cases when a block hash is not yet known. Confirm that zero block hash doesn't cause confusion or erroneous lookups later.
Also applies to: 35-39
48-48: Pending/removed logs handling.
The bail! call aborts early when log.removed is true, preventing partial indexing of temporary chain reorg logs. This approach is correct for discarding ephemeral logs.
83-84: Log matching logic and new match_and_parse approach.
- The matchers are now more explicit, checking
log.topic0() == Some(&SIGNATURE). This is correct but be mindful of any logs that could have aNonesignature in certain edge cases. match_and_parseusesSolEvent::decode_log, which can throw parse errors. The usage ofOption<sol_types::Result<T>>is a clean approach. If logs are essential, consider logging decode failures for debugging.
Also applies to: 88-89, 92-94
106-107: Generics for Indexer struct.
Allowing a generic Provider (rather than a fixed transport) makes this indexer flexible. The constructor properly sets up client, db, settings, and logic.
Also applies to: 116-118
133-141: trace_client resolution logic.
Falling back to the runtime-detected client version if custom config isn't provided is a good fallback. This is especially helpful if the user sets no explicit mode.
146-160: Realtime logs subscription vs. polling.
The ability to handle either WebSocket-based streaming or RPC polling is a nice modular approach. The reconnect logic using alloy-rs is correct, but watch for potential silent failures if the WS is continuously unstable.
169-185: Refetch block logs for missed ranges.
Pulling missed logs from both DB and RPC ensures consistency. The arithmetic on subtracting (or adding) the block range is correct. Double-check that negative offsets never yield underflow when saturating sub is used with large ints.
Also applies to: 191-191
256-258: Block range fetch signature.
Changing fn fetch_jobs_for_block_range(&self, from_block: u64, to_block: u64) is a good clarity improvement. Confirm all existing calls abide by u64 usage to avoid casting issues.
305-306: handle_tx method updated to accept TraceClient.
Switching from NodeClient to TraceClient is consistent with the rest of the code. The initial retrieval of transaction and receipt is standard.
Also applies to: 309-309
322-323: Accessing logs via receipt.inner.logs().
This usage is straightforward. Just remain cautious about indexing into these logs if they’re unexpectedly empty or missing.
338-339: Indirect calls and bundling logic.
- The check for single-bundle direct calls vs. multiple-bundle indirect calls is helpful.
- Using
common_trace_transactionto gather thecalldatasis a good fallback.
This logic is quite robust.
Also applies to: 345-345
434-447: handle_tx usage in handle_tx_v06_ok.
Verifies that user ops from a known transaction land in the DB as expected with TraceClient::Parity. This is a valid test approach.
Line range hint 450-475: UserOp correctness check.
The fields (e.g., nonce: B256::ZERO, block_number: 18774992) appear correct for the tested data.
494-503: handle_tx_v06_with_tracing_ok with multiple trace clients.
Exercising both Geth and Parity modes ensures broader coverage. Deleting the existing record in the DB to confirm new data is reindexed is good.
Also applies to: 505-507, 509-522
527-563: Large snapshot test of UserOp fields.
This thoroughly validates many possible fields, ensuring the decoders stay accurate. The zero or extended fields look correct for the mainnet example.
570-588: Handling v07 similarly.
Mirroring the approach from v06 ensures consistent indexing logic for new entry point versions.
591-630: Extended UserOp fields for v07.
The new signature checks, paymaster fields, and aggregator logic are exercised. This deeper test coverage is excellent.
user-ops-indexer/user-ops-indexer-logic/src/indexer/rpc_utils.rs (7)
1-14: Importing alloy trace structures and TraceApi.
This consolidates all Ethereum tracing under the new library. The separation of Geth/Parity in ring-fenced modules is a neat solution.
19-23: Enum TraceClient.
Explicitly enumerating Geth vs. Parity clarifies which tracing method is used. The fallback to TraceClient::Parity in Default is logical.
63-64: CallTracer trait changes.
Renaming the parameter from NodeClient to TraceClient clarifies the intention. Return type changed to TransportResult<Vec<CommonCallTrace>>—no issues spotted.
Also applies to: 68-68
72-104: Geth-based tracing.
- Uses
GethDebugTracingOptionswith memory & storage disabled for performance. Good choice. - The fallback to
TransportErrorKind::custom_strif the result can’t be parsed is correct.
105-132: Parity-based tracing.
Filters out only Action::Call and Action::Create. Steps that are Refund, Suicide, or others are skipped, which is consistent with the prior logic. This approach remains functionally similar to what we had in Ethers.
154-160: flatten_geth_trace logic.
Implements a DFS approach, pushing frames onto a stack-like structure. The if idx == 0 condition is a neat technique to collect the current frame’s data. This is well-structured.
[approve]
Line range hint 170-196: test_flatten_geth_trace with a large sample structure.
Validates the behavior on a real-world transaction example. Great coverage.
user-ops-indexer/user-ops-indexer-logic/src/types/factory.rs (1)
2-2: Using alloy::primitives::Address.
This is consistent with the rest of the migration away from Ethers.
user-ops-indexer/user-ops-indexer-logic/src/types/paymaster.rs (1)
2-2: Switched to alloy::primitives::Address.
Same as with factory.rs, consistent migration.
user-ops-indexer/user-ops-indexer-logic/src/types/common.rs (3)
1-3: Replacing Ethers U256 with alloy::primitives::U256.
Straight swap is consistent with the rest of the codebase.
6-7: u256_to_decimal now uses BigDecimal::from_str.
Implementation is simpler. The .unwrap() is acceptable if we trust U256::to_string() to be valid decimal, which it is.
9-22: Unit tests for u256_to_decimal.
Checks boundary values, including U256::MAX. This is thorough coverage.
user-ops-indexer/user-ops-indexer-logic/src/types/bundler.rs (2)
2-2: Good transition to alloy's Address.
Replacing ethers::prelude::Address with alloy::primitives::Address aligns with the PR's objective to migrate away from ethers-rs.
24-24: Confirm the string representation.
Ensure the .to_string() approach is acceptable for downstream consumers of this address (e.g., for logs, DB storage, or external services). Previously, a checksummed address might have been expected.
user-ops-indexer/user-ops-indexer-logic/src/types/bundle.rs (4)
2-2: Import changes are consistent with the alloy migration.
6-6: Type change to TxHash looks appropriate.
17-17: Validate input slice length.
TxHash::from_slice(&v.transaction_hash) requires that v.transaction_hash is 32 bytes. Consider adding a validation if there's any possibility of incorrect length data.
33-34: Check output format consistency.
Transitioning from encode_hex() or checksummed representation to .to_string() might affect downstream usage or integration.
user-ops-indexer/user-ops-indexer-logic/src/types/account.rs (4)
2-2: Imports align well with project-wide alloy usage.
8-9: Migration from Option<H256> to Option<TxHash> / Option<B256> aligns with new library.
19-20: Verify byte slice length.
As with the other from_slice() calls, confirm that the storage or DB columns always supply the correct length to avoid potential panics.
33-36: Confirm string output sufficiency.
Ensure .to_string() meets any UI or API requirements, especially if checksummed or hex-encoded strings were previously required.
user-ops-indexer/user-ops-indexer-logic/src/repository/factory.rs (7)
2-2: Transition to alloy import looks good.
24-24: Ensure .as_slice() replacement is correct.
Previously, as_bytes() might have returned the same data, but confirm that .as_slice() indeed provides the identical 20-byte slice needed by the filter.
39-39: Using Address::ZERO instead of a manual zero is a clear improvement.
50-50: Double-check tuple arguments.
Confirm that passing page_token.1.as_slice() in the tuple matches the DB column type.
76-76: U160 usage is consistent with alloy library design.
83-83: Hex literal usage is fine.
87-87: Addresses set for testing.
No issues. They are straightforward demonstrations for test coverage.
user-ops-indexer/user-ops-indexer-logic/src/repository/bundle.rs (6)
2-2: Prefer consistent usage of library imports throughout the repository.
It looks good that you're consistently using alloy::primitives::{Address, B256} here instead of mixing imports from different libraries. Ensure other files use the same naming convention for clarity.
22-22: Safe default for page token boundary.
Using (i64::MAX as u64, B256::ZERO, 0) as the default boundary is sensible. Make sure it does not create any undesired edge-case behavior for large block numbers in your environment.
37-38: Efficient byte vector extraction.
The use of to_vec() avoids unnecessary complexity. Confirm that none of the calling wrappers rely on .as_bytes() specifically, or it might break tests unexpectedly.
40-40: Consistent handling of B256.
Replacing as_bytes() with as_slice() is appropriate for the new type. This should maintain clarity in how the hash bytes are handled.
78-78: Confirm bundler address usage.
The test code sets a bundler address to Address::from(U160::from(0x0105)). Verify that it matches the actual bundler addresses expected in the data set.
19-21: Ensure alignment with downstream usage.
The page_token type signature is updated to (u64, B256, u32). Verify that all other references to list_bundles in your code have been updated to use the new tuple structure.
✅ Verification successful
Let me gather more information about the usage of the page token.
Let me analyze the usage of list_bundles in the codebase to verify the page token changes.
Let me check how the page token is parsed and handled in the server code.
Based on the analysis of the codebase, I can now provide the final response:
The page token changes are properly aligned with all usages.
The verification shows that:
- The
list_bundlesfunction's new page token type(u64, B256, u32)is correctly handled in the server code throughparse_filter_3which parses all three components. - The server properly formats the page token in the response by combining all three values:
format!("{},{},{}", b, t, i). - All test cases in
bundle.rsare using the correctNonevalue for the page token parameter, showing they've been updated to match the new signature. - The server-side pagination logic correctly handles the new tuple structure when making calls to
list_bundles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all references to list_bundles have been updated with the new tuple signature
rg -A2 "list_bundles"
Length of output: 2924
Script:
#!/bin/bash
# Check how the page token is used in the codebase
rg -A 5 "page_token"
Length of output: 70753
Script:
#!/bin/bash
# Check the implementation of list_bundles and its callers
ast-grep --pattern 'list_bundles($$$)'
Length of output: 20673
Script:
#!/bin/bash
# Check how page token is parsed in the server code
rg -A 5 "parse_filter_3"
Length of output: 1282
user-ops-indexer/user-ops-indexer-logic/src/repository/paymaster.rs (4)
2-2: Use a single Address library import.
The shift to alloy::primitives::Address is consistent with the rest of the PR, ensuring type uniformity. Good job keeping it consistent across multiple modules.
24-24: Prevent possible mismatches in address filtering.
Calling addr.as_slice() is correct for the new type, but confirm that any previous usage of as_bytes() is updated.
39-39: Reasonable default for page token on paymasters.
Address::ZERO is a good choice. Confirm that this fallback does not accidentally exclude valid data if an actual zero address was meant to be real.
88-92: Validate nonexistent vs. existing data coverage.
These lines test for None and Some(...) paymasters. Confirm that existing test fixtures in your DB stand for both scenarios.
user-ops-indexer/user-ops-indexer-logic/src/indexer/settings.rs (2)
33-34: Ensure all references to trace_client handle None gracefully.
The added field trace_client: Option<TraceClient> may cause unwrap() or runtime issues if not carefully handled. Validate that call sites check for None.
121-121: Default trace client matches new field.
Providing a default None for trace_client is aligned with the new field. Great job ensuring a consistent default.
user-ops-indexer/user-ops-indexer-logic/src/repository/bundler.rs (4)
2-2: Avoid mixing different Address types.
You’re now using alloy::primitives::Address. Check if any leftover usage from ethers or other crates remains to maintain consistency.
27-27: Confirm indexing logic with new address slices.
Replacing addr.as_bytes() with addr.as_slice() is typically correct, but watch for off-by-one issues in slice length if any older code relies on a 20-byte length assumption.
41-41: Page token fallback.
Using (i64::MAX as u64, Address::ZERO) is consistent with other modules. Well done.
88-92: Test coverage for “none found” scenarios.
Verifying None is returned if the bundler isn’t present ensures robust code coverage. Great job.
user-ops-indexer/user-ops-indexer-logic/src/repository/account.rs (10)
2-2: Import from alloy::primitives is consistent.
These imports align with the switch from ethers to alloy for address handling.
38-38: Correct usage of as_slice() for query parameters.
This appears correct for passing the address as bytes into the SQL query.
82-82: Ensure the .to_vec() conversion is necessary.
Converting an Address to a Vec<u8> is valid here, but verify that the database column expects a byte array and not a fixed-size array.
83-83: Verify zero-address fallback logic.
Using the zero address when page_token is absent is acceptable, but confirm it’s the intended default.
103-103: Alloy imports for B256, U160, U256 look consistent.
These type imports are in line with the rest of the migration away from ethers.
110-110: Test address creation is consistent.
Creating addresses from U160 is coherent with the new alloy usage.
114-114: Repetitive test is fine.
Again, verifying that Address::from(U160::from(...)) usage is aligned with new library usage.
128-128: Account scenario with specific address.
This test covers the newly handled address. No issues found.
134-136: Setting factory & transaction hashes with B256.
These test lines verify consistent usage of B256 for creation hashes. Looks correct.
155-155: Filtering on factory address in tests.
Ensures that list_accounts respects the factory filter. This is correct and consistent.
user-ops-indexer/user-ops-indexer-logic/src/repository/user_op.rs (27)
2-2: Alloy imports for Address and B256 are aligned with the library transition.
No issues found.
50-50: Method signature updated to use B256 instead of H256.
This adjustment is in line with the broader migration to alloy.
54-54: Entity lookup using op_hash.as_slice().
Looks correct for retrieving user ops by ID in the database.
92-92: tx_hash_filter changed to Option.
Consistent with the new alloy-based type usage for transaction hashes.
96-96: page_token signature updated to (u64, B256).
Enhanced consistency. Ensure downstream calls also reflect this signature.
98-99: Return type and page_token unwrap to (u64, B256).
The logic uses i64::MAX as u64 and B256::ZERO as defaults. Confirm large range usage is intended.
116-116: Filtering by sender with sender.as_slice().
Aligns with the new Address usage in queries.
119-119: Filtering by bundler address.
Analogous to sender logic. No issues detected.
122-122: Filtering by paymaster address.
Follows the same pattern; no issues.
125-125: Filtering by factory address.
Matches the established approach for other address filters.
128-128: Transaction hash filter uses B256.
Properly channels transaction hash as bytes for the query.
134-134: Filtering by entry point’s address bytes.
Maintains uniform logic for address filters.
147-147: page_token.1 is used as slice in tuple comparison.
Ensure that the database column for the hash is consistently sized.
200-200: topic: B256 for streaming logs.
Matches the new B256 usage for topics.
203-203: Return type streaming B256 items.
Appropriately typed for unprocessed log retrieval.
217-218: Variables passed into SQL statement use .as_slice().
Confirm the database schema expects bytes for address/topic.
227-227: Converting transaction_hash to B256 with from_slice().
Looks correct for bridging raw bytes to B256.
242-242: U160/U256 imported for tests.
Keeps consistent test usage with the new alloy library.
250-250: Testing with B256::from(U256::from(...)).
Verifies user operation by hash correctly.
254-254: Additional test scenario with B256::from(U256::from(...)).
Ensures multiple cases are covered.
261-261: Testing non-consensus scenario with B256.
Checks that field is Some(false). Good coverage.
268-268: Testing partial consensus scenario with None.
Valid edge case coverage.
315-319: Filters on sender and transaction hash using B256.
Confirms that searches with these fields lead to correct result sets.
333-338: ListUserOp fields set to B256 / Address.
This confirms the new type usage in test objects.
344-349: Another test entry with B256 / Address.
Ensures consistent coverage of all fields.
364-366: Topic is B256 in logs test.
Maintains uniform approach to topic usage.
371-371: Asserts the result with B256.
Checks retrieval of unprocessed logs for the provided topic.
user-ops-indexer/user-ops-indexer-server/src/services/user_ops.rs (9)
2-2: Usage of Address and B256 from alloy.
Consistent with the rest of the system’s migration from ethers.
156-156: Next page token now stored as plain string.
Ensures a simpler format for pagination.
173-173: 3-tuple (u64, B256, u32) for bundles.
Reflects expanded pagination token fields.
193-193: Formatting page token with commas for bundler.
Matches the parse_filter_3 logic.
216-216: Two-component page token (u64, B256).
Consistent with list_user_ops pagination scheme.
241-241: Formatting page token (b, o) with commas.
Uniform approach to pagination.
270-270: list_bundlers next_page_params format.
Matches parse_filter_2 usage.
299-299: list_paymasters next_page_params format.
Follows the same approach for building comma-delimited tokens.
328-328: list_factories next_page_params format.
Again consistent with the established pattern.
user-ops-indexer/user-ops-indexer-logic/Cargo.toml (1)
32-32: New alloy dependency.
Introduces the replacement library for ethers. Ensure all relevant features are truly required.
k1rill-fedoseev
force-pushed
the
kf/feat/rewrite-to-alloy
branch
from
ce6bfdc to
bce5c26
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
user-ops-indexer/user-ops-indexer-logic/src/indexer/v07/mod.rs (4)
22-22: Consider using a more robust path resolution for the ABI file.The relative path
"./src/indexer/v07/abi.json"might break if the working directory changes during compilation. Consider using a path relative to the crate root or implementing a build-time solution to ensure reliable ABI file resolution.
Line range hint
59-88: Consider improving error handling and reducing nesting.The calldata parsing logic could benefit from:
- Using custom error types instead of generic
anyhow::Errorfor better error handling- Extracting the mapping logic into separate functions to reduce nesting
Example refactor for the mapping logic:
fn map_aggregated_ops(cd: handleAggregatedOpsCall) -> Vec<ExtendedUserOperation> { cd.opsPerAggregator .into_iter() .flat_map(|agg_ops| map_single_aggregator_ops(agg_ops, cd.beneficiary)) .collect() } fn map_single_aggregator_ops( agg_ops: AggregatedOps, beneficiary: Address, ) -> impl Iterator<Item = ExtendedUserOperation> { agg_ops.userOps.into_iter().map(move |op| ExtendedUserOperation { user_op: op, bundler: beneficiary, aggregator: Some(agg_ops.aggregator), aggregator_signature: Some(agg_ops.signature.clone()), }) }
160-163: Document magic numbers used in byte slicing.The constants
52and20are used for byte slicing without explanation. Consider extracting these as named constants with documentation explaining their significance in the paymaster data structure.const PAYMASTER_DATA_MIN_LENGTH: usize = 52; const PAYMASTER_ADDRESS_LENGTH: usize = 20;
Line range hint
165-171: Add documentation for gas calculations.The gas calculation logic combines multiple components but lacks documentation explaining the formula and its components. Consider adding a doc comment explaining:
- The purpose of each gas component
- The calculation formula
- Any assumptions or constraints
user-ops-indexer/user-ops-indexer-logic/src/indexer/v06/mod.rs (3)
21-21: Consider externalizing ABI references for flexibility.
sol!(IEntrypointV06, "./src/indexer/v06/abi.json")directly references the ABI file path. If your build or environment expects a configurable ABI location or includes multiple ABIs, you may consider loading it via configuration.
108-108: Logging on failure to build a user op is helpful.
You currently log the error and continue with the remaining user ops. This behavior is beneficial if partial ingestion of valid user ops is acceptable. Double-check that this is the intended design.
187-190: Factor out the magic constant for paymaster overhead.
The* 3in the gas calculation is a “magic number” that might benefit from a named constant (e.g.,PAYMASTER_VERIFICATION_MULTIPLIER) to document its purpose.user-ops-indexer/user-ops-indexer-logic/src/indexer/common.rs (1)
Line range hint
12-22: Consider adding graceful error handling for ABI parsing.
Relying on.unwrap()will panic if the ABI is invalid. For better reliability in production, you might prefer a safer alternative like.expect("parse error")or a proper error return.- .unwrap() + .expect("Failed to parse JSON ABI in EXECUTE_SELECTORS")Also applies to: 31-31
user-ops-indexer/user-ops-indexer-logic/src/indexer/base_indexer.rs (2)
131-141: Fallback logic for determiningtrace_client.
Auto-detecting viaget_client_versionis a convenient approach. Ensure you handle edge cases for other node clients (e.g., Erigon, Nethermind).
231-233: Retry/backoff logic.
Good to see exponential(ish) backoff for network or RPC issues. Consider capping the max delay if you anticipate lengthy outages.user-ops-indexer/user-ops-indexer-logic/src/indexer/rpc_utils.rs (1)
35-39: Choosing defaultTraceClient::Parity.
Beware that if the node identifies as anything other than Geth, the logic defaults to Parity. You might want future fallback for other clients (e.g., Erigon).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
user-ops-indexer/user-ops-indexer-logic/src/indexer/base_indexer.rs(17 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/common.rs(8 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/rpc_utils.rs(4 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/settings.rs(4 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/v06/mod.rs(6 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/v07/mod.rs(7 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/account.rs(6 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/bundle.rs(5 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/bundler.rs(6 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/factory.rs(5 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/paymaster.rs(5 hunks)user-ops-indexer/user-ops-indexer-logic/src/repository/user_op.rs(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- user-ops-indexer/user-ops-indexer-logic/src/repository/paymaster.rs
- user-ops-indexer/user-ops-indexer-logic/src/repository/factory.rs
- user-ops-indexer/user-ops-indexer-logic/src/repository/bundler.rs
- user-ops-indexer/user-ops-indexer-logic/src/repository/bundle.rs
- user-ops-indexer/user-ops-indexer-logic/src/repository/user_op.rs
🔇 Additional comments (42)
user-ops-indexer/user-ops-indexer-logic/src/indexer/v07/mod.rs (2)
37-49: Well-structured trait implementation with performance improvements!The changes improve the code by:
- Using constants for version and event signatures instead of runtime computations
- Leveraging
alloy's type-safe ABI decoding for handler matching
Line range hint
1-217: Overall excellent migration toalloy!The migration from
etherstoalloyhas been implemented successfully with:
- Proper type conversions
- Improved type safety through
alloy's ABI handling- Maintained existing functionality
- Better performance through constant usage
The code is production-ready, with only minor suggestions for improved maintainability.
user-ops-indexer/user-ops-indexer-logic/src/repository/account.rs (6)
2-2: LGTM! Import changes align with migration objectives.The change from
etherstoalloyfor theAddresstype is consistent with the PR's goal of migrating toalloy-rs.
38-38: LGTM! Correct address conversion for alloy.The change from
as_bytes()toas_slice()is the appropriate method for converting alloy'sAddresstype to bytes for database operations.
82-83: LGTM! Proper handling of address conversions and defaults.The changes correctly implement:
- Address conversion using
to_vec()for database parameters- Default address handling using
Address::ZERO
103-103: LGTM! Test cases properly updated for alloy types.The changes correctly implement:
- Use of
address!macro for creating test addresses- Proper conversion to
B256usingU256for hashes- Maintained test coverage and assertions while adapting to new types
Also applies to: 110-110, 114-114, 128-128, 134-136
155-155: LGTM! Factory filter test case properly updated.The test case correctly uses the
address!macro for the factory address filter.
Line range hint
1-161: Verify consistent address handling across the codebase.Let's ensure all address conversions have been consistently updated across the codebase.
✅ Verification successful
Based on the search results, I can now provide the final response:
Address handling has been consistently migrated to alloy across the codebase
The verification shows consistent usage of alloy's Address type and its methods:
- All imports use
alloy::primitives::Address- Address conversions consistently use:
as_slice()for converting to byte slicesto_vec()for converting to vectorAddress::from_slice()for creating from bytesAddress::ZEROfor zero address constant- No remaining ethers Address usages found in the core codebase (only in test fixtures)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining ethers Address usage rg -l "ethers::.*Address" # Search for address conversion patterns to ensure consistency rg "as_bytes\(\)" rg "as_slice\(\)" rg "to_vec\(\)" # Search for Address type usage ast-grep --pattern 'Address'Length of output: 49072
user-ops-indexer/user-ops-indexer-logic/src/indexer/v06/mod.rs (13)
7-7: Good consolidation of imports into thev06::IEntrypointV06path.
This change helps ensure that we keep the references toIEntrypointV06CallsandUserOperationclosely coupled, improving readability.
11-16: Smooth transition to thealloyimports.
Usingalloyprimitives and types appears consistent with the overall migration plan away fromethers.
36-40: Constants for version and event signatures look good.
Having these asconstvalues in the trait improves clarity and avoids magic values scattered across the code.
42-48: Well-structured entry point accessor and calldata matching.
Thematches_handler_calldatamethod correctly checks for successful decoding of eitherhandleOpsorhandleAggregatedOps.
Line range hint
58-75: Validate aggregator presence in aggregated ops.
WhenhandleAggregatedOpsis used, the aggregator is set toSome(...). For reliability, consider verifying that the aggregator isn’t a zero address, if that is relevant to your logic. This helps to detect potential invalid aggregator data.
138-143: Use ofmatch_and_parseto interpret logs is clean and composable.
Decoding distinct event types into strongly typed structures simplifies further logic. No issues noted.
147-150: Receipt logs extraction aligns with existing processing.
Filtering logs forDepositedevents here appears consistent with the rest of the code. No issues found.
155-155: Factory extraction frominitCodeis straightforward.
The helperextract_addressis a neat abstraction. No changes recommended.
161-171: UserOp field assignments align with the updatedIEntrypointV06struct.
Transitioning nonce to aB256type, while storing others asU256orBytes, is consistent with the new library’s design.
178-179: Verify defaulting block number and hash to 0 or zero address is correct.
Fallbacking to0orBlockHash::ZEROcan mask situations where the transaction is pending or unconfirmed. Ensure that these defaults fit your downstream logic.
186-186: Mapping the revert reason is clear.
Settingrevert_reasondirectly fromUserOperationRevertReasonkeeps error context intact.
191-191: Ensure no divide-by-zero risk when computing gas price.
user_op_event.actualGasUsedlikely should never be zero, but consider guarding against unexpected conditions to avoid panic.
196-196: Storing the final fee is well-structured.
fee: user_op_event.actualGasCostis straightforward, keeping the final cost easily accessible.user-ops-indexer/user-ops-indexer-logic/src/indexer/common.rs (7)
1-6: Adoption of thealloylibrary looks good.
These imports cleanly replace the olderethersreferences. No immediate issues identified.
64-66: Boundary logic appears robust; verify edge cases with no logs.
The loops skip logs at both ends. If all logs belong to the entry point or paymaster, verify thatrdoes not underflow relative tolin more extreme scenarios. Your test coverage suggests it’s probably handled, but keep an eye on potential off-by-one errors.Also applies to: 69-73
96-102: Fine-grained handling of unknown call data.
The fallback branch(None, None)is appropriate if the call data doesn’t start with a recognized selector or if the decode fails. Logging the error helps debugging. Looks good.
117-120: Test imports are consistent with the new library.
This ensures that the test module references the correctalloyprimitives and data structures.
124-126: Straightforward address definitions.
These are cleaner than hex parsing and reduce boilerplate.
138-141: Comprehensive log creation in tests.
Usinglog_indexoffsets is helpful for boundary testing. Proceed as is.Also applies to: 147-148
180-186: decode_execute_call_data test is clear.
The checks on parsed addresses and call data confirm correctness. Good coverage.user-ops-indexer/user-ops-indexer-logic/src/indexer/settings.rs (4)
1-2: Migration toalloy::primitives::Address.
The import shift fromethersis aligned with the PR goal.
33-34: Newtrace_clientfield.
Allowing optional specification is a nice touch. This design simplifies fallback to auto-detection.
121-121: Defaultingtrace_clienttoNone.
This supports the runtime fallback logic inbase_indexer.rs. No concerns here.
130-132: Usage ofaddress!macro is concise and safe.
The default addresses for entry points are nicely embedded for versions 0.6 and 0.7.user-ops-indexer/user-ops-indexer-logic/src/indexer/base_indexer.rs (6)
65-70: Replacing method-returned signatures withconstis efficient.
TheUSER_OPERATION_EVENT_SIGNATUREandBEFORE_EXECUTION_SIGNATUREconstants improve clarity and performance, removing the overhead of function calls.
106-107: GenericP: Providerpattern.
Dropping the previousCommonTransportin favor of a generic provider is an excellent step for extensibility and testability.
256-258: Expanded block range arguments tou64.
Good practice for potential large chain heights. No immediate concerns.
305-306: Trace client usage in transaction handling.
The method signature is more flexible, offering both Geth and Parity trace flows.
364-364: Aborting when calls/log bundles mismatch.
Surface-level consistency check prevents partial or corrupted data from persisting. Logging the error is helpful.
405-422: Tests thoroughly reference real-world mainnet transactions.
This integration style approach ensures that the indexer logic is validated against realistic data.Also applies to: 428-444, 490-556, 558-617
user-ops-indexer/user-ops-indexer-logic/src/indexer/rpc_utils.rs (4)
19-23:TraceClientenum introduction.
This is a clean and readable abstraction for deciding whether to use Geth or Parity logic incommon_trace_transaction.
74-104: Branching logic for Geth vs. Parity traces.
The separation is clear, with Geth employing built-in call tracers. Returning a custom error for unexpected results is good practice.
154-160: Non-recursive flattening of Geth’s call frames.
This iterative approach is straightforward and mitigates stack overflows on deep call chains. Nicely done.
Line range hint
170-195: Comprehensive test forflatten_geth_trace.
Covers nested calls extensively. The final assertion on the length of results ensures correct expansion.
|
|
||
| impl From<String> for TraceClient { | ||
| fn from(value: String) -> Self { | ||
| if value.contains("Geth/") { |
There was a problem hiding this comment.
didnt get it, why contains Geth/ and not simple derive_fromstr_from_deserialize
There was a problem hiding this comment.
Introduced a second enum with logic similar to the one from ethers-rs https://github.com/gakonst/ethers-rs/blob/6e2ff0ef8af8c0ee3c21b7e1960f8c025bcd5588/ethers-providers/src/rpc/provider.rs#L43-L69 . Seems like they removed it from alloy, unfortunately.
| disable_stack: Some(true), | ||
| client: TraceClient, | ||
| ) -> TransportResult<Vec<CommonCallTrace>> { | ||
| if client == TraceClient::Geth { |
There was a problem hiding this comment.
will you add more options to TraceClient? if so, this code still will compile. I would use match here with explicit indication of options
There was a problem hiding this comment.
Created a second enum for client type, makes sense to keep them separate. So far no plans to support any other tracing method apart from debug_* and trace_*, and I think some listing rule suggested to use if-else instead of match for such simple cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
user-ops-indexer/user-ops-indexer-logic/src/indexer/base_indexer.rs (1)
428-429: Consider implementing proper mocking for tests.The tests currently use a live RPC endpoint which is not ideal for unit testing. Consider implementing proper mocking once the alloy MockIpcServer is fixed or implement a custom mock provider.
Also applies to: 492-493
user-ops-indexer/user-ops-indexer-logic/src/indexer/rpc_utils.rs (4)
51-63: Consider using explicit match arms for better maintainability.The current implementation groups most clients under the
Tracevariant. While this works, explicit match arms would make it clearer which clients support which tracing methods.impl TraceClient { pub fn default_for(client: NodeClient) -> Self { match client { NodeClient::Geth => Self::Debug, - NodeClient::Erigon => Self::Trace, - NodeClient::OpenEthereum => Self::Trace, - NodeClient::Nethermind => Self::Trace, - NodeClient::Besu => Self::Trace, - NodeClient::Reth => Self::Trace, - NodeClient::Unknown => Self::Debug, + NodeClient::Erigon => Self::Trace, // Erigon supports trace_* namespace + NodeClient::OpenEthereum => Self::Trace, // OpenEthereum supports trace_* namespace + NodeClient::Nethermind => Self::Trace, // Nethermind supports trace_* namespace + NodeClient::Besu => Self::Trace, // Besu supports trace_* namespace + NodeClient::Reth => Self::Trace, // Reth supports trace_* namespace + NodeClient::Unknown => Self::Debug, // Default to debug_* namespace for unknown clients } } }
108-124: Extract tracing configuration into constants.The tracing options contain magic values that should be extracted into named constants for better maintainability and documentation.
+const DEFAULT_TRACE_TIMEOUT: &str = "60s"; + GethDebugTracingOptions { config: GethDefaultTracingOptions { enable_memory: Some(false), disable_memory: Some(true), disable_stack: Some(true), disable_storage: Some(true), enable_return_data: Some(false), disable_return_data: Some(true), debug: None, limit: None, }, tracer: Some(GethDebugTracerType::BuiltInTracer( GethDebugBuiltInTracerType::CallTracer, )), tracer_config: GethDebugTracerConfig::default(), - timeout: Some("60s".to_string()), + timeout: Some(DEFAULT_TRACE_TIMEOUT.to_string()), }
Line range hint
166-194: Improve readability of the trace flattening logic.The tree traversal algorithm could be made more readable by:
- Adding comments explaining the algorithm
- Using more descriptive variable names
- Extracting the type mapping into a separate function
+/// Maps Geth trace types to internal trace types +fn map_trace_type(typ: &str) -> TraceType { + match typ { + "CALL" => TraceType::Call, + "CALLCODE" => TraceType::CallCode, + "STATICCALL" => TraceType::StaticCall, + "DELEGATECALL" => TraceType::DelegateCall, + "CREATE" | "CREATE2" => TraceType::Create, + _ => TraceType::Other, + } +} + fn flatten_geth_trace(root: CallFrame) -> Vec<CommonCallTrace> { + // Stack for iterative tree traversal + // Each tuple contains (frame, child_index) let mut path = Vec::from([(&root, 0)]); let mut res = Vec::new(); while let Some((frame, idx)) = path.pop() { + // Process the current frame if we haven't seen it before if idx == 0 { res.push(CommonCallTrace { - typ: match frame.typ.as_str() { - "CALL" => TraceType::Call, - "CALLCODE" => TraceType::CallCode, - "STATICCALL" => TraceType::StaticCall, - "DELEGATECALL" => TraceType::DelegateCall, - "CREATE" => TraceType::Create, - "CREATE2" => TraceType::Create, - _ => TraceType::Other, - }, + typ: map_trace_type(frame.typ.as_str()), from: frame.from, to: frame.to, input: frame.input.clone(), }); } + // Continue traversing children if there are more if frame.calls.len() > idx { + // Schedule the current frame to process its next child path.push((frame, idx + 1)); + // Schedule the child frame to be processed path.push((&frame.calls[idx], 0)); } } res }
205-224: Add more test cases for edge cases.While the current tests are good, consider adding tests for:
- Empty trace trees
- Deep trace trees (to verify stack usage)
- Malformed client strings
- All trace types
- Error cases in trace parsing
Example additional test:
#[test] fn test_node_client_edge_cases() { // Test malformed strings assert_eq!(NodeClient::from("geth".to_string()), NodeClient::Geth); assert_eq!(NodeClient::from("/".to_string()), NodeClient::Unknown); assert_eq!(NodeClient::from("invalid/v1.0".to_string()), NodeClient::Unknown); // Test case sensitivity assert_eq!(NodeClient::from("GETH/v1.0".to_string()), NodeClient::Geth); } #[test] fn test_flatten_geth_trace_empty() { let root = CallFrame { from: address!("0x0000000000000000000000000000000000000000"), to: None, input: bytes!(""), output: bytes!(""), calls: vec![], value: 0.into(), typ: "CALL".to_string(), }; let res = flatten_geth_trace(root); assert_eq!(res.len(), 1); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
user-ops-indexer/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
user-ops-indexer/README.md(1 hunks)user-ops-indexer/user-ops-indexer-logic/Cargo.toml(1 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/base_indexer.rs(17 hunks)user-ops-indexer/user-ops-indexer-logic/src/indexer/rpc_utils.rs(3 hunks)user-ops-indexer/user-ops-indexer-server/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- user-ops-indexer/README.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (6)
user-ops-indexer/user-ops-indexer-server/Cargo.toml (1)
22-22: LGTM! Clean dependency update.The addition of alloy with appropriate features and removal of ethers-related dependencies aligns well with the PR objective.
user-ops-indexer/user-ops-indexer-logic/Cargo.toml (1)
32-32: LGTM! Comprehensive feature selection.The alloy dependency is configured with all necessary features for the logic component, maintaining version consistency with the server component.
user-ops-indexer/user-ops-indexer-logic/src/indexer/base_indexer.rs (4)
9-16: LGTM! Clean migration to alloy types.The migration from ethers to alloy types is comprehensive and well-structured.
67-69: Great improvements to the IndexerLogic trait!
- Converting event signatures to constants improves efficiency.
- The new
match_and_parsemethod enhances type safety withSolEvent.Also applies to: 92-97
Line range hint
231-246: Improved error handling for transport issues.The error handling now properly detects WebSocket connection closures and implements appropriate retry logic.
133-141: Well-implemented trace client handling.The code properly handles trace client selection with fallback mechanism based on node client version.
Rewrite
user-ops-indexerto usealloy-rsexclusively instead of deprecatedethers-rsSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
etherstoalloylibrary for various functionalities, improving type safety and modularity.Chores