refactor(da): Wrapping FinalizedEpoch in a trait to make mockable
#342
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update introduces a trait-based abstraction for epoch verification, refactors event handling to use a unified event channel and event type, and removes optionality from verifying key usage. The data availability layer and light client now interact through boxed verifiable epochs and publish events using a new event channel interface. Several APIs and struct fields are updated for consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LightClient
participant DA_Layer as DataAvailabilityLayer
participant EventChannel
User->>LightClient: Start client (run)
LightClient->>DA_Layer: Subscribe to event_channel
DA_Layer->>EventChannel: Publish PrismEvent::UpdateDAHeight
EventChannel->>LightClient: Receive UpdateDAHeight event
LightClient->>DA_Layer: get_finalized_epoch (returns Vec<VerifiableEpoch>)
LightClient->>VerifiableEpoch: verify (with verifying key)
VerifiableEpoch-->>LightClient: Commitments, verification result
LightClient->>EventChannel: Publish PrismEvent (sync events)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
distractedm1nd
force-pushed
the
verifiable-epochs
branch
from
b51b7fd to
52faab8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
crates/cli/src/cfg.rs (1)
235-300: Incorrect fallback ignoresverifying_keyloaded from the config file
network_configis derived fromconfig.network.network.config()– a fresh default for the selected network – not the already-deserializedconfig.network.
If the user specifies a verifying key only inconfig.toml, it will be silently discarded and replaced by the default key produced inNetwork::config(). This breaks existing deployments and violates the new “verifying key is mandatory” contract.- verifying_key: args - .verifying_key - .and_then(|x| VerifyingKey::from_base64(x).ok()) - .unwrap_or(network_config.verifying_key.clone()), + verifying_key: args + .verifying_key + .and_then(|x| VerifyingKey::from_base64(x).ok()) + // fall back to the key that was *actually* read from the config file + .unwrap_or_else(|| config.network.verifying_key.clone()),Failing to fix this will cause signature-verification mismatches at runtime.
crates/da/src/celestia/utils.rs (1)
60-70: Random placeholderverifying_keycauses inconsistent configs
SigningKey::new_ed25519().verifying_key()generates a new random key every timeNetworkConfig::default()is called. As a result:• Two components in the same process may hold different “default” keys.
• The key written to a freshly-generatedconfig.tomlwill not match the key later produced byNetwork::Custom(..).config()during runtime, breaking signature verification.- // TODO: This is just a placeholder, don't let this get merged - verifying_key: SigningKey::new_ed25519().verifying_key(), + // MUST be supplied by the user; use a clearly invalid sentinel to fail fast + verifying_key: VerifyingKey::from_base64("").unwrap_or_else(|_| { + panic!("verifying_key must be provided in the configuration") + }),Alternatively, keep it
Option<VerifyingKey>and enforce presence during CLI validation.Leaving this as-is will yield hard-to-trace verification failures.
crates/da/src/memory.rs (1)
92-117: Release write-locks before broadcasting / publishing events.
blocks,pending_*, andlatest_heightwrite-locks remain held while:
- sending over the height/channel
- publishing
PrismEvent::UpdateDAHeightIf any subscriber tries to acquire a read-lock on
blocksin its handler you have a classic write-→read dead-lock.Move the lock scopes into a separate block so they are dropped before I/O:
{ let mut blocks = self.blocks.write().await; /* build & push new_block */ } // <- locks released here // now broadcast + publish let _ = self.height_update_tx.send(*latest_height); let _ = self.block_update_tx.send(new_block.clone()); event_publisher.send(PrismEvent::UpdateDAHeight { height: *latest_height });
🧹 Nitpick comments (18)
crates/da/Cargo.toml (2)
21-25: Move test-only crates to[dev-dependencies]
serde_json,web-time(withserdefeature),sp1-verifier, and especiallymockallpull in large transitive graphs.
Unless they are needed in the final WASM / binary, place them under[dev-dependencies](or behind a feature flag) to keep compile time and binary size down.-[dependencies] +# Runtime deps [dependencies] … -[dependencies] -mockall = { workspace = true } +[dev-dependencies] +mockall = { workspace = true }Also applies to: 29-30, 35-36
24-24:web-timeis notwasm32-friendly by default
web-timerelies onstd::timefor native targets. On thewasm32build you already gatetokio, butweb-timeis unconditional here.
If you need it in WASM, enable"wasm-bindgen"or guard the entire dep withcfg(not(target_arch="wasm32")).crates/tests/src/lib.rs (1)
17-18:EventChannelimport is now unusedAfter inlining event handling inside
LightClient, this import can be dropped to avoid theunused_importswarning.-use prism_da::events::EventChannel;crates/cli/src/main.rs (1)
17-19: Remove staleEventChannelimportNo symbol from
prism_da::eventsis used after the refactor.-use prism_da::events::EventChannel;crates/da/src/utils.rs (1)
11-17: Return theJoinHandle(or at least log it) to avoid silently dropped panics
tokio::spawnreturns aJoinHandle. By discarding it we (a) detach the task, making shutdown coordination harder, and (b) lose the panic/error information. Consider:- tokio::spawn(future); + tokio::spawn(future); + // Option A – propagate handle + // Option B – at minimum: tracing::debug!("spawned task: {:?}", handle);Keeping the handle (or logging it) is a low-cost way to surface runtime failures.
crates/node_types/prover/src/prover_engine.rs (2)
2-3: Remove unused import
VerifiableEpochis imported but not referenced anywhere in this file.
Drop it to silence theunused importlint.-use prism_da::{VerifiableEpoch, VerificationKeys}; +use prism_da::VerificationKeys;
50-61:verification_keyscan avoid repeated allocationsCalling
bytes32()on each access clones the full 32-byte array every timeverification_keys()is called.
Given this method is expected to be invoked inside hot paths (e.g. once per-epoch verification), cache the pre-computedVerificationKeysinside the struct or return references instead of freshly-allocated copies.This is optional, but it will shave a few heap-free copies per call.
crates/node_types/prover/src/syncer.rs (2)
167-171: Logging shows the wrong valueInside the loop we log
debug!("Found finalized epoch {} at height {}", epoch.height(), height);Good.
But a few lines below, when skipping an already-processed epoch, the branch logscurrent_epochinstead of the skippedheight(see lines 229-231). That obscures which epoch was dropped.Consider:
- debug!("epoch {} already processed internally", current_epoch); + debug!("epoch {} already processed internally", height);
233-235: Avoid creating a freshVerificationKeysfor every epoch
epoch.verify(&self.verifying_key, &self.prover_engine.verification_keys())
verification_keys()allocates new 32-byte arrays on each call.
Compute once atSyncerconstruction and reuse, or hold anArc<VerificationKeys>insideProverEngineand return a reference to prevent needless cloning.crates/da/src/celestia/full_node.rs (2)
3-6: Drop unusedEventPublisherimportThe file never references the type directly; inference handles it.
Removing the import eliminates the warning seen in CI.-use crate::{ - FinalizedEpoch, LightDataAvailabilityLayer, VerifiableEpoch, - events::{EventChannel, EventPublisher, PrismEvent}, -}; +use crate::{ + FinalizedEpoch, LightDataAvailabilityLayer, VerifiableEpoch, + events::{EventChannel, PrismEvent}, +};
128-141: Handle send failures on saturated event channel
event_publisher.send(PrismEvent::UpdateDAHeight { height });
EventPublisher::senddrops the message silently when the channel is full.
For long-running tasks this can mask back-pressure problems. At minimum log the error:if let Err(e) = event_publisher.send(PrismEvent::UpdateDAHeight { height }) { trace!("event channel full, dropping DA-height update: {}", e); }crates/da/src/events.rs (2)
1-1: Remove unusedNodeEventInfoimportIt’s not referenced after the refactor and triggers
unused importlints.-use lumina_node::events::{NodeEvent, NodeEventInfo}; +use lumina_node::events::NodeEvent;
112-138: Non-wasm32 builds silently drop forwarded Lumina events
start_forwardingonly re-publishes Lumina events on WASM targets; desktop builds merely log them withtrace!.
If downstream consumers rely on those events they will never receive them.Consider always forwarding, gating the log behind target-specific code instead:
publisher.send(PrismEvent::LuminaEvent { event: event.clone() }); #[cfg(not(target_arch = "wasm32"))] trace!("lumina event: {:?}", event);crates/da/src/celestia/light_client.rs (1)
125-129: Redundant clone oflumina_sub
EventChannel::from(lumina_sub.clone())clones theArcimmediately after creation; the originallumina_subis never reused.
Create theArconce:-let lumina_sub = Arc::new(Mutex::new(event_subscriber)); -// Creates an EventChannel that starts forwarding lumina events -let prism_chan = EventChannel::from(lumina_sub.clone()); +let lumina_sub = Arc::new(Mutex::new(event_subscriber)); +let prism_chan = EventChannel::from(lumina_sub);crates/da/src/memory.rs (1)
128-136: Avoid the extra epoch clone per call.
map(|epoch| Box::new(epoch) …)clones everyFinalizedEpoch; for large proofs this is expensive.
If the caller only needs read-only access consider storing epochs inArcand returningArc<dyn VerifiableStateTransition>to eliminate the copy.crates/node_types/lightclient/src/lightclient.rs (2)
62-67: RedundantArcaroundEventPublisher.
event_chan.publisher()already returns a cheap, cloneable handle; wrapping it inside anotherArcadds an extra layer and forcesevent_pub.send(..)to rely on deref-coercion.
Consider using the value directly:- let event_pub = Arc::new(event_chan.publisher()); + let event_pub = event_chan.publisher();
261-270: Suppress unused variable warning forprev_commitment.
prev_commitmentis never used after verification. Prefix with_or drop the binding:- let (prev_commitment, curr_commitment) = + let (_, curr_commitment) = epoch.verify(&self.prover_pubkey, &self.sp1_vkeys)?;crates/node_types/uniffi-lightclient/src/lib.rs (1)
9-13: Remove or use the now-unused imports to avoiddead_code/unused_importswarnings
LightDataAvailabilityLayerandEventChannelare brought into scope but never referenced in this file.
If the workspace has#, this will turn into a compilation error.-use prism_da::{ - LightDataAvailabilityLayer, - celestia::{light_client::LightClientConnection, utils::Network}, - events::{EventChannel, EventSubscriber}, -}; +use prism_da::{ + celestia::{light_client::LightClientConnection, utils::Network}, + events::EventSubscriber, +};Or keep them and add a TODO explaining their future use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
crates/cli/src/cfg.rs(1 hunks)crates/cli/src/main.rs(3 hunks)crates/da/Cargo.toml(1 hunks)crates/da/src/celestia/full_node.rs(8 hunks)crates/da/src/celestia/light_client.rs(5 hunks)crates/da/src/celestia/utils.rs(4 hunks)crates/da/src/events.rs(5 hunks)crates/da/src/lib.rs(6 hunks)crates/da/src/memory.rs(9 hunks)crates/da/src/utils.rs(1 hunks)crates/node_types/lightclient/src/lib.rs(0 hunks)crates/node_types/lightclient/src/lightclient.rs(10 hunks)crates/node_types/prover/src/prover/tests/mod.rs(1 hunks)crates/node_types/prover/src/prover_engine.rs(2 hunks)crates/node_types/prover/src/syncer.rs(4 hunks)crates/node_types/uniffi-lightclient/src/lib.rs(4 hunks)crates/node_types/uniffi-lightclient/src/types.rs(2 hunks)crates/tests/src/lib.rs(2 hunks)verification_keys/keys.json(1 hunks)
💤 Files with no reviewable changes (1)
- crates/node_types/lightclient/src/lib.rs
🧰 Additional context used
🧬 Code Graph Analysis (5)
crates/node_types/prover/src/prover/tests/mod.rs (1)
crates/da/src/lib.rs (2)
height(48-48)height(90-92)
crates/tests/src/lib.rs (1)
crates/node_types/lightclient/src/lightclient.rs (1)
new(58-76)
crates/node_types/prover/src/syncer.rs (3)
crates/telemetry/src/metrics_registry.rs (1)
get_metrics(100-108)crates/da/src/lib.rs (2)
height(48-48)height(90-92)crates/node_types/lightclient/src/lightclient.rs (1)
process_epoch(261-273)
crates/da/src/celestia/light_client.rs (6)
crates/da/src/events.rs (2)
new(95-98)from(142-146)crates/da/src/celestia/full_node.rs (3)
new(43-71)event_channel(114-116)get_finalized_epoch(76-112)crates/da/src/memory.rs (3)
new(40-61)event_channel(141-143)get_finalized_epoch(128-139)crates/node_types/lightclient/src/lightclient.rs (1)
new(58-76)crates/da/src/celestia/utils.rs (1)
create_namespace(110-120)crates/da/src/lib.rs (5)
event_channel(226-226)get_finalized_epoch(224-224)height(48-48)height(90-92)try_from(211-217)
crates/da/src/celestia/full_node.rs (5)
crates/da/src/lib.rs (5)
event_channel(226-226)get_finalized_epoch(224-224)height(48-48)height(90-92)try_from(211-217)crates/da/src/celestia/light_client.rs (3)
event_channel(183-185)new(99-137)get_finalized_epoch(187-227)crates/da/src/memory.rs (3)
event_channel(141-143)new(40-61)get_finalized_epoch(128-139)crates/da/src/events.rs (1)
new(95-98)crates/node_types/lightclient/src/lightclient.rs (1)
new(58-76)
🪛 GitHub Check: integration-test
crates/da/src/events.rs
[failure] 1-1:
unused import: NodeEventInfo
crates/da/src/celestia/full_node.rs
[failure] 5-5:
unused import: EventPublisher
crates/da/src/lib.rs
[failure] 19-19:
unused import: EventSubscriber
[failure] 108-108:
unused borrow that must be used
🪛 GitHub Check: unit-test
crates/da/src/events.rs
[failure] 1-1:
unused import: NodeEventInfo
crates/da/src/celestia/full_node.rs
[failure] 5-5:
unused import: EventPublisher
crates/da/src/lib.rs
[failure] 19-19:
unused import: EventSubscriber
[failure] 108-108:
unused borrow that must be used
🪛 GitHub Check: clippy
crates/da/src/events.rs
[failure] 1-1:
unused import: NodeEventInfo
crates/da/src/celestia/full_node.rs
[failure] 5-5:
unused import: EventPublisher
crates/da/src/lib.rs
[failure] 19-19:
unused import: EventSubscriber
[failure] 108-108:
unused borrow that must be used
[failure] 108-108:
unnecessary operation
🪛 GitHub Check: coverage
crates/da/src/events.rs
[failure] 1-1:
unused import: NodeEventInfo
crates/da/src/celestia/full_node.rs
[failure] 5-5:
unused import: EventPublisher
crates/da/src/lib.rs
[failure] 19-19:
unused import: EventSubscriber
[failure] 108-108:
unused borrow that must be used
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: unused dependencies
🔇 Additional comments (7)
verification_keys/keys.json (1)
1-1: Hex length / prefix sanity-check required before mergingThe two verifying-key blobs look well-formed (
0x+ 64 hex chars) but this file is consumed by several crates at runtime. A single wrong nibble bricks verification and the failure mode is silent (proof always fails).Please add an integrity assertion (e.g. unit test that parses the keys and matches the expected byte-length) or, at minimum, document the commit / circuit these keys were generated from so future rotations are traceable.
crates/node_types/prover/src/prover/tests/mod.rs (2)
116-118: Height expectation likely off by the configured gapYou wait until
height >= initial_epoch_height + 6, then expect
gap_proof.height() == initial_epoch.height() + 1.
Shouldn’t the gap proof be atinitial_epoch.height() + 6(or+ max_epochless_gap)?
Please double-check the business rule; otherwise this assertion may produce false positives.
114-122: Type mismatch inassert_eq!will not compile
gap_proof.commitments()returns a pair by reference;commitment_after_epoch.commitmentis an owned value.
assert_eq!between&TandTfails unlessPartialEq<&T>is implemented.-let (_, current_commitment) = gap_proof.commitments(); -… -assert_eq!( - current_commitment, commitment_after_epoch.commitment, +let (_, current_commitment) = gap_proof.commitments(); +assert_eq!( + *current_commitment, commitment_after_epoch.commitment,Likely an incorrect or invalid review comment.
crates/tests/src/lib.rs (1)
103-104:LightClient::newcall reflects new API – LGTMThe constructor now only needs
(da, vk); test code adapts correctly.crates/cli/src/main.rs (1)
94-95: Constructor adaption looks correct
LightClient::new(da, verifying_key)matches the new signature – good catch on simplifying setup.crates/node_types/uniffi-lightclient/src/types.rs (1)
62-95: Ensurematchis kept exhaustive when newPrismEventvariants are addedThe current conversion matches every variant that exists today, but any future addition to
PrismEventwill silently trigger a compiler-silenced “non-exhaustive” pattern via the wildcard arm you just removed. Please add a final_ => unreachable!()(or equivalent) to make missing mappings a compile-time error, or explicitly markPrismEventas#[non_exhaustive]upstream.This prevents the mobile bindings from diverging from core functionality.
crates/node_types/uniffi-lightclient/src/lib.rs (1)
26-27: Good move: eliminate theOptionwrapperBy storing
EventSubscriberdirectly in theMutexyou guarantee the subscriber is always initialised, simplifying the control flow and removing one failure branch innext_event(). 👍
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the data‐availability and lightclient layers to make FinalizedEpoch mockable, centralizes event definitions in the da crate, and updates downstream code to use the new trait and event API.
- Introduces
VerifiableStateTransitiontrait and updatesLightDataAvailabilityLayerto returnVerifiableEpoch - Moves event types into
crates/da/src/events.rs(renamed toPrismEvent) and updates subscribers/publishers - Updates lightclient and prover code (and tests) to consume
VerifiableEpochand the new event channel API
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/da/src/lib.rs | Defined VerifiableStateTransition trait and VerifiableEpoch alias |
| crates/da/src/events.rs | Moved event enum into DA crate, renamed to PrismEvent |
| crates/da/src/memory.rs | In-memory DA layer now returns VerifiableEpoch and publishes events |
| crates/da/src/utils.rs | Added spawn_task helper for WASM vs native |
| crates/da/src/celestia/utils.rs | Changed NetworkConfig.verifying_key to non-optional |
| crates/node_types/prover/src/syncer.rs | Refactored process_epoch to use VerifiableEpoch |
| crates/node_types/prover/src/prover_engine.rs | Added verification_keys() helper, removed old proof‐verify method |
| crates/node_types/lightclient/src/lightclient.rs | Refactored to use the new event_channel API and VerifiableEpoch |
| crates/node_types/uniffi-lightclient/src/types.rs | Switched mapping from LightClientEvent to PrismEvent |
| crates/tests/src/lib.rs | Updated tests to accommodate new LightClient::new signature |
Comments suppressed due to low confidence (1)
crates/node_types/lightclient/src/lightclient.rs:4
- The
FinalizedEpochimport isn’t referenced in this file after the refactor. You can remove it to avoid dead code.
use prism_da::{FinalizedEpoch, LightDataAvailabilityLayer, VerifiableEpoch, VerificationKeys, events::EventChannel,};
52faab8 to
05cf862
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/da/src/lib.rs (2)
19-20: Remove unusedEventSubscriberimport
EventSubscriberis no longer referenced – keeping it breakscargo checkwhenrustcis run with-D warnings.
115-116:&self.verify_signature(..)?causes a hard compile-error
?cannot be applied to&Result<…>.
Remove the leading&(and avoid the unnecessaryclone()if you accept a reference).- &self.verify_signature(vk.clone())?; + self.verify_signature(vk)?;
🧹 Nitpick comments (5)
crates/da/src/lib.rs (2)
184-184: Accept&VerifyingKeyto avoid needless cloningThe key is never mutated; passing by reference removes the
clone()and allocations elsewhere.-pub fn verify_signature(&self, vk: VerifyingKey) -> Result<(), EpochVerificationError> { +pub fn verify_signature(&self, vk: &VerifyingKey) -> Result<(), EpochVerificationError> {Call-sites can then pass
vkby reference.
236-237: Minor typo in error message
"epoch proo verification error"→"epoch proof verification error".crates/node_types/lightclient/src/lightclient.rs (3)
2-2: Delete unusedNodeEventimportThe symbol isn’t used anywhere in this module and will trigger the linter.
-use lumina_node::events::NodeEvent;
11-11: Drop redundantstd::selfin use-statement-use std::{self, sync::Arc}; +use std::sync::Arc;
262-263: Suppress unused variable warning
prev_commitmentisn’t used after verification; prefix with an underscore to silence the warning cleanly.- let (prev_commitment, curr_commitment) = + let (_, curr_commitment) =
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
crates/cli/src/cfg.rs(1 hunks)crates/cli/src/main.rs(3 hunks)crates/da/Cargo.toml(1 hunks)crates/da/src/celestia/full_node.rs(8 hunks)crates/da/src/celestia/light_client.rs(5 hunks)crates/da/src/celestia/utils.rs(4 hunks)crates/da/src/events.rs(5 hunks)crates/da/src/lib.rs(4 hunks)crates/da/src/memory.rs(9 hunks)crates/da/src/utils.rs(1 hunks)crates/node_types/lightclient/src/lib.rs(0 hunks)crates/node_types/lightclient/src/lightclient.rs(10 hunks)crates/node_types/prover/src/prover/tests/mod.rs(1 hunks)crates/node_types/prover/src/prover_engine.rs(2 hunks)crates/node_types/prover/src/syncer.rs(4 hunks)crates/node_types/uniffi-lightclient/src/lib.rs(4 hunks)crates/node_types/uniffi-lightclient/src/types.rs(2 hunks)crates/tests/src/lib.rs(2 hunks)verification_keys/keys.json(1 hunks)
💤 Files with no reviewable changes (1)
- crates/node_types/lightclient/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- verification_keys/keys.json
- crates/da/Cargo.toml
- crates/da/src/utils.rs
- crates/node_types/prover/src/prover/tests/mod.rs
- crates/tests/src/lib.rs
- crates/cli/src/cfg.rs
- crates/cli/src/main.rs
- crates/node_types/prover/src/prover_engine.rs
- crates/da/src/celestia/utils.rs
- crates/node_types/uniffi-lightclient/src/types.rs
- crates/node_types/prover/src/syncer.rs
- crates/da/src/events.rs
- crates/da/src/celestia/full_node.rs
- crates/da/src/memory.rs
- crates/node_types/uniffi-lightclient/src/lib.rs
- crates/da/src/celestia/light_client.rs
🔇 Additional comments (1)
crates/node_types/lightclient/src/lightclient.rs (1)
65-67: Potential double-ArcaroundEventPublisher
event_chan.publisher()already returns anArc<EventPublisher>(per typical channel APIs). Wrapping it again nests twoArcs and forces extra derefs on every call.- let event_pub = Arc::new(event_chan.publisher()); + let event_pub = event_chan.publisher();Please verify the actual return type of
publisher().
If it already is anArc, adjust the field type toArc<EventPublisher>and remove the extra wrapper.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
crates/da/src/lib.rs (2)
124-129: Illegal move ofself.proofon the WASM path
self.proofis moved out of&self, which is not allowed becauseVec<u8>is notCopy.
Borrow instead:-#[cfg(target_arch = "wasm32")] -let finalized_epoch_proof = self.proof; +#[cfg(target_arch = "wasm32")] +let finalized_epoch_proof = &self.proof;The same pattern is already used for the native path; keep both consistent.
150-153: Do notunwrap()during signature insertion
encode_to_bytes()can fail; anunwrap()here will crash the node.
Propagate the error instead:-let plaintext = self.encode_to_bytes().unwrap(); +let plaintext = self + .encode_to_bytes() + .map_err(|e| EpochVerificationError::SerializationError(e.to_string()))?;
🧹 Nitpick comments (3)
crates/node_types/lightclient/src/lightclient.rs (1)
306-364: Remove dead code: local commitment-verification helpers
extract_commitments,verify_commitments, andverify_snark_proofare no longer used after delegating verification toVerifiableStateTransition::verify.
Leaving them in place bloats the binary and risks divergence if future logic changes.Consider deleting them or making them
#[cfg(test)]helpers if they are only needed in unit tests.crates/da/src/lib.rs (2)
114-115: Avoid cloning the verifying key on every epoch verification
verify_signatureonly needs a shared reference. Change the signature to accept&VerifyingKeyand pass it directly:-self.verify_signature(vk.clone())?; +self.verify_signature(vk)?;This removes an unnecessary heap allocation and improves performance.
235-236: Typo in error message
"epoch proo verification error"→"epoch proof verification error".Even minor typos make debugging harder and pollute logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
crates/cli/src/cfg.rs(2 hunks)crates/cli/src/main.rs(3 hunks)crates/da/src/celestia/full_node.rs(8 hunks)crates/da/src/events.rs(5 hunks)crates/da/src/lib.rs(4 hunks)crates/node_types/lightclient/src/lightclient.rs(10 hunks)crates/node_types/prover/src/prover_engine.rs(2 hunks)crates/node_types/prover/src/syncer.rs(4 hunks)crates/node_types/uniffi-lightclient/src/lib.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/cli/src/cfg.rs
- crates/cli/src/main.rs
- crates/node_types/uniffi-lightclient/src/lib.rs
- crates/node_types/prover/src/prover_engine.rs
- crates/da/src/celestia/full_node.rs
- crates/node_types/prover/src/syncer.rs
- crates/da/src/events.rs
🔇 Additional comments (1)
crates/node_types/lightclient/src/lightclient.rs (1)
86-100: Possible mismatch between subscriber item type and access pattern
event_sub.recv().awaittypically yields the event itself (PrismEvent); the code then accesses an.eventfield:if let PrismEvent::UpdateDAHeight { height } = event_info.event { ... }Unless
EventChannelwraps events in a struct with aneventfield, this will not compile.
Please verify the actual type returned byrecv()and remove the extra indirection if necessary.
distractedm1nd
force-pushed
the
verifiable-epochs
branch
from
8d0a875 to
01c8caa
Compare
…eltadevsde#342) * verifiable epochs * refactor: moving event channel code to DA crate * refactor: renaming LightClientEvent to PrismEvent * feat: implement event channel for inmemory da and fn celestia * a bit more sensible errors for FinalizedEpoch verification * clippy * fix: review comments * update
This PR:
FinalizedEpochmockable by wrapping it in a traitdacrateSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores