feat!: sync mode and EventSystem [skip-line-limit]#1086
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduce an EventSystem abstraction (in‑memory or persisted), rewire builder/startup to use it, add event-store/index traits and actors, move KV messages to an event-driven WriteBuffer, add sled/commitlog backends, and migrate tests/helpers to EventSystem wiring. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant CB as CiphernodeBuilder
participant ES as EventSystem
participant Bus as EventBus
participant Seq as Sequencer
participant EStore as EventStore
participant WB as WriteBuffer
participant Store as DataStore
App->>CB: CiphernodeBuilder::new(name, rng, cipher)
CB->>ES: select backend (InMem | Persisted)
CB->>ES: with_fresh_bus / with_event_bus
CB->>App: build() -> node { bus, store, ... }
App->>Bus: publish EnclaveEvent(Unsequenced)
Bus->>Seq: deliver Unsequenced event
Seq->>EStore: StoreEventRequested(event, reply_to=ctx.address())
EStore->>EStore: append -> seq
EStore->>Seq: EventStored(sequenced_event)
Seq->>WB: CommitSnapshot(seq)
WB->>Store: InsertBatch (batched KV writes)
Seq->>Bus: publish EnclaveEvent(Sequenced)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧠 Learnings (18)📓 Common learnings📚 Learning: 2024-10-08T19:45:18.209ZApplied to files:
📚 Learning: 2024-10-10T23:24:43.341ZApplied to files:
📚 Learning: 2025-04-30T06:25:14.721ZApplied to files:
📚 Learning: 2025-10-27T15:37:59.138ZApplied to files:
📚 Learning: 2024-10-22T03:39:29.448ZApplied to files:
📚 Learning: 2024-10-16T09:52:53.807ZApplied to files:
📚 Learning: 2024-10-28T12:04:26.578ZApplied to files:
📚 Learning: 2024-10-03T23:02:41.732ZApplied to files:
📚 Learning: 2024-11-25T09:56:11.195ZApplied to files:
📚 Learning: 2024-11-05T06:49:46.285ZApplied to files:
📚 Learning: 2024-11-05T06:51:46.701ZApplied to files:
📚 Learning: 2024-10-16T09:51:10.038ZApplied to files:
📚 Learning: 2025-12-17T05:58:25.984ZApplied to files:
📚 Learning: 2024-10-08T07:15:06.690ZApplied to files:
📚 Learning: 2024-11-05T06:48:58.177ZApplied to files:
📚 Learning: 2024-10-29T00:05:52.278ZApplied to files:
📚 Learning: 2024-10-23T02:03:02.008ZApplied to files:
🧬 Code graph analysis (3)crates/tests/tests/integration.rs (3)
crates/ciphernode-builder/src/ciphernode_builder.rs (3)
crates/net/src/document_publisher.rs (2)
⏰ 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). (8)
🔇 Additional comments (6)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Author
|
I have found this has a difficult construction dependency tree that needs to be calculated and constructed: Persisted: graph TD
subgraph data
SB[SnapshotBuffer]
DS[DataStore]
SS[SledStore]
SSI[SledSequenceIndex]
CLEL[CommitLogEventLog]
DB[SledFactory]
end
subgraph events
BH[BusHandle] --> EB[EventBus]
BH --> SEQ[Sequencer]
BH --> HLC[Hlc]
ES[EventStore]
end
subgraph config
P["./some/sled/path"]
CP["./some/commit/log"]
end
SEQ --> EB
SEQ --> ES
SEQ --> SB
DS --> SB
SB --> SS
ES --> SSI
ES --> CLEL
DS --> BH
SS --> DB
SSI --> DB
DB --> P
CLEL --> CP
In Mem graph TD
subgraph data
SB[SnapshotBuffer]
DS[DataStore]
SS[InMemStore]
SSI[InMemSequenceIndex]
CLEL[InMemEventLog]
end
subgraph events
BH[BusHandle] --> EB[EventBus]
BH --> SEQ[Sequencer]
BH --> HLC[Hlc]
ES[EventStore]
end
SEQ --> EB
SEQ --> ES
SEQ --> SB
DS --> SB
SB --> SS
ES --> SSI
ES --> CLEL
DS --> BH
The factories constructing this should be in |
ryardley
force-pushed
the
ry/1050-storage
branch
from
97c9f18 to
2ee0073
Compare
ryardley
force-pushed
the
ry/1050-storage
branch
from
2ee0073 to
892a42b
Compare
Contributor
Author
|
@coderabbitai generate docstrings |
Contributor
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
Contributor
|
Note Docstrings generation - SUCCESS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/ciphernode-builder/src/ciphernode_builder.rs (1)
335-365: Consider flattening nested conditionals for readability.The nested
if let ... else { if let ... else { ... } }pattern at lines 340-344 can be simplified to a more readableelse ifchain or match expression.let event_system = if let EventSystemType::Persisted { kv_path, log_path } = self.event_system.clone() { EventSystem::persisted(&addr, log_path, kv_path).with_event_bus(local_bus) + } else if let Some(ref store) = self.in_mem_store { + EventSystem::in_mem_from_store(&addr, store).with_event_bus(local_bus) } else { - if let Some(ref store) = self.in_mem_store { - EventSystem::in_mem_from_store(&addr, store).with_event_bus(local_bus) - } else { - EventSystem::in_mem(&addr).with_event_bus(local_bus) - } + EventSystem::in_mem(&addr).with_event_bus(local_bus) };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/ciphernode-builder/src/ciphernode_builder.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 1086
File: crates/events/src/eventstore.rs:21-29
Timestamp: 2025-12-17T05:58:15.450Z
Learning: In the EventStore implementation (crates/events/src/eventstore.rs), if log.append succeeds but index.insert fails in handle_store_event_requested, the event is persisted in the log but not indexed. This is intentional design: such events are ignored during sync operations, as the index is the source of truth for timestamp-based event retrieval.
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/evm/src/event_reader.rs:270-286
Timestamp: 2024-11-05T06:51:46.701Z
Learning: In `packages/ciphernode/evm/src/event_reader.rs`, the `state.ids` HashSet is intended to grow indefinitely to store all processed event IDs for deduplication purposes until an alternative like a bloom filter is implemented.
📚 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/ciphernode-builder/src/ciphernode_builder.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/ciphernode-builder/src/ciphernode_builder.rs
📚 Learning: 2024-11-05T06:51:46.701Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/evm/src/event_reader.rs:270-286
Timestamp: 2024-11-05T06:51:46.701Z
Learning: In `packages/ciphernode/evm/src/event_reader.rs`, the `state.ids` HashSet is intended to grow indefinitely to store all processed event IDs for deduplication purposes until an alternative like a bloom filter is implemented.
Applied to files:
crates/ciphernode-builder/src/ciphernode_builder.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/ciphernode-builder/src/ciphernode_builder.rs
📚 Learning: 2024-10-12T10:24:07.572Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/evm/src/ciphernode_registry_sol.rs:133-143
Timestamp: 2024-10-12T10:24:07.572Z
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/ciphernode-builder/src/ciphernode_builder.rs
📚 Learning: 2024-10-28T12:04:26.578Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave_node/src/ciphernode.rs:26-28
Timestamp: 2024-10-28T12:04:26.578Z
Learning: In the `setup_ciphernode` function in `packages/ciphernode/enclave_node/src/ciphernode.rs`, panicking on errors during setup is acceptable.
Applied to files:
crates/ciphernode-builder/src/ciphernode_builder.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/ciphernode-builder/src/ciphernode_builder.rs
📚 Learning: 2024-10-29T00:05:52.278Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/config/src/app_config.rs:133-144
Timestamp: 2024-10-29T00:05:52.278Z
Learning: In the `load_config` function in `packages/ciphernode/config/src/app_config.rs`, Figment ensures correct types for the final configuration from defaults, reducing the need for additional validation after loading.
Applied to files:
crates/ciphernode-builder/src/ciphernode_builder.rs
📚 Learning: 2024-10-23T02:03:02.008Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/encryption.rs:45-45
Timestamp: 2024-10-23T02:03:02.008Z
Learning: In the `packages/ciphernode/keyshare/src/encryption.rs` file, the environment variable `CIPHERNODE_SECRET` is used for the encryption password. A secure secret management solution is not currently available, but may be considered in future iterations.
Applied to files:
crates/ciphernode-builder/src/ciphernode_builder.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/ciphernode-builder/src/ciphernode_builder.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). (8)
- GitHub Check: build_sdk
- GitHub Check: build_crisp_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: rust_unit
- GitHub Check: test_net
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_integration
🔇 Additional comments (5)
crates/ciphernode-builder/src/ciphernode_builder.rs (5)
7-7: LGTM!Import changes correctly support the new EventSystem integration and persistence configuration.
Also applies to: 18-19, 35-39
42-46: LGTM!The
EventSystemTypeenum cleanly encapsulates the deferred configuration choice between persisted and in-memory event systems.
54-77: LGTM!Struct fields are well-organized. The addition of
name, optionalin_mem_store, andevent_systemconfiguration cleanly supports both in-memory and persisted modes.
99-129: LGTM!Constructor signature change is a breaking change (consistent with
feat!in PR title). Thenameparameter is well-documented and defaults sensibly to in-memory mode.
347-350: LGTM!The EventSystem integration is well-wired:
handle()andstore()provide the bus and store respectively, which are then used consistently throughout the build process for router construction and the finalCiphernodeHandle.Also applies to: 470-470, 518-524
hmzakhalid
reviewed
Dec 22, 2025
hmzakhalid
left a comment
hmzakhalid
left a comment
Collaborator
There was a problem hiding this comment.
Thank a lot for this Rudi, left a few comments.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/data/src/commit_log_event_log.rs (2)
20-23: Drive message size from config before production deployment.The 32MB
message_max_byteslimit is hardcoded and marked with a TODO. This should be configurable to handle different deployment scenarios and prevent issues with oversized events.
50-52: Log read errors for debugging.Read errors from
self.log.read()silently break the loop. Consider logging these errors to aid troubleshooting.🔎 Proposed fix
- let message_buf = match self.log.read(current_offset, ReadLimit::default()) { - Ok(msgs) => msgs, - Err(_) => break, - }; + let message_buf = match self.log.read(current_offset, ReadLimit::default()) { + Ok(msgs) => msgs, + Err(e) => { + error!("Error reading from log at offset {}: {:?}", current_offset, e); + break; + } + };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Cargo.tomlcrates/data/Cargo.tomlcrates/data/src/commit_log_event_log.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/data/Cargo.toml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 1086
File: crates/events/src/eventstore.rs:21-29
Timestamp: 2025-12-17T05:58:25.984Z
Learning: In the EventStore implementation (crates/events/src/eventstore.rs), if log.append succeeds but index.insert fails in handle_store_event_requested, the event is persisted in the log but not indexed. This is intentional design: such events are ignored during sync operations, as the index is the source of truth for timestamp-based event retrieval.
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/evm/src/event_reader.rs:270-286
Timestamp: 2024-11-05T06:51:46.701Z
Learning: In `packages/ciphernode/evm/src/event_reader.rs`, the `state.ids` HashSet is intended to grow indefinitely to store all processed event IDs for deduplication purposes until an alternative like a bloom filter is implemented.
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
Cargo.toml
📚 Learning: 2025-12-17T05:58:25.984Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 1086
File: crates/events/src/eventstore.rs:21-29
Timestamp: 2025-12-17T05:58:25.984Z
Learning: In the EventStore implementation (crates/events/src/eventstore.rs), if log.append succeeds but index.insert fails in handle_store_event_requested, the event is persisted in the log but not indexed. This is intentional design: such events are ignored during sync operations, as the index is the source of truth for timestamp-based event retrieval.
Applied to files:
crates/data/src/commit_log_event_log.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/data/src/commit_log_event_log.rs
📚 Learning: 2025-12-17T05:55:15.023Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 1086
File: crates/events/src/eventstore.rs:52-70
Timestamp: 2025-12-17T05:55:15.023Z
Learning: In the EventStore implementation (crates/events/src/eventstore.rs), error handling in handlers for StoreEventRequested and GetEventsAfter intentionally logs errors using tracing::error without sending error responses back to the sender. This is by design.
Applied to files:
crates/data/src/commit_log_event_log.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/data/src/commit_log_event_log.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). (8)
- GitHub Check: rust_integration
- GitHub Check: build_crisp_sdk
- GitHub Check: integration_prebuild
- GitHub Check: rust_unit
- GitHub Check: build_sdk
- GitHub Check: test_net
- GitHub Check: build_ciphernode_image
- GitHub Check: build_enclave_cli
🔇 Additional comments (6)
Cargo.toml (1)
118-118: No action required. Version 0.2.0 is the latest release of the commitlog crate and has no known security vulnerabilities.crates/data/src/commit_log_event_log.rs (5)
28-36: LGTM!The offset-to-sequence conversion (0-indexed to 1-indexed) is clearly documented and correctly implemented.
39-42: LGTM!The serialization and append logic is straightforward and correct.
49-72: Infinite loop bug is resolved.The previous concern about an infinite loop when deserialization fails has been addressed:
- Line 64 always advances
current_offsetregardless of deserialization success- Line 69 breaks the loop when no messages are read
The test at lines 130-143 validates this fix by ensuring corrupted events don't cause hangs.
57-63: Deserialization error handling is intentional.Corrupted events are logged and skipped rather than stopping iteration or returning errors. Based on learnings, this aligns with the EventStore design where the index is the source of truth and unindexed events are intentionally ignored during sync operations.
78-166: Excellent test coverage!The test suite comprehensively covers:
- Basic append/read operations
- Reading from specific offsets
- Handling of corrupted data without infinite loops
- Empty log scenarios
- Reading beyond available data
The corruption test (lines 130-143) specifically validates the infinite loop fix from the previous review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/events/src/eventstore.rs (1)
21-32: Consider adding explicit context for try_send failures.Line 30's
try_sendfailure propagates through?and logs at line 60, but the error context may not clearly indicate whether the failure was fromlog.append,index.insert, ortry_send. As discussed in past reviews, adding explicit logging or panic fortry_sendfailures would aid debugging when the sequencer channel is full or closed.🔎 Optional: Add explicit context for try_send failures
- sender.try_send(EventStored(event.into_sequenced(seq)))?; - Ok(()) + sender + .try_send(EventStored(event.into_sequenced(seq))) + .map_err(|e| anyhow::anyhow!("Failed to send EventStored to sequencer: {e}"))?; + Ok(())Based on past review discussions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/events/src/eventstore.rs
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 1086
File: crates/events/src/eventstore.rs:21-29
Timestamp: 2025-12-17T05:58:25.984Z
Learning: In the EventStore implementation (crates/events/src/eventstore.rs), if log.append succeeds but index.insert fails in handle_store_event_requested, the event is persisted in the log but not indexed. This is intentional design: such events are ignored during sync operations, as the index is the source of truth for timestamp-based event retrieval.
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/evm/src/event_reader.rs:270-286
Timestamp: 2024-11-05T06:51:46.701Z
Learning: In `packages/ciphernode/evm/src/event_reader.rs`, the `state.ids` HashSet is intended to grow indefinitely to store all processed event IDs for deduplication purposes until an alternative like a bloom filter is implemented.
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.
📚 Learning: 2025-12-17T05:58:25.984Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 1086
File: crates/events/src/eventstore.rs:21-29
Timestamp: 2025-12-17T05:58:25.984Z
Learning: In the EventStore implementation (crates/events/src/eventstore.rs), if log.append succeeds but index.insert fails in handle_store_event_requested, the event is persisted in the log but not indexed. This is intentional design: such events are ignored during sync operations, as the index is the source of truth for timestamp-based event retrieval.
Applied to files:
crates/events/src/eventstore.rs
📚 Learning: 2025-12-17T05:55:15.023Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 1086
File: crates/events/src/eventstore.rs:52-70
Timestamp: 2025-12-17T05:55:15.023Z
Learning: In the EventStore implementation (crates/events/src/eventstore.rs), error handling in handlers for StoreEventRequested and GetEventsAfter intentionally logs errors using tracing::error without sending error responses back to the sender. This is by design.
Applied to files:
crates/events/src/eventstore.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/events/src/eventstore.rs
📚 Learning: 2024-10-22T02:30:19.534Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/src/data_store.rs:0-0
Timestamp: 2024-10-22T02:30:19.534Z
Learning: In the `write` method of the `DataStore` struct in `data_store.rs`, errors during serialization are logged instead of returning a `Result`.
Applied to files:
crates/events/src/eventstore.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/events/src/eventstore.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/events/src/eventstore.rs
📚 Learning: 2024-10-28T12:00:09.010Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave/src/commands/wallet/set.rs:17-23
Timestamp: 2024-10-28T12:00:09.010Z
Learning: In the `enclave` package of the `ciphernode` project, prefer using `println!` over logging macros like `error!` from the `tracing` crate for error output in CLI commands.
Applied to files:
crates/events/src/eventstore.rs
🧬 Code graph analysis (1)
crates/events/src/eventstore.rs (3)
crates/events/src/events.rs (6)
events(71-73)seq(21-23)new(17-19)new(35-43)new(55-60)new(68-70)crates/utils/src/actix.rs (1)
bail(12-14)crates/events/src/bus_handle.rs (1)
ts(67-70)
⏰ 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). (9)
- GitHub Check: build_crisp_sdk
- GitHub Check: build_sdk
- GitHub Check: rust_unit
- GitHub Check: test_net
- GitHub Check: build_ciphernode_image
- GitHub Check: build_e3_support_dev
- GitHub Check: integration_prebuild
- GitHub Check: rust_integration
- GitHub Check: build_enclave_cli
🔇 Additional comments (2)
crates/events/src/eventstore.rs (2)
34-35: Verify intended behavior when seek returns None.When
index.seek(msg.ts)returnsNone, the code defaults to sequence1, effectively replaying all events from the beginning. Is this the intended behavior? For example, ifmsg.tsis older than all indexed events or if the index is empty, should the response include all events, or should it return an empty result or error?Based on unresolved question from past review.
55-73: LGTM: Error handling follows established pattern.The handlers correctly delegate to the public methods and log errors without sending responses to callers, which is consistent with the established design pattern for this codebase.
Based on learnings.
Contributor
Author
|
Fixed up the minor things with this PR. Will look at it again tomorrrow. |
hmzakhalid
previously approved these changes
Dec 25, 2025
hmzakhalid
approved these changes
Dec 28, 2025
29 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #1050
This sets up the
EventSystemwhich logs events so they can be fetched and synced.It stores them in a file:
./.enclave/data/logIt also creates a "sequence index" that is stored as a separate tree in:
./.enclave/data/dbThis is preparation for the last couple of PRs in this task:
The following shows the event sequence the
EventSystemmakes:This lists the things this PR does.
EventSystemto act as an idempotent injector / factory for the event system: sequencer, eventstore, bus handle, write buffer, datastore.EventBusFactorysingleton tociphernode-builderas it is is more appropriate and enables more ergonomic dependency resollution to separate factories from crates being createdlog_fileto app config default is./.enclave/data/lognamewhich should be unique.EventSystemtoCiphernodeBuilderSledDbholdsTreeinstead ofDbKeySeekerforSledDbseek_after(target:&[u8]) -> V.SequenceIndexresponds toSeekAfter(target: &[u8])for getting key close to but just after the target.SequenceIndexresponds toInsertthe same asSledStoreInMemSequenceIndexresponds to the same events asSledSequenceIndexin the same way thatInMemdoesSledStoreEventLogwrapscommitlogEventStoreholdsSequenceIndexEventStoreholdsEventLogDatastoresends toWriteBufferInMemandSledStoreboth supportInsertBatchevent that uses transactions in the case ofSledStoreCiphernodeSelectorto use persistable snapshot to hold its own E3Meta cache.sled_utilsfileNotice an issue when HLC is too old we should refuse as a security measure. This has NOT been completed as part of this as this can be done at the net level.
next phase will involve fetching events part of which has already been completed:
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.