feat: prefactor for sync mode tidy up event structure [skip-line-limit]#1056
Conversation
WalkthroughSplit EnclaveEvent into a timestamped Changes
Sequence Diagram(s)sequenceDiagram
participant Producer as Component
participant BusH as BusHandle<EnclaveEvent>
participant Manager as EventBus (Addr<EventBus>)
participant SubA as Subscriber A
participant SubB as Subscriber B
Note over Producer,Manager: Legacy flow
Producer->>Manager: do_send(EnclaveEvent::from(payload))
Manager-->>SubA: deliver EnclaveEvent
Manager-->>SubB: deliver EnclaveEvent
Note over Producer,BusH: New flow
Producer->>BusH: publish(EnclaveEventData::Variant)
BusH->>Manager: do_send(EnclaveEvent constructed with timestamp + payload)
Manager-->>SubA: deliver EnclaveEvent (subscribers call .get_data()/into_data())
Manager-->>SubB: deliver EnclaveEvent
Note over SubA,SubB: Subscribers register via BusHandle.subscribe / subscribe_all
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ryardley
changed the title
ryardley
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
crates/fhe/src/ext.rs (1)
12-15: Data‑oriented event match is correct; you can drop a redundant clone.Switching to
evt.get_data()andEnclaveEventData::E3Requestedkeeps behavior aligned with the oldEnclaveEvent::E3Requestedmatch. The only nit is the extra clone:let EnclaveEventData::E3Requested(data) = evt.get_data() else { return; }; let E3Requested { .. } = data.clone();
datais already owned at this point, so you can destructure it directly:- let EnclaveEventData::E3Requested(data) = evt.get_data() else { - return; - }; - - let E3Requested { + let EnclaveEventData::E3Requested(data) = evt.get_data() else { + return; + }; + + let E3Requested { params, seed, e3_id, .. - } = data.clone(); + } = data;This avoids an unnecessary clone of the E3Requested payload while keeping semantics identical.
Also applies to: 40-53
crates/request/src/meta.rs (1)
11-12: Meta extension’s event handling matches the new model; avoid the extra clone.Switching to
event.get_data()andEnclaveEventData::E3Requestedis consistent with the EnclaveEventData refactor and preserves the original behavior. You can, however, simplify the destructuring:let EnclaveEventData::E3Requested(data) = event.get_data() else { return; }; let E3Requested { .. } = data.clone();Since
datais already owned, you don’t need to clone it:- let EnclaveEventData::E3Requested(data) = event.get_data() else { - return; - }; - let E3Requested { + let EnclaveEventData::E3Requested(data) = event.get_data() else { + return; + }; + let E3Requested { threshold_m, threshold_n, seed, e3_id, params, esi_per_ct, error_size, .. - } = data.clone(); + } = data;This keeps the same semantics with a bit less allocation.
Also applies to: 36-49
crates/sortition/src/sortition.rs (1)
235-244: Unnecessary.clone()calls on already-owned data.Since
into_data()consumesmsgand the match arms destructure into owneddatavalues, the.clone()calls are redundant. The data is already owned and can be passed directly toctx.notify().Apply this diff to remove the unnecessary clones:
fn handle(&mut self, msg: EnclaveEvent, ctx: &mut Self::Context) -> Self::Result { match msg.into_data() { - EnclaveEventData::CiphernodeAdded(data) => ctx.notify(data.clone()), - EnclaveEventData::CiphernodeRemoved(data) => ctx.notify(data.clone()), - EnclaveEventData::TicketBalanceUpdated(data) => ctx.notify(data.clone()), - EnclaveEventData::OperatorActivationChanged(data) => ctx.notify(data.clone()), - EnclaveEventData::ConfigurationUpdated(data) => ctx.notify(data.clone()), - EnclaveEventData::CommitteePublished(data) => ctx.notify(data.clone()), - EnclaveEventData::PlaintextOutputPublished(data) => ctx.notify(data.clone()), - EnclaveEventData::CommitteeFinalized(data) => ctx.notify(data.clone()), + EnclaveEventData::CiphernodeAdded(data) => ctx.notify(data), + EnclaveEventData::CiphernodeRemoved(data) => ctx.notify(data), + EnclaveEventData::TicketBalanceUpdated(data) => ctx.notify(data), + EnclaveEventData::OperatorActivationChanged(data) => ctx.notify(data), + EnclaveEventData::ConfigurationUpdated(data) => ctx.notify(data), + EnclaveEventData::CommitteePublished(data) => ctx.notify(data), + EnclaveEventData::PlaintextOutputPublished(data) => ctx.notify(data), + EnclaveEventData::CommitteeFinalized(data) => ctx.notify(data), _ => (), } }crates/events/src/enclave_event/mod.rs (1)
248-257: Consider usinginto_data()to avoid the clone.The
value.payload.clone()creates an unnecessary copy. Since thevalueparameter is owned, you could useinto_data()and restructure to avoid cloning.Apply this diff to eliminate the clone:
impl TryFrom<EnclaveEvent> for EnclaveError { type Error = anyhow::Error; fn try_from(value: EnclaveEvent) -> Result<Self, Self::Error> { - if let EnclaveEventData::EnclaveError(data) = value.payload.clone() { + if let EnclaveEventData::EnclaveError(data) = value.payload { Ok(data) } else { - return Err(anyhow::anyhow!("Not an enclave error {:?}", value)); + Err(anyhow::anyhow!("Not an enclave error")) } } }Note: This removes the debug output of
valuein the error message, which may be acceptable since it's already evident from context that the conversion failed.crates/keyshare/src/ext.rs (1)
16-16: CiphernodeSelected data handling is correct; consider trimming extra clonesMatching on
EnclaveEventData::CiphernodeSelectedand wiring upKeyshare/ThresholdKeyshareviaset_event_recipientpreserves existing behavior.You can slightly reduce cloning by pulling fields directly instead of cloning the whole payload, e.g.:
let EnclaveEventData::CiphernodeSelected(data) = evt.get_data() else { return; }; let e3_id = data.e3_id.clone(); // and in ThresholdKeyshareExtension: let party_id = data.party_id;This is minor and purely optional.
Also applies to: 47-80, 146-188
crates/net/src/document_publisher.rs (1)
72-80: is_document_publisher_event now classifies via EnclaveEventData variantsUsing event.get_data() and matching on EnclaveEventData::PublishDocumentRequested / ::ThresholdShareCreated cleanly mirrors the earlier enum-variant check and keeps this helper tightly scoped to payload-carrying events.
You could also express this as
matches!(event.get_data(), EnclaveEventData::PublishDocumentRequested(_) | EnclaveEventData::ThresholdShareCreated(_))for slightly more compact code, if you prefer.crates/tests/tests/integration_legacy.rs (1)
238-240: Legacy integration tests now consume EnclaveEventData via into_data()/get_data as intendedThe
filter_map(|e| match e.into_data() { EnclaveEventData::PublicKeyAggregated(..) | ::PlaintextAggregated(..) => Some(..), _ => None })patterns, plus the KeyshareCreated extraction viaevt.get_data(), mirror the previous direct enum matches while working with the new wrapper type; the downstream comparisons to expected events and decoded outputs remain intact.If this EnclaveEventData filtering pattern appears in more tests, consider a small helper (e.g.
fn collect_data<T>(events: Vec<EnclaveEvent>, f: fn(EnclaveEventData) -> Option<T>)) to deduplicate the match boilerplate, but it’s fine to leave as-is here.Also applies to: 272-274, 398-400, 427-429
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
crates/aggregator/src/committee_finalizer.rs(2 hunks)crates/aggregator/src/ext.rs(4 hunks)crates/aggregator/src/plaintext_aggregator.rs(2 hunks)crates/aggregator/src/publickey_aggregator.rs(2 hunks)crates/aggregator/src/threshold_plaintext_aggregator.rs(2 hunks)crates/config/src/load_config.rs(1 hunks)crates/data/src/sled_store.rs(2 hunks)crates/events/Cargo.toml(1 hunks)crates/events/src/enclave_event/mod.rs(7 hunks)crates/evm-helpers/tests/helpers.rs(0 hunks)crates/evm-helpers/tests/integration.rs(0 hunks)crates/evm/src/ciphernode_registry_sol.rs(2 hunks)crates/evm/src/enclave_sol_writer.rs(2 hunks)crates/evm/src/event_reader.rs(2 hunks)crates/evm/tests/integration.rs(4 hunks)crates/fhe/src/ext.rs(2 hunks)crates/fhe/src/fhe.rs(1 hunks)crates/indexer/src/callback_queue.rs(4 hunks)crates/indexer/tests/helpers.rs(0 hunks)crates/keyshare/src/ext.rs(3 hunks)crates/keyshare/src/keyshare.rs(2 hunks)crates/keyshare/src/threshold_keyshare.rs(2 hunks)crates/logger/src/logger.rs(2 hunks)crates/net/src/bin/p2p_test.rs(0 hunks)crates/net/src/document_publisher.rs(6 hunks)crates/net/src/events.rs(0 hunks)crates/net/src/net_event_translator.rs(2 hunks)crates/program-server/src/types.rs(1 hunks)crates/request/src/meta.rs(2 hunks)crates/request/src/router.rs(4 hunks)crates/sortition/src/ciphernode_selector.rs(2 hunks)crates/sortition/src/sortition.rs(2 hunks)crates/test-helpers/src/ciphernode_system.rs(1 hunks)crates/test-helpers/src/plaintext_writer.rs(2 hunks)crates/test-helpers/src/public_key_writer.rs(2 hunks)crates/tests/tests/integration.rs(3 hunks)crates/tests/tests/integration_legacy.rs(6 hunks)crates/trbfv/src/calculate_threshold_decryption.rs(1 hunks)
💤 Files with no reviewable changes (5)
- crates/net/src/bin/p2p_test.rs
- crates/indexer/tests/helpers.rs
- crates/evm-helpers/tests/integration.rs
- crates/net/src/events.rs
- crates/evm-helpers/tests/helpers.rs
🧰 Additional context used
🧠 Learnings (30)
📓 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-10-28T10:45:29.100Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/cipher/src/cipher.rs:164-164
Timestamp: 2024-10-28T10:45:29.100Z
Learning: In test code, it's acceptable to use `.unwrap()` instead of propagating errors with the `?` operator in Rust.
Applied to files:
crates/indexer/src/callback_queue.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/keyshare/src/ext.rscrates/evm/src/enclave_sol_writer.rscrates/net/src/net_event_translator.rscrates/evm/tests/integration.rscrates/logger/src/logger.rscrates/request/src/router.rscrates/fhe/src/ext.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/test-helpers/src/ciphernode_system.rscrates/keyshare/src/keyshare.rscrates/sortition/src/sortition.rscrates/evm/src/event_reader.rscrates/net/src/document_publisher.rscrates/test-helpers/src/plaintext_writer.rscrates/request/src/meta.rscrates/sortition/src/ciphernode_selector.rscrates/aggregator/src/committee_finalizer.rscrates/keyshare/src/threshold_keyshare.rscrates/aggregator/src/plaintext_aggregator.rscrates/aggregator/src/ext.rscrates/data/src/sled_store.rscrates/tests/tests/integration.rscrates/evm/src/ciphernode_registry_sol.rscrates/aggregator/src/publickey_aggregator.rscrates/tests/tests/integration_legacy.rscrates/events/src/enclave_event/mod.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/keyshare/src/ext.rscrates/evm/src/enclave_sol_writer.rscrates/net/src/net_event_translator.rscrates/evm/tests/integration.rscrates/logger/src/logger.rscrates/request/src/router.rscrates/fhe/src/ext.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/test-helpers/src/ciphernode_system.rscrates/keyshare/src/keyshare.rscrates/trbfv/src/calculate_threshold_decryption.rscrates/config/src/load_config.rscrates/sortition/src/sortition.rscrates/evm/src/event_reader.rscrates/net/src/document_publisher.rscrates/test-helpers/src/plaintext_writer.rscrates/request/src/meta.rscrates/sortition/src/ciphernode_selector.rscrates/fhe/src/fhe.rscrates/aggregator/src/committee_finalizer.rscrates/keyshare/src/threshold_keyshare.rscrates/aggregator/src/plaintext_aggregator.rscrates/events/Cargo.tomlcrates/aggregator/src/ext.rscrates/data/src/sled_store.rscrates/test-helpers/src/public_key_writer.rscrates/tests/tests/integration.rscrates/evm/src/ciphernode_registry_sol.rscrates/aggregator/src/publickey_aggregator.rscrates/tests/tests/integration_legacy.rscrates/events/src/enclave_event/mod.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/keyshare/src/ext.rscrates/evm/src/enclave_sol_writer.rscrates/net/src/net_event_translator.rscrates/evm/tests/integration.rscrates/request/src/router.rscrates/fhe/src/ext.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/keyshare/src/keyshare.rscrates/sortition/src/sortition.rscrates/evm/src/event_reader.rscrates/net/src/document_publisher.rscrates/test-helpers/src/plaintext_writer.rscrates/request/src/meta.rscrates/sortition/src/ciphernode_selector.rscrates/aggregator/src/committee_finalizer.rscrates/keyshare/src/threshold_keyshare.rscrates/aggregator/src/plaintext_aggregator.rscrates/aggregator/src/ext.rscrates/data/src/sled_store.rscrates/test-helpers/src/public_key_writer.rscrates/tests/tests/integration.rscrates/evm/src/ciphernode_registry_sol.rscrates/aggregator/src/publickey_aggregator.rscrates/tests/tests/integration_legacy.rs
📚 Learning: 2024-10-23T02:35:07.689Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:58-65
Timestamp: 2024-10-23T02:35:07.689Z
Learning: In `packages/ciphernode/keyshare/src/keyshare.rs`, the data being decrypted in the `get_secret` method of the `Keyshare` struct is not sensitive, so wrapping it with `Zeroizing` is unnecessary.
Applied to files:
crates/keyshare/src/ext.rscrates/keyshare/src/keyshare.rscrates/trbfv/src/calculate_threshold_decryption.rscrates/aggregator/src/publickey_aggregator.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/keyshare/src/ext.rscrates/evm/src/enclave_sol_writer.rscrates/evm/tests/integration.rscrates/logger/src/logger.rscrates/request/src/router.rscrates/fhe/src/ext.rscrates/keyshare/src/keyshare.rscrates/net/src/document_publisher.rscrates/test-helpers/src/plaintext_writer.rscrates/sortition/src/ciphernode_selector.rscrates/aggregator/src/plaintext_aggregator.rscrates/aggregator/src/ext.rscrates/data/src/sled_store.rscrates/test-helpers/src/public_key_writer.rscrates/aggregator/src/publickey_aggregator.rscrates/tests/tests/integration_legacy.rs
📚 Learning: 2024-10-22T03:17:41.617Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/hooks.rs:0-0
Timestamp: 2024-10-22T03:17:41.617Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, in the `hydrate` methods of the `E3Feature` implementations, it's acceptable to return `Ok(())` when dependencies are missing, as the error is reported to the bus.
Applied to files:
crates/keyshare/src/ext.rscrates/request/src/router.rscrates/fhe/src/ext.rscrates/test-helpers/src/ciphernode_system.rscrates/keyshare/src/keyshare.rscrates/aggregator/src/ext.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/keyshare/src/ext.rscrates/evm/src/enclave_sol_writer.rscrates/net/src/net_event_translator.rscrates/logger/src/logger.rscrates/fhe/src/ext.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/keyshare/src/keyshare.rscrates/sortition/src/sortition.rscrates/net/src/document_publisher.rscrates/sortition/src/ciphernode_selector.rscrates/aggregator/src/committee_finalizer.rscrates/keyshare/src/threshold_keyshare.rscrates/aggregator/src/plaintext_aggregator.rscrates/aggregator/src/ext.rscrates/data/src/sled_store.rscrates/tests/tests/integration.rscrates/evm/src/ciphernode_registry_sol.rscrates/aggregator/src/publickey_aggregator.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/keyshare/src/ext.rscrates/tests/tests/integration.rscrates/tests/tests/integration_legacy.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/evm/src/enclave_sol_writer.rscrates/net/src/net_event_translator.rscrates/logger/src/logger.rscrates/keyshare/src/threshold_keyshare.rscrates/tests/tests/integration.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/net/src/net_event_translator.rscrates/request/src/router.rscrates/request/src/meta.rscrates/sortition/src/ciphernode_selector.rscrates/aggregator/src/committee_finalizer.rscrates/keyshare/src/threshold_keyshare.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/net/src/net_event_translator.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/sortition/src/sortition.rscrates/sortition/src/ciphernode_selector.rscrates/aggregator/src/committee_finalizer.rscrates/keyshare/src/threshold_keyshare.rscrates/aggregator/src/plaintext_aggregator.rscrates/aggregator/src/publickey_aggregator.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/evm/tests/integration.rscrates/test-helpers/src/ciphernode_system.rscrates/evm/src/event_reader.rscrates/tests/tests/integration.rscrates/tests/tests/integration_legacy.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/logger/src/logger.rscrates/data/src/sled_store.rs
📚 Learning: 2024-10-22T03:47:04.014Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/e3_request_router.rs:202-235
Timestamp: 2024-10-22T03:47:04.014Z
Learning: In `packages/ciphernode/router/src/e3_request_router.rs`, within the `E3RequestRouter::from_snapshot` method, errors during context restoration propagate upwards due to the `?` operator, and skipping contexts when `repositories.context(&e3_id).read().await?` returns `Ok(None)` is acceptable, as missing snapshots are expected.
Applied to files:
crates/fhe/src/ext.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/test-helpers/src/ciphernode_system.rscrates/trbfv/src/calculate_threshold_decryption.rscrates/test-helpers/src/plaintext_writer.rscrates/program-server/src/types.rscrates/fhe/src/fhe.rscrates/aggregator/src/plaintext_aggregator.rscrates/test-helpers/src/public_key_writer.rscrates/tests/tests/integration.rscrates/tests/tests/integration_legacy.rs
📚 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:
crates/test-helpers/src/ciphernode_system.rscrates/config/src/load_config.rs
📚 Learning: 2024-10-23T01:59:42.967Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:274-274
Timestamp: 2024-10-23T01:59:42.967Z
Learning: In the `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs` file and other test files within this project, hardcoding `CIPHERNODE_SECRET` is acceptable for testing purposes.
Applied to files:
crates/test-helpers/src/ciphernode_system.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/test-helpers/src/ciphernode_system.rscrates/sortition/src/sortition.rscrates/tests/tests/integration_legacy.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/test-helpers/src/ciphernode_system.rscrates/evm/src/ciphernode_registry_sol.rs
📚 Learning: 2024-10-22T03:30:21.818Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/enclave_node/src/shutdown.rs:12-12
Timestamp: 2024-10-22T03:30:21.818Z
Learning: In shutdown code (e.g., in `packages/ciphernode/enclave_node/src/shutdown.rs`), if signal creation fails, it's acceptable to panic rather than handle the error gracefully.
Applied to files:
crates/test-helpers/src/ciphernode_system.rscrates/evm/src/event_reader.rs
📚 Learning: 2025-11-08T01:31:47.505Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 969
File: packages/enclave-sdk/src/enclave-sdk.ts:101-103
Timestamp: 2025-11-08T01:31:47.505Z
Learning: In packages/enclave-sdk/src/enclave-sdk.ts, the TRBFV protocol parameter setup is intentionally incomplete as of PR #969. The constructor sets BFV_THRESHOLD parameters for TRBFV, but the encryption methods (encryptNumber, encryptVector, encryptNumberAndGenInputs, encryptVectorAndGenInputs) intentionally only support BFV and will throw "Protocol not supported" for TRBFV until future implementation is completed.
Applied to files:
crates/trbfv/src/calculate_threshold_decryption.rs
📚 Learning: 2025-10-04T14:00:17.916Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 660
File: crates/events/src/enclave_event/decryptionshare_created.rs:23-26
Timestamp: 2025-10-04T14:00:17.916Z
Learning: In the enclave codebase using threshold BFV cryptography, decryption shares (as in DecryptionshareCreated events) are not sensitive data and can be safely logged or displayed, as individual shares reveal nothing without reaching the threshold number of shares.
Applied to files:
crates/trbfv/src/calculate_threshold_decryption.rscrates/keyshare/src/threshold_keyshare.rscrates/aggregator/src/plaintext_aggregator.rs
📚 Learning: 2024-10-28T10:59:42.202Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/config/src/app_config.rs:146-175
Timestamp: 2024-10-28T10:59:42.202Z
Learning: In `packages/ciphernode/config/src/app_config.rs`, the `normalize_path` function uses custom path normalization instead of `std::fs::canonicalize` because using `canonicalize` requires actual files to exist, making testing more difficult.
Applied to files:
crates/config/src/load_config.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/net/src/document_publisher.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
crates/fhe/src/fhe.rscrates/tests/tests/integration_legacy.rs
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
crates/events/Cargo.toml
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
crates/tests/tests/integration_legacy.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: Adding Display as a requirement to the Event trait in packages/ciphernode/events/src/eventbus.rs doesn't break compilation because all implementations of Event already satisfy the Display trait requirement.
Applied to files:
crates/events/src/enclave_event/mod.rs
🧬 Code graph analysis (8)
crates/logger/src/logger.rs (1)
crates/events/src/eventbus.rs (1)
error(92-96)
crates/aggregator/src/threshold_plaintext_aggregator.rs (1)
packages/enclave-sdk/src/types.ts (2)
EnclaveEvent(226-233)EnclaveEventData(192-211)
crates/test-helpers/src/ciphernode_system.rs (1)
crates/test-helpers/src/lib.rs (1)
simulate_libp2p_net(126-148)
crates/test-helpers/src/plaintext_writer.rs (1)
packages/enclave-sdk/src/types.ts (2)
EnclaveEvent(226-233)EnclaveEventData(192-211)
crates/keyshare/src/threshold_keyshare.rs (1)
packages/enclave-sdk/src/types.ts (2)
EnclaveEvent(226-233)EnclaveEventData(192-211)
crates/tests/tests/integration.rs (1)
packages/enclave-sdk/src/types.ts (2)
EnclaveEvent(226-233)EnclaveEventData(192-211)
crates/tests/tests/integration_legacy.rs (1)
packages/enclave-sdk/src/types.ts (1)
EnclaveEventData(192-211)
crates/events/src/enclave_event/mod.rs (2)
crates/events/src/event_id.rs (1)
hash(20-27)crates/keyshare/src/threshold_keyshare.rs (1)
get_e3_id(202-204)
🔇 Additional comments (43)
crates/indexer/src/callback_queue.rs (1)
102-102: Remove unnecessarymutbindings in test queue instances.Since
CallbackQueue::push()andexecute_until_including()both take&self(not&mut self), the queue bindings don't need to be mutable. This change improves code clarity by removing misleading mutability annotations.Also applies to: 120-120, 138-138, 174-174
crates/config/src/load_config.rs (1)
64-64: LGTM! Clean removal of unused import.The removal of the unused
Pathimport is correct, as onlyPathBufis used throughout the test module.crates/events/Cargo.toml (1)
24-24: Workspace dependency addition forstrumlooks consistent.Adding
strumas a workspace dependency matches the new event data model usage and is aligned with existing dependency style; no issues spotted.crates/fhe/src/fhe.rs (1)
18-21: FHE import narrowing is safe and reduces noise.Dropping unused
Encoding/FheDecoderwhile keeping the actually used BFV types and traits is a straightforward cleanup with no behavioral impact.crates/trbfv/src/calculate_threshold_decryption.rs (1)
14-16: ExplicitPlaintextimport matches usage.Bringing
fhe::bfv::Plaintextin via a dedicated use aligns with how it’s used inInnerResponseand keeps the import list minimal.crates/program-server/src/types.rs (1)
179-185: Large-payload test keeps intent while avoiding warnings.Renaming the binding to
_payloadkeeps the “just don’t crash” assertion while eliminating unused-variable noise; the test behavior is unchanged.crates/evm/src/event_reader.rs (2)
17-20: Event/EnclaveEventData imports correctly support the new model.Importing
EventplusEnclaveEventDataensuresevent_type()and data‑oriented matching both compile cleanly and stays consistent with the broader EnclaveEventData refactor.
293-301: Shutdown handling correctly migrated to EnclaveEventData.Using
msg.into_data()and matchingEnclaveEventData::Shutdown(_)preserves the old behavior of reacting only to shutdown events while keeping the oneshot shutdown channel single‑use.crates/evm/tests/integration.rs (1)
19-22: Integration tests cleanly migrated toEnclaveEventDataviainto_data().Updating the imports and history processing to use
evt.into_data()withEnclaveEventData::TestEvent(data)keeps the assertions intact while exercising the new data‑centric event API across all the EVM tests.Also applies to: 58-61, 123-128, 202-207
crates/logger/src/logger.rs (1)
51-52: LGTM!The use of
get_data()is appropriate here since thelogmethod takes&self, and the pattern matching onEnclaveEventData::EnclaveErroraligns with the new data-centric event handling approach across the codebase.crates/evm/src/ciphernode_registry_sol.rs (1)
315-336: LGTM!The refactored event handling correctly uses
into_data()and maintains the chain ID filtering logic for cross-chain safety. TheShutdownevent appropriately bypasses the chain check since it's a global signal.crates/events/src/enclave_event/mod.rs (2)
89-118: Well-structured data-centric event payload enum.The introduction of
EnclaveEventDatawithIntoStaticStrprovides efficient event type resolution and cleanly separates event metadata from payload data. The enum correctly covers all event variants.
196-201: Clean accessor API for payload extraction.The
get_data()(borrow) andinto_data()(consume) accessors provide appropriate flexibility for different usage patterns across the codebase.crates/evm/src/enclave_sol_writer.rs (1)
77-86: LGTM!The refactored event handling correctly uses
into_data()and maintains the chain ID filtering forPlaintextAggregatedwhile allowingShutdownto pass through unconditionally.crates/request/src/router.rs (1)
24-24: EnclaveEventData-based routing inE3Routerlooks consistentShutdown short-circuit via
msg.get_data()andctx.notify(data.clone())correctly reuses the existingHandler<Shutdown>path, and the latermatch msg.into_data()forPlaintextAggregated/E3RequestCompletepreserves the previous lifecycle semantics (emit completion, then tear down context/mark completed). No issues spotted with ownership or ordering.Also applies to: 151-205
crates/sortition/src/ciphernode_selector.rs (1)
14-16: Data-oriented dispatch forCiphernodeSelectoris correctMatching on
msg.into_data()and re‑emittingE3Requested,CommitteeFinalized, andShutdownviactx.notifycleanly forwards to the existing typed handlers without changing behavior.Also applies to: 68-72
crates/test-helpers/src/ciphernode_system.rs (1)
7-7: Import tidy-up and simulate hook are soundBringing
simulate_libp2p_netandtimeoutinto scope matches their existing usage inbuild()and history helpers; no functional change introduced.Also applies to: 10-11, 13-13
crates/test-helpers/src/public_key_writer.rs (1)
11-11: PublicKeyAggregated data extraction viainto_data()is fineSwitching to
EnclaveEventData::PublicKeyAggregated(data)onmsg.into_data()maintains the previous behavior of writing the aggregated public key to disk, with correct ownership and no extra cloning.Also applies to: 36-42
crates/net/src/net_event_translator.rs (1)
18-20: Forwardable-event check viaEnclaveEventDatapreserves behaviorUsing
event.get_data()and matching onEnclaveEventData::{DecryptionshareCreated,KeyshareCreated,PlaintextAggregated,PublicKeyAggregated}keeps the same forwarding set as before while aligning with the new data-centric event model.Also applies to: 99-111
crates/aggregator/src/committee_finalizer.rs (1)
9-11: CommitteeRequested/Shutdown dispatch viaEnclaveEventDatais correctMatching on
msg.into_data()and re‑notifyingCommitteeRequestedandShutdownpreserves the existing scheduling and shutdown behavior while conforming to the new EnclaveEventData payload model.Also applies to: 48-56
crates/keyshare/src/threshold_keyshare.rs (2)
11-14: LGTM!Import changes correctly add
EnclaveEventDatato support the new data-oriented event handling pattern.
754-766: LGTM!The event handler correctly uses
into_data()to extract the payload and matches onEnclaveEventDatavariants. The inline handling ofThresholdShareCreated(rather than usingctx.notify()) is appropriate since it requires bothselfandctx.address()in the same call.crates/test-helpers/src/plaintext_writer.rs (2)
11-11: LGTM!Import correctly updated to include
EnclaveEventData.
37-53: LGTM!The
if letpattern withinto_data()is the correct approach for single-variant matching, consuming the owned event and extracting thePlaintextAggregateddata.crates/aggregator/src/threshold_plaintext_aggregator.rs (2)
12-15: LGTM!Import changes correctly add
EnclaveEventDataalongside the existing event types.
235-244: LGTM!The handler correctly extracts data via
into_data()and routes:
DecryptionshareCreated→ forwards to the dedicated handler viactx.notify()E3RequestComplete→ signals actor termination viaDiecrates/aggregator/src/plaintext_aggregator.rs (2)
10-13: LGTM!Import changes correctly add
EnclaveEventDatato support the new event handling pattern.
145-154: LGTM!The handler correctly uses
into_data()and matches onEnclaveEventDatavariants. The pattern is consistent with the non-deprecatedThresholdPlaintextAggregator.crates/aggregator/src/ext.rs (4)
18-18: LGTM!Import correctly adds
EnclaveEventDatafor the data-oriented event handling.
44-48: LGTM!Correctly uses
get_data()instead ofinto_data()sinceevtis a borrowed reference (&EnclaveEvent). The early-return pattern is appropriate for filtering to the relevant event variant.
164-168: LGTM!Consistent use of
get_data()for the borrowed event reference, matching the pattern established inPlaintextAggregatorExtension.
275-279: LGTM!Consistent application of the
get_data()pattern forThresholdPlaintextAggregatorExtension, maintaining uniformity across all three extension implementations.crates/aggregator/src/publickey_aggregator.rs (2)
10-13: LGTM!Import changes correctly add
EnclaveEventDatato support the refactored event handling.
137-146: LGTM!The handler correctly uses
into_data()to consume the owned event and routes:
KeyshareCreated→ forwards to the dedicated handler viactx.notify()E3RequestComplete→ signals actor termination viaDieThis pattern is consistent with the other aggregator implementations.
crates/keyshare/src/keyshare.rs (2)
13-15: EnclaveEventData/E3RequestComplete imports correctly support new event payload modelBringing EnclaveEventData and E3RequestComplete into scope matches later usage and keeps Keyshare aligned with the refactored events crate; nothing to change here.
80-85: into_data()-based EnclaveEvent handler preserves original routingMatching on EnclaveEventData and forwarding CiphernodeSelected, CiphertextOutputPublished, E3RequestComplete, and Shutdown (via Die) maintains the previous behaviour while fitting the new wrapper type; this looks correct and non-breaking.
crates/net/src/document_publisher.rs (4)
16-20: Updated imports for EnclaveEventData/E3RequestComplete are consistent with usagesPulling EnclaveEventData and E3RequestComplete into scope matches their later use in handlers and classifiers; no issues here.
133-141: DocumentPublisher’s EnclaveEvent handler delegates correctly via into_data()The into_data() match and ctx.notify(..) calls for PublishDocumentRequested, CiphernodeSelected, and E3RequestComplete are consistent with the actor’s dedicated handlers and keep the routing logic straightforward.
452-459: EventConverter’s EnclaveEvent handler cleanly forwards ThresholdShareCreated/DocumentReceivedRouting EnclaveEvent through into_data() and only notifying on ThresholdShareCreated and DocumentReceived maintains the converter’s focused role without altering behaviour.
791-793: test_notified_of_document correctly asserts on EnclaveEventData::DocumentReceivedSwitching the assertion to
events.last().map(|e| e.get_data())and matching on EnclaveEventData::DocumentReceived preserves the original intent while aligning with the new payload API; the extracted value is used safely.crates/tests/tests/integration.rs (2)
13-16: EnclaveEventData/PlaintextAggregated imports line up with downstream test usageAdding EnclaveEventData and PlaintextAggregated to the imports matches how the tests now destructure payloads; nothing extraneous or missing.
323-328: History-based assertions updated to use EnclaveEventData payloads correctlyUsing
h.last().map(|e| e.get_data())to match EnclaveEventData::PublicKeyAggregated and EnclaveEventData::PlaintextAggregated { decrypted_output, .. } keeps the tests verifying the same final events while conforming to the new EnclaveEvent representation; the destructuring and subsequent usage of pubkey/decrypted_output look sound.Also applies to: 392-396
crates/tests/tests/integration_legacy.rs (1)
16-16: Additional EnclaveEventData/Serialize imports are minimal and justifiedImporting EnclaveEventData for pattern matching and Serialize for
to_bytes()use aligns with the updated event and crypto utilities; both are used and non-redundant.Also applies to: 40-40
|
Thought of some more stuff that aught to go in this PR. Gonna make it draft. |
Pull request was converted to draft
|
Investigating test failures...sofar:
|
|
Looks like it is an issue around the timing of the committee finalization and when the keyshares are received by the ciphernodes. I can confirm it receives It is possible I made some events go faster so I will try and fix here so that it does not matter when events come in and then request a new review It is actually good this has come up as it allows us to be more defensive around our message handling |
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
Refactored to this sort of model removing all the sync calls to sortition. (We should avoid sync calls if possible) There is more to do here but we can look at it when we refresh the sortition architecture. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/ciphernode-builder/src/ciphernode_builder.rs (1)
437-481: E3Router, extensions, andCiphernodeHandlecorrectly migrated to BusHandleUsing
E3Router::builder(&bus, ...)and wiring all E3 extensions with&busmatches the new signatures, and passingbusintoCiphernodeHandle::newcleanly exposes the handle at the top level. If you want to reduce ambiguity, you could rename the localbusvariable tobus_handle, but that’s purely cosmetic.Also applies to: 485-489
crates/aggregator/src/publickey_aggregator.rs (1)
20-48: State expansion withnodesis sound; consider serde defaults for backward compatibilityAdding
nodes: OrderedSet<String>to allPublicKeyAggregatorStatevariants and initializing it ininitmakes it straightforward to include node membership in the finalPublicKeyAggregatedevent and keeps state shape consistent.If there are already persisted
PublicKeyAggregatorStatesnapshots (viaPersistable/serde), deserializing old records that lack thenodesfield will fail unless you provide a default. If you expect upgrades over existing data, it’s worth adding an explicit default:#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] pub enum PublicKeyAggregatorState { Collecting { threshold_n: usize, keyshares: OrderedSet<ArcBytes>, seed: Seed, + #[serde(default)] nodes: OrderedSet<String>, }, Computing { keyshares: OrderedSet<ArcBytes>, + #[serde(default)] nodes: OrderedSet<String>, }, Complete { public_key: Vec<u8>, keyshares: OrderedSet<ArcBytes>, + #[serde(default)] nodes: OrderedSet<String>, }, }If this state is only ever created fresh in the current version (e.g., tests or ephemeral environments), you may not need this, but it’s a low-cost guard.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/aggregator/src/ext.rs(8 hunks)crates/aggregator/src/keyshare_created_filter_buffer.rs(1 hunks)crates/aggregator/src/lib.rs(1 hunks)crates/aggregator/src/publickey_aggregator.rs(6 hunks)crates/ciphernode-builder/src/ciphernode_builder.rs(10 hunks)crates/events/src/ordered_set.rs(1 hunks)crates/sortition/Cargo.toml(1 hunks)crates/utils/src/utility_types.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 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.
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.
📚 Learning: 2024-10-29T01:04:19.137Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:53-70
Timestamp: 2024-10-29T01:04:19.137Z
Learning: In the `Keyshare` struct within `packages/ciphernode/keyshare/src/keyshare.rs`, the `self.secret` field is stored in encrypted form and is not considered sensitive.
Applied to files:
crates/aggregator/src/lib.rscrates/aggregator/src/publickey_aggregator.rs
📚 Learning: 2024-10-08T01:48:49.778Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-08T01:48:49.778Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
Applied to files:
crates/aggregator/src/lib.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/aggregator/src/ext.rscrates/aggregator/src/keyshare_created_filter_buffer.rscrates/aggregator/src/publickey_aggregator.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/aggregator/src/lib.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/aggregator/src/ext.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/aggregator/src/lib.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
crates/utils/src/utility_types.rscrates/aggregator/src/publickey_aggregator.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/ciphernode-builder/src/ciphernode_builder.rscrates/aggregator/src/publickey_aggregator.rs
📚 Learning: 2024-11-05T06:56:49.157Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/aggregator.rs:0-0
Timestamp: 2024-11-05T06:56:49.157Z
Learning: `RegistryFilterSol` does not have a reader and does not require a repository reader or deploy block when calling `RegistryFilterSol::attach` in `packages/ciphernode/enclave_node/src/aggregator.rs`.
Applied to files:
crates/ciphernode-builder/src/ciphernode_builder.rs
📚 Learning: 2024-10-22T03:17:41.617Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/hooks.rs:0-0
Timestamp: 2024-10-22T03:17:41.617Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, in the `hydrate` methods of the `E3Feature` implementations, it's acceptable to return `Ok(())` when dependencies are missing, as the error is reported to the bus.
Applied to files:
crates/ciphernode-builder/src/ciphernode_builder.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/aggregator/src/ext.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/aggregator/src/ext.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/aggregator/src/keyshare_created_filter_buffer.rscrates/aggregator/src/publickey_aggregator.rs
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
crates/sortition/Cargo.toml
📚 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:
crates/sortition/Cargo.toml
🧬 Code graph analysis (3)
crates/ciphernode-builder/src/ciphernode_builder.rs (7)
crates/ciphernode-builder/src/ciphernode.rs (2)
bus(37-39)store(53-55)crates/evm/src/enclave_sol.rs (1)
attach(20-48)crates/evm/src/enclave_sol_writer.rs (1)
attach(54-62)crates/sortition/src/sortition.rs (2)
attach(171-212)new(161-168)crates/request/src/router.rs (1)
builder(120-130)crates/aggregator/src/ext.rs (3)
create(36-41)create(155-157)create(256-266)crates/keyshare/src/ext.rs (2)
create(29-35)create(127-139)
crates/aggregator/src/keyshare_created_filter_buffer.rs (1)
crates/aggregator/src/publickey_aggregator.rs (4)
handle(141-147)handle(153-173)handle(179-204)handle(209-211)
crates/aggregator/src/publickey_aggregator.rs (2)
crates/events/src/ordered_set.rs (2)
new(16-18)len(28-30)crates/keyshare/src/keyshare.rs (1)
new(41-49)
⏰ 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: template_integration
- GitHub Check: crisp_e2e
- GitHub Check: test_enclave_init
- GitHub Check: rust_unit
- GitHub Check: test_net
- GitHub Check: rust_integration
- GitHub Check: integration_prebuild
- GitHub Check: Build & Push Image
🔇 Additional comments (12)
crates/events/src/ordered_set.rs (1)
12-12: DerivingDefaultis consistent with existing semantics
Defaultwill produce an emptyBTreeSet, matchingOrderedSet::new(), so this is a safe, non-breaking enhancement that makes the type easier to use in generic contexts and struct defaults. No further changes needed here.crates/utils/src/utility_types.rs (1)
21-21: LGTM! Useful addition of Default.Adding
DefaulttoArcBytesis a sensible improvement that enables default initialization to an empty byte array, making the type more ergonomic in generic contexts.crates/sortition/Cargo.toml (1)
24-25: Remove the unusedtokiodependency from line 24.The sortition crate does not use
tokioanywhere in its source code. Thenum-bigintdependency on line 25 is necessary for the ticket scoring logic and should remain.⛔ Skipped due to learnings
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]`.Learnt from: ctrlc03 Repo: gnosisguild/enclave PR: 657 File: Cargo.toml:32-34 Timestamp: 2025-08-25T10:28:56.174Z Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.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.crates/ciphernode-builder/src/ciphernode_builder.rs (2)
301-303: BusHandle derivation fromlocal_busis clean and localizedDeriving
busonce vialocal_bus.into()and never usinglocal_busafterwards keeps Addr vs BusHandle concerns well-isolated while preserving existing history/error wiring on the underlying bus.
324-337: All EventBus-dependent attachments now consistently useBusHandle<EnclaveEvent>Passing
&bus/bus.clone()intoSortition::attach,CiphernodeSelector::attach,HistoricalEventCoordinator::setup, all Sol readers/writers, andCommitteeFinalizer::attachkeeps every consumer on the same BusHandle-based API with no remaining directAddr<EventBus<_>>usage in the construction flow.Also applies to: 352-423
crates/aggregator/src/lib.rs (1)
7-9: New internal module wiring looks consistentAdding
mod keyshare_created_filter_buffer;cleanly registers the new actor in this crate without changing the public API (pub useblock unchanged). No issues here.crates/aggregator/src/publickey_aggregator.rs (2)
89-118: Keyshare ingestion and node tracking look correct and defensiveThe new
add_keyshare(&mut self, keyshare, node)implementation:
- Restricts additions to the
Collectingstate.- Deduplicates both keyshares and nodes via
OrderedSet.- Logs progress
keyshares {len}/{threshold_n}.- Transitions to
Computingby taking ownership of bothkeysharesandnodeswhen the threshold is reached.
Handler<KeyshareCreated>correctly:
- Validates
e3_idand logs an error on mismatch before no-op’ing.- Delegates to
add_keyshare, propagating any state misuse as an error.- Triggers
ComputeAggregateonly when the state is observed asComputing.This all reads as consistent with the new buffered flow from
KeyshareCreatedFilterBufferand should ensure that the finalPublicKeyAggregatedincludes the full node set.Also applies to: 150-173
176-204: Final aggregate publication flows cleanly from state to busThe
Handler<ComputeAggregate>implementation:
- Logs when starting computation.
- Uses
GetAggregatePublicKeyto derive the aggregate.- Calls
set_pubkeyto transition intoComplete.- Reads back the state and, if
Complete, publishesPublicKeyAggregated { pubkey, e3_id, nodes }viaself.bus.publish.This keeps all event construction data (
pubkey,nodes,e3_id) coming from the persisted state, and the publish happens only once per successful transition toComplete. The logging around computation and notification is also helpful for diagnosing timing/order issues noted in the PR description.crates/aggregator/src/ext.rs (3)
29-42: Plaintext extension’s BusHandle migration andget_data()usage are consistentFor
PlaintextAggregatorExtension:
- Switching
bustoBusHandle<EnclaveEvent>and updatingcreateto take&BusHandlealigns with the new event-bus API while keeping the external surface identical for callers.on_eventnow pattern-matches onevt.get_data()withEnclaveEventData::CiphertextOutputPublished(data), which is consistent with the new EnclaveEventData-centric design.- Error paths correctly use
self.bus.errwithEnclaveErrorType::PlaintextAggregation, matching previous guidance that errors should go through the event bus instead of directtracing.- Hydration reconstructs the actor with the same deps (
fhe,bus,sortition,e3_id) it had on initial creation.Overall, the refactor looks correct and preserves behavior.
Also applies to: 47-147
150-195: PublicKey extension correctly wraps the aggregator with the new keyshare filterThe
PublicKeyAggregatorExtensionandcreate_publickey_aggregatorhelper together:
- Move
bustoBusHandle<EnclaveEvent>, consistent with the rest of the PR.- Use
evt.get_data()withEnclaveEventData::E3Requested(data)to trigger aggregator creation only on the relevant event.- Initialize state via
PublicKeyAggregatorState::init(meta.threshold_n, meta.seed)and persist it through the publickey repo before starting the actor, as before.- Wrap the
PublicKeyAggregatoraddress inKeyshareCreatedFilterBuffer::new(...), and then expose the filter’sRecipient<EnclaveEvent>back to theE3Context.This means the context’s
"publickey"recipient now points at the buffer actor, which in turn forwards only committee-approvedKeyshareCreatedevents downstream to the aggregator. Hydration uses the samecreate_publickey_aggregatorhelper, so resumed flows will also go through the filter.The wiring and id usage (
e3_idfromE3Requestedon create,ctx.e3_idon hydrate) are consistent, so this refactor looks solid.Also applies to: 197-232, 234-247
249-267: Threshold plaintext extension’s BusHandle update is straightforward and consistentFor
ThresholdPlaintextAggregatorExtension:
- Updating
bustoBusHandle<EnclaveEvent>andcreateto accept&BusHandle<EnclaveEvent>mirrors the Plaintext extension changes.on_eventnow matchesEnclaveEventData::CiphertextOutputPublished(data)viaevt.get_data(), then constructs and persistsThresholdPlaintextAggregatorStateas before.- Error reporting continues through
self.bus.errwithEnclaveErrorType::PlaintextAggregation.- Hydration and initial creation both wire
bus,sortition,multithread, ande3_ididentically, preserving behavior.Given this, the threshold plaintext path appears behaviorally unchanged aside from the new event API, which is what we want here.
Also applies to: 271-345
crates/aggregator/src/keyshare_created_filter_buffer.rs (1)
47-72: No changes needed. The current implementation is safe and follows idiomatic Rust patterns. Thematch msg.get_data()followed by movingmsgin the match arms is standard practice and does not violate borrow checker rules. The temporary borrow fromget_data()is scoped to the match expression and ends before the move operations in the arms execute. The suggested refactor would only add unnecessary clones.Likely an incorrect or invalid review comment.
2 participants
First stage of #1050
Sorry for the long PR but we had to change every eventbus call site.
Before starting sync mode with event HLCs we need to clean up.
We need to be able to add timestamps to events and provide custom data to events and do it in a centralized way.
We must not use a static var for our HLC because we will want to create tests testing multiple nodes with differing clocks on the same machine.
In order to do this we must have a factory that will eventually hold a local HLC but that factory would need to be passed in everywhere we dispatch events.
The best way I could come up with how to handle this was to replace
Addr<EventBus<E>>with a "handle" (NOTE: NOT "handler") as inJoinHandle. Taking the term from https://ryhl.io/blog/actors-with-tokio as we are doing effectively what she does with those actor handles.I setup a bunch of new tidier minimal traits:
Fix up some other issues within
EnclaveEventinto_data() -> EnclaveEventDataandget_data() -> &EnclaveEventDataevent_type()custom implementation in favour of the more standardstrumcrate.Intotrait in favor of explicit functions despite being idiomatic as we will need to add clocks etc and often using into() and from() can get confusing. We can consider this later if absolutely necessary. Replaced with:bus.publish(SomeEventData);bus.publish_from_remote(SomeEventData, remote_timestamp);(for a later point where we use timestamps)Connected but orthogonal PR: #1057 need to get both of these in to progress now.
I would prefer to get this in quicker as it is a bigger PR to avoid merge issues. We can then add the HLC stuff and then incorporate it with tests and event ordering.
Summary by CodeRabbit
Refactor
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.