Add comprehensive test coverage for RPC handlers, database operations, and timing utilities#11
Add comprehensive test coverage for RPC handlers, database operations, and timing utilities#11sjustintaylor merged 17 commits intomainfrom
Conversation
WalkthroughMake BeaconApiClient generic over an injectable HttpClient and add a ReqwestClient default constructor; propagate the typed client across services, context, server, RPC, and tests. Add extensive tests and mocks, adjust DB tests to use DATABASE_URL, update timing/test helpers, refine constraint submission timing/filtering, and add new dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Beacon as BeaconApiClient<ReqwestClient>
participant Http as HttpClient
participant Primary as Primary Endpoint
participant Fallback as Fallback Endpoint
Caller->>Beacon: get_proposer_duties(epoch)
Beacon->>Http: GET primary URL
Http-->>Beacon: HttpResponse(status, body)
alt success (2xx + parse)
Beacon-->>Caller: ProposerDutiesResponse
else fail
Note right of Beacon: try fallback endpoints sequentially
Beacon->>Http: GET fallback URL
Http-->>Beacon: HttpResponse(status, body)
alt success
Beacon-->>Caller: ProposerDutiesResponse
else all failed
Beacon-->>Caller: Error (aggregated)
end
end
sequenceDiagram
autonumber
participant Scheduler
participant Poller as DelegationPollingService
participant Beacon as BeaconApiClient<ReqwestClient>
participant DB as Database
Scheduler->>Poller: poll_delegations()
Poller->>Beacon: get_proposer_for_slot(slot)
Beacon-->>Poller: Option<ValidatorDuty> / Error
alt proposer found
Poller->>DB: fetch/update delegations (filter by committer)
DB-->>Poller: OK
Poller-->>Scheduler: done
else not found / error
Poller-->>Scheduler: none/error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/rpc/handlers.rs (1)
418-489: Consider usingtokio::sync::Mutexfor async methods.The mock uses
std::sync::Mutex(blocking lock) inasyncmethods, which is an anti-pattern in async Rust. While the current implementation works because there are no actualawaitpoints, it's not idiomatic. Consider either:Solution 1 (preferred): Use
tokio::sync::Mutexfor proper async semantics.Solution 2: Remove the
asynckeyword and returnReadyfutures if you need to match async trait signatures without actual async operations.Since this is test-only code and functionally correct, this is a lower-priority improvement that enhances code quality and avoids potential confusion.
Apply this diff to use
tokio::sync::Mutex:use anyhow::Result; - use std::sync::Mutex as StdMutex; + use tokio::sync::Mutex as TokioMutex; /// Mock database context for testing error scenarios struct MockDatabaseContext { /// Control whether commitment_exists should return true - simulate_exists: Arc<StdMutex<bool>>, + simulate_exists: Arc<TokioMutex<bool>>, /// Control whether save should fail with conflict - simulate_conflict: Arc<StdMutex<bool>>, + simulate_conflict: Arc<TokioMutex<bool>>, /// Control whether operations should fail with database error - simulate_error: Arc<StdMutex<bool>>, + simulate_error: Arc<TokioMutex<bool>>, /// Store commitments in memory for testing - commitments: Arc<StdMutex<std::collections::HashMap<String, crate::types::SignedCommitment>>>, + commitments: Arc<TokioMutex<std::collections::HashMap<String, crate::types::SignedCommitment>>>, } impl MockDatabaseContext { fn new() -> Self { Self { - simulate_exists: Arc::new(StdMutex::new(false)), - simulate_conflict: Arc::new(StdMutex::new(false)), - simulate_error: Arc::new(StdMutex::new(false)), - commitments: Arc::new(StdMutex::new(std::collections::HashMap::new())), + simulate_exists: Arc::new(TokioMutex::new(false)), + simulate_conflict: Arc::new(TokioMutex::new(false)), + simulate_error: Arc::new(TokioMutex::new(false)), + commitments: Arc::new(TokioMutex::new(std::collections::HashMap::new())), } } fn set_simulate_exists(&self, value: bool) { - *self.simulate_exists.lock().unwrap() = value; + *self.simulate_exists.blocking_lock() = value; } fn set_simulate_conflict(&self, value: bool) { - *self.simulate_conflict.lock().unwrap() = value; + *self.simulate_conflict.blocking_lock() = value; } fn set_simulate_error(&self, value: bool) { - *self.simulate_error.lock().unwrap() = value; + *self.simulate_error.blocking_lock() = value; } async fn commitment_exists(&self, request_hash: &str) -> Result<bool> { - if *self.simulate_error.lock().unwrap() { + if *self.simulate_error.lock().await { return Err(anyhow::anyhow!("Mock database error")); } - if *self.simulate_exists.lock().unwrap() { + if *self.simulate_exists.lock().await { return Ok(true); } - Ok(self.commitments.lock().unwrap().contains_key(request_hash)) + Ok(self.commitments.lock().await.contains_key(request_hash)) } async fn save_commitment( &self, signed_commitment: &crate::types::SignedCommitment, ) -> Result<Option<uuid::Uuid>> { - if *self.simulate_error.lock().unwrap() { + if *self.simulate_error.lock().await { return Err(anyhow::anyhow!("Mock database error")); } - if *self.simulate_conflict.lock().unwrap() { + if *self.simulate_conflict.lock().await { return Ok(None); // Simulate ON CONFLICT } let hash = signed_commitment.commitment.request_hash.clone(); - if self.commitments.lock().unwrap().contains_key(&hash) { + if self.commitments.lock().await.contains_key(&hash) { return Ok(None); // Already exists } - self.commitments.lock().unwrap().insert(hash, signed_commitment.clone()); + self.commitments.lock().await.insert(hash, signed_commitment.clone()); Ok(Some(uuid::Uuid::new_v4())) } async fn get_commitment_by_hash( &self, request_hash: &str, ) -> Result<Option<crate::types::SignedCommitment>> { - if *self.simulate_error.lock().unwrap() { + if *self.simulate_error.lock().await { return Err(anyhow::anyhow!("Mock database error")); } - Ok(self.commitments.lock().unwrap().get(request_hash).cloned()) + Ok(self.commitments.lock().await.get(request_hash).cloned()) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/rpc/handlers.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with rustfmt using hard tabs and 120-character width
Place unit tests within each module using #[cfg(test)] and #[test]
Files:
src/rpc/handlers.rs
src/rpc/{handlers.rs,methods.rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Support only Hoodi chain (chain_id 560048) and inclusion commitments (type 1) in RPC exposure (e.g., slots/catalog)
Files:
src/rpc/handlers.rs
src/rpc/handlers.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/rpc/handlers.rs: In commitment_request handler, enforce delegation-first validation before any signing
Slots endpoint must provide a 64-future-slot service catalog (2 epochs lookahead) using real-time beacon timing
Ensure the slots endpoint performs no database access (computational-only)
Place integration tests for RPC flows in handlers.rs test modules
Files:
src/rpc/handlers.rs
src/rpc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Return proper JSON-RPC error codes from RPC handlers
Files:
src/rpc/handlers.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to src/rpc/handlers.rs : Place integration tests for RPC flows in handlers.rs test modules
📚 Learning: 2025-10-14T01:24:29.560Z
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to src/rpc/handlers.rs : Place integration tests for RPC flows in handlers.rs test modules
Applied to files:
src/rpc/handlers.rs
🧬 Code graph analysis (1)
src/rpc/handlers.rs (7)
src/db/mod.rs (4)
new(111-113)commitment_exists(180-182)save_commitment(149-154)get_commitment_by_hash(164-166)src/crypto/bls.rs (2)
new(67-72)parse_private_key(244-253)src/types/payload.rs (6)
new(189-191)new(264-266)encode_inclusion_payload(162-165)slot(201-203)slot(276-278)extract_slot(63-69)src/db/operations.rs (3)
commitment_exists(147-154)save_commitment(27-88)get_commitment_by_hash(101-137)src/testing/fixtures.rs (1)
create_inclusion_commitment_request(19-35)src/crypto/mod.rs (3)
generate_request_hash(43-52)sign_commitment(111-137)parse_private_key(190-193)src/utils/address.rs (1)
normalize_address(8-12)
⏰ 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). (1)
- GitHub Check: Code Coverage
🔇 Additional comments (2)
src/rpc/handlers.rs (2)
415-416: LGTM! Clean import organization.The
anyhow::Resultimport aligns with the test-only mock implementation, and theStdMutexalias clearly distinguishes the synchronous mutex from async alternatives.
886-1209: Excellent comprehensive test coverage for error paths!The
error_path_testsmodule provides thorough coverage of failure scenarios, including:
- Invalid commitment types (0, 99, unsupported types)
- Malformed/empty payloads
- Whitelist validation and address normalization edge cases
- Signing key lookup and case-insensitive matching
- Request hash uniqueness and consistency
- Slots lookahead calculations
- Mock database operations (exists, conflict, error simulation)
This aligns well with the coding guideline to place integration tests in
handlers.rstest modules and significantly improves the robustness of the codebase.Based on learnings.
📊 Test Coverage ReportOverall Coverage: 56.79% 📁 Coverage by File
📈 Summary
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/services/constraint_submission.rs (3)
296-349: Enforce the 8-second slot deadline when submitting constraintsSubmission in process_delegation_constraints isn’t bounded to the per-slot deadline; a slow call can overrun the window. Compute remaining budget to BeaconTiming’s deadline and wrap the submission in a timeout.
Apply this diff:
// Create SignedConstraints object let signed_constraints = SignedConstraints { message: constraints_message, signature: BlsSignature(signature_bytes) }; - // Submit to the constraints API - let submission_response = constraints_client - .submit_constraints(&signed_constraints) - .await - .context("Failed to submit constraints to API")?; + // Enforce 8s-from-slot-start deadline + let deadline_secs = + BeaconTiming::constraint_deadline_for_slot(config.beacon_api.genesis_time, slot); + let now_secs = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map_err(|_| anyhow::anyhow!("System clock error"))? + .as_secs(); + if now_secs >= deadline_secs { + return Err(anyhow::anyhow!( + "Constraint submission deadline elapsed for slot {}", + slot + )); + } + let remaining = Duration::from_secs(deadline_secs - now_secs); + + // Submit to the constraints API within remaining budget + let submission_response = timeout(remaining, constraints_client.submit_constraints(&signed_constraints)) + .await + .context("Constraint submission timed out")? + .context("Failed to submit constraints to API")?;As per coding guidelines
456-497: Filter commitments by committer to avoid submitting/marking wrong datacreate_constraints_from_commitments collects all unprocessed commitments for the slot, regardless of committer. This risks submitting constraints for other committers and marking them processed. Pass the delegation’s committer and filter.
Apply these diffs:
-async fn create_constraints_from_commitments(db_pool: &PgPool, slot: u64) -> Result<(Vec<Constraint>, Vec<String>)> { +async fn create_constraints_from_commitments( + db_pool: &PgPool, + slot: u64, + committer_address: &str, +) -> Result<(Vec<Constraint>, Vec<String>)> { @@ - for signed_commitment in &commitments { + for signed_commitment in &commitments { let commitment = &signed_commitment.commitment; // Only process inclusion commitments (type 1) - if commitment.commitment_type == 1 { + if commitment.commitment_type == 1 && commitment.slasher == committer_address { constraints.push(Constraint::from_inclusion_commitment(commitment.payload.clone())); processed_hashes.push(commitment.request_hash.clone()); } else { warn!("Skipping commitment with unsupported type {} for slot {}", commitment.commitment_type, slot); // Don't add to processed_hashes - we only track successfully converted commitments } }And update the call site in process_delegation_constraints:
- let (constraints, processed_hashes) = create_constraints_from_commitments(_db_pool, slot).await?; + let (constraints, processed_hashes) = + create_constraints_from_commitments(_db_pool, slot, &delegation.message.committer).await?;As per coding guidelines
427-440: Use dynamic time budget (to 8s deadline) instead of fixed 5sFixed 5s can either overshoot near the deadline or be overly conservative. Compute remaining time to slot’s 8s deadline.
Apply this diff:
- // Submit with timeout to ensure we don't exceed the 8-second deadline - let submission_result = timeout( - Duration::from_secs(5), // Give ourselves 5 seconds to submit - constraints_client.submit_constraints(&signed_constraints), - ) - .await - .context("Constraint submission timed out")? - .context("Failed to submit constraint to API")?; + // Submit with timeout bounded by the 8-second deadline from slot start + let deadline_secs = + BeaconTiming::constraint_deadline_for_slot(config.beacon_api.genesis_time, slot); + let now_secs = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map_err(|_| anyhow::anyhow!("System clock error"))? + .as_secs(); + if now_secs >= deadline_secs { + return Err(anyhow::anyhow!( + "Constraint submission deadline elapsed for slot {}", + slot + )); + } + let remaining = Duration::from_secs(deadline_secs - now_secs); + let submission_result = timeout(remaining, constraints_client.submit_constraints(&signed_constraints)) + .await + .context("Constraint submission timed out")? + .context("Failed to submit constraint to API")?;As per coding guidelines
🧹 Nitpick comments (11)
src/testing/mod.rs (1)
2-2: Expose constraint_fixtures behind the same tarpaulin gate as its contentsConsider gating the module itself with #[cfg(not(tarpaulin_include))] to match the inner items and avoid an empty module during coverage runs.
src/testing/helpers.rs (4)
55-74: Avoid real network clients in test helpersThis constructs real Reth/Beacon clients; tests can become flaky/offline. Prefer mocks (already available in crate::testing::mocks) or inject traits to decouple.
229-262: Use BeaconTiming and safe arithmetic for deadline calculationReplace manual math with BeaconTiming and avoid overflow. Also reduces duplication.
Apply this diff:
- let slot_start_time = genesis_time + (commitment_slot * 12); - let submission_deadline = slot_start_time + 8; // 8-second deadline + let submission_deadline = + crate::types::beacon::BeaconTiming::constraint_deadline_for_slot(genesis_time, commitment_slot); - if submission_unix > submission_deadline { + if submission_unix > submission_deadline { return Err(anyhow::anyhow!( "Constraint submission too late: submitted at {}, deadline was {}", submission_unix, submission_deadline )); }Based on learnings
285-296: Derive slot via BeaconTiming, not raw time/12Aligns all timing with chain genesis and avoids drift in tests.
Apply this diff:
- let slot = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_secs() / 12; // Current slot + let slot = crate::types::beacon::BeaconTiming::current_slot_estimate( + context.config.beacon_api.genesis_time, + );Based on learnings
473-483: Clamp percentile input to [0, 100]Prevents surprising indices for out-of-range inputs.
Apply this diff:
pub fn percentile_response_time(&self, percentile: f64) -> Duration { if self.response_times.is_empty() { return Duration::default(); } let mut sorted_times = self.response_times.clone(); sorted_times.sort(); - let index = (sorted_times.len() as f64 * percentile / 100.0) as usize; + let p = percentile.clamp(0.0, 100.0); + let index = (sorted_times.len() as f64 * p / 100.0) as usize; sorted_times.get(index.min(sorted_times.len() - 1)).copied().unwrap_or_default() }src/services/constraint_submission.rs (1)
211-223: Optional: process multiple slots concurrentlyTo reduce latency under load, consider spawning per-slot tasks (bounded) within the current window instead of sequential awaits. Keep each submission wrapped by the deadline budget.
src/testing/fixtures.rs (3)
117-123: Prefer BeaconTiming for slot computation in scenariosUsing SystemTime/12 ignores genesis offset. Consider BeaconTiming::current_slot_estimate with a provided genesis_time for consistency.
Based on learnings
322-336: Base timing helpers on BeaconTimingCreate slots using BeaconTiming to reflect chain timing relative to genesis.
Apply this diff:
- pub fn create_timing_test_slots() -> Vec<u64> { - let current_time = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_secs(); - - let current_slot = current_time / 12; + pub fn create_timing_test_slots() -> Vec<u64> { + let genesis_time = 1606824023; // or pass as a parameter if preferred + let current_slot = crate::types::beacon::BeaconTiming::current_slot_estimate(genesis_time);Based on learnings
353-361: Delegate window checks to BeaconTimingMirror production logic for acceptance/rejection.
Apply this diff:
- pub fn is_within_submission_window(_genesis_time: u64, slot: u64) -> bool { - let current_time = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_secs(); - - let current_slot = current_time / 12; - let slot_diff = slot.abs_diff(current_slot); - - // Allow slots within 10 slots of current (reasonable constraint submission window) - slot_diff <= 10 - } + pub fn is_within_submission_window(genesis_time: u64, slot: u64) -> bool { + crate::types::beacon::BeaconTiming::is_within_constraint_window(genesis_time, slot) + }Based on learnings
src/testing/constraint_fixtures.rs (2)
24-32: Generate full-width hashes and consider deterministic RNGrequest_hash is formatted as 64 hex chars from a u64 (8 bytes). Prefer 32 bytes to better match typical 256-bit hashes; optionally allow seeding for deterministic tests.
Apply this diff:
- let commitment = Commitment { - request_hash: format!("0x{:064x}", rand::random::<u64>()), + let request_hash = { + let bytes: [u8; 32] = rand::random(); + format!("0x{}", hex::encode(bytes)) + }; + let commitment = Commitment { + request_hash, commitment_type, payload, slasher: slasher.to_string(), };Note: requires hex. If not present, we can switch to a crate-local hex helper.
41-47: Unused slot parameter_slot is unused in insert_test_delegation; either drop it or use it for test assertions to reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/services/constraint_submission.rs(1 hunks)src/testing/constraint_fixtures.rs(1 hunks)src/testing/fixtures.rs(5 hunks)src/testing/helpers.rs(6 hunks)src/testing/mocks.rs(15 hunks)src/testing/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with rustfmt using hard tabs and 120-character width
Place unit tests within each module using #[cfg(test)] and #[test]
Files:
src/testing/mod.rssrc/testing/helpers.rssrc/testing/fixtures.rssrc/services/constraint_submission.rssrc/testing/constraint_fixtures.rssrc/testing/mocks.rs
src/testing/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain complete mock service implementations under src/testing
Files:
src/testing/mod.rssrc/testing/helpers.rssrc/testing/fixtures.rssrc/testing/constraint_fixtures.rssrc/testing/mocks.rs
src/services/constraint_submission.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/services/constraint_submission.rs: Ensure constraints are submitted within 8 seconds of slot start
Use a scheduler (tokio-cron-scheduler) for timing-critical constraint submission
Files:
src/services/constraint_submission.rs
🧠 Learnings (1)
📚 Learning: 2025-10-14T01:24:29.560Z
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to src/types/beacon.rs : Base timing operations on BeaconTiming::current_slot_estimate()
Applied to files:
src/testing/fixtures.rs
🧬 Code graph analysis (2)
src/services/constraint_submission.rs (3)
src/testing/constraint_fixtures.rs (5)
create_test_commitment_with_slot(15-32)insert_test_commitment(56-59)insert_test_delegation(41-47)create_test_commitments_for_slot(63-65)create_test_commitment_with_type(69-71)src/testing/mocks.rs (5)
create_test_bls_keypair(531-539)new(42-44)new(129-137)new(323-330)create_test_config(485-521)src/testing/fixtures.rs (1)
create_signed_delegation(44-61)
src/testing/constraint_fixtures.rs (1)
src/types/payload.rs (1)
encode_inclusion_payload(162-165)
⏰ 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). (1)
- GitHub Check: Code Coverage
sjustintaylor
force-pushed
the
test-coverage
branch
from
a03bd37 to
2f12f3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/rpc/handlers.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with rustfmt using hard tabs and 120-character width
Place unit tests within each module using #[cfg(test)] and #[test]
Files:
src/rpc/handlers.rs
src/rpc/{handlers.rs,methods.rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Support only Hoodi chain (chain_id 560048) and inclusion commitments (type 1) in RPC exposure (e.g., slots/catalog)
Files:
src/rpc/handlers.rs
src/rpc/handlers.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/rpc/handlers.rs: In commitment_request handler, enforce delegation-first validation before any signing
Slots endpoint must provide a 64-future-slot service catalog (2 epochs lookahead) using real-time beacon timing
Ensure the slots endpoint performs no database access (computational-only)
Place integration tests for RPC flows in handlers.rs test modules
Files:
src/rpc/handlers.rs
src/rpc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Return proper JSON-RPC error codes from RPC handlers
Files:
src/rpc/handlers.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to src/rpc/handlers.rs : Place integration tests for RPC flows in handlers.rs test modules
📚 Learning: 2025-10-14T01:24:29.560Z
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to src/rpc/handlers.rs : Place integration tests for RPC flows in handlers.rs test modules
Applied to files:
src/rpc/handlers.rs
🧬 Code graph analysis (1)
src/rpc/handlers.rs (6)
src/db/mod.rs (4)
new(111-113)commitment_exists(180-182)save_commitment(149-154)get_commitment_by_hash(164-166)src/types/payload.rs (6)
new(189-191)new(264-266)encode_inclusion_payload(162-165)slot(201-203)slot(276-278)extract_slot(63-69)src/db/operations.rs (3)
commitment_exists(147-154)save_commitment(27-88)get_commitment_by_hash(101-137)src/testing/fixtures.rs (1)
create_inclusion_commitment_request(19-35)src/crypto/mod.rs (3)
generate_request_hash(43-52)sign_commitment(111-137)parse_private_key(190-193)src/utils/address.rs (1)
normalize_address(8-12)
⏰ 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). (1)
- GitHub Check: Code Coverage
🔇 Additional comments (5)
src/rpc/handlers.rs (5)
415-417: LGTM! Clean test imports.The imports are minimal and appropriate for the test module. The
StdMutexalias avoids potential naming conflicts with other mutex implementations.
1122-1133: Test verifies mock behavior but not handler integration.This test confirms that
MockDatabaseContext::commitment_exists()respects thesimulate_existsflag. While useful for validating the mock itself, it doesn't test howcommitment_request_handlerbehaves when the exists check is triggered (line 195).Consider adding a corresponding handler test that uses this mock behavior:
#[tokio::test] async fn test_commitment_request_handler_duplicate_early_rejection() { let mock_db = MockDatabaseContext::new(); mock_db.set_simulate_exists(true); // Simulate duplicate exists let context = create_test_context_with_mock_db(mock_db); let request = TestFixtures::create_inclusion_commitment_request(12345, &context.config.validation.slasher_whitelist[0]); // Call handler and verify it returns InvalidRequest at line 200 }
1136-1146: Test verifies mock error simulation but not handler error handling.This test confirms that the mock can simulate DB errors. However, the handler's error handling at lines 195-198 and 233-236 remains untested.
Add a handler test that exercises the error path:
#[tokio::test] async fn test_commitment_request_handler_db_error_on_exists_check() { let mock_db = MockDatabaseContext::new(); mock_db.set_simulate_error(true); let context = create_test_context_with_mock_db(mock_db); // Handler should return InternalError when exists check fails (line 197) }
1149-1178: Test verifies mock save/retrieve but not handler save conflict detection.This test confirms the mock can store and retrieve commitments. The handler's ON CONFLICT detection at lines 238-243 is not tested.
Add a handler test for the defensive duplicate check:
#[tokio::test] async fn test_commitment_request_handler_save_returns_none() { let mock_db = MockDatabaseContext::new(); // First save succeeds, then enable conflict for subsequent saves let context = create_test_context_with_mock_db(mock_db); // Test that handler returns InvalidRequest when save_result.is_none() at line 240 }
1181-1208: Test verifies conflict simulation but not handler's defensive check.This test confirms the mock simulates ON CONFLICT (returns
Nonefromsave_commitment). The handler's defensive check at lines 240-243 that logs "race condition" is not exercised.Add a handler test for the race condition path:
#[tokio::test] async fn test_commitment_request_handler_detects_save_conflict() { let mock_db = MockDatabaseContext::new(); mock_db.set_simulate_conflict(true); let context = create_test_context_with_mock_db(mock_db); // Handler should log warning at line 241 and return InvalidRequest at line 242 }
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/testing/helpers.rs (1)
229-262: Fix potential u64 overflow in deadline computationAddition/multiplication on u64 can overflow (debug panic or wrap in release), producing wrong deadlines. Use checked_mul/checked_add and return errors.
- let slot_start_time = genesis_time + (commitment_slot * 12); - let submission_deadline = slot_start_time + 8; // 8-second deadline + let slot_offset = commitment_slot + .checked_mul(12) + .ok_or_else(|| anyhow::anyhow!("Slot offset overflow"))?; + let slot_start_time = genesis_time + .checked_add(slot_offset) + .ok_or_else(|| anyhow::anyhow!("Slot start time overflow"))?; + let submission_deadline = slot_start_time + .checked_add(8) + .ok_or_else(|| anyhow::anyhow!("Submission deadline overflow"))?; // 8-second deadlinesrc/rpc/handlers.rs (1)
315-323: Enforce exactly 64-slot (2 epochs) lookahead in slots endpointGuideline requires a 64-future-slot catalog. Using config may diverge. Compute
2 * SLOTS_PER_EPOCHexplicitly.As per coding guidelines
- let lookahead_slots = _context.config.delegation.lookahead_epochs * timing::SLOTS_PER_EPOCH; + let lookahead_slots = 2 * timing::SLOTS_PER_EPOCH; // Fixed 2-epoch (64 slots) lookahead
♻️ Duplicate comments (3)
src/db/delegation_ops.rs (2)
811-813: Same issue: test silently skips on connection failure.This test has the same silent-skip pattern as
test_delegation_crud_operations. Apply the same fix as suggested in the previous comment.
843-845: Same issue: test silently skips on connection failure.This test has the same silent-skip pattern. Apply the same fix as suggested for
test_delegation_crud_operations.src/rpc/handlers.rs (1)
421-492: Mock DB is not injected into handler tests; error paths untestedThis mirrors earlier feedback: tests define a
MockDatabaseContextbut cannot inject it intoRpcContext, so handler error paths aren’t exercised.As per coding guidelines
🧹 Nitpick comments (20)
src/db/delegation_ops.rs (1)
757-871: Consider test isolation and cleanup for robustness.These integration tests use fixed test data (e.g., slot 12345, specific public keys) and don't include visible cleanup logic. While acceptable for manually-run
#[ignore]d tests, this could cause issues:
- Parallel execution: Tests could interfere with each other if run concurrently
- State pollution: Failed tests may leave data behind affecting subsequent runs
Consider adding:
- Unique test data (e.g., random slot numbers or test-specific prefixes)
- Cleanup in a
Dropguard or explicit cleanup calls- Transaction-based rollback for test isolation
Example cleanup pattern:
// At test end sqlx::query!("DELETE FROM delegations WHERE slot_number IN ($1, $2, $3)", 12345_i64, 12346_i64, 12347_i64) .execute(&pool) .await .ok(); // Ignore cleanup errorssrc/db/slot_congestion_ops/slot_congestion_tests.rs (3)
120-166: LGTM with minor suggestion.Good coverage of the
get_congestion_statsfunction with and without data. The empty-case test (lines 120-136) could optionally verify thattotal_slots_trackedis 0 andhighest_congestion_slotis None when no data exists, but the current assertions are sufficient.
243-253: LGTM with optional enhancement.Good test for saturating addition behavior. The assertion could optionally be more specific by verifying
preconfirmed_gas == u64::MAXafter adding two large values, but the current check is sufficient to verify no panic occurs.
1-316: Consider documenting test isolation strategy.The test suite uses unique slot numbers and
#[serial]execution for isolation, which is appropriate. However, there's no cleanup of created records. This is acceptable if:
- The test database is reset between CI runs, or
- Tests are designed to handle pre-existing data
Consider adding a module-level comment documenting the test isolation approach and cleanup expectations.
Additionally, the hard-coded
genesis_time = 1606824023(appearing in multiple tests) represents December 1, 2020. Consider extracting this to a constant with a descriptive comment for better readability:// Ethereum mainnet genesis time: December 1, 2020 const TEST_GENESIS_TIME: u64 = 1606824023;src/testing/helpers.rs (2)
145-160: Consider spawning closures without pre-running themMinor: prefer spawning the future via async closure to avoid executing closures on the current task before spawn.
- let handles: Vec<_> = operations.into_iter().map(|op| tokio::spawn(op())).collect(); + let handles: Vec<_> = operations + .into_iter() + .map(|op| tokio::spawn(async move { op().await })) + .collect();
47-74: Use mocks in test context to avoid real network dependenciesThis constructs real clients (Reth/Beacon). For unit tests, prefer the mock clients from src/testing/mocks.rs to avoid network calls/timeouts.
Based on learnings
src/testing/mocks.rs (2)
101-108: Avoid std::sync::Mutex in async contexts; use tokio::MutexLocking std::sync::Mutex inside async fns can block the runtime. Switch submitted_constraints to tokio::Mutex and make getters async.
- use std::sync::{Arc, Mutex}; + use std::sync::Arc; + use tokio::sync::{Mutex, RwLock}; - pub submitted_constraints: Arc<Mutex<Vec<SignedConstraints>>>, + pub submitted_constraints: Arc<Mutex<Vec<SignedConstraints>>>,And update methods:
- pub fn get_submitted_constraints(&self) -> Vec<SignedConstraints> { - self.submitted_constraints.lock().unwrap().clone() - } + pub async fn get_submitted_constraints(&self) -> Vec<SignedConstraints> { + self.submitted_constraints.lock().await.clone() + } - pub fn clear_submitted_constraints(&self) { - self.submitted_constraints.lock().unwrap().clear(); - } + pub async fn clear_submitted_constraints(&self) { + self.submitted_constraints.lock().await.clear(); + } - self.submitted_constraints.lock().unwrap().push(constraints.clone()); + self.submitted_constraints.lock().await.push(constraints.clone());Also applies to: 157-167, 216-241
513-521: Nit: prefer rng.gen() for clarity
rng.r#gen()is unusual; userng.gen::<u8>().- let ikm: Vec<u8> = (0..32).map(|_| rng.r#gen()).collect(); + let ikm: Vec<u8> = (0..32).map(|_| rng.gen::<u8>()).collect();src/services/fee_pricing.rs (1)
289-319: Add a shutdown signal to background refresh taskThe spawned loop runs forever with no cancellation path. Acceptable for tests, but add a shutdown handle or signal for production use.
src/services/fee_pricing/fee_pricing_tests.rs (1)
1-8: Remove redundant nested test moduleThis file is already included as
mod fee_pricing_tests;. Wrapping contents in anothermod fee_pricing_testsnests modules unnecessarily. Flatten to top-level anduse super::*;.- #[cfg(test)] - mod fee_pricing_tests { - use super::super::*; + use super::*;src/rpc/handlers.rs (1)
156-176: JSON-RPC error codes: minor consistencyConsider returning InvalidParams for validation failures (slasher whitelist, payload extraction) and reserving InvalidRequest for structurally bad requests. Current mix is acceptable but inconsistent.
Also applies to: 179-201, 203-215, 232-243
src/rpc/handlers/handlers_integration_tests.rs (5)
322-323: Serialize the concurrency test to avoid DB races across tests.Add #[serial] to ensure isolation with other DB-using tests.
#[tokio::test] +#[serial] async fn test_concurrent_commitment_requests() -> Result<()> {
116-129: Use TestHelpers RPC context (mocks) to avoid external deps (reth/beacon).This test only validates setup; prefer the existing test context builder to remove network sensitivity. As per coding guidelines (src/testing/** mocks).
- // Create RPC context - let database = DatabaseContext::new(pool.clone()); - let reth_config = crate::api::reth::RethApiConfig::default(); - let reth_client = Arc::new(crate::api::reth::RethApiClient::new(reth_config)?); - let database_arc = Arc::new(database.clone()); - let config_arc = Arc::new(config.clone()); - let fee_engine = Arc::new(crate::services::fee_pricing::FeePricingEngine::new( - reth_client, - database_arc, - config_arc.clone(), - )); - let beacon_client = Arc::new(crate::api::beacon::BeaconApiClient::new(config.beacon_api.clone())?); - let _context = Arc::new(RpcContext::new(database, config, fee_engine, beacon_client)); + // Create RPC context (test helper uses lazy DB and mock-friendly wiring) + let _context = crate::testing::helpers::TestHelpers::create_test_rpc_context(Arc::new(config.clone()));
211-218: Remove unnecessary DB dependency in test_fee_handler_with_valid_request.This test performs no I/O. Drop the env gate and pool connect to speed up and reduce flakiness.
- // Skip test if no database available - let _ = std::env::var("DATABASE_URL").unwrap(); - - let _pool = setup_test_pool().await?; + // No DB required for this unit-style validation
238-246: No async work here — use #[test] (optional).Simplify the test that only checks struct fields.
-#[tokio::test] -async fn test_fee_handler_invalid_commitment_type() -> Result<()> { +#[test] +fn test_fee_handler_invalid_commitment_type() -> Result<()> {
6-9: Colocate integration tests in handlers.rs
They’re already included viamod handlers_integration_testsin src/rpc/handlers.rs, but consider relocating them into a#[cfg(test)] mod tests { … }block in handlers.rs for simpler imports and direct access to private helpers.src/testing/fixtures.rs (4)
12-12: Revisit tarpaulin gating approach.Gating entire impls with #[cfg(not(tarpaulin_include))] can surprise builds. Prefer function-level skipping via tarpaulin attributes.
Options:
- Annotate functions with: #[cfg_attr(tarpaulin, skip)]
- Or use: #[cfg_attr(tarpaulin, coverage(off))] (if using nightly coverage)
- Or keep code compiled and add #[allow(unused)] where needed
Please verify your tarpaulin setup defines the expected cfg flag.Also applies to: 264-264, 312-312
118-121: Base slot math on BeaconTiming and genesis_time.Use BeaconTiming::current_slot_estimate(genesis_time) for consistency with production timing. Based on learnings.
- // Use current slot numbers for realistic testing - let current_slot = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_secs() / 12; + // Use current slot estimate from configured genesis_time + let genesis_time = crate::testing::mocks::create_test_config().beacon_api.genesis_time; + let current_slot = crate::types::beacon::BeaconTiming::current_slot_estimate(genesis_time);Note: Add imports if desired for brevity:
- use crate::testing::mocks::create_test_config;
- use crate::types::beacon::BeaconTiming;
322-335: Timing helpers: accept/use genesis_time instead of wall‑clock/12.Align with production timing and avoid magic 12s. Based on learnings.
-pub fn create_timing_test_slots() -> Vec<u64> { - let current_time = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_secs(); - let current_slot = current_time / 12; +pub fn create_timing_test_slots(genesis_time: u64) -> Vec<u64> { + let current_slot = crate::types::beacon::BeaconTiming::current_slot_estimate(genesis_time); vec![ current_slot - 50, current_slot - 5, current_slot, current_slot + 5, current_slot + 50, ] } -pub fn is_within_submission_window(_genesis_time: u64, slot: u64) -> bool { - let current_time = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_secs(); - let current_slot = current_time / 12; +pub fn is_within_submission_window(genesis_time: u64, slot: u64) -> bool { + let current_slot = crate::types::beacon::BeaconTiming::current_slot_estimate(genesis_time); let slot_diff = slot.abs_diff(current_slot); slot_diff <= 10 }Also applies to: 353-361
58-62: Mock BLS signature: clarify or provide a valid-signature helper.This 96‑byte constant will fail any signature verification path.
Options:
- Rename to create_mock_signed_delegation to reduce misuse.
- Add a real signer variant for verification flows:
pub fn create_valid_signed_delegation( slot: u64, proposer_sk: blst::min_pk::SecretKey, delegate_pk: BlsPublicKey, committer_address: &str, ) -> SignedDelegation { // mirror logic from handlers_integration_tests::create_properly_signed_delegation // (ABI encode, domain-separate, keccak256, sign with proposer_sk) // return SignedDelegation { message, signature } }I can wire this up if you want it in fixtures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/db/delegation_ops.rs(3 hunks)src/db/slot_congestion_ops.rs(1 hunks)src/db/slot_congestion_ops/slot_congestion_tests.rs(1 hunks)src/rpc/handlers.rs(3 hunks)src/rpc/handlers/handlers_integration_tests.rs(1 hunks)src/services/fee_pricing.rs(1 hunks)src/services/fee_pricing/fee_pricing_tests.rs(1 hunks)src/testing/fixtures.rs(3 hunks)src/testing/helpers.rs(3 hunks)src/testing/mocks.rs(14 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/db/slot_congestion_ops.rs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with rustfmt using hard tabs and 120-character width
Place unit tests within each module using #[cfg(test)] and #[test]
Files:
src/services/fee_pricing.rssrc/db/delegation_ops.rssrc/db/slot_congestion_ops/slot_congestion_tests.rssrc/rpc/handlers.rssrc/testing/fixtures.rssrc/services/fee_pricing/fee_pricing_tests.rssrc/testing/helpers.rssrc/testing/mocks.rssrc/rpc/handlers/handlers_integration_tests.rs
src/db/**
📄 CodeRabbit inference engine (CLAUDE.md)
src/db/**: Return detailed error context using anyhow in all database operations
Use SQLx with connection pooling and async operations for all database access
Files:
src/db/delegation_ops.rssrc/db/slot_congestion_ops/slot_congestion_tests.rs
src/rpc/{handlers.rs,methods.rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Support only Hoodi chain (chain_id 560048) and inclusion commitments (type 1) in RPC exposure (e.g., slots/catalog)
Files:
src/rpc/handlers.rs
src/rpc/handlers.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/rpc/handlers.rs: In commitment_request handler, enforce delegation-first validation before any signing
Slots endpoint must provide a 64-future-slot service catalog (2 epochs lookahead) using real-time beacon timing
Ensure the slots endpoint performs no database access (computational-only)
Place integration tests for RPC flows in handlers.rs test modules
Files:
src/rpc/handlers.rs
src/rpc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Return proper JSON-RPC error codes from RPC handlers
Files:
src/rpc/handlers.rssrc/rpc/handlers/handlers_integration_tests.rs
src/testing/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain complete mock service implementations under src/testing
Files:
src/testing/fixtures.rssrc/testing/helpers.rssrc/testing/mocks.rs
🧠 Learnings (3)
📚 Learning: 2025-10-14T01:24:29.560Z
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to **/*.rs : Place unit tests within each module using #[cfg(test)] and #[test]
Applied to files:
src/services/fee_pricing.rs
📚 Learning: 2025-10-14T01:24:29.560Z
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to src/rpc/handlers.rs : Place integration tests for RPC flows in handlers.rs test modules
Applied to files:
src/rpc/handlers.rssrc/rpc/handlers/handlers_integration_tests.rs
📚 Learning: 2025-10-14T01:24:29.560Z
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to src/types/beacon.rs : Base timing operations on BeaconTiming::current_slot_estimate()
Applied to files:
src/testing/fixtures.rs
🧬 Code graph analysis (4)
src/db/slot_congestion_ops/slot_congestion_tests.rs (3)
src/rpc/handlers/handlers_integration_tests.rs (1)
setup_test_pool(22-29)src/services/fee_pricing/fee_pricing_tests.rs (1)
setup_test_pool(19-23)src/db/slot_congestion_ops.rs (5)
get_or_create_slot_congestion(138-192)update_slot_congestion_gas_usage(258-330)get_slot_congestion(202-237)get_congestion_stats(407-449)new(38-52)
src/rpc/handlers.rs (7)
src/db/mod.rs (4)
new(111-113)commitment_exists(180-182)save_commitment(149-154)get_commitment_by_hash(164-166)src/types/context.rs (1)
new(24-31)src/types/payload.rs (6)
new(189-191)new(264-266)encode_inclusion_payload(162-165)slot(201-203)slot(276-278)extract_slot(63-69)src/db/operations.rs (3)
commitment_exists(147-154)save_commitment(27-88)get_commitment_by_hash(101-137)src/testing/fixtures.rs (1)
create_inclusion_commitment_request(20-36)src/crypto/mod.rs (3)
generate_request_hash(43-52)sign_commitment(111-137)parse_private_key(190-193)src/utils/address.rs (1)
normalize_address(8-12)
src/services/fee_pricing/fee_pricing_tests.rs (5)
src/db/slot_congestion_ops/slot_congestion_tests.rs (1)
setup_test_pool(11-16)src/db/slot_congestion_ops.rs (1)
new(38-52)src/services/fee_pricing.rs (2)
new(48-50)is_slot_acceptable_for_fees(274-279)src/db/mod.rs (2)
new_for_testing(122-127)pool(135-137)src/types/payload.rs (1)
encode_inclusion_payload(162-165)
src/rpc/handlers/handlers_integration_tests.rs (8)
src/crypto/mod.rs (3)
generate_request_hash(43-52)sign_commitment(111-137)keccak256(24-30)src/testing/mocks.rs (8)
create_test_bls_keypair(513-521)create_test_config(467-503)new(42-44)new(130-138)new(305-312)default(29-31)default(116-118)default(295-297)src/rpc/handlers.rs (4)
new(434-441)validate_and_extract_slot(16-19)validate_slasher_address(22-43)find_signing_key_for_committer(142-154)src/services/fee_pricing.rs (2)
new(48-50)is_slot_acceptable_for_fees(274-279)src/types/beacon.rs (1)
current_slot_estimate(155-163)src/db/delegation_ops.rs (1)
save_delegations_batch(396-461)src/testing/fixtures.rs (1)
create_inclusion_commitment_request(20-36)src/testing/helpers.rs (1)
create_test_rpc_context(47-74)
⏰ 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). (1)
- GitHub Check: Code Coverage
🔇 Additional comments (10)
src/db/slot_congestion_ops/slot_congestion_tests.rs (9)
18-44: LGTM!This test correctly verifies the idempotency of
get_or_create_slot_congestion, ensuring that subsequent calls return the same record by checking ID equality.
46-76: LGTM!Excellent test coverage for gas usage accumulation, verifying both single updates and cumulative behavior across multiple updates.
78-118: LGTM!These tests provide comprehensive coverage of the
get_slot_congestionfunction, testing both the success path (existing record) and the not-found path (non-existent slot).
168-204: LGTM!Excellent concurrency test that verifies the database locking mechanism (FOR UPDATE) correctly handles simultaneous gas usage updates, ensuring accurate accumulation.
206-220: LGTM!Proper error path coverage, verifying that attempting to update a non-existent slot returns an error as expected.
222-241: LGTM!Excellent boundary testing covering zero gas limit (division by zero) and maximum value scenarios, ensuring robust handling of edge cases without panics or overflows.
255-267: LGTM!Good test verifying that the fee multiplier calculation is independent of base gas price while correctly scaling the
current_tx_price.
269-282: LGTM!Simple but useful test verifying the Debug implementation for
CongestionStatsproduces expected output.
284-315: LGTM!Excellent real-world scenario testing with realistic gas prices spanning low (1 gwei), medium (50 gwei), and high (1000 gwei) ranges, verifying correct handling across different price levels.
src/rpc/handlers/handlers_integration_tests.rs (1)
31-77: BLS signing helper looks correct.Good: domain separation, keccak256 over ABI-encoded message, POP DST, and 96‑byte sig formatting.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/rpc/handlers/handlers_integration_tests.rs (1)
22-26: Still need to skip tests whenDATABASE_URLis absent.As noted previously, calling
std::env::var("DATABASE_URL")?;here turns a missing env var into a hard failure. Please guard and early-returnOk(())from the tests when the variable is unset so local/CI runs without a DB don’t fail spuriously.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/db/slot_congestion_ops/slot_congestion_tests.rs(1 hunks)src/rpc/handlers/handlers_integration_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with rustfmt using hard tabs and 120-character width
Place unit tests within each module using #[cfg(test)] and #[test]
Files:
src/rpc/handlers/handlers_integration_tests.rssrc/db/slot_congestion_ops/slot_congestion_tests.rs
src/rpc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Return proper JSON-RPC error codes from RPC handlers
Files:
src/rpc/handlers/handlers_integration_tests.rs
src/db/**
📄 CodeRabbit inference engine (CLAUDE.md)
src/db/**: Return detailed error context using anyhow in all database operations
Use SQLx with connection pooling and async operations for all database access
Files:
src/db/slot_congestion_ops/slot_congestion_tests.rs
🧠 Learnings (1)
📚 Learning: 2025-10-14T01:24:29.560Z
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to src/rpc/handlers.rs : Place integration tests for RPC flows in handlers.rs test modules
Applied to files:
src/rpc/handlers/handlers_integration_tests.rs
🧬 Code graph analysis (2)
src/rpc/handlers/handlers_integration_tests.rs (7)
src/crypto/mod.rs (3)
generate_request_hash(43-52)sign_commitment(111-137)keccak256(24-30)src/testing/mocks.rs (8)
create_test_bls_keypair(513-521)create_test_config(467-503)new(42-44)new(130-138)new(305-312)default(29-31)default(116-118)default(295-297)src/services/fee_pricing.rs (2)
new(48-50)is_slot_acceptable_for_fees(274-279)src/rpc/handlers.rs (4)
new(434-441)validate_and_extract_slot(16-19)validate_slasher_address(22-43)find_signing_key_for_committer(142-154)src/testing/fixtures.rs (1)
create_inclusion_commitment_request(20-36)src/types/payload.rs (1)
encode_inclusion_payload(162-165)src/testing/helpers.rs (1)
create_test_rpc_context(47-74)
src/db/slot_congestion_ops/slot_congestion_tests.rs (1)
src/db/slot_congestion_ops.rs (5)
get_or_create_slot_congestion(138-192)update_slot_congestion_gas_usage(258-330)get_slot_congestion(202-237)get_congestion_stats(407-449)new(38-52)
⏰ 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). (1)
- GitHub Check: Code Coverage
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/rpc/handlers.rs (1)
408-811: Past review concerns about mock integration tests remain unaddressed.Previous reviews flagged missing integration tests that exercise
commitment_request_handlerandcommitment_result_handlerwithMockDatabaseContextto verify error paths (DB check failures, conflicts, save errors). These concerns are still valid and should be addressed in a follow-up.Based on past review comments.
🧹 Nitpick comments (10)
src/services/validator_status_cache.rs (1)
21-21: Consider a type alias to reduce verbosity.The type
Arc<BeaconApiClient<crate::api::beacon::ReqwestClient>>appears frequently. A type alias would improve readability and maintainability.Consider adding a type alias in a common location (e.g.,
src/api/beacon.rsorsrc/types/mod.rs):pub type BeaconClient = Arc<BeaconApiClient<crate::api::beacon::ReqwestClient>>;Then update call sites:
-pub beacon_client: Arc<BeaconApiClient<crate::api::beacon::ReqwestClient>>, +pub beacon_client: BeaconClient,Also applies to: 34-34, 111-111, 119-119
src/api/beacon.rs (5)
58-59: Use Accept for GET; avoid Content-Type on GETSetting Content-Type on a GET is atypical; servers key on Accept. Suggest swapping to Accept and drop Content-Type.
Apply:
- .header("Content-Type", "application/json") + .header("Accept", "application/json")
325-328: Treat all 2xx as success; improve body decoding on errorOnly allowing 200 rejects valid 2xx responses and can cause false errors. Also avoid UTF‑8 panic/clone for error text.
Apply:
- if response.status != 200 { - let error_text = String::from_utf8(response.body.clone()).unwrap_or_else(|_| "Unknown error".to_string()); - anyhow::bail!("Beacon API request failed with status {}: {}", response.status, error_text); - } + if !(200..=299).contains(&response.status) { + let error_text = std::string::String::from_utf8_lossy(&response.body); + anyhow::bail!("Beacon API request failed with status {}: {}", response.status, error_text); + }
341-343: Docstring is misleading for testing pathwith_default_client always builds a ReqwestClient; you can’t pass a mock here. For tests, use BeaconApiClient::new(config, mock) with MockHttpClient.
Apply:
- /// This is the standard constructor for production use. For testing, use - /// `BeaconApiClient::with_default_client()` with a mock HTTP client. + /// This is the standard constructor for production use. + /// For tests, construct with a mock HTTP client: + /// `BeaconApiClient::new(config, MockHttpClient::new())`.
1361-1372: Avoid mutable state in mockall returning closureMutating call_count inside returning(...) relies on FnMut; depending on mockall version this can break or be brittle. Prefer a Sequence with per‑call expectations.
Example:
let mut seq = mockall::Sequence::new(); mock_client .expect_get() .in_sequence(&mut seq) .returning(|url| { assert!(url.contains("eth-mainnet.g.alchemy.com")); Err(anyhow::anyhow!("Primary endpoint failed")) }); mock_client .expect_get() .in_sequence(&mut seq) .returning(move |url| { assert!(url.contains("beacon-nd-123-456-789.p2pify.com")); Ok(HttpResponse { status: 200, body: serde_json::to_vec(&duties_response).unwrap() }) });If you prefer to keep a counter, wrap it in std::cell::Cell or std::sync::atomic::AtomicUsize.
Based on learnings
1413-1423: Same pattern: prefer Sequence to call_count mutationReplace the manual call_count mutation with explicit call ordering via mockall::Sequence for clarity and robustness.
src/testing/mocks.rs (4)
14-22: Gate test-only mocks behind cfg(test) or a “testing” feature, not tarpaulin flags#[cfg(not(tarpaulin_include))] includes these mocks in production builds, merely excluding them from coverage. Prefer #[cfg(test)] or #[cfg(any(test, feature = "testing"))] to avoid shipping test helpers and extra deps (rand, blst) in release.
82-93: Remove BeaconApiClient::mock() from the production API surfaceExposing an associated fn mock() returning MockBeaconApiClient on BeaconApiClient is misleading and leaks test helpers into the main API. Tests should construct MockBeaconApiClient directly (or mock HttpClient).
Apply:
-#[cfg(not(tarpaulin_include))] -impl<H: crate::api::beacon::HttpClient> crate::api::beacon::BeaconApiClient<H> { - /// Constructs a MockBeaconApiClient preconfigured for testing. - /// - /// The returned mock simulates the Beacon API and exposes controls for network delay, - /// failure mode, and injectable proposer duties. - /// - /// # Examples - /// - /// - pub fn mock() -> MockBeaconApiClient { - MockBeaconApiClient::new() - } -}
101-108: Avoid std::sync::Mutex in async code pathsmock_submit_constraints and helpers lock a std::sync::Mutex inside async fns. Prefer tokio::sync::Mutex to avoid potential scheduler blocking. This will require making get_submitted_constraints/clear_submitted_constraints async or restructuring to avoid awaiting under lock.
Also applies to: 157-167, 165-167, 231-236
516-517: Style nit: use rng.gen() instead of rng.r#gen()Raw identifiers are unnecessary here. Simplify for readability.
Apply:
- let ikm: Vec<u8> = (0..32).map(|_| rng.r#gen()).collect(); + let ikm: Vec<u8> = (0..32).map(|_| rng.gen()).collect();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.toml(2 hunks)src/api/beacon.rs(15 hunks)src/main.rs(1 hunks)src/main_components_tests.rs(4 hunks)src/rpc/handlers.rs(1 hunks)src/server.rs(1 hunks)src/services/delegation_polling.rs(15 hunks)src/services/validator_status_cache.rs(3 hunks)src/testing/helpers.rs(4 hunks)src/testing/mocks.rs(14 hunks)src/types/context.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/testing/helpers.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with rustfmt using hard tabs and 120-character width
Place unit tests within each module using #[cfg(test)] and #[test]
Files:
src/main.rssrc/types/context.rssrc/rpc/handlers.rssrc/services/validator_status_cache.rssrc/server.rssrc/main_components_tests.rssrc/services/delegation_polling.rssrc/api/beacon.rssrc/testing/mocks.rs
src/rpc/{handlers.rs,methods.rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Support only Hoodi chain (chain_id 560048) and inclusion commitments (type 1) in RPC exposure (e.g., slots/catalog)
Files:
src/rpc/handlers.rs
src/rpc/handlers.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/rpc/handlers.rs: In commitment_request handler, enforce delegation-first validation before any signing
Slots endpoint must provide a 64-future-slot service catalog (2 epochs lookahead) using real-time beacon timing
Ensure the slots endpoint performs no database access (computational-only)
Place integration tests for RPC flows in handlers.rs test modules
Files:
src/rpc/handlers.rs
src/rpc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Return proper JSON-RPC error codes from RPC handlers
Files:
src/rpc/handlers.rs
src/testing/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain complete mock service implementations under src/testing
Files:
src/testing/mocks.rs
🧬 Code graph analysis (7)
src/main.rs (2)
src/types/context.rs (2)
beacon_client(51-53)new(24-31)src/api/beacon.rs (3)
new(43-49)new(105-116)with_default_client(353-356)
src/rpc/handlers.rs (2)
src/types/context.rs (2)
beacon_client(51-53)new(24-31)src/api/beacon.rs (3)
new(43-49)new(105-116)with_default_client(353-356)
src/services/validator_status_cache.rs (3)
src/api/beacon.rs (3)
new(43-49)new(105-116)with_default_client(353-356)src/services/delegation_polling.rs (1)
new(31-48)src/types/context.rs (2)
new(24-31)beacon_client(51-53)
src/server.rs (2)
src/types/context.rs (1)
beacon_client(51-53)src/api/beacon.rs (1)
with_default_client(353-356)
src/main_components_tests.rs (2)
src/api/beacon.rs (1)
with_default_client(353-356)src/types/context.rs (1)
beacon_client(51-53)
src/services/delegation_polling.rs (3)
src/types/context.rs (2)
beacon_client(51-53)new(24-31)src/api/beacon.rs (3)
new(43-49)new(105-116)with_default_client(353-356)src/services/validator_status_cache.rs (1)
new(34-38)
src/api/beacon.rs (4)
src/services/delegation_polling.rs (1)
new(31-48)src/services/validator_status_cache.rs (1)
new(34-38)src/types/context.rs (1)
new(24-31)src/types/beacon.rs (4)
validator_index(116-116)slot(105-105)parse_slot(104-106)slot_to_epoch(128-130)
⏰ 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). (1)
- GitHub Check: Code Coverage
🔇 Additional comments (6)
src/server.rs (1)
188-188: LGTM!The migration to
BeaconApiClient::with_default_client(...)is correct and consistent with the new constructor pattern introduced across the codebase.src/main.rs (1)
49-49: LGTM!The main startup path correctly adopts the new
with_default_client(...)constructor, which encapsulates the default HTTP client setup.src/main_components_tests.rs (1)
117-117: LGTM!All test contexts consistently migrate to
with_default_client(...), maintaining functional equivalence while adopting the new constructor pattern.Also applies to: 165-165, 236-236, 318-318
src/rpc/handlers.rs (1)
479-479: LGTM!The test context correctly adopts
with_default_client(...).src/types/context.rs (1)
16-16: LGTM!Type updates to
Arc<BeaconApiClient<crate::api::beacon::ReqwestClient>>are consistent. Consider the type alias suggestion fromsrc/services/validator_status_cache.rsto reduce verbosity project-wide.Also applies to: 28-28, 47-47, 51-51
src/services/delegation_polling.rs (1)
20-20: LGTM!All type updates and constructor migrations are consistent and correct. The 13 test context updates properly adopt
with_default_client(...), and the type signature changes align with the project-wide refactor.Also applies to: 32-32, 132-132, 484-484, 508-508, 522-522, 552-552, 572-572, 598-598, 615-615, 638-638, 659-659, 755-755, 820-820, 888-888
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
Cargo.toml (1)
41-41: mockall correctly moved to dev-dependencies.This resolves the earlier review. Good change.
src/services/fee_pricing/fee_pricing_tests.rs (3)
19-26: Avoid panic on missing DATABASE_URL: LGTM.Graceful error propagation replaces unwrap. Good.
232-241: Guard DB test when DATABASE_URL is absent: LGTM.Skips cleanly instead of panicking.
266-275: Second DB test guarded on DATABASE_URL: LGTM.Consistent skip behavior.
🧹 Nitpick comments (4)
src/services/fee_pricing/fee_pricing_tests.rs (1)
328-350: Test asserts local math, not engine behavior.This test only checks TTL/2 arithmetic in test code, not engine logic. Consider exposing a small helper or public method for the refresh interval (or asserting via a scheduled task/interval value) to exercise real behavior.
src/api/beacon.rs (3)
137-176: Minor: rename _last_error to last_error.Leading underscore implies “unused” but the variable is used, which can hide issues. Rename for clarity.
Apply:
- let mut _last_error = None; + let mut last_error = None; ... - _last_error = Some(e); + last_error = Some(e); ... - Err(_last_error.unwrap_or_else(|| anyhow::anyhow!("No beacon endpoints configured"))) + Err(last_error.unwrap_or_else(|| anyhow::anyhow!("No beacon endpoints configured")))
229-268: Minor: rename _last_error to last_error.Same clarity improvement in validator status path.
- let mut _last_error = None; + let mut last_error = None; ... - _last_error = Some(e); + last_error = Some(e); ... - Err(_last_error.unwrap_or_else(|| anyhow::anyhow!("No beacon endpoints configured"))) + Err(last_error.unwrap_or_else(|| anyhow::anyhow!("No beacon endpoints configured")))
312-336: Broaden success to 2xx and avoid lossy UTF-8 handling issues.Accept all 2xx statuses and avoid cloning/UTF‑8 errors when logging body.
- let response = - self.http_client.get(&url).await.with_context(|| format!("Failed to send request to {}", url))?; + let response = + self.http_client.get(&url).await.with_context(|| format!("Failed to send request to {}", url))?; - if response.status != 200 { - let error_text = String::from_utf8(response.body.clone()).unwrap_or_else(|_| "Unknown error".to_string()); + if !(200..=299).contains(&response.status) { + let error_text = String::from_utf8_lossy(&response.body).to_string(); anyhow::bail!("Beacon API request failed with status {}: {}", response.status, error_text); } - let result: T = - serde_json::from_slice(&response.body).with_context(|| format!("Failed to parse response from {}", url))?; + let result: T = serde_json::from_slice(&response.body) + .with_context(|| format!("Failed to parse response from {}", url))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
Cargo.toml(3 hunks)src/api/beacon.rs(15 hunks)src/main.rs(1 hunks)src/main_components_tests.rs(4 hunks)src/server.rs(1 hunks)src/services/fee_pricing/fee_pricing_tests.rs(1 hunks)src/testing/helpers.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main.rs
- src/testing/helpers.rs
- src/server.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with rustfmt using hard tabs and 120-character width
Place unit tests within each module using #[cfg(test)] and #[test]
Files:
src/main_components_tests.rssrc/services/fee_pricing/fee_pricing_tests.rssrc/api/beacon.rs
🧬 Code graph analysis (3)
src/main_components_tests.rs (2)
src/api/beacon.rs (1)
with_default_client(355-358)src/types/context.rs (1)
beacon_client(51-53)
src/services/fee_pricing/fee_pricing_tests.rs (5)
src/db/slot_congestion_ops/slot_congestion_tests.rs (1)
setup_test_pool(11-15)src/services/fee_pricing.rs (2)
new(48-50)is_slot_acceptable_for_fees(274-279)src/db/slot_congestion_ops.rs (1)
new(38-52)src/db/mod.rs (2)
new_for_testing(122-127)pool(135-137)src/types/payload.rs (1)
encode_inclusion_payload(162-165)
src/api/beacon.rs (4)
src/services/validator_status_cache.rs (1)
new(34-38)src/types/context.rs (1)
new(24-31)src/testing/mocks.rs (4)
new(42-44)new(130-138)new(305-312)create_test_config(467-503)src/services/delegation_polling.rs (1)
new(31-48)
⏰ 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). (1)
- GitHub Check: Code Coverage
🔇 Additional comments (5)
Cargo.toml (1)
9-9: async-trait addition: LGTM.Required for async trait object mocks and HttpClient. No concerns.
src/main_components_tests.rs (4)
165-166: DelegationPolling test updated to with_default_client: LGTM.Correctly constructs the default Reqwest-backed client.
237-238: RPC context test updated to with_default_client: LGTM.Consistent with new generic client type.
319-321: Integration flow uses with_default_client: LGTM.Aligned with startup path changes.
117-117: No remaining single-argBeaconApiClient::new(config)calls. Tests rightly usenew(config, mock_client)for injection.
sjustintaylor
changed the title
…tandardize timing This commit addresses all CodeRabbit review comments for PR #11: ## Critical Security & Correctness Fixes 1. **Enforce 8-second constraint deadline dynamically** (constraint_submission.rs) - Replace fixed 5-second timeout with dynamic calculation based on actual slot deadline - Use BeaconTiming::constraint_deadline_for_slot() for accurate deadline calculation - Check if deadline has elapsed before attempting submission - Prevents overshoot or overly conservative timeouts 2. **Fix constraint filtering bug** (constraint_submission.rs) - Add committer_address parameter to create_constraints_from_commitments() - Filter commitments by matching slasher with delegation's committer - Prevents cross-committer constraint submission (critical security issue) - Update function signature, implementation, and test ## Timing Logic Standardization 3. **Replace manual slot calculations with BeaconTiming** (helpers.rs, fixtures.rs) - validate_constraint_timing: Use BeaconTiming for deadline calculation - load_test_commitment_request_handler: Use current_slot_estimate() - create_test_scenarios: Add genesis_time parameter, use current_slot_estimate() - create_timing_test_slots: Add genesis_time parameter, use current_slot_estimate() - is_within_submission_window: Use current_slot_estimate() instead of manual time/12 - All timing now respects beacon chain genesis offset ## Code Quality Improvements 4. **Add percentile input clamping** (helpers.rs) - Clamp percentile to [0, 100] range to prevent unexpected index calculations - Improves robustness of statistics calculations 5. **Update PR title** - Changed from generic "Test coverage" to descriptive "Add comprehensive test coverage for RPC handlers, database operations, and timing utilities" ## Testing - All constraint_submission tests pass (15/15) - Changes align with BeaconTiming utility already in codebase - No breaking changes to existing test infrastructure Addresses: CodeRabbit review comments on PR #11 commits 2f12f3d, a03bd37, 803d321
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/testing/helpers.rs (1)
469-471: Update documentation to reflect clamping behavior.The documentation states "behavior for values outside that range is undefined," but the implementation now clamps percentile to [0, 100]. This inconsistency should be resolved.
Apply this diff to update the documentation:
- /// `percentile` must be in the range 0.0..=100.0; behavior for values outside - /// that range is undefined. + /// `percentile` is clamped to the range 0.0..=100.0. Values outside this range + /// will be adjusted to the nearest boundary.Also applies to: 482-484
🧹 Nitpick comments (3)
src/services/constraint_submission.rs (3)
429-446: Consider a minimum timeout threshold to avoid futile submissions.The dynamic timeout correctly enforces the 8-second deadline. However, if very little time remains (e.g., < 100ms), the submission is likely to fail even for a valid, fast API call. This could waste resources and create noisy error logs.
Consider adding a minimum viable timeout check:
// Calculate remaining budget for submission let remaining = Duration::from_secs(deadline_secs - now_secs); + +// Fail fast if remaining time is too small for a realistic submission +const MIN_SUBMISSION_TIME: Duration = Duration::from_millis(100); +if remaining < MIN_SUBMISSION_TIME { + return Err(anyhow::anyhow!( + "Insufficient time remaining ({:?}) to submit constraint for slot {}", + remaining, + slot + )); +} // Submit with dynamic timeout bounded by the remaining time to deadline let submission_result = timeout(remaining, constraints_client.submit_constraints(&signed_constraints))This ensures we fail fast with a clear message rather than attempting submissions that are almost certain to timeout.
836-848: Consider adding tests for dynamic timeout behavior.The test correctly verifies the 8-second deadline calculation. However, it would be beneficial to add tests that verify:
- Dynamic timeout is applied based on remaining time
- Committer-based filtering correctly includes/excludes commitments
- Behavior near deadline (e.g., when only 1 second remains)
Example test structure:
#[tokio::test] async fn test_dynamic_timeout_near_deadline() { // Test that submission fails or succeeds appropriately when very close to deadline // This would help verify the timeout logic works correctly in edge cases } #[tokio::test] async fn test_committer_filtering() { // Test that commitments are correctly filtered by committer address // Verify that only matching commitments are converted to constraints }These tests would help ensure the new timing and filtering behaviors work as intended.
502-509: Unifyslasherandcommitterterminology
Rename theslasherfields insrc/types/rpc.rs(lines 12 & 22) tocommitterand update the debug message insrc/services/constraint_submission.rsto use the same term.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/services/constraint_submission.rs(5 hunks)src/testing/fixtures.rs(6 hunks)src/testing/helpers.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with rustfmt using hard tabs and 120-character width
Place unit tests within each module using #[cfg(test)] and #[test]
Files:
src/services/constraint_submission.rssrc/testing/helpers.rssrc/testing/fixtures.rs
src/services/constraint_submission.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/services/constraint_submission.rs: Ensure constraints are submitted within 8 seconds of slot start
Use a scheduler (tokio-cron-scheduler) for timing-critical constraint submission
Files:
src/services/constraint_submission.rs
src/testing/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain complete mock service implementations under src/testing
Files:
src/testing/helpers.rssrc/testing/fixtures.rs
🧠 Learnings (3)
📚 Learning: 2025-10-14T01:24:29.560Z
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to src/services/constraint_submission.rs : Ensure constraints are submitted within 8 seconds of slot start
Applied to files:
src/services/constraint_submission.rssrc/testing/helpers.rs
📚 Learning: 2025-10-14T01:24:29.560Z
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to src/types/beacon.rs : Base timing operations on BeaconTiming::current_slot_estimate()
Applied to files:
src/testing/helpers.rssrc/testing/fixtures.rs
📚 Learning: 2025-10-14T01:24:29.560Z
Learnt from: CR
PR: Labrys-Group/preconfirmation-gateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T01:24:29.560Z
Learning: Applies to src/services/constraint_submission.rs : Use a scheduler (tokio-cron-scheduler) for timing-critical constraint submission
Applied to files:
src/testing/helpers.rs
🧬 Code graph analysis (3)
src/services/constraint_submission.rs (1)
src/types/beacon.rs (2)
slot(105-105)constraint_deadline_for_slot(193-195)
src/testing/helpers.rs (3)
src/types/context.rs (2)
beacon_client(51-53)new(24-31)src/api/beacon.rs (3)
new(43-49)new(106-117)with_default_client(355-358)src/types/beacon.rs (3)
constraint_deadline_for_slot(193-195)slot(105-105)current_slot_estimate(155-163)
src/testing/fixtures.rs (2)
src/services/constraint_submission.rs (2)
current_slot(209-209)is_within_submission_window(142-145)src/types/beacon.rs (2)
current_slot_estimate(155-163)slot(105-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code Coverage
- GitHub Check: Lint
🔇 Additional comments (14)
src/testing/fixtures.rs (4)
12-12: LGTM: Appropriate test coverage exclusion.Excluding test fixture implementations from tarpaulin coverage is a common practice, as these utilities are exercised indirectly through the tests that use them.
119-123: LGTM: Correct use of BeaconTiming for deterministic slot calculation.The addition of the
genesis_timeparameter and use ofBeaconTiming::current_slot_estimate(genesis_time)aligns with the project's timing conventions. This change makes test scenarios deterministic and independent of system time, which is essential for reliable testing.Based on learnings: Timing operations should be based on
BeaconTiming::current_slot_estimate().
330-332: LGTM: Consistent timing approach for test slot generation.Using
BeaconTiming::current_slot_estimate(genesis_time)ensures consistency with the rest of the codebase's timing utilities and enables deterministic test scenarios.Based on learnings.
360-362: LGTM: Deterministic submission window validation.The updated implementation correctly uses
BeaconTiming::current_slot_estimate(genesis_time)to determine the current slot, enabling predictable and testable timing validation.Based on learnings.
src/testing/helpers.rs (6)
14-14: LGTM: Appropriate test coverage exclusion.Excluding test helper implementations from tarpaulin coverage is consistent with standard practice for test infrastructure code.
71-72: LGTM: Correct use of with_default_client constructor.The change to use
BeaconApiClient::with_default_client(...)aligns with the PR's objective of making the BeaconApiClient generic over an HTTP client, improving testability by enabling mock injection.
251-253: LGTM: Correct use of BeaconTiming utility for deadline calculation.Using
BeaconTiming::constraint_deadline_for_slot(genesis_time, commitment_slot)replaces manual deadline computation with the standard timing utility, ensuring consistency across the codebase.Based on learnings: Constraints must be submitted within 8 seconds of slot start.
286-288: LGTM: Deterministic slot calculation for load generation.Using
BeaconTiming::current_slot_estimate(context.config.beacon_api.genesis_time)ensures realistic load generation uses consistent timing logic aligned with the rest of the codebase.Based on learnings.
380-414: LGTM: Useful accessor methods for test metrics.The new public accessors (
total_operations,success_rate,operations_per_second) provide convenient ways to evaluate database stress test results. The implementations are straightforward and correct.
436-461: LGTM: Useful accessor methods for load test metrics.The new public accessors (
success_rate,average_response_time) provide convenient ways to evaluate load test results. The implementations handle edge cases appropriately (empty response times, zero requests).src/services/constraint_submission.rs (4)
308-310: LGTM!The committer-based filtering is correctly propagated from the delegation to the constraint creation function. This ensures that only commitments matching the delegation's committer address are converted into constraints.
454-479: LGTM!The function signature and documentation correctly describe the committer-based filtering behavior. The parameters and return values are clearly documented.
754-757: LGTM!The test correctly updates the function call to include the committer address parameter, maintaining consistency with the new signature.
434-446: Good error handling for deadline enforcement.The code correctly checks if the deadline has elapsed before attempting submission and provides clear error messages for different failure modes (deadline elapsed, timeout, submission failure).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/testing/mocks.rs (2)
512-520: Fix unnecessary raw identifier syntax.Line 515 uses
rng.r#gen()which unnecessarily escapes thegenmethod call. Sincegenis not a reserved keyword in this context, the raw identifier syntax is not needed.Apply this diff:
let mut rng = rand::thread_rng(); - let ikm: Vec<u8> = (0..32).map(|_| rng.r#gen()).collect(); + let ikm: Vec<u8> = (0..32).map(|_| rng.gen()).collect(); let secret_key = SecretKey::key_gen(&ikm, &[]).unwrap();
81-92: Implement HttpClient for MockBeaconApiClient
BeaconApiClient::mock()returnsMockBeaconApiClient, but it doesn’t satisfy theH: HttpClientbound. Add animpl crate::api::beacon::HttpClient for MockBeaconApiClient { … }.
🧹 Nitpick comments (2)
src/testing/mocks.rs (2)
215-240: Consider VecDeque for queue operations.The method uses
responses.remove(0)which is O(n) forVec. Sincesubmission_responsesis used as a FIFO queue, consider usingVecDequeinstead for O(1)pop_front()operations.Apply this diff:
pub struct MockConstraintsApiClient { /// Stored delegations by slot pub delegations: Arc<RwLock<HashMap<u64, Vec<SignedDelegation>>>>, /// Submitted constraints log pub submitted_constraints: Arc<Mutex<Vec<SignedConstraints>>>, /// Network delay simulation pub network_delay_ms: u64, /// Failure simulation pub should_fail: bool, /// Mock responses for constraint submission - pub submission_responses: Arc<RwLock<Vec<ConstraintSubmissionResponse>>>, + pub submission_responses: Arc<RwLock<std::collections::VecDeque<ConstraintSubmissionResponse>>>, }And update the usage:
// Return a mock response (consume from queue in FIFO order) let mut responses = self.submission_responses.write().await; if !responses.is_empty() { - Ok(responses.remove(0)) + Ok(responses.pop_front().unwrap()) } else {Don't forget to initialize with
VecDeque::new()in the constructor.
1-540: Remove empty "# Examples" sections from documentation.Many functions throughout the file include an "# Examples" section header in their doc comments but provide no actual example code. This is misleading and adds no value. Consider removing these empty sections or adding actual usage examples.
For instance, at lines 26-30, 39-43, 54-58, and many others, you can remove the empty "# Examples" section:
/// Creates a new in-memory MockBeaconApiClient with default test settings. /// /// The default instance has an empty proposer duties map, a 50 ms simulated network delay, /// and failure simulation disabled. - /// - /// # Examples - /// pub fn new() -> Self {Apply this pattern throughout the file for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/db/delegation_ops.rs(3 hunks)src/testing/mocks.rs(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/db/delegation_ops.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with rustfmt using hard tabs and 120-character width
Place unit tests within each module using #[cfg(test)] and #[test]
Files:
src/testing/mocks.rs
src/testing/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain complete mock service implementations under src/testing
Files:
src/testing/mocks.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Coverage
🔇 Additional comments (4)
src/testing/mocks.rs (4)
465-502: LGTM!The test configuration provides appropriate test-oriented values with localhost endpoints, short timeouts, and deterministic settings suitable for isolated test environments.
277-456: LGTM!The
MockDatabaseimplementation provides comprehensive mock functionality with proper thread-safe storage, latency simulation, and failure mode support. The methods correctly implement the expected behavior for testing database interactions.
94-214: LGTM!The
MockConstraintsApiClientstructure and its helper methods provide comprehensive mock functionality with appropriate storage, network simulation, and failure modes for testing.
529-540: LGTM!The ECDSA keypair generation correctly creates a deterministic test key and derives the Ethereum address following the standard process (keccak256 hash of uncompressed public key, taking the last 20 bytes).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.gitignore (1)
23-25: .env patterns: keep templates tracked; ignore direnv/.envrcGood call adding .env.* to prevent secrets in VCS. If you intend to keep example templates committed, add negations; also consider ignoring direnv artifacts.
Apply near this block:
.env .env.* +!.env.example +!.env.template +.envrc +.direnv/If you do want to ignore template files too, ignore the negations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.gitignore(1 hunks)
⏰ 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). (2)
- GitHub Check: Code Coverage
- GitHub Check: Lint
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Taskfile.yml (1)
23-26: Remove the duplicatedlinttask entry.You now declare
linttwice; Task will only keep the last definition, so this new block is redundant and risks confusion. Drop one of the duplicates (e.g., the earlier block) so there’s a single source of truth.- - lint: - cmds: - - cargo clippy
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
Cargo.toml(3 hunks)Taskfile.yml(1 hunks)src/services/fee_pricing/fee_pricing_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with rustfmt using hard tabs and 120-character width
Place unit tests within each module using #[cfg(test)] and #[test]
Files:
src/services/fee_pricing/fee_pricing_tests.rs
🧬 Code graph analysis (1)
src/services/fee_pricing/fee_pricing_tests.rs (5)
src/db/slot_congestion_ops/slot_congestion_tests.rs (1)
setup_test_pool(11-15)src/services/fee_pricing.rs (2)
new(48-50)is_slot_acceptable_for_fees(274-279)src/db/slot_congestion_ops.rs (1)
new(38-52)src/db/mod.rs (2)
new_for_testing(122-127)pool(135-137)src/types/payload.rs (1)
encode_inclusion_payload(162-165)
⏰ 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). (2)
- GitHub Check: Lint
- GitHub Check: Code Coverage
🔇 Additional comments (3)
src/services/fee_pricing/fee_pricing_tests.rs (3)
1-18: LGTM!The module setup and imports are well-organized. The use of
serial_testfor database-dependent tests is appropriate to avoid test interference.
19-26: LGTM!The setup helper now properly handles missing
DATABASE_URLby returning an error instead of panicking. This addresses the previous review concern.
28-369: Excellent comprehensive test coverage!The test suite thoroughly exercises the fee pricing engine across multiple dimensions:
- Gas estimation for various payload types and edge cases
- Congestion calculation with proper bounds checking and overflow protection
- Slot acceptability and timing boundaries
- Rounding behavior and price calculations
- Database integration with proper guards for
DATABASE_URLThe previous review concerns about panic-on-missing-DATABASE_URL have been properly addressed with early returns in the guarded tests (lines 233-236, 267-270).
1 participant
Summary
Comprehensive test coverage improvements across the codebase.
Key Changes
Testing
All new tests pass with database integration when is configured.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores