feat: event feed [skip-line-limit]#1475
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace TCP socket-server with an HTTP-based daemon-server; add a BytesSerde/AsBytesSerde proc-macro and migrate many binary fields to Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Client
participant Daemon as Daemon HTTP Server
participant EventActor as EventStore Actor
rect rgba(100, 150, 255, 0.5)
Note over CLI,EventActor: Daemon Event Query Flow
CLI->>Daemon: POST / (EventsQuery JSON: agg,since,limit)
Daemon->>Daemon: Parse HTTP request (Content-Length, body)
Daemon->>EventActor: Send EventStoreQueryBy<SeqAgg> via actor
EventActor-->>Daemon: EventStoreQueryResponse (events)
Daemon->>Daemon: Serialize events + cursor to JSON lines
Daemon-->>CLI: HTTP 200 OK with JSON lines
CLI->>CLI: Parse & display each EnclaveEvent
end
sequenceDiagram
participant CLI as CLI Client
participant Daemon as Daemon HTTP Server
participant Config as AppConfig
rect rgba(100, 150, 255, 0.5)
Note over CLI,Config: Daemon Config Query Flow
CLI->>Daemon: POST / (ConfigGet JSON: param)
Daemon->>Daemon: Parse HTTP request body
Daemon->>Config: Lookup requested parameter
Config-->>Daemon: Return parameter value(s)
Daemon-->>CLI: HTTP 200 OK (JSON formatted value)
CLI->>CLI: Display config value
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
crates/zk-prover/tests/common/helpers.rs (1)
142-142: Consider using.expect()for clearer panic messages.While
.unwrap()is acceptable in test code, using.expect()provides better diagnostics whenE3_CUSTOM_BBis set to an invalid path.Suggested improvement
- let bb_binary = BBPath::check(noir_dir.join("bin").join("bb")).unwrap(); + let bb_binary = BBPath::check(noir_dir.join("bin").join("bb")) + .expect("Failed to resolve bb binary path (check E3_CUSTOM_BB if set)");Also note that
test_backend()(lines 162-165) manually handlesE3_CUSTOM_BBrather than usingBBPath::check(). Consider unifying these for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/tests/common/helpers.rs` at line 142, The current call let bb_binary = BBPath::check(noir_dir.join("bin").join("bb")).unwrap(); should use .expect(...) to give a clearer panic when E3_CUSTOM_BB points to an invalid path—replace the .unwrap() with .expect("invalid E3_CUSTOM_BB or bb binary not found: ...") to include context; additionally, update the manual handling in test_backend() to reuse BBPath::check(...) (and propagate its expect message) so both bb_binary initialization and test_backend() consistently validate E3_CUSTOM_BB using BBPath::check.crates/cli/src/main.rs (1)
74-80: Optional: update stale “socket” phrasing in the branch comment.The condition now checks daemon availability, so the comment should match current behavior.
📝 Proposed wording tweak
- // If the socket exists and the command can be parsed as remote + // If the daemon is available and the command can be parsed as remote @@ - // Run the command over the socket + // Run the command over the daemon🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/main.rs` around lines 74 - 80, Comment above the conditional still refers to "socket" but the code checks for daemon availability via maybe_server and maybe_remote_command; update the comment to accurately describe this branch (e.g., "If the daemon is available and the command can be parsed as a remote command") and place it near the conditional that uses maybe_server, maybe_remote_command and calls run_on_daemon so the comment reflects current behavior.crates/cli/src/start.rs (1)
58-58: Renamelaunch_socket_serverwording to match daemon transport.The implementation is daemon-based now; keeping “socket server” naming/comments will cause confusion over time.
♻️ Proposed rename
- launch_socket_server(config.ctrl_port()); + launch_daemon_server(config.ctrl_port()); @@ -/// Launch a socket server to read RemoteCli commands -pub fn launch_socket_server(ctrl_port: u16) { - // Setup socket server for daemon +/// Launch a daemon server to read RemoteCli commands +pub fn launch_daemon_server(ctrl_port: u16) { + // Setup daemon server for remote CLI commands🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/start.rs` at line 58, References to "socket server" should be renamed to reflect the daemon-based transport: update the function name launch_socket_server (and any related identifiers and comments) to launch_daemon_server (or launch_daemon_transport) and adjust all call sites such as start_daemon_server(...) and any docs/comments to use "daemon" wording so names and comments consistently describe the daemon transport implementation.crates/zk-prover/src/backend/tests.rs (1)
64-67: Optional: deduplicate the repeatedE3_CUSTOM_BBskip guard.The same early-return block appears in three tests; a tiny helper would reduce repetition.
♻️ Proposed cleanup
+fn has_custom_bb() -> bool { + env::var("E3_CUSTOM_BB").is_ok() +} + #[tokio::test] async fn test_check_status_full_setup_needed() { - if let Some(_) = env::var("E3_CUSTOM_BB").ok() { + if has_custom_bb() { return; } @@ #[tokio::test] async fn test_check_status_ready_when_installed() { - if let Some(_) = env::var("E3_CUSTOM_BB").ok() { + if has_custom_bb() { return; } @@ #[tokio::test] async fn test_check_status_bb_needs_update() { - if let Some(_) = env::var("E3_CUSTOM_BB").ok() { + if has_custom_bb() { return; }Also applies to: 81-84, 114-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/backend/tests.rs` around lines 64 - 67, Create a small helper to remove the repeated env-check: add a function like fn skip_if_e3_custom_bb() -> bool { std::env::var("E3_CUSTOM_BB").is_ok() } (or fn skip_if_e3_custom_bb() { if std::env::var("E3_CUSTOM_BB").is_ok() { return; } } depending on preferred style) in the tests module and replace the three occurrences of if let Some(_) = env::var("E3_CUSTOM_BB").ok() { return; } with a single call to that helper (e.g., if skip_if_e3_custom_bb() { return; } or skip_if_e3_custom_bb();) so all tests use the same guard and duplication is removed.crates/cli/src/config.rs (1)
80-109: Avoid usingDebugdumps as the control-plane output contract.
nodes,program, and the no-param branch currently expose{:?}output from internal structs. That format is unstable and awkward for scripts; prefer explicit fields or a structured format if these branches are meant to be consumed outside the process.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/config.rs` around lines 80 - 109, The current code in config.rs prints internal structs using "{:?}" (see calls to log!(out, "{:?}", config.program()) and log!(out, "nodes: {:?}", config.nodes()) and the no-param branch that logs many fields with "{:?}"), which exposes unstable Debug output; change these branches to emit a stable, script-friendly representation — either by serializing the relevant objects to a well-known structured format (e.g., JSON via serde) or by printing explicit, documented fields (e.g., iterate config.nodes() and print each node's name and explicit attributes, and for config.program() print its named fields rather than Debug). Update the log! invocations for the "nodes", "program", and None branches to use the chosen structured output (serialization or formatted field strings) so external consumers get stable, parseable output.crates/daemon-server/src/lib.rs (3)
100-111: Manual HTTP header parsing lacks bounds on header count.While the server is localhost-only (reducing attack surface), the header-reading loop has no limit on the number of header lines. A malicious local process could send an endless stream of headers without a blank line, causing unbounded memory growth.
Since this is localhost-only and serves a trusted CLI, this is low-severity but worth noting for defense-in-depth.
♻️ Optional: add a header count limit
// Read headers until blank line let mut content_length: usize = 0; + let mut header_count = 0; + const MAX_HEADERS: usize = 100; loop { let mut line = String::new(); buf_reader.read_line(&mut line).await?; if line.trim().is_empty() { break; } + header_count += 1; + if header_count > MAX_HEADERS { + anyhow::bail!("Too many headers"); + } if let Some(val) = line.to_ascii_lowercase().strip_prefix("content-length:") { content_length = val.trim().parse().unwrap_or(0); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon-server/src/lib.rs` around lines 100 - 111, The header-reading loop that uses buf_reader.read_line to parse headers (and sets content_length) has no bound on how many header lines it will accept; add a max-header limit (e.g., const MAX_HEADERS) and a counter inside the loop that increments per iteration and returns an error (or closes the connection) if the count exceeds the limit before hitting the blank line; update the code around the content_length variable and the loop so it enforces this limit and returns a clear error when exceeded.
113-116: LargeContent-Lengthvalues cause unbounded allocation.If a client sends a huge
Content-Lengthheader, line 114 allocates that many bytes before reading. While localhost-only mitigates external attacks, a buggy or malicious local process could trigger OOM.Consider capping
content_lengthto a reasonable maximum (e.g., 1MB) for robustness.♻️ Optional: cap body size
+ const MAX_BODY_SIZE: usize = 1024 * 1024; // 1MB + if content_length > MAX_BODY_SIZE { + anyhow::bail!("Request body too large"); + } // Read body let mut body = vec![0u8; content_length];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon-server/src/lib.rs` around lines 113 - 116, The code allocates body using content_length which allows unbounded allocation; before creating the Vec check and cap content_length against a constant MAX_BODY_SIZE (e.g., 1_048_576) and return a clear error (or respond with 413) if the header exceeds the cap. Then read at most the capped size (use tokio::io::AsyncReadExt::take on buf_reader or allocate Vec with the bounded size) into body and convert to String as before; update references to content_length where used to the validated/capped value to avoid OOM from malicious/buggy large Content-Length values.
38-57: Consider adding a timeout torun_on_daemon.Unlike
connect_daemon, this function has no timeout. If the daemon accepts the connection but hangs while processing, the client will block indefinitely.♻️ Suggested timeout addition
pub async fn run_on_daemon<T: Serialize>( out: Console, server: ServerInfo, cli: T, ) -> anyhow::Result<()> { let url = format!("http://{}:{}", TCP_ADDRESS, server.port); let client = reqwest::Client::new(); let resp = client .post(&url) .json(&cli) + .timeout(std::time::Duration::from_secs(60)) .send() .await? .error_for_status()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon-server/src/lib.rs` around lines 38 - 57, run_on_daemon currently uses reqwest::Client::new() with no timeout so a hung daemon can block the caller; replace the client creation with a Client built via reqwest::Client::builder().timeout(Duration::from_secs(N)).build() (import std::time::Duration) or set a per-request timeout on the RequestBuilder, and keep the rest of the logic the same so send().await will error on timeout; update the error propagation to let the timeout bubble up from run_on_daemon.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cli/src/events.rs`:
- Around line 89-94: The pagination cursor currently sets
BatchCursor::Next(last_event_seq) which causes the next read_from() call
(inclusive: seq >= from) to re-fetch the last event; update the logic in the
block that computes `next` so it returns BatchCursor::Next(last_event_seq + 1)
instead, locating the code around the `events`, `limit`, `last_event_seq`
calculation and the use of `seq()` to ensure the next query starts from the
following event.
In `@crates/config/src/app_config.rs`:
- Line 182: Fix the typo in the doc comment in app_config.rs: change
"otherwisse" to "otherwise" in the comment above the relevant function or method
that checks the environment variable and falls back to the given path (the doc
comment starting "Check the environment variable and if found use that
otherwisse use the given path").
In `@crates/daemon-server/src/lib.rs`:
- Line 1: The SPDX header at the top of this file uses "LGPL-2.0-only" but other
files in the PR use "LGPL-3.0-only"; confirm which license applies to this crate
and make them consistent by updating the SPDX-License-Identifier line in
crates/daemon-server/src/lib.rs (the top-of-file SPDX comment) to the chosen
license, and also ensure the crate's Cargo.toml and any other license metadata
match the same SPDX identifier across the repository.
In `@crates/events/src/seed.rs`:
- Around line 39-42: The deserialize error message in Seed::try_from_bytes is a
copy/paste bug referencing "EventId requires 32 bytes"; update the map_err
message in the Seed::try_from_bytes function to a correct, clear message (e.g.
"Seed requires 32 bytes" or similar) so failures point to Seed instead of
EventId, leaving the rest of the try_from_bytes logic unchanged.
In `@tests/integration/fns.sh`:
- Around line 283-285: The port lookup assigns the result of ENCLAVE_BIN config
get ctrl_port to local ctrl_port in a single statement which masks the command's
exit status; change this to declare local ctrl_port first and then run
ENCLAVE_BIN config get ctrl_port into ctrl_port so the actual exit code is
preserved, and while doing that quote the name argument (use "$name") when
invoking ENCLAVE_BIN config get ctrl_port and keep the same --config
"$SCRIPT_DIR/enclave.config.yaml" flag so failures surface immediately.
---
Nitpick comments:
In `@crates/cli/src/config.rs`:
- Around line 80-109: The current code in config.rs prints internal structs
using "{:?}" (see calls to log!(out, "{:?}", config.program()) and log!(out,
"nodes: {:?}", config.nodes()) and the no-param branch that logs many fields
with "{:?}"), which exposes unstable Debug output; change these branches to emit
a stable, script-friendly representation — either by serializing the relevant
objects to a well-known structured format (e.g., JSON via serde) or by printing
explicit, documented fields (e.g., iterate config.nodes() and print each node's
name and explicit attributes, and for config.program() print its named fields
rather than Debug). Update the log! invocations for the "nodes", "program", and
None branches to use the chosen structured output (serialization or formatted
field strings) so external consumers get stable, parseable output.
In `@crates/cli/src/main.rs`:
- Around line 74-80: Comment above the conditional still refers to "socket" but
the code checks for daemon availability via maybe_server and
maybe_remote_command; update the comment to accurately describe this branch
(e.g., "If the daemon is available and the command can be parsed as a remote
command") and place it near the conditional that uses maybe_server,
maybe_remote_command and calls run_on_daemon so the comment reflects current
behavior.
In `@crates/cli/src/start.rs`:
- Line 58: References to "socket server" should be renamed to reflect the
daemon-based transport: update the function name launch_socket_server (and any
related identifiers and comments) to launch_daemon_server (or
launch_daemon_transport) and adjust all call sites such as
start_daemon_server(...) and any docs/comments to use "daemon" wording so names
and comments consistently describe the daemon transport implementation.
In `@crates/daemon-server/src/lib.rs`:
- Around line 100-111: The header-reading loop that uses buf_reader.read_line to
parse headers (and sets content_length) has no bound on how many header lines it
will accept; add a max-header limit (e.g., const MAX_HEADERS) and a counter
inside the loop that increments per iteration and returns an error (or closes
the connection) if the count exceeds the limit before hitting the blank line;
update the code around the content_length variable and the loop so it enforces
this limit and returns a clear error when exceeded.
- Around line 113-116: The code allocates body using content_length which allows
unbounded allocation; before creating the Vec check and cap content_length
against a constant MAX_BODY_SIZE (e.g., 1_048_576) and return a clear error (or
respond with 413) if the header exceeds the cap. Then read at most the capped
size (use tokio::io::AsyncReadExt::take on buf_reader or allocate Vec with the
bounded size) into body and convert to String as before; update references to
content_length where used to the validated/capped value to avoid OOM from
malicious/buggy large Content-Length values.
- Around line 38-57: run_on_daemon currently uses reqwest::Client::new() with no
timeout so a hung daemon can block the caller; replace the client creation with
a Client built via
reqwest::Client::builder().timeout(Duration::from_secs(N)).build() (import
std::time::Duration) or set a per-request timeout on the RequestBuilder, and
keep the rest of the logic the same so send().await will error on timeout;
update the error propagation to let the timeout bubble up from run_on_daemon.
In `@crates/zk-prover/src/backend/tests.rs`:
- Around line 64-67: Create a small helper to remove the repeated env-check: add
a function like fn skip_if_e3_custom_bb() -> bool {
std::env::var("E3_CUSTOM_BB").is_ok() } (or fn skip_if_e3_custom_bb() { if
std::env::var("E3_CUSTOM_BB").is_ok() { return; } } depending on preferred
style) in the tests module and replace the three occurrences of if let Some(_) =
env::var("E3_CUSTOM_BB").ok() { return; } with a single call to that helper
(e.g., if skip_if_e3_custom_bb() { return; } or skip_if_e3_custom_bb();) so all
tests use the same guard and duplication is removed.
In `@crates/zk-prover/tests/common/helpers.rs`:
- Line 142: The current call let bb_binary =
BBPath::check(noir_dir.join("bin").join("bb")).unwrap(); should use .expect(...)
to give a clearer panic when E3_CUSTOM_BB points to an invalid path—replace the
.unwrap() with .expect("invalid E3_CUSTOM_BB or bb binary not found: ...") to
include context; additionally, update the manual handling in test_backend() to
reuse BBPath::check(...) (and propagate its expect message) so both bb_binary
initialization and test_backend() consistently validate E3_CUSTOM_BB using
BBPath::check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c91cf65-8a5e-4a81-8d09-747b65fd71f8
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
.github/workflows/ci.ymlCargo.tomlcrates/Dockerfilecrates/ciphernode-builder/src/ciphernode_builder.rscrates/cli/Cargo.tomlcrates/cli/src/cli.rscrates/cli/src/config.rscrates/cli/src/events.rscrates/cli/src/main.rscrates/cli/src/start.rscrates/config/src/app_config.rscrates/crypto/src/sensitive.rscrates/daemon-server/Cargo.tomlcrates/daemon-server/src/lib.rscrates/entrypoint/Cargo.tomlcrates/events/src/enclave_event/accusation_vote.rscrates/events/src/enclave_event/committee_published.rscrates/events/src/enclave_event/plaintext_output_published.rscrates/events/src/enclave_event/proof_failure_accusation.rscrates/events/src/event_id.rscrates/events/src/events.rscrates/events/src/seed.rscrates/evm/src/slashing_manager_sol_writer.rscrates/socket-server/src/lib.rscrates/tests/tests/integration.rscrates/utils-derive/Cargo.tomlcrates/utils-derive/src/lib.rscrates/utils/Cargo.tomlcrates/utils/src/lib.rscrates/utils/src/serde_bytes.rscrates/utils/src/utility_types.rscrates/zk-prover/src/actors/accusation_manager.rscrates/zk-prover/src/backend/tests.rscrates/zk-prover/tests/common/helpers.rsexamples/CRISP/server/Dockerfiletests/integration/base.shtests/integration/fns.sh
💤 Files with no reviewable changes (1)
- crates/socket-server/src/lib.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/events/src/seed.rs`:
- Around line 44-45: The impl block for AsBytesSerde on Seed contains an extra
closing brace; locate the impl AsBytesSerde for Seed block and remove the
redundant `}` so there is only a single closing brace terminating the impl,
ensuring the block compiles correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28dc26df-e7b0-48ff-b444-ef520d4d49b0
📒 Files selected for processing (2)
crates/config/src/app_config.rscrates/events/src/seed.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/config/src/app_config.rs
added 2 commits
April 6, 2026 06:40
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/events/src/seed.rs (1)
39-42: Consider including the received length intry_from_byteserrors.This makes malformed payload debugging faster without changing behavior.
💡 Optional refactor
fn try_from_bytes(bytes: Vec<u8>) -> Result<Self, String> { Ok(Seed( - bytes.try_into().map_err(|_| "Seed requires 32 bytes")?, + bytes + .try_into() + .map_err(|b: Vec<u8>| format!("Seed requires 32 bytes, got {}", b.len()))?, )) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/seed.rs` around lines 39 - 42, The error message in try_from_bytes should include the actual received length to aid debugging: in the Seed::try_from_bytes implementation, capture bytes.len() before the try_into and change the map_err closure to return a String (e.g. "Seed requires 32 bytes, got N") that interpolates the captured length instead of the current constant message; keep the Result<Self, String> signature and behavior otherwise.crates/daemon-server/src/lib.rs (2)
108-110: Malformed Content-Length silently treated as zero.If
Content-Lengthheader exists but contains a non-numeric value,unwrap_or(0)will treat it as zero, causing the body to be ignored. This could lead to confusing behavior during debugging.💡 Optional: Log a warning on parse failure
if let Some(val) = line.to_ascii_lowercase().strip_prefix("content-length:") { - content_length = val.trim().parse().unwrap_or(0); + content_length = val.trim().parse().unwrap_or_else(|_| { + tracing::warn!("Malformed Content-Length header, defaulting to 0"); + 0 + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon-server/src/lib.rs` around lines 108 - 110, The current code parsing the Content-Length header uses val.trim().parse().unwrap_or(0), which silently treats malformed non-numeric values as zero; change it so you only update content_length when parse::<usize>() succeeds and, on Err, emit a warning (e.g., log::warn! or tracing::warn!) including the raw header value rather than silently setting zero—i.e., replace the unwrap_or(0) with a match or if let Ok(n) => content_length = n; Err(e) => log a warning referencing the raw val and parsing error, leaving content_length unchanged or handling the error explicitly.
44-50: Consider adding a timeout to the POST request.Unlike
connect_daemonwhich has a 1-second timeout, this POST request has no timeout configured. If the daemon hangs while processing, the CLI will block indefinitely.💡 Suggested improvement
let client = reqwest::Client::new(); let resp = client .post(&url) .json(&cli) + .timeout(std::time::Duration::from_secs(30)) .send() .await? .error_for_status()?;Adjust the timeout duration based on expected maximum command execution time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/daemon-server/src/lib.rs` around lines 44 - 50, The POST request created with reqwest::Client::new() (variables client and resp) has no timeout and can block indefinitely; modify the code to use a client with a timeout (e.g., via reqwest::Client::builder().timeout(Duration::from_secs(…)) or wrap the send in tokio::time::timeout) similar to connect_daemon’s 1-second behavior, choose an appropriate Duration for command execution, and ensure the send().await is aborted/returns an error on timeout so resp.error_for_status() isn’t awaited indefinitely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/daemon-server/src/lib.rs`:
- Around line 108-110: The current code parsing the Content-Length header uses
val.trim().parse().unwrap_or(0), which silently treats malformed non-numeric
values as zero; change it so you only update content_length when
parse::<usize>() succeeds and, on Err, emit a warning (e.g., log::warn! or
tracing::warn!) including the raw header value rather than silently setting
zero—i.e., replace the unwrap_or(0) with a match or if let Ok(n) =>
content_length = n; Err(e) => log a warning referencing the raw val and parsing
error, leaving content_length unchanged or handling the error explicitly.
- Around line 44-50: The POST request created with reqwest::Client::new()
(variables client and resp) has no timeout and can block indefinitely; modify
the code to use a client with a timeout (e.g., via
reqwest::Client::builder().timeout(Duration::from_secs(…)) or wrap the send in
tokio::time::timeout) similar to connect_daemon’s 1-second behavior, choose an
appropriate Duration for command execution, and ensure the send().await is
aborted/returns an error on timeout so resp.error_for_status() isn’t awaited
indefinitely.
In `@crates/events/src/seed.rs`:
- Around line 39-42: The error message in try_from_bytes should include the
actual received length to aid debugging: in the Seed::try_from_bytes
implementation, capture bytes.len() before the try_into and change the map_err
closure to return a String (e.g. "Seed requires 32 bytes, got N") that
interpolates the captured length instead of the current constant message; keep
the Result<Self, String> signature and behavior otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e210de8f-fb5b-4752-801f-704582929149
📒 Files selected for processing (2)
crates/daemon-server/src/lib.rscrates/events/src/seed.rs
added 3 commits
April 6, 2026 07:16
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/events/src/cursor.rs (1)
15-18:⚠️ Potential issue | 🟠 Major
SeqCursor::Nextis off-by-one for inclusive sequence reads.
compute_seq_cursorcurrently returns the last seen sequence, which causes duplicate events on the next page when consumed assince(inclusive read semantics). Also,limit == 0currently yieldsNext(0)for an empty page.💡 Proposed fix
pub fn compute_seq_cursor<T: EventContextSeq>(events: &[T], limit: usize) -> SeqCursor { - if events.len() == limit { - let last_seq = events.last().map(|e| e.seq()).unwrap_or(0); - SeqCursor::Next(last_seq) - } else { - SeqCursor::Done - } + if limit == 0 || events.len() < limit { + return SeqCursor::Done; + } + + let next_seq = events + .last() + .map(|e| e.seq().saturating_add(1)) + .unwrap_or(0); + SeqCursor::Next(next_seq) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/cursor.rs` around lines 15 - 18, compute_seq_cursor currently returns the last seen sequence directly, causing off-by-one duplicates and producing Next(0) for an empty page; update the branch in compute_seq_cursor so that when events.len() == limit and events.last() is Some(last_seq) you return SeqCursor::Next(last_seq + 1) (use checked_add if needed), and if there is no last element (empty slice / limit == 0) return the appropriate terminal variant (e.g., SeqCursor::Done) instead of Next(0); reference compute_seq_cursor and SeqCursor::Next in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/events/src/cursor.rs`:
- Around line 15-18: compute_seq_cursor currently returns the last seen sequence
directly, causing off-by-one duplicates and producing Next(0) for an empty page;
update the branch in compute_seq_cursor so that when events.len() == limit and
events.last() is Some(last_seq) you return SeqCursor::Next(last_seq + 1) (use
checked_add if needed), and if there is no last element (empty slice / limit ==
0) return the appropriate terminal variant (e.g., SeqCursor::Done) instead of
Next(0); reference compute_seq_cursor and SeqCursor::Next in the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22976c4a-2741-404f-bc8c-3a5bcf81d0be
📒 Files selected for processing (5)
crates/cli/src/events.rscrates/events/src/cursor.rscrates/events/src/eventstore.rscrates/events/src/lib.rscrates/net/src/net_event_batch.rs
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.
Closes: #1468
In order to better test this I added new config command to extract relevant config information:
Can also pass
--nameand other cli args:cd tests/integration ../../target/debug/enclave --config ./enclave.config.yaml --name cn4 config get ctrl_port 50504../../target/debug/enclave --config ./enclave.config.yaml --name cn4 events query {"payload":{"HistoricalEvmSyncStart":{"evm_config":{"config":{"31337":{"deploy_block":1}}}}},"ctx":{"id":"0x8b26b731e9e24fb57dbe4da4cbe61c807de82021329a6e11837c91ae34b90217","causation_id":"0x8b26b731e9e24fb57dbe4da4cbe61c807de82021329a6e11837c91ae34b90217","origin_id":"0x8b26b731e9e24fb57dbe4da4cbe61c807de82021329a6e11837c91ae34b90217","seq":1,"ts":32750184476743752752765064395235073,"aggregate_id":0,"block":null,"source":"Local"}} {"payload":{"NetReady":{"correlation_id":{"id":4}}},"ctx":{"id":"0x275855bf297a99c1b8f8472bf8fe2afae9a1f349f2036d2071dff43f3087a8f4","causation_id":"0x275855bf297a99c1b8f8472bf8fe2afae9a1f349f2036d2071dff43f3087a8f4","origin_id":"0x275855bf297a99c1b8f8472bf8fe2afae9a1f349f2036d2071dff43f3087a8f4","seq":2,"ts":32750184476964800087000325952249601,"aggregate_id":0,"block":null,"source":"Local"}} {"payload":{"HistoricalNetSyncStart":{"since":{"0":0}}},"ctx":{"id":"0x10ef59e77c2543b3ce7504266a273a5defa433dfeae367908af75da00caa688a","causation_id":"0x10ef59e77c2543b3ce7504266a273a5defa433dfeae367908af75da00caa688a","origin_id":"0x10ef59e77c2543b3ce7504266a273a5defa433dfeae367908af75da00caa688a","seq":3,"ts":32750184477568156192163217966505729,"aggregate_id":0,"block":null,"source":"Local"}} {"payload":{"HistoricalNetSyncEventsReceived":{"events":[{"payload":{"HistoricalNetSyncEventsReceived":{"events":[]}},"ctx":{"id":"0x457f64c5fdcab36b391a2fcdc43cfbe1015786fc362a4ed04db272246beceb8e","causation_id":"0x457f64c5fdcab36b391a2fcdc43cfbe1015786fc362a4ed04db272246beceb8e","origin_id":"0x10ef59e77c2543b3ce7504266a273a5defa433dfeae367908af75da00caa688a","seq":null,"ts":32750184477313259082552699411169349,"aggregate_id":0,"block":null,"source":"Net"}}]}},"ctx":{"id":"0x8ef61f12fe08aa0b17a3044d62c8854feb02b8daf63f9fdc9d4826669caf29c0","causation_id":"0x10ef59e77c2543b3ce7504266a273a5defa433dfeae367908af75da00caa688a","origin_id":"0x10ef59e77c2543b3ce7504266a273a5defa433dfeae367908af75da00caa688a","seq":4,"ts":32750184477749690600592593663958785,"aggregate_id":0,"block":null,"source":"Net"}} {"payload":{"EffectsEnabled":{"correlation_id":{"id":47}}},"ctx":{"id":"0xc30815bbd741b9a19b7d8870ac3461230676f3911a2bd28f7265ac21f609ed88","causation_id":"0xc30815bbd741b9a19b7d8870ac3461230676f3911a2bd28f7265ac21f609ed88","origin_id":"0xc30815bbd741b9a19b7d8870ac3461230676f3911a2bd28f7265ac21f609ed88","seq":5,"ts":32750184477780828704589015387086593,"aggregate_id":0,"block":null,"source":"Local"}} {"payload":{"HistoricalNetSyncEventsReceived":{"events":[]}},"ctx":{"id":"0x457f64c5fdcab36b391a2fcdc43cfbe1015786fc362a4ed04db272246beceb8e","causation_id":"0x457f64c5fdcab36b391a2fcdc43cfbe1015786fc362a4ed04db272246beceb8e","origin_id":"0x10ef59e77c2543b3ce7504266a273a5defa433dfeae367908af75da00caa688a","seq":6,"ts":32750184477313259082552699411169349,"aggregate_id":0,"block":null,"source":"Net"}} {"payload":{"ConfigurationUpdated":{"parameter":"ticketPrice","old_value":"0x0","new_value":"0x989680","chain_id":31337}},"ctx":{"id":"0x8797d73ea66a3ec4b5f741d87331fa155006d9f4055bd784aac2e9f30f7ce64f","causation_id":"0x8797d73ea66a3ec4b5f741d87331fa155006d9f4055bd784aac2e9f30f7ce64f","origin_id":"0x8797d73ea66a3ec4b5f741d87331fa155006d9f4055bd784aac2e9f30f7ce64f","seq":7,"ts":32750184477429824058354470038937345,"aggregate_id":0,"block":15,"source":"Evm"}} {"payload":{"ConfigurationUpdated":{"parameter":"licenseRequiredBond","old_value":"0x0","new_value":"0x56bc75e2d63100000","chain_id":31337}},"ctx":{"id":"0xf72e8150a2c853004a88e5adbb6db94072e5c9cb7254f55b31c642b390b1c6e2","causation_id":"0xf72e8150a2c853004a88e5adbb6db94072e5c9cb7254f55b31c642b390b1c6e2","origin_id":"0xf72e8150a2c853004a88e5adbb6db94072e5c9cb7254f55b31c642b390b1c6e2","seq":8,"ts":32750184477430986203231113740689153,"aggregate_id":0,"block":15,"source":"Evm"}} {"payload":{"ConfigurationUpdated":{"parameter":"minTicketBalance","old_value":"0x0","new_value":"0x1","chain_id":31337}},"ctx":{"id":"0x96244bcc590d168617d112e5d697fe88e17e5dc748e2fcf535d37d1166032306","causation_id":"0x96244bcc590d168617d112e5d697fe88e17e5dc748e2fcf535d37d1166032306","origin_id":"0x96244bcc590d168617d112e5d697fe88e17e5dc748e2fcf535d37d1166032306","seq":9,"ts":32750184477431336691368514222169857,"aggregate_id":0,"block":15,"source":"Evm"}} {"payload":{"ConfigurationUpdated":{"parameter":"exitDelay","old_value":"0x0","new_value":"0x93a80","chain_id":31337}},"ctx":{"id":"0xf292c657ad25ee1fbab57742659d1b9a2831a7990dcad9d505b00177161448a9","causation_id":"0xf292c657ad25ee1fbab57742659d1b9a2831a7990dcad9d505b00177161448a9","origin_id":"0xf292c657ad25ee1fbab57742659d1b9a2831a7990dcad9d505b00177161448a9","seq":10,"ts":32750184477431668732761840994098945,"aggregate_id":0,"block":15,"source":"Evm"}} {"Next":10}This is available as a REST API when the node is running
AsBytesSerdeandBytesSerdetrait and macro combination so that bytes like values such asArcBytesSeedandEventIdserialize in concise human readable ways for json but compact ways for binary.Vec<u8>within events withArcBytesto take advantage of serialization memory and debugging advantagesenclave --name ag config get ctrl_portand other config params to aid bash script configuration.Summary by CodeRabbit
New Features
Improvements
Tests / Integration