feat: indexer refactor - consolidate listeners and add ctx#1043
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughProvider trait bounds were tightened (Clone added) and EnclaveContract made generic to expose uniform address/provider accessors. EventListener now accepts multiple addresses, uses non-mutable handlers, and exposes a public async listen. EnclaveIndexer gained an IndexerContext<S,R>, became generic over ProviderType, and event handlers use Arc. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Listener as EventListener
participant ContractA as Contract(addrA)
participant ContractB as Contract(addrB)
participant Indexer as EnclaveIndexer
participant Ctx as IndexerContext
participant Handler as EventHandler
Note over Test,Listener: create listener for multiple addresses
Test->>Listener: create_contract_listener(ws_url, &[addrA, addrB])
Listener->>ContractA: subscribe filter(addrA)
Listener->>ContractB: subscribe filter(addrB)
Note over Test,Listener: register handler (non-mutable receiver)
Test->>Listener: add_event_handler(&self, callback)
ContractA->>Listener: emit Event
Listener->>Indexer: dispatch event
Indexer->>Ctx: build Arc<IndexerContext>
Listener->>Handler: invoke callback(event, ctx)
Handler->>Ctx: ctx.store() / ctx.contract()
ContractB->>Listener: emit PublishMessage
Listener->>Handler: invoke callback(event, ctx)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/evm-helpers/src/contracts.rs (1)
167-200: Generic ProviderType + EnclaveContract helpers look sound; only minor nitsThe new
ProviderTypebounds and the genericEnclaveContract<R>accessors (address,get_provider) mesh cleanly with the existing read/write aliases and the indexer’s generic usage.Two small, non-blocking cleanups you might consider:
- In the
EnclaveRead for EnclaveContract<T>impl,T: Send + Syncis redundant becauseProviderTypealready impliesSend + Sync.get_providercurrently returns a clonedArc<R::Provider>. If call sites don’t actually need ownership, an alternative would be returning&Arc<R::Provider>to avoid an extraArcclone on hot paths.These are purely stylistic/efficiency nits; the current implementation is correct.
crates/evm-helpers/src/listener.rs (1)
39-64: Listener refactor enables shared, immutable handle and multi-address filtersThe
add_event_handlerchange to take&selfand store handlers behindArc<RwLock<...>>looks solid: it lets you register handlers from multiple owners without mutable access, and the per-log decode plustokio::spawnpreserves concurrent handling.The new
create_contract_listener(ws_url, addresses: &[&str])correctly parses all addresses into aVec<Address>and builds a multi-address filter, which lines up with the updated tests and indexer constructor.Only very minor, optional nits you might consider:
- Renaming the local
addressVec toaddressesto mirror the parameter name.- If you ever expect to call this with an empty slice, you might want to assert or early-return to fail fast.
Functionally this is in good shape.
Also applies to: 97-107
crates/indexer/tests/integration.rs (1)
57-84: Async handlers and state assertions are correct; consider an async mutex for captured messagesThe
InputPublishedhandler that increments"input_count"viactx.store().modifyand thePublishMessagehandler that push values intocaptured_messagesboth look logically correct and give good coverage of the new context-based handler API and two-contract flow.One optional improvement: inside the
PublishMessagehandler you’re usingArc<Mutex<Vec<String>>>fromstd::sync. Because these handlers are run on the Tokio runtime, atokio::sync::Mutex<Vec<String>>would avoid blocking the executor thread if the lock ever contends and is generally preferred in async code. For this test it’s unlikely to matter, but it would align the style with the rest of the async stack.Also applies to: 149-195
crates/indexer/src/indexer.rs (1)
206-214: Pattern confirmed: multi-address listening is intentional design.Verification shows the multi-address listener pattern is correct and intentional:
from_endpoint_address_in_memis the only constructor accepting&[&str], specifically designed for listening to multiple contract addresses- The listener (via
create_contract_listener) filters events from all provided addresses- The contract instance is created for the primary address (first in array)
- The test in
crates/indexer/tests/integration.rs:51-54demonstrates this usage with bothenclave_addressandemit_logs_address- Similar pattern used in
new_writewhich explicitly listens to bothcontract_addressandregistry_addressThe code is correct. Adding a doc comment explaining this multi-contract listening capability would improve clarity but is not required.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/evm-helpers/src/contracts.rs(2 hunks)crates/evm-helpers/src/listener.rs(2 hunks)crates/evm-helpers/tests/integration.rs(2 hunks)crates/indexer/src/indexer.rs(3 hunks)crates/indexer/tests/helpers.rs(2 hunks)crates/indexer/tests/integration.rs(5 hunks)examples/CRISP/server/src/server/indexer.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
crates/evm-helpers/tests/integration.rscrates/evm-helpers/src/contracts.rs
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/indexer/tests/helpers.rscrates/indexer/src/indexer.rscrates/indexer/tests/integration.rsexamples/CRISP/server/src/server/indexer.rs
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
crates/indexer/src/indexer.rscrates/indexer/tests/integration.rsexamples/CRISP/server/src/server/indexer.rs
📚 Learning: 2025-04-30T06:25:14.721Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 372
File: packages/ciphernode/events/src/eventbus.rs:15-15
Timestamp: 2025-04-30T06:25:14.721Z
Learning: EnclaveEvent implements Display in packages/ciphernode/events/src/enclave_event/mod.rs, which satisfies the Event trait requirement. Static analysis tools may incorrectly flag implementations as missing when they do exist.
Applied to files:
crates/indexer/src/indexer.rsexamples/CRISP/server/src/server/indexer.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 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.
Applied to files:
crates/indexer/src/indexer.rscrates/indexer/tests/integration.rs
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-11-05T03:56:22.254Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 170
File: packages/ciphernode/evm/tests/evm_reader.rs:63-63
Timestamp: 2024-11-05T03:56:22.254Z
Learning: In the Rust test function `test_logs` in `packages/ciphernode/evm/tests/evm_reader.rs`, a sleep duration of 1 millisecond is sufficient for reliable event processing, even in CI environments.
Applied to files:
crates/indexer/tests/integration.rs
🧬 Code graph analysis (6)
crates/evm-helpers/tests/integration.rs (1)
crates/evm-helpers/src/listener.rs (1)
create_contract_listener(97-108)
crates/evm-helpers/src/listener.rs (1)
crates/indexer/src/indexer.rs (5)
add_event_handler(287-302)new(46-50)new(115-117)new(269-285)e(369-369)
crates/indexer/tests/helpers.rs (2)
crates/evm-helpers/tests/helpers.rs (1)
setup_provider(44-55)crates/evm-helpers/src/listener.rs (1)
new(31-37)
crates/indexer/src/indexer.rs (3)
packages/enclave-sdk/src/index.ts (2)
EventListener(11-11)E3(16-16)crates/evm-helpers/src/contracts.rs (3)
new(203-209)create_read(276-290)create_write(252-273)crates/evm-helpers/src/listener.rs (3)
new(31-37)create_contract_listener(97-108)add_event_handler(39-64)
crates/indexer/tests/integration.rs (2)
crates/indexer/tests/helpers.rs (1)
setup_two_contracts(45-60)crates/indexer/src/indexer.rs (7)
enclave_address(186-188)from_endpoint_address_in_mem(207-214)store(176-178)store(447-450)new(46-50)new(115-117)new(269-285)
examples/CRISP/server/src/server/indexer.rs (1)
crates/indexer/src/indexer.rs (7)
store(176-178)store(447-450)contract(183-185)new_write(251-265)register_e3_activated(304-355)register_ciphertext_output_published(377-395)register_plaintext_output_published(397-414)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (10)
crates/evm-helpers/tests/integration.rs (1)
28-32: Tests correctly adapted to multi-address listener APIBoth
test_event_listenerandtest_overlapping_listener_handlersare wired correctly to the newcreate_contract_listener(&ws_url, &[&str])signature, and the temporaryto_string()usage is safe since the slice is only consumed synchronously inside the constructor.No changes needed here.
Also applies to: 107-112
crates/indexer/tests/helpers.rs (1)
7-8: Two-contract helper is well-structured for shared-provider tests
setup_two_contractscleanly reuses a single provider (wrapped inArc) to deploy bothEnclaveandEmitLogs, and returns both instances plus their addresses and the endpoint/Anvil handle. This matches how the integration test consumes it and avoids duplicating provider setup.Looks good as-is.
Also applies to: 45-60
crates/indexer/tests/integration.rs (1)
12-35: Indexer wiring and two-contract setup align with new generic designUsing
EnclaveIndexer::<InMemoryStore, ReadOnly>::from_endpoint_address_in_memtogether withsetup_two_contractsand passing both the enclave and logs addresses into the listener nicely exercises the new multi-address indexer path.The added EmitLogs binding and
PublishMessageimport are consistent with how you later assert on cross-contract events. Overall the top-level test scaffolding looks correct and matches the updated indexer API.Also applies to: 38-56
examples/CRISP/server/src/server/indexer.rs (3)
39-177: register_e3_requested correctly migrated to context-based handlerThe
register_e3_requestedpath now usesindexer.add_event_handler(|event, ctx| ...)withctx.store()to build aCrispE3Repository, and the rest of the logic (decodingcustomParams, fetching token holders, building the Merkle tree, and callingCRISPContractFactory::create_write().set_round_data) is essentially unchanged.The interplay between the module-level
Result<T> = std::result::Result<..., Box<dyn Error>>and the inner handler’seyre-based errors is handled correctly via?, and returningOk(indexer)preserves the fluent registration pattern. No issues here.
179-285: Activation and output-published handlers use ctx cleanly and preserve semanticsFor
register_e3_activated,register_ciphertext_output_published, andregister_plaintext_output_published, switching to handlers of the form|event, ctx|and obtaining the store viactx.store()keeps the previous behavior (round lifecycle management, status updates, compute trigger, and vote tallying) while decoupling the code from direct indexer internals.The long sleep in
register_e3_activatedbased onexpirationremains confined to the per-event task spawned by the listener and doesn’t block handler registration or other events.These functions look correct after the refactor.
330-385: CommitteePublished handling is now fully integrated into the write indexerThe new
register_committee_publishedimplementation:
- Uses
ctx.contract()to callget_e3and perform the idempotency check (expiration > 0),- Computes the wait duration until
startWindow[0]usingget_current_timestamp_rpc(),- Sleeps until the start time (if needed) and then calls
contract.activate.Hooking this into
start_indexervia the unifiedEnclaveIndexer::new_write(ws_url, contract_address, registry_address, store, private_key)means a single indexer instance now listens to both the enclave and registry addresses and drives activation, which matches the PR objective of integrating the registry into the CRISP indexer.Overall, the flow and error handling look consistent with the rest of the module.
Also applies to: 397-418
crates/indexer/src/indexer.rs (4)
16-18: LGTM! Clean introduction of generic provider support.The addition of
ProviderType,ReadOnly, andReadWritetypes, along with parameterizingEnclaveIndexeroverR: ProviderType, correctly enables the indexer to work with both read-only and read-write contracts. The generic approach is idiomatic and maintains type safety.Also applies to: 148-154
156-193: Well-structured context abstraction.The
IndexerContextstruct cleanly encapsulates shared state for event handlers, and the accessor pattern provides a clear API. The use ofClonefor accessors is appropriate since these are Arc-wrapped types. This design successfully decouples event handlers from the full indexer state.
287-302: Excellent refactoring of event handler API.The updated
add_event_handlersignature now provides handlers withArc<IndexerContext<S, R>>, enabling them to access the listener, contract, and store. This change directly supports the PR objective of allowing handlers to add timeouts and other event listeners during execution. The Arc wrapping and closure capture pattern is correct.
304-414: Event handlers correctly refactored to use context.All four event handler registrations (
register_e3_activated,register_input_published,register_ciphertext_output_published,register_plaintext_output_published) have been consistently updated to receive and use theIndexerContext. The core business logic remains unchanged—only the mechanism for accessing shared state has shifted from direct captures to context accessors. The refactoring is sound.
ryardley
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/evm-helpers/src/listener.rs (1)
97-105: Guard against emptyaddressesand improve parse error diagnosticsRight now an empty
addressesslice quietly produces a filter with no addresses; that’s easy to misuse and hard to debug. Also, parse errors don’t say which address failed.Consider early-returning on empty input and enriching the parse error with the offending string:
- pub async fn create_contract_listener(ws_url: &str, addresses: &[&str]) -> Result<Self> { - let provider = Arc::new(ProviderBuilder::new().connect(ws_url).await?); - - let address = addresses - .iter() - .map(|a| a.parse::<Address>().map_err(|e| eyre::eyre!("{e}"))) - .collect::<Result<Vec<_>>>()?; - let filter = Filter::new() - .address(address) - .from_block(BlockNumberOrTag::Latest); + pub async fn create_contract_listener(ws_url: &str, addresses: &[&str]) -> Result<Self> { + if addresses.is_empty() { + return Err(eyre::eyre!("at least one contract address is required")); + } + + let provider = Arc::new(ProviderBuilder::new().connect(ws_url).await?); + + let addresses = addresses + .iter() + .map(|a| a + .parse::<Address>() + .map_err(|e| eyre::eyre!("invalid address `{a}`: {e}"))) + .collect::<Result<Vec<_>>>()?; + let filter = Filter::new() + .address(addresses) + .from_block(BlockNumberOrTag::Latest);crates/indexer/src/indexer.rs (2)
156-193:IndexerContextdesign cleanly encapsulates shared stateThe
IndexerContext<S, R>layout andfrom_indexerconstructor correctly clone the shared store (SharedStoreoverArc<RwLock<S>>), listener, contract, and static metadata. Accessors returning clones ofSharedStore,EventListener, andEnclaveContract<R>make it straightforward for handlers to work with shared resources without borrowingEnclaveIndexeritself. Only minor nit:enclave_addressreturning aStringclone is fine, but if this ever becomes hot-path you might consider returning an&stror an address type instead.
233-250: Combined enclave+registry listener is fine; confirm registry contract needs
new_writenow listens to bothcontract_addressandregistry_addressbut only instantiates anEnclaveContractforcontract_address. That’s a reasonable first step if registry events are handled purely as logs and don’t need contract calls, but if you later need typed registry reads/writes you’ll probably want a second contract handle in the context.No change needed right now, but keep this in mind when you start wiring registry event handlers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/evm-helpers/src/listener.rs(2 hunks)crates/indexer/src/indexer.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2025-04-30T06:25:14.721Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 372
File: packages/ciphernode/events/src/eventbus.rs:15-15
Timestamp: 2025-04-30T06:25:14.721Z
Learning: EnclaveEvent implements Display in packages/ciphernode/events/src/enclave_event/mod.rs, which satisfies the Event trait requirement. Static analysis tools may incorrectly flag implementations as missing when they do exist.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 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.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/indexer/src/indexer.rs
🧬 Code graph analysis (1)
crates/evm-helpers/src/listener.rs (1)
crates/indexer/src/indexer.rs (5)
add_event_handler(271-286)new(46-50)new(115-117)new(253-269)e(353-353)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: build_sdk
- GitHub Check: test_contracts
- GitHub Check: crisp_rust_unit
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (7)
crates/evm-helpers/src/listener.rs (1)
39-64: Non-mutating handler registration with shared handler map looks goodUsing
&selfwithArc<RwLock<HashMap<…>>>forhandlerskeeps registration safe and allows adding handlers from contexts that only hold an immutable listener reference. The wrapping closure correctly decodesLogintoE, dispatches via anArc-wrapped handler, and preserves the topic-keyed dispatch semantics.crates/indexer/src/indexer.rs (6)
16-18: Generalized contract imports align with provider-parameterized indexerImporting
EnclaveContract,EnclaveContractFactory,ProviderType,ReadOnly, andReadWriteprovides the right building blocks for a provider-generic indexer without hard-coding read-only contracts. This matches the new genericEnclaveIndexer<S, R>design.
148-154: GenericEnclaveIndexer<S, R>construction and stored metadata look consistentMaking
EnclaveIndexergeneric overS: DataStoreandR: ProviderType, and wiringnewto derivechain_idfromcontract.providerplus cachingcontract_addressas aString, is coherent with the rest of the API. The struct initialization correctly wrapsstoreinArc<RwLock<_>>and runssetup_listeners()before returning the instance.Also applies to: 252-269
195-205:new_with_in_mem_storecorrectly reuses the generic constructorConstructing an
InMemoryStoreand delegating toEnclaveIndexer::newkeeps the in-memory variant thin and consistent with the generic path. This is a nice simplification over duplicating initialization logic.
271-285: ctx-awareadd_event_handlerwiring looks correctWrapping the user handler as
Fn(E, Arc<IndexerContext<S, R>>)and building anArc<IndexerContext>once per registration cleanly decouples handler lifetimes fromEnclaveIndexer. Each event gets the same shared context instance viactx.clone(), and store/contract access is safely shared via the underlyingArc<RwLock<_>>. This matches the PR objective of allowing handlers to access indexer context and even register new listeners.
288-338:E3Activatedhandler correctly migrated to useIndexerContextThe refactored
register_e3_activatedclosure pullscontract,store,enclave_address, andchain_idfromctx, reconstructsE3using on-chain data plus the event, and persists viaE3Repositoryas before. The conversion helpers (u64_try_from) and field mapping remain consistent, so behavior should be unchanged apart from sourcing values fromIndexerContext.
341-359: Other event registrations (InputPublished/ ciphertext / plaintext) are consistent and context-awareAll three handlers now use
ctx.store()and log viaprintln!, then delegate toE3Repositoryto update the appropriate ciphertext/PLAINTEXT fields. The refactor keeps the logic symmetric across events and aligns with the new context-based design, without changing semantics.Also applies to: 361-379, 381-398
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/indexer/src/indexer.rs (1)
206-231: Still need a guard for empty contract_addresses to avoid panicsBoth
from_endpoint_address_in_memandfrom_endpoint_addressstill indexcontract_addresses[0]without checking that the slice is non-empty; if called with an empty slice this will panic at runtime. Adding an explicit guard makes this API safer and matches the earlier review feedback.Apply a guard like this:
pub async fn from_endpoint_address_in_mem( ws_url: &str, contract_addresses: &[&str], ) -> Result<Self> { + if contract_addresses.is_empty() { + return Err(eyre("contract_addresses must contain at least the enclave contract address"))); + } let listener = EventListener::create_contract_listener(ws_url, contract_addresses).await?; let contract = EnclaveContractFactory::create_read(ws_url, contract_addresses[0]).await?; EnclaveIndexer::<InMemoryStore, ReadOnly>::new_with_in_mem_store(listener, contract).await } @@ pub async fn from_endpoint_address( ws_url: &str, contract_addresses: &[&str], store: InMemoryStore, ) -> Result<Self> { + if contract_addresses.is_empty() { + return Err(eyre("contract_addresses must contain at least the enclave contract address"))); + } let listener = EventListener::create_contract_listener(ws_url, contract_addresses).await?; let contract = EnclaveContractFactory::create_read(ws_url, contract_addresses[0]).await?; EnclaveIndexer::new(listener, contract, store).await }
🧹 Nitpick comments (2)
examples/CRISP/server/src/server/indexer.rs (1)
39-45: E3Requested handler correctly uses ctx.store; consider reusing ctx.contractThe refactor to take
ctxand derive the repository fromctx.store()looks correct and keeps the handler context-aware. Given that the indexer is now constructed with a write-capable contract, you could optionally reusectx.contract()forsetRoundDatainstead of constructing a freshCRISPContractFactory::create_writeper event, to avoid duplicated config and extra client setup, assuming the genericEnclaveContract<ReadWrite>exposes the same write method.Also applies to: 150-176
crates/indexer/src/indexer.rs (1)
341-359: Consider using u64_try_from for index conversion in InputPublished
register_input_publishedstill usese.index.to::<u64>()when inserting the ciphertext input, while the rest of the module prefersu64_try_fromto surface conversion errors. Depending onUint::to::<u64>()’s semantics, this could panic or silently truncate for large indices; aligning this withu64_try_fromwould make error handling more consistent.For example:
- let e3_id = u64_try_from(e.e3Id)?; - - let mut repo = E3Repository::new(store, e3_id); - repo.insert_ciphertext_input(e.data.to_vec(), e.index.to::<u64>()) + let e3_id = u64_try_from(e.e3Id)?; + let index = u64_try_from(e.index)?; + + let mut repo = E3Repository::new(store, e3_id); + repo.insert_ciphertext_input(e.data.to_vec(), index) .await?;Please verify the overflow behavior of
Uint::to::<u64>()in the alloy docs and decide whether this change is desirable for your use case.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/indexer/src/indexer.rs(3 hunks)examples/CRISP/server/src/server/indexer.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
examples/CRISP/server/src/server/indexer.rscrates/indexer/src/indexer.rs
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
examples/CRISP/server/src/server/indexer.rscrates/indexer/src/indexer.rs
📚 Learning: 2025-04-30T06:25:14.721Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 372
File: packages/ciphernode/events/src/eventbus.rs:15-15
Timestamp: 2025-04-30T06:25:14.721Z
Learning: EnclaveEvent implements Display in packages/ciphernode/events/src/enclave_event/mod.rs, which satisfies the Event trait requirement. Static analysis tools may incorrectly flag implementations as missing when they do exist.
Applied to files:
examples/CRISP/server/src/server/indexer.rscrates/indexer/src/indexer.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 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.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
crates/indexer/src/indexer.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Validate PR Title
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: rust_integration
- GitHub Check: test_contracts
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: crisp_rust_unit
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (11)
examples/CRISP/server/src/server/indexer.rs (5)
18-28: Context-aware EnclaveIndexer and ReadWrite usage look consistentSwitching to
EnclaveIndexer<impl DataStore, ReadWrite>and importingReadWritefrom the contracts module aligns this example with the new generic indexer API; no issues spotted with the new types or their usage instart_indexer.Also applies to: 405-412
179-266: E3Activated handler’s migration to ctx-based store access looks correctThe handler now derives both
CrispE3RepositoryandCurrentRoundRepositoryfromctx.store(), and the expiration/wait/sleep logic is preserved; I don’t see behavioral regressions introduced by the ctx refactor here.
268-285: Ciphertext/plaintext handlers correctly switched to ctx.store without behavior changeBoth
register_ciphertext_output_publishedandregister_plaintext_output_publishednow takectxand usectx.store()while keeping the repository interactions and status transitions the same as before; this refactor looks safe.Also applies to: 287-328
330-385: CommitteePublished handler appropriately uses ctx.contract with consolidated listenerUsing
ctx.contract()inside theCommitteePublishedhandler matches the newnew_with_write_contractsetup (single indexer watching enclave + registry addresses) and removes the need for a separate contract wiring; the activation flow and timing logic remain correct.
397-418: start_indexer wiring with new_with_write_contract matches the new indexer designConstructing the CRISP indexer via
EnclaveIndexer::new_with_write_contract(passing both enclave and registry addresses) and then chaining theregister_*calls beforestart()cleanly matches the consolidated-listener/ctx-based API; this is a straightforward and readable entrypoint.crates/indexer/src/indexer.rs (6)
147-193: New generic EnclaveIndexer and IndexerContext design looks solidMaking
EnclaveIndexer<S, R>generic over bothDataStoreandProviderTypeand introducingIndexerContext<S, R>(with accessors for store, listener, contract, address, and chain_id) cleanly supports the ctx-based handler pattern without obvious ownership or concurrency issues.
195-204: Generic new_with_in_mem_store helper is correctly parameterized
EnclaveIndexer<InMemoryStore, R>::new_with_in_mem_storenow accepting a genericEnclaveContract<R>and delegating toEnclaveIndexer::newkeeps the in-memory convenience constructor aligned with the new ProviderType-generic contract without changing behavior.
233-250: new_with_write_contract correctly wires a write-capable indexer with two-address listener
EnclaveIndexer<S, ReadWrite>::new_with_write_contractbuilding a listener over[contract_address, registry_address]and a write-capable contract matches the consolidated-listener design and correctly delegates toEnclaveIndexer::new; this looks good.
252-287: add_event_handler’s ctx-based API is well-structuredPassing
Fn(E, Arc<IndexerContext<S, R>>) -> Futintoadd_event_handler, wrapping it inArc, and creating a singleArc<IndexerContext<S, R>>per registration that is cloned per event provides a clean, thread-safe way for handlers to access store, contract, listener, and metadata via ctx.
288-339: E3Activated handler correctly migrated to IndexerContext and centralized conversionsThe handler now pulls
contract, shared store, enclave address, and chain_id fromctx, and usesu64_try_fromconsistently for 256-bit to u64 conversions before buildingE3; the repository write is unchanged aside from using the shared store wrapper.
361-398: Ciphertext/plaintext output handlers’ ctx refactor looks correctBoth output handlers now obtain the shared store via
ctx.store()and otherwise preserve the existing logging and repository writes (setting ciphertext/plaintext output and leaving status handling to downstream logic); the ctx-based change here looks safe.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/indexer/src/indexer.rs (1)
266-297: Weak reference pattern correctly prevents memory leaks.The refactored
add_event_handlercorrectly uses weak references to prevent the circular reference issue flagged in earlier reviews. When theEnclaveIndexeris dropped, handlers will gracefully fail to upgrade their context reference.Consider replacing
println!with proper logging on line 291:- println!("Context was dropped!"); + info!("Context was dropped - handler skipped");This would be consistent with other logging in the module and provide better observability control.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/indexer/src/indexer.rs(3 hunks)examples/CRISP/packages/crisp-contracts/README.md(1 hunks)examples/CRISP/packages/crisp-contracts/deployed_contracts.json(1 hunks)examples/CRISP/server/src/server/indexer.rs(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/CRISP/packages/crisp-contracts/README.md
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 752
File: packages/enclave-contracts/contracts/Enclave.sol:15-17
Timestamp: 2025-09-19T11:16:53.825Z
Learning: The Enclave contract in the gnosisguild/enclave repository has not been deployed yet as of September 2025, so storage layout considerations for upgradeable contracts don't apply to current changes.
Applied to files:
examples/CRISP/packages/crisp-contracts/deployed_contracts.json
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
examples/CRISP/server/src/server/indexer.rscrates/indexer/src/indexer.rs
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
examples/CRISP/server/src/server/indexer.rscrates/indexer/src/indexer.rs
📚 Learning: 2025-04-30T06:25:14.721Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 372
File: packages/ciphernode/events/src/eventbus.rs:15-15
Timestamp: 2025-04-30T06:25:14.721Z
Learning: EnclaveEvent implements Display in packages/ciphernode/events/src/enclave_event/mod.rs, which satisfies the Event trait requirement. Static analysis tools may incorrectly flag implementations as missing when they do exist.
Applied to files:
examples/CRISP/server/src/server/indexer.rscrates/indexer/src/indexer.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 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.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-08T01:50:45.185Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/core/src/events.rs:323-323
Timestamp: 2024-10-08T01:50:45.185Z
Learning: When suggesting that instances of `E3Requested` should include `src_chain_id`, double-check to ensure that the field is actually missing before making the suggestion.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-22T03:41:21.226Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/repositories.rs:40-71
Timestamp: 2024-10-22T03:41:21.226Z
Learning: In `packages/ciphernode/router/src/repositories.rs`, prefer to keep method implementations as they are if they are straightforward and maintainable, even if refactoring could reduce duplication.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-10T23:24:43.341Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 143
File: packages/ciphernode/sortition/src/sortition.rs:4-9
Timestamp: 2024-10-10T23:24:43.341Z
Learning: In the `Sortition` module (`packages/ciphernode/sortition/src/sortition.rs`), errors are sent to the event bus using `self.bus.err`, which handles logging and printing. Therefore, explicit use of the `tracing` crate for logging errors may not be necessary in this context.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-16T09:52:53.807Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/src/data_store.rs:74-75
Timestamp: 2024-10-16T09:52:53.807Z
Learning: In this project, the actor model handles potential errors internally, so methods like `checkpoint` in the `Checkpoint` trait do not need to explicitly handle or propagate errors.
Applied to files:
crates/indexer/src/indexer.rs
🧬 Code graph analysis (2)
examples/CRISP/server/src/server/indexer.rs (1)
crates/indexer/src/indexer.rs (7)
store(167-169)store(424-427)contract(174-176)new_with_write_contract(226-241)register_e3_activated(299-350)register_ciphertext_output_published(352-370)register_plaintext_output_published(372-389)
crates/indexer/src/indexer.rs (2)
crates/evm-helpers/src/listener.rs (5)
new(38-44)create_contract_listener(100-111)addresses(103-106)add_event_handler(46-71)listen(73-97)crates/evm-helpers/src/contracts.rs (5)
new(203-209)create_read(276-290)create_write(252-273)get_e3(96-96)get_e3(305-309)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build_e3_support_dev
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
- GitHub Check: rust_unit
- GitHub Check: crisp_rust_unit
- GitHub Check: rust_integration
- GitHub Check: test_net
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (16)
examples/CRISP/packages/crisp-contracts/deployed_contracts.json (1)
113-235: Verify this file is related to PR objectives.The added
localhostdeployment block is well-formed and consistent with the existingsepoliastructure, with appropriate network-specific addresses and low block numbers. However, this appears to be deployment configuration data rather than changes to the indexer refactoring described in the PR objectives (consolidating listeners and adding context to event handlers).Clarify whether this file is:
- Auto-generated by deployment scripts (in which case, verify the generation process was re-run correctly)?
- Manually maintained configuration (in which case, verify the addresses and constructorArgs are accurate for your localhost setup)?
- Supporting infrastructure for the CRISP example tests?
examples/CRISP/server/src/server/indexer.rs (6)
39-180: LGTM! Context-based handler pattern applied correctly.The refactoring to use
EnclaveIndexer<impl DataStore, ReadWrite>and context-based event handlers is consistent and correct. The handler properly accesses the store viactx.store().
182-271: LGTM! Timeout logic correctly implemented.The refactored function correctly uses the context pattern and implements the timeout logic mentioned in the PR objectives. The async sleep mechanism properly waits until the E3 expiration time.
273-333: LGTM! Consistent refactoring applied.Both
register_ciphertext_output_publishedandregister_plaintext_output_publishedcorrectly adopt the context-based pattern withctx.store()access.
335-390: LGTM! Contract access via context works well.The refactoring demonstrates the benefit of the context pattern - the handler can access both the contract via
ctx.contract()and implement the idempotency check and timed activation correctly.
402-425: LGTM! Consistent context pattern.The function correctly adopts the context-based handler pattern with
ctx.store()access.
427-455: LGTM! Excellent consolidation of listener creation.The refactored
start_indexersuccessfully consolidates multiple contracts into a single listener as mentioned in the PR objectives. The chaining pattern is clean, and the added logging provides good observability for the indexer lifecycle.crates/indexer/src/indexer.rs (9)
16-29: LGTM! Imports aligned with new generic design.The added imports for
ReadWrite,ProviderType, andtracingsupport the new context-based architecture.
148-165: LGTM! Clean generic design with proper lifecycle tracking.The introduction of generic parameters
R: ProviderTypeenables flexible contract interactions (ReadOnly/ReadWrite), and theIndexerContextconsolidates all state in a clean, centralized structure. TheDropimplementation provides useful lifecycle visibility.
166-184: LGTM! Clean accessor pattern.The context accessors provide proper, safe access to the indexer state. Cloning Arc-wrapped types is appropriate and efficient.
186-222: LGTM! Well-organized constructor variants.The constructors provide clean initialization paths for different scenarios (in-memory store, custom store, read-only contracts). The design is clear and purposeful.
224-242: LGTM! Proper validation for write contract.The
new_with_write_contractconstructor includes appropriate validation to ensure at least one address is provided, with a clear error message.
244-264: LGTM! Proper initialization flow.The core constructor correctly initializes the context with all necessary state, wraps the store in
SharedStore, and sets up default listeners. The logging provides good visibility.
299-350: LGTM! Context accessors used correctly.The refactored handler correctly accesses contract, store, enclave address, and chain ID through the context. The E3 processing logic is sound.
352-389: LGTM! Consistent context pattern applied.Both
register_ciphertext_output_publishedandregister_plaintext_output_publishedcorrectly adopt the context-based access pattern withctx.store().
391-417: LGTM! Clean public API with proper delegation.The methods correctly delegate to the context for accessing state, and the public API remains clean and intuitive.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/indexer/tests/integration.rs (2)
36-199: Two‑contract integration test and ctx‑based handlers look solid; only minor robustness nitsThe refactored
test_indexercorrectly:
- Uses
setup_two_contractsand a singleEnclaveIndexer::<InMemoryStore, ReadOnly>::from_endpoint_address_in_memwith both enclave and logs addresses.- Switches the
InputPublishedhandler to usectx.store().modifyand then verifies viaindexer.get_store().get("input_count").- Adds a second handler for
EmitLogs::PublishMessageand asserts the captured message, plus verifies E3 state and ciphertext output.All of that aligns well with the PR goal of consolidating listeners and exposing
ctx. The only optional improvement here is test robustness: if you ever see flakes in CI, you could replace the fixedsleep(Duration::from_millis(INDEXER_DELAY_MS))with a small retry/poll loop (e.g., wait up to N ms forciphertext_inputs.len() == expected_input_count/ message vector size) to make the test more resilient to slow environments, but at 10 ms this is probably fine for now.
231-239: Consider building the leak‑test indexer viafrom_endpoint_address_in_memfor API parityIn
memory_leak::create_indexeryou go through the lower‑levelEventListener::create_contract_listener+EnclaveContractFactory::create_read+EnclaveIndexer::new_with_in_mem_store. That works, but it means the leak test is exercising a different construction path than the one used intest_indexerand (presumably) production code, which now goes throughEnclaveIndexer::<InMemoryStore, ReadOnly>::from_endpoint_address_in_mem.If you want the leak test to cover the consolidated‑listener constructor as well, consider reusing that here, e.g.:
- async fn create_indexer() -> Result<EnclaveIndexer<InMemoryStore, ReadOnly>> { - let (_, enclave_address, _, _, endpoint, _anvil) = setup_two_contracts().await?; - - let listener = - EventListener::create_contract_listener(&endpoint, &[&enclave_address]).await?; - let contract = EnclaveContractFactory::create_read(&endpoint, &enclave_address).await?; - - EnclaveIndexer::<InMemoryStore, ReadOnly>::new_with_in_mem_store(listener, contract).await - } + async fn create_indexer() -> Result<EnclaveIndexer<InMemoryStore, ReadOnly>> { + let (_, enclave_address, _, _, endpoint, _anvil) = setup_two_contracts().await?; + + EnclaveIndexer::<InMemoryStore, ReadOnly>::from_endpoint_address_in_mem( + &endpoint.to_string(), + &[&enclave_address.to_string()], + ) + .await + }That keeps the leak test closer to real‑world usage and ensures any future changes in the primary constructor path are covered here as well.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/indexer/tests/integration.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 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.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-11-05T03:56:48.126Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 170
File: packages/ciphernode/evm/tests/evm_reader.rs:79-80
Timestamp: 2024-11-05T03:56:48.126Z
Learning: In `packages/ciphernode/evm/tests/evm_reader.rs`, the `Anvil` instance is automatically cleaned up when its value is dropped, so explicit calls to `anvil.kill()` are unnecessary.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-11-05T03:56:22.254Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 170
File: packages/ciphernode/evm/tests/evm_reader.rs:63-63
Timestamp: 2024-11-05T03:56:22.254Z
Learning: In the Rust test function `test_logs` in `packages/ciphernode/evm/tests/evm_reader.rs`, a sleep duration of 1 millisecond is sufficient for reliable event processing, even in CI environments.
Applied to files:
crates/indexer/tests/integration.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
- GitHub Check: crisp_rust_unit
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: test_net
- GitHub Check: build_sdk
- GitHub Check: rust_unit
- GitHub Check: build_e3_support_dev
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (1)
crates/indexer/tests/integration.rs (1)
241-286: LeakDetector‑based memory‑leak test now actually exercises handler lifetimeThe new
memory_leak::test_memory_leakcorrectly:
- Uses
LeakDetectorwithCREATE_COUNT/DROP_COUNTand aDropCounterthat bumpsDROP_COUNTon finalArcdrop.- Registers an event handler that captures a cloned
LeakDetectorin its environment, then drops the indexer by exiting the inner block.- Asserts
created == droppedafter a short delay, which should reliably catch any retained handler/env that survives indexer drop.This addresses the earlier issue where the leak‑tracking scaffolding existed but wasn’t wired into a real test. The structure looks good and should give you a meaningful safety net around handler/context retention.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
crates/evm-helpers/src/listener.rs (3)
19-19: Remove unused import.The
tracing::infoimport is never used in this file.Apply this diff:
-use tracing::info;
80-85: Consider using tracing for error logging.The error logging uses
eprintln!instead of structured logging. Sincetracingis available in the workspace, consider usingtracing::error!for better observability.Apply this diff:
+use tracing::error; + tokio::spawn(async move { // Spawn the future so that the handlers are processed concurrently if let Err(e) = fut.await { - eprintln!("Error processing event 0x{:x}: {:?}", topic_val, e); + error!("Error processing event 0x{:x}: {:?}", topic_val, e); } });
93-94: Clarify doc comment wording.The doc comment "listen to events from all addresses" could be clearer. It currently suggests listening to events from any address, but it actually listens only to the specified addresses.
Apply this diff:
- /// Create a contract listener that will listen to events from all addresses. + /// Create a contract listener that will listen to events from the specified addresses. pub async fn create_contract_listener(ws_url: &str, addresses: &[&str]) -> Result<Self> {crates/indexer/src/indexer.rs (4)
177-179: Consider returning reference for enclave_address().The
enclave_address()method returns aString, requiring a clone. If the address is frequently accessed, consider returning&strto avoid allocations.Apply this diff:
- pub fn enclave_address(&self) -> String { - self.contract_address.clone() + pub fn enclave_address(&self) -> &str { + &self.contract_address }
197-219: Consider defensive check for empty addresses.While past review notes these are test-only, both
from_endpoint_address_in_memandfrom_endpoint_addressindexaddresses[0]without validation. Consider adding the same guard asnew_with_write_contractfor consistency and robustness.Apply this diff to both methods:
pub async fn from_endpoint_address_in_mem(ws_url: &str, addresses: &[&str]) -> Result<Self> { + let Some(contract_address) = addresses.first() else { + return Err(eyre::eyre!("addresses must contain at least the enclave contract address")); + }; let listener = EventListener::create_contract_listener(ws_url, addresses).await?; - let contract = EnclaveContractFactory::create_read(ws_url, addresses[0]).await?; + let contract = EnclaveContractFactory::create_read(ws_url, contract_address).await?; EnclaveIndexer::<InMemoryStore, ReadOnly>::new_with_in_mem_store(listener, contract).await }And similarly for
from_endpoint_address.
288-289: Use tracing for context drop notification.The
println!should use structured logging for consistency with the rest of the codebase.Apply this diff:
if let Some(ctx) = ctx_weak.upgrade() { handler(e, ctx).await } else { - println!("Context was dropped!"); + info!("Event handler skipped: context was dropped"); Ok(()) }
302-307: Consider using tracing for event logging.The
println!statements for event logging should usetracing::info!for better observability and consistency with the configured logger.Apply this diff:
- println!( - "E3Activated: id={}, expiration={}, pubkey=0x{}...", - e.e3Id, - e.expiration, - hex::encode(&e.committeePublicKey[..8.min(e.committeePublicKey.len())]) - ); + info!( + "E3Activated: id={}, expiration={}, pubkey=0x{}...", + e.e3Id, + e.expiration, + hex::encode(&e.committeePublicKey[..8.min(e.committeePublicKey.len())]) + );Apply similar changes to other event handlers in
register_ciphertext_output_publishedandregister_plaintext_output_published.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/evm-helpers/src/listener.rs(4 hunks)crates/indexer/src/indexer.rs(3 hunks)crates/indexer/tests/integration.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
crates/indexer/tests/integration.rscrates/indexer/src/indexer.rs
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
crates/indexer/tests/integration.rscrates/indexer/src/indexer.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 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.
Applied to files:
crates/indexer/tests/integration.rscrates/indexer/src/indexer.rs
📚 Learning: 2024-11-05T03:56:22.254Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 170
File: packages/ciphernode/evm/tests/evm_reader.rs:63-63
Timestamp: 2024-11-05T03:56:22.254Z
Learning: In the Rust test function `test_logs` in `packages/ciphernode/evm/tests/evm_reader.rs`, a sleep duration of 1 millisecond is sufficient for reliable event processing, even in CI environments.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-11-05T03:56:48.126Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 170
File: packages/ciphernode/evm/tests/evm_reader.rs:79-80
Timestamp: 2024-11-05T03:56:48.126Z
Learning: In `packages/ciphernode/evm/tests/evm_reader.rs`, the `Anvil` instance is automatically cleaned up when its value is dropped, so explicit calls to `anvil.kill()` are unnecessary.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/evm-helpers/src/listener.rscrates/indexer/src/indexer.rs
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-08T01:50:45.185Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/core/src/events.rs:323-323
Timestamp: 2024-10-08T01:50:45.185Z
Learning: When suggesting that instances of `E3Requested` should include `src_chain_id`, double-check to ensure that the field is actually missing before making the suggestion.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-22T03:41:21.226Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/repositories.rs:40-71
Timestamp: 2024-10-22T03:41:21.226Z
Learning: In `packages/ciphernode/router/src/repositories.rs`, prefer to keep method implementations as they are if they are straightforward and maintainable, even if refactoring could reduce duplication.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2025-04-30T06:25:14.721Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 372
File: packages/ciphernode/events/src/eventbus.rs:15-15
Timestamp: 2025-04-30T06:25:14.721Z
Learning: EnclaveEvent implements Display in packages/ciphernode/events/src/enclave_event/mod.rs, which satisfies the Event trait requirement. Static analysis tools may incorrectly flag implementations as missing when they do exist.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-10T23:24:43.341Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 143
File: packages/ciphernode/sortition/src/sortition.rs:4-9
Timestamp: 2024-10-10T23:24:43.341Z
Learning: In the `Sortition` module (`packages/ciphernode/sortition/src/sortition.rs`), errors are sent to the event bus using `self.bus.err`, which handles logging and printing. Therefore, explicit use of the `tracing` crate for logging errors may not be necessary in this context.
Applied to files:
crates/indexer/src/indexer.rs
📚 Learning: 2024-10-16T09:52:53.807Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/src/data_store.rs:74-75
Timestamp: 2024-10-16T09:52:53.807Z
Learning: In this project, the actor model handles potential errors internally, so methods like `checkpoint` in the `Checkpoint` trait do not need to explicitly handle or propagate errors.
Applied to files:
crates/indexer/src/indexer.rs
🧬 Code graph analysis (1)
crates/indexer/src/indexer.rs (2)
crates/evm-helpers/src/listener.rs (5)
new(32-38)addresses(97-100)create_contract_listener(94-105)add_event_handler(40-65)listen(67-91)crates/evm-helpers/src/contracts.rs (3)
new(203-209)create_read(276-290)create_write(252-273)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: rust_integration
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: test_contracts
- GitHub Check: rust_unit
- GitHub Check: crisp_rust_unit
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (7)
crates/evm-helpers/src/listener.rs (1)
40-65: LGTM: Concurrent handler registration.The signature change from
&mut selfto&selfcombined withArc<RwLock<HashMap<...>>>correctly enables adding handlers without exclusive ownership, which is essential for the context-based architecture.crates/indexer/tests/integration.rs (4)
36-57: LGTM: Well-structured test setup.The two-contract setup and context-based indexer initialization correctly demonstrate the new multi-address listening capability.
59-88: LGTM: Context-based event handlers.The event handlers correctly demonstrate the new signature that receives both the event and the
IndexerContext. The use ofctx.store()for accessing the store is proper.
90-198: LGTM: Comprehensive multi-contract test.The test thoroughly validates event processing from two contracts, including proper state updates and message capture. The parameterized delay constant is a good practice.
202-287: LGTM: Proper memory leak detection.The memory leak test module correctly implements drop tracking to verify that handlers and captured context are properly cleaned up. The use of
LeakDetectorwith atomic counters validates that the weak reference pattern inadd_event_handlerprevents reference cycles.crates/indexer/src/indexer.rs (2)
388-414: LGTM: Clean public API.The public methods correctly delegate to the context, providing a clean separation between the public API and internal state management.
147-414: Excellent refactor: context-driven architecture.The refactor successfully achieves the PR objectives:
- Consolidates listeners to handle multiple contracts efficiently
- Provides
ctx(IndexerContext) to handlers, enabling dynamic listener addition- Generic design over
S: DataStoreandR: ProviderTypeoffers flexibility- Weak reference pattern properly prevents memory leaks from circular references
The architecture is clean, well-structured, and properly implements the context-based event handling pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/evm-helpers/tests/integration.rs (2)
31-37: EventListener creation via Arc looks correct; consider a tiny helper to DRY itThe
Arc::new(EventListener::create_contract_listener(&anvil.ws_endpoint(), &[&contract.address().to_string()]).await?)pattern correctly matches the new multi-address API and theArc-based usage model.Given the identical setup in both tests, you might consider extracting a small helper like
fn make_listener(anvil: &Anvil, contract: &EmitLogs) -> Result<Arc<EventListener>>to keep the tests a bit drier and make future changes to the listener wiring (e.g., multiple addresses) easier to apply in one place.Also applies to: 113-119
59-60: Background listener tasks are fire‑and‑forget; fine for tests, but be explicit if reused elsewhereSpawning
listen()on a clonedArc<EventListener>is a good fit for a long‑running background listener in these tests, and relying on runtime teardown to stop it is acceptable here.If a similar pattern gets used outside tests (e.g., in production services), consider capturing the
JoinHandleand providing an explicit shutdown/abort path so that listener failures or panics can be surfaced and controlled more directly.Also applies to: 151-152
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/evm-helpers/tests/integration.rs(5 hunks)crates/indexer/tests/integration.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 372
File: packages/ciphernode/events/src/eventbus.rs:15-15
Timestamp: 2025-04-30T06:25:14.721Z
Learning: EnclaveEvent implements Display in packages/ciphernode/events/src/enclave_event/mod.rs, which satisfies the Event trait requirement. Static analysis tools may incorrectly flag implementations as missing when they do exist.
📚 Learning: 2024-11-05T03:56:22.254Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 170
File: packages/ciphernode/evm/tests/evm_reader.rs:63-63
Timestamp: 2024-11-05T03:56:22.254Z
Learning: In the Rust test function `test_logs` in `packages/ciphernode/evm/tests/evm_reader.rs`, a sleep duration of 1 millisecond is sufficient for reliable event processing, even in CI environments.
Applied to files:
crates/evm-helpers/tests/integration.rscrates/indexer/tests/integration.rs
📚 Learning: 2024-10-08T07:15:06.690Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/evm/src/ciphernode_registry_sol.rs:133-143
Timestamp: 2024-10-08T07:15:06.690Z
Learning: In `packages/ciphernode/evm/src/ciphernode_registry_sol.rs`, the function `helpers::stream_from_evm` in Rust returns `()`, not a `Result`, so error handling with `if let Err(e) = ...` is not applicable.
Applied to files:
crates/evm-helpers/tests/integration.rs
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
crates/indexer/tests/integration.rs
📚 Learning: 2024-11-05T03:56:48.126Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 170
File: packages/ciphernode/evm/tests/evm_reader.rs:79-80
Timestamp: 2024-11-05T03:56:48.126Z
Learning: In `packages/ciphernode/evm/tests/evm_reader.rs`, the `Anvil` instance is automatically cleaned up when its value is dropped, so explicit calls to `anvil.kill()` are unnecessary.
Applied to files:
crates/indexer/tests/integration.rs
🧬 Code graph analysis (2)
crates/evm-helpers/tests/integration.rs (1)
crates/evm-helpers/src/listener.rs (2)
new(32-38)create_contract_listener(94-105)
crates/indexer/tests/integration.rs (3)
crates/indexer/tests/helpers.rs (1)
setup_two_contracts(45-60)crates/indexer/src/indexer.rs (9)
new(46-50)new(115-117)new(242-261)from_endpoint_address_in_mem(201-205)store(167-169)store(421-424)listener(171-173)contract(174-176)new_with_in_mem_store(187-194)crates/evm-helpers/src/listener.rs (1)
create_contract_listener(94-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Validate PR Title
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: test_contracts
- GitHub Check: crisp_rust_unit
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (7)
crates/indexer/tests/integration.rs (6)
8-34: LGTM!The imports and sol! declarations are well-organized, supporting the new two-contract test setup. The
EmitLogsbinding enables capturing events from the secondary contract.
36-57: LGTM!The test setup correctly initializes a multi-contract environment with constants for clarity. The
Arcwrapping enables safe sharing of the indexer between the main test and the listening task.
59-88: LGTM!The event handlers demonstrate the new context-based API (
ctx.store()) and proper multi-contract event capture. TheArc<Mutex<Vec<_>>>pattern is appropriate for test collection, and the.unwrap()on the mutex lock is acceptable in test code since a poisoned mutex would indicate a prior test failure.
90-184: LGTM!The test comprehensively validates multi-contract event handling:
- Emits events from both contracts in the expected order
- Properly releases the mutex guard before proceeding (line 160)
- Verifies both indexer state (
get_e3,get_store) and handler-captured messagesBased on learnings, the 10ms delay should be sufficient for CI environments.
187-224: LGTM!The memory leak detection approach is now properly implemented:
LeakDetectorwraps anArc<DropCounter>, so all clones share the same counterCREATE_COUNTtracks how many uniqueDropCounterinstances were createdDROP_COUNTis only incremented when the lastArcreference to aDropCounteris dropped- The
create_indexer()helper properly spins up a real indexer for realistic testingThis addresses the previous review's concerns about incomplete infrastructure.
226-271: LGTM!The memory leak test is well-structured:
- Scoped block ensures indexer/handler go out of scope
- 100ms delay gives async cleanup time to complete
- Assertion
created == droppedconfirms no lingering referencesOne minor observation: the static
DROP_COUNT/CREATE_COUNTare reset at the start of the test (lines 234-235), which handles test isolation. If additional memory leak tests are added to this module in the future, ensure they also reset counters or consider using thread-local storage.crates/evm-helpers/tests/integration.rs (1)
12-15: Arc/time imports are correct and fully usedThe added
Arcand time-related imports align with the new async listener and timestamp helper usage; no issues here.
|
Ok this is ready I believe if folks want to review this. I will re-do #1042 to be based off this. |
cedoor
left a comment
cedoor
left a comment
There was a problem hiding this comment.
utACK, approved with some comments
3 participants
Problem:
In #1020 we need to be able to add a timeout during an event handler. In order to do this we must provide an API that has access to the indexer context in order to further add listeners.
Secondly we are making multiple listeners for single contracts instead of making single listeners for multiple contracts
ctxto event handlers in order to be able to access the ability to add event handlers during an event handler.Summary by CodeRabbit
New Features
Breaking Changes
Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.