feat(wip): make saveLastProcessedEvent part of transaction #60
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive refactoring of event registry handling across multiple packages in the project. The changes focus on replacing the in-memory cached event registry with a direct database-backed event registry implementation. Key modifications include updating the The changes streamline event processing by removing the caching layer and providing more flexible transaction handling. The modifications span several packages, including data-flow, repository, and processing services, with updates to interfaces, repositories, and type definitions to support the new event registry approach. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/data-flow/src/orchestrator.ts (2)
Line range hint
99-121: Consider aligning property naming for consistency.
Within the constructor, you defineeventsRegistry: IEventRegistryRepositorybut useeventRegistryin theDataLoaderconfiguration. Consider keeping the naming consistent to avoid confusion:- eventRegistry: this.eventsRegistry, + eventsRegistry: this.eventsRegistry,
157-172: Apply changesets efficiently.
The logic to apply changesets and then insert the processed event is clear. As a small optimization, consider directly pushing theInsertProcessedEventchangeset to the array before callingapplyChanges. This slightly reduces the spread operation overhead in tight loops.const changesets = await this.handleEvent(event!); if (changesets) { + changesets.push({ + type: "InsertProcessedEvent", + args: { + chainId: this.chainId, + processedEvent: { + ...event!, + rawEvent: event + }, + }, + }); await this.dataLoader.applyChanges(changesets); }packages/data-flow/src/data-loader/handlers/processedEvent.handlers.ts (1)
8-14: Enhance JSDoc comments for ProcessedEventHandlers type.The current JSDoc comment incorrectly refers to "Application" operations instead of "ProcessedEvent" operations.
/** - * Collection of handlers for application-related operations. - * Each handler corresponds to a specific Application changeset type. + * Collection of handlers for processed event operations. + * Each handler corresponds to a specific ProcessedEvent changeset type. + * @template K - The type of ProcessedEventChangeset */packages/data-flow/src/retroactiveProcessor.ts (1)
Line range hint
71-81: Update JSDoc for constructor parameters.The JSDoc for the
registriesparameter should be updated to reflect thateventsRegistryis now of typeIEventRegistryRepository.* @param registries - Event and strategy registries for tracking processing state + * @param registries.eventsRegistry - Repository for managing processed events * @param registries.strategyRegistry - Registry for tracking strategy processing state * @param registries.checkpointRepository - Repository for managing processing checkpoints
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/processing/src/services/processing.service.ts(2 hunks)packages/data-flow/src/data-loader/dataLoader.ts(4 hunks)packages/data-flow/src/data-loader/handlers/index.ts(1 hunks)packages/data-flow/src/data-loader/handlers/processedEvent.handlers.ts(1 hunks)packages/data-flow/src/orchestrator.ts(8 hunks)packages/data-flow/src/retroactiveProcessor.ts(4 hunks)packages/data-flow/test/data-loader/dataLoader.spec.ts(3 hunks)packages/repository/src/external.ts(1 hunks)packages/repository/src/interfaces/eventsRepository.interface.ts(2 hunks)packages/repository/src/repositories/kysely/eventRegistry.repository.ts(2 hunks)packages/repository/src/types/changeset.types.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
packages/data-flow/src/data-loader/handlers/index.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/repository/src/external.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/repository/src/types/changeset.types.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/data-flow/test/data-loader/dataLoader.spec.ts (2)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
Pattern **/*.spec.ts: Review the unit test files with the following guidelines:
- Avoid using the word "should" in test descriptions.
- Ensure descriptive test names convey the intent of each test.
- Validate adherence to the Mocha/Chai/Jest test library best practices.
- Be concise and avoid overly nitpicky feedback outside of these best practices.
apps/processing/src/services/processing.service.ts (2)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
Pattern **/services/**/*.ts: Review service files with the following considerations:
- A Service encapsulates a broader business workflow and might orchestrate various components or interact with multiple data sources or APIs.
- Ensure proper composability: Services should use Providers for data/resource access and apply business/orchestration logic.
- Validate naming conventions for aggregating services (e.g., AggregatorService or domain-specific names like MetricsService).
- path: "/providers//*.ts"
instructions: |
Review provider files for the following:
- Providers should supply narrowly scoped data/resources.
- Ensure classes interacting with metadata sources (e.g., GitHub, JSON files, IPFS) implement the IMetadataProvider interface and follow naming conventions (e.g., GithubProvider, JsonFileProvider).
- Be concise and avoid overly nitpicky feedback outside of these best practices.
packages/data-flow/src/data-loader/dataLoader.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/repository/src/interfaces/eventsRepository.interface.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/data-flow/src/data-loader/handlers/processedEvent.handlers.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/repository/src/repositories/kysely/eventRegistry.repository.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/data-flow/src/orchestrator.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/data-flow/src/retroactiveProcessor.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
🔇 Additional comments (19)
packages/data-flow/src/orchestrator.ts (7)
10-15: Imports look consistent and aligned with the new repository usage.
No concerns with the updated import statements forChangeset,IEventRegistryRepository,RoundNotFound, andRoundNotFoundForId.
37-37: Type imports are properly declared.
The usage ofIEventsFetcherandIStrategyRegistryvia type imports is correct and follows best practices.
80-80: Field declaration is consistent with the new interface.
Declaringprivate readonly eventsRegistry: IEventRegistryRepository;aligns with the updated repository-based approach. This is clear and maintains strong typing.
428-430: Return type provides flexibility.
Shifting fromPromise<void>toPromise<Changeset[] | undefined>is consistent with storing processed changes downstream. This improves clarity and data flow orchestration.
448-448: Skipping unsupported strategy events.
Returningundefinedhere clearly indicates no changesets are generated for unrecognized strategy IDs.
452-452: Forwarding events to the processor.
The direct return ofthis.eventsProcessor.processEvent(event);is straightforward and maintains a clean separation of concerns between Orchestrator logic and event processing.
177-182: Verify partial-processing intent.
Saving the last processed event on error can skip a non-fully-processed event. Confirm if marking this event as processed is intended when an error occurs, as it may lead to data inconsistencies.✅ Verification successful
Partial-processing behavior is intentional and correct
The current implementation correctly saves the last processed event to maintain processing order and prevent infinite loops on failed events. This is a standard pattern in event processing systems where progress tracking is separate from success/failure handling. The behavior is well-tested and changing it could lead to processing getting stuck.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify usage of saveLastProcessedEvent across the codebase # to confirm partial-processing is intentional rg -A 5 'saveLastProcessedEvent'Length of output: 21701
packages/data-flow/src/data-loader/handlers/index.ts (1)
6-6: Exporting processed event handlers for a clearer API surface.
Exporting fromprocessedEvent.handlers.jscentralizes handler definitions and improves discoverability.packages/repository/src/interfaces/eventsRepository.interface.ts (3)
3-3: Importing extended types for improved repository functionality.
IncludingTransactionConnectionfosters type-safe transaction support across the repository layer.
5-7: Generic interface extension ensures flexible transaction handling.
Defining<TxConnection extends TransactionConnection = TransactionConnection>is a good pattern for extending or customizing transaction types in different environments.
20-24: Optional txConnection for transactional operations.
Allowing an optionaltxConnectionparameter aligns with the PR’s goal of integrating last processed events into transactions without mandating it in all cases.packages/data-flow/src/data-loader/handlers/processedEvent.handlers.ts (1)
22-32: Well-structured handler implementation with proper type safety!The implementation is clean, type-safe, and follows dependency injection principles. Good use of the
satisfiesoperator to ensure type compatibility.packages/repository/src/repositories/kysely/eventRegistry.repository.ts (1)
Line range hint
31-57: Clean implementation of transaction support!The changes effectively integrate transaction support while maintaining existing functionality:
- Good use of TypeScript generics for transaction typing
- Clean pattern for fallback to this.db when no transaction is provided
- Comprehensive error handling with proper context
packages/repository/src/external.ts (1)
51-51: Clean addition of ProcessedEventChangeset export.The export follows the established pattern and TypeScript guidelines for explicit public exports.
packages/data-flow/src/data-loader/dataLoader.ts (2)
Line range hint
77-77: Consider implementing Zod validation as mentioned in TODO.The TODO comment suggests adding Zod validation for changeset args. This aligns with the TypeScript guidelines about runtime type-checking and would enhance type safety.
Would you like me to help implement the Zod validation schema for the ProcessedEventChangeset args?
Line range hint
46-59: Clean integration of event registry and handlers!The changes effectively integrate the event registry:
- Follows dependency injection pattern
- Clean integration of new handlers
- Maintains consistent structure with other repositories
packages/repository/src/types/changeset.types.ts (1)
170-176: LGTM! ProcessedEventChangeset type is well-defined.The type follows the established changeset pattern and includes all necessary fields for tracking processed events.
apps/processing/src/services/processing.service.ts (1)
80-82: LGTM! Successfully removed InMemoryCache dependency.The changes correctly implement direct usage of DatabaseEventRegistry, simplifying the event handling flow while maintaining proper dependency injection.
Also applies to: 93-96
packages/data-flow/src/retroactiveProcessor.ts (1)
65-65: LGTM! Type declarations properly updated.The changes correctly update the event registry types and maintain proper dependency injection throughout the class.
Also applies to: 86-86, 109-109
| const mockEventRegistryRepository = { | ||
| saveLastProcessedEvent: vi.fn(), | ||
| } as unknown as IEventRegistryRepository; |
There was a problem hiding this comment.
💡 Codebase verification
Add test cases for ProcessedEventChangeset handling in dataLoader.spec.ts
The review comment is valid. While saveLastProcessedEvent is mocked in the test file, there are no specific test cases verifying the DataLoader's handling of ProcessedEventChangeset. The handler implementation exists in processedEvent.handlers.ts, but its behavior needs to be tested.
Suggested test cases to add:
- Successful processing of InsertProcessedEvent changeset
- Error handling during event processing
- Transaction handling with the repository
🔗 Analysis chain
Add test cases for ProcessedEventChangeset handling.
While the mock is correctly set up, there are no test cases verifying the behavior of saveLastProcessedEvent. Consider adding test cases for:
- Successful processing of InsertProcessedEvent changeset
- Error handling during event processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any test cases for ProcessedEventChangeset
rg -A 5 "InsertProcessedEvent" "packages/data-flow/test/**/*.spec.ts"
Length of output: 146
Script:
#!/bin/bash
# Search for test files and relevant patterns
echo "=== Searching for saveLastProcessedEvent tests ==="
rg -A 5 "saveLastProcessedEvent" --type ts
echo -e "\n=== Searching for ProcessedEventChangeset tests ==="
rg -A 5 "ProcessedEventChangeset" --type ts
echo -e "\n=== Looking for related test files ==="
fd "dataLoader.spec.ts"
Length of output: 24168
🤖 Linear
Closes GIT-232
Description
We want to make last processed event part of the transaction on the happy path, so we increase reliability and error handling of the system.
For this we are:
saveLastProcessedEventmethodChecklist before requesting a review
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates