testing(lightclient): Mocking DA layer #348
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Case
participant MockDA as Mock DAL
participant LightClient as LightClient
participant EventChannel as Event Channel
Test->>MockDA: Setup finalized epochs and responses
Test->>LightClient: Spawn run loop
LightClient->>EventChannel: Publish PrismEvent::Ready
Test->>EventChannel: Await Ready event
Test->>EventChannel: Send UpdateDAHeight event(s)
LightClient->>MockDA: Request finalized epoch data
MockDA-->>LightClient: Return verifiable state transition(s)
LightClient->>EventChannel: Publish Verification events
Test->>EventChannel: Await verification events
Test->>LightClient: Query get_sync_state / assert commitment
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
distractedm1nd
force-pushed
the
verifiable-epochs
branch
from
8d0a875 to
01c8caa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/node_types/lightclient/src/lightclient.rs (1)
49-55: Consider documenting the public fields.Since
SyncStateis now public with public fields, consider adding documentation comments to explain the purpose of each field, especially the distinction betweeninitial_sync_completedandinitial_sync_in_progress.crates/node_types/lightclient/src/tests/mod.rs (1)
110-112: Consider cleanup of spawned tasks.The spawned tasks running
lc.run()will continue running after the test completes. Consider usingtokio::task::JoinHandleand cancelling the task at the end of the test.- spawn(async move { + let handle = spawn(async move { runner.run().await.unwrap(); }); + // At the end of the test: + handle.abort();Also applies to: 154-156, 184-186
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Cargo.toml(1 hunks)crates/da/src/lib.rs(1 hunks)crates/node_types/lightclient/src/lib.rs(1 hunks)crates/node_types/lightclient/src/lightclient.rs(6 hunks)crates/node_types/lightclient/src/tests/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: coverage
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
🔇 Additional comments (7)
crates/node_types/lightclient/src/lib.rs (1)
4-5: LGTM!Standard pattern for organizing test modules with conditional compilation.
crates/da/src/lib.rs (1)
69-69: Good addition for testing infrastructure.Adding
#[automock]enables automatic mock generation for theVerifiableStateTransitiontrait, which is essential for the comprehensive tests being added in this PR.Cargo.toml (1)
187-187: Minor formatting improvement.Removing spaces for consistency with other patch entries.
crates/node_types/lightclient/src/lightclient.rs (3)
43-43: Good refactoring for centralized state management.Embedding the
sync_statewithin theLightClientstruct simplifies the API and provides better encapsulation of synchronization state.Also applies to: 69-70, 78-79
82-84: Good addition for external state access.The
get_sync_state()method provides safe read-only access to the current sync state, which is useful for monitoring and testing.
102-102: Simplified method signatures improve maintainability.Removing the need to pass sync state as parameters makes the API cleaner and reduces the chance of state management errors.
Also applies to: 109-109, 116-116, 155-155, 168-168
crates/node_types/lightclient/src/tests/mod.rs (1)
13-60: Well-designed macro for mocking DA layer behavior.The
mock_da!macro provides a clean DSL for setting up mock DA layer responses with support for success cases, error cases, and multiple epochs at the same height.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/node_types/lightclient/src/tests/mod.rs (1)
164-173: Unreachable code detected.Lines 171-173 are unreachable because the function returns on line 167 when
RecursiveVerificationCompletedis received.Move the sync state assertions before the return statement:
while let Ok(event_info) = sub.recv().await { if let PrismEvent::RecursiveVerificationCompleted { height: _ } = event_info.event { assert_current_commitment!(lc, "d"); + let sync_state = lc.get_sync_state().await; + assert!(sync_state.initial_sync_completed); + assert!(!sync_state.initial_sync_in_progress); return; } } - - let sync_state = lc.get_sync_state().await; - assert!(sync_state.initial_sync_completed); - assert!(!sync_state.initial_sync_in_progress);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/da/src/events.rs(3 hunks)crates/node_types/lightclient/src/lightclient.rs(5 hunks)crates/node_types/lightclient/src/tests/mod.rs(1 hunks)crates/node_types/uniffi-lightclient/src/types.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/da/src/events.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/node_types/lightclient/src/lightclient.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: clippy
- GitHub Check: integration-test
- GitHub Check: coverage
- GitHub Check: unit-test
- GitHub Check: unused dependencies
- GitHub Check: build-and-push-image
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
crates/node_types/uniffi-lightclient/src/types.rs (1)
6-7: Clean implementation of the Ready event variant.The addition of the
Readyevent variant is well-implemented and consistent with the existing event structure. The documentation clearly indicates its purpose for signaling when the LightClient is ready to start syncing.Also applies to: 67-67
crates/node_types/lightclient/src/tests/mod.rs (2)
89-109: Well-structured test setup with proper initialization.The setup function properly initializes all components and waits for the
Readyevent before proceeding, ensuring deterministic test behavior. Good use of Arc for shared ownership and proper event channel setup.
111-156: Comprehensive test coverage for sync scenarios.The tests effectively cover real-time sync, backwards sync, and concurrent sync scenarios. Good use of the
mock_da!macro to create flexible test fixtures and proper assertion of commitments at each stage.
There was a problem hiding this comment.
Pull Request Overview
This PR adds event handling for light client readiness and sync completion, centralizes sync state management under a new SyncState, and introduces extensive asynchronous integration tests using a mocked DA layer.
- Added new Uniffi events (
Ready,SyncCompleted) and updated event-to-enum mappings - Introduced
SyncStatewith concurrency-safe getters and updatedrun/backwards sync logic - Added comprehensive async tests in
lightclient/src/testswith amock_da!macro
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/node_types/uniffi-lightclient/src/types.rs | Added Ready & SyncCompleted events and mappings |
| crates/node_types/lightclient/src/lightclient.rs | Introduced SyncState, updated imports and sync logic |
| crates/node_types/lightclient/src/tests/mod.rs | New async integration tests with mock_da! macro |
| crates/node_types/lightclient/src/lib.rs | Exposed test module |
| crates/da/src/lib.rs | Added #[automock] to VerifiableStateTransition |
| crates/da/src/events.rs | Expanded PrismEvent variants and formatting |
| crates/da/src/celestia/light_client.rs | Unified imports and added a TODO for retries |
Comments suppressed due to low confidence (4)
crates/node_types/uniffi-lightclient/src/types.rs:73
- [nitpick] Mapping
BackwardsSyncStartedtoSyncStartedmay be misleading. Consider renamingUniffiLightClientEvent::SyncStartedto something likeBackwardsSyncStartedfor clarity.
UniffiLightClientEvent::SyncStarted { height }
crates/node_types/lightclient/src/lightclient.rs:5
- The code references
PrismEventandEventPublisherbut neither is imported. Consider importingprism_da::events::{PrismEvent, EventPublisher}to avoid unresolved name errors.
};
crates/node_types/lightclient/src/lightclient.rs:236
- In the backward sync loop, if no epochs are found,
current_heightis never updated and the loop never breaks, leading to an infinite loop. You should eitherbreakor decrementcurrent_heightbefore looping.
light_client.event_pub.send(PrismEvent::BackwardsSyncCompleted { height: None });
crates/node_types/lightclient/src/tests/mod.rs:253
- [nitpick] Relying on
sleepfor test synchronization is fragile. Consider waiting on a specific event or using a notification mechanism instead of a fixed delay.
tokio::time::sleep(Duration::from_secs(1)).await;
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/node_types/lightclient/src/tests/mod.rs (2)
252-255: Replace sleep with deterministic event waiting.The TODO comment indicates this is a known issue. Using
tokio::time::sleepmakes tests non-deterministic and slower.Consider implementing an event-based approach:
- // TODO: Find better way - tokio::time::sleep(Duration::from_secs(1)).await; + // Wait for potential backward sync events and ensure none occur + let timeout = tokio::time::timeout(Duration::from_millis(100), async { + wait_for_event(&mut sub, |event| { + matches!(event, PrismEvent::BackwardsSyncStarted { .. }) + }).await; + }); + assert!(timeout.await.is_err(), "No backward sync should start");
265-271: Replace sleep with deterministic event waiting.Similar to the previous case, this sleep should be replaced with proper event synchronization:
- // TODO: replace with event listener - tokio::time::sleep(Duration::from_secs(1)).await; + // Verify no processing occurs by checking that sync state remains unchanged + let initial_state = lc.get_sync_state().await; + tokio::time::timeout(Duration::from_millis(100), async { + wait_for_sync(&mut sub, 8).await; + }).await.unwrap_err(); // Should timeout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/da/src/celestia/light_client.rs(2 hunks)crates/da/src/events.rs(3 hunks)crates/node_types/lightclient/src/lightclient.rs(8 hunks)crates/node_types/lightclient/src/tests/mod.rs(1 hunks)crates/node_types/uniffi-lightclient/src/types.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/da/src/celestia/light_client.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/da/src/events.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/node_types/lightclient/src/tests/mod.rs (4)
crates/da/src/events.rs (2)
new(110-113)publisher(115-119)crates/node_types/lightclient/src/lightclient.rs (3)
new(58-79)get_latest_commitment(303-305)get_sync_state(81-83)crates/da/src/lib.rs (3)
new(50-52)height(76-76)height(118-120)crates/keys/src/signing_keys.rs (1)
new_ed25519(49-51)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: coverage
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
- GitHub Check: clippy
- GitHub Check: unit-test
- GitHub Check: unused dependencies
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (20)
crates/node_types/uniffi-lightclient/src/types.rs (3)
6-7: LGTM! New Ready event variant is well-documented.The addition of the
Readyvariant with clear documentation aligns with the enhanced synchronization lifecycle.
13-16: LGTM! Optional height in SyncCompleted provides flexibility.Making the height optional in
SyncCompletedis a good design choice that allows signaling completion even when no specific height was reached (e.g., when backward sync finds no epochs).
71-77: LGTM! Event mapping correctly handles new synchronization events.The From implementation properly maps the new
PrismEventvariants:
Ready→ReadyBackwardsSyncStarted→SyncStartedBackwardsSyncCompleted→SyncCompletedThis maintains the external API while supporting the enhanced internal event model.
crates/node_types/lightclient/src/lightclient.rs (8)
42-42: LGTM! Embedding sync state improves encapsulation.Moving the sync state inside the
LightClientstruct eliminates the need to pass it as a parameter and provides better encapsulation.
48-54: LGTM! Public SyncState provides necessary access.Making
SyncStateand its fields public allows external access to sync information while maintaining the internal state management.
81-83: LGTM! Async getter provides safe state access.The
get_sync_statemethod provides a safe way to access the sync state by returning a clone, avoiding potential lock contention issues.
89-89: LGTM! Ready event signals proper initialization.Emitting the
PrismEvent::Readyevent at startup provides a clear signal that the light client is ready to begin processing sync events.
120-131: LGTM! Height validation prevents processing stale updates.The check to skip processing when the new height is lower than the current synced height is a good optimization that prevents unnecessary work on stale data.
243-244: LGTM! Method signature improvement adds boundary checking.Adding the
min_heightparameter allows the method to respect the backward search depth limit, improving the search efficiency.
279-301: LGTM! Enhanced error handling in epoch processing.The updated
process_epochmethod now properly emitsEpochVerificationFailedevents on verification errors, providing better observability of sync issues.
91-111: To determine whetherstart_backward_synctruly awaits the full sync or merely spawns background tasks (which could re-enter the loop), let’s inspect its implementation and anyspawncalls:#!/bin/bash # 1. Locate and display the async fn start_backward_sync implementation rg -n "async fn start_backward_sync" -A15 -B5 crates/node_types/lightclient/src/lightclient.rs # 2. Check for tokio::spawn or other spawn calls in the same file rg -n "spawn" -C3 crates/node_types/lightclient/src/lightclient.rscrates/node_types/lightclient/src/tests/mod.rs (9)
14-61: LGTM! Comprehensive mocking macro with good error handling support.The
mock_da!macro provides excellent flexibility for creating test scenarios:
- Supports success cases with tuple syntax
($h1, $h2)- Handles error cases with
Err(...)and string literal shortcuts- Uses proper trait expectations with
MockVerifiableStateTransitionThe macro design makes tests readable and maintainable.
63-70: LGTM! Helper function provides clean event waiting.The
wait_for_syncfunction abstracts the common pattern of waiting for epoch verification events, making tests more readable.
72-77: LGTM! Commitment assertion macro provides clear test validation.The macro simplifies commitment verification by handling the hash computation and assertion, making test intentions clear.
90-110: LGTM! Test setup function provides proper initialization.The setup function correctly:
- Configures the mock DA layer with event channels
- Creates a light client with proper key generation
- Spawns the run loop asynchronously
- Waits for the Ready event before returning
This ensures tests start with a properly initialized light client.
112-142: LGTM! Comprehensive real-time sync test.The test covers multiple scenarios:
- No epochs at height 3
- Single epoch at heights 4, 5, 8
- Multiple epochs at height 7
- Proper commitment progression
The test validates the forward sync behavior thoroughly.
144-156: LGTM! Backward sync test validates historical epoch discovery.The test confirms that the light client can find and process historical epochs when starting from a height with no data.
158-170: LGTM! Error resilience test validates robust sync behavior.The test confirms that backward sync continues searching when encountering verification errors, finding the first valid epoch.
274-297: LGTM! Complex concurrent sync test validates state coordination.This test covers a sophisticated scenario where incoming epoch processing occurs during backward sync, validating that both operations can complete correctly and the final state is consistent.
299-318: LGTM! Sequential sync test validates proper ordering.The test ensures that backward sync completes before processing new incoming epochs, validating the correct sequencing of sync operations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/node_types/lightclient/src/tests/mod.rs (2)
252-254: Address remaining TODO: Replace sleep with event-based synchronization.This sleep statement still uses the non-deterministic approach that was flagged in previous reviews. Consider waiting for a specific event or state change instead.
- // TODO: Find better way - tokio::time::sleep(Duration::from_secs(1)).await; + // Wait for any potential events to be processed + wait_for_event(&mut sub, |_| false).timeout(Duration::from_millis(100)).await.ok();
266-268: Address remaining TODO: Replace sleep with event-based synchronization.Another instance of the non-deterministic sleep pattern that should be replaced with proper event handling.
Consider adding a mechanism to verify that no events are emitted within a reasonable timeframe, or check the sync state directly after a short deterministic wait.
🧹 Nitpick comments (1)
crates/node_types/lightclient/src/tests/mod.rs (1)
72-77: Consider handling the case where no commitment exists.The macro uses
unwrap()which will panic ifget_latest_commitment()returnsNone. While this is acceptable in test context to indicate test failure, consider making the expectation explicit.macro_rules! assert_current_commitment { ($lc:expr, $expected:expr) => { - let actual = $lc.get_latest_commitment().await.unwrap(); + let actual = $lc.get_latest_commitment().await + .expect("Expected a commitment to be present"); assert_eq!(Digest::hash($expected), actual); }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/node_types/lightclient/src/lightclient.rs(8 hunks)crates/node_types/lightclient/src/tests/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/node_types/lightclient/src/lightclient.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/node_types/lightclient/src/tests/mod.rs (4)
crates/da/src/events.rs (2)
new(110-113)publisher(115-119)crates/da/src/lib.rs (3)
new(50-52)height(76-76)height(118-120)crates/node_types/lightclient/src/lightclient.rs (3)
new(58-79)get_latest_commitment(307-309)get_sync_state(81-83)crates/keys/src/signing_keys.rs (1)
new_ed25519(49-51)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: coverage
- GitHub Check: unit-test
- GitHub Check: build-and-push-image
- GitHub Check: integration-test
- GitHub Check: unused dependencies
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
crates/node_types/lightclient/src/tests/mod.rs (5)
1-12: LGTM! Imports are well-organized and complete.The import statements are properly structured and include all necessary dependencies. The
EpochVerificationErrorimport at line 8 correctly addresses the previous review feedback.
14-61: Well-designed macro for flexible test setup.The
mock_da!macro provides a clean DSL for setting up mock data availability layer scenarios. The macro supports both success cases with tuple syntax and error cases with string or explicit error types. The internal pattern matching logic is sound and the move closures properly capture the necessary data.
90-110: Excellent improvement with deterministic synchronization.The setup function now properly waits for the
Readyevent instead of using arbitrary sleep durations. This addresses the previous review feedback and makes tests more reliable and faster.
273-327: Complex test structure handles concurrent scenarios well.This test properly handles the challenging scenario of concurrent backward sync and incoming epoch processing. The use of
tokio::time::timeoutprevents hanging, and the event tracking logic correctly validates both expected events occur.The small delay at lines 303-304 is justified here for ensuring the receiver is ready before sending events.
112-362: Comprehensive test coverage with good scenario diversity.The test suite covers essential synchronization scenarios including:
- Real-time sync progression
- Backward sync with and without errors
- Error handling and recovery
- Concurrent sync prevention
- Edge cases like underflow and older epoch handling
The tests effectively validate the light client's event-driven architecture and state management.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Tests
Chores