feat: 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. Tip 🌐 Web search-backed reviews and chat
✨ 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
0xnigir1
changed the title
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/data-flow/src/orchestrator.ts (1)
475-476: Add error handling for missingstrategyIdafter enhancementAfter enhancing the event with
strategyId, it's possible thatstrategyIdmay still be undefined if the strategy is not found. Consider adding error handling to manage this scenario gracefully.Add a check for
strategyIdand handle accordingly:event.strategyId = strategyId; +if (!event.strategyId) { + throw new Error(`Strategy ID could not be determined for strategy address ${strategyAddress}`); +} return event;
🧹 Nitpick comments (5)
packages/repository/src/repositories/kysely/eventRegistry.repository.ts (2)
14-18: Add JSDoc comments to the class and constructorTo adhere to the project's coding guidelines and improve code documentation, please add JSDoc comments to the
KyselyEventRegistryRepositoryclass and its constructor. This will enhance maintainability and help other developers understand the purpose and usage of the class.Here is a suggested addition:
+/** + * Repository for managing the events registry using Kysely. + */ export class KyselyEventRegistryRepository implements IEventRegistryRepository<KyselyTransaction> { constructor( private readonly db: Kysely<Database>, private readonly schemaName: string, + ) { + /** + * Creates an instance of KyselyEventRegistryRepository. + * @param db - The Kysely database instance. + * @param schemaName - The schema name to use. + */ + } }
31-35: Document thesaveLastProcessedEventmethod with JSDocThe
saveLastProcessedEventmethod is missing JSDoc comments. Adding documentation will clarify the method's purpose, parameters, and usage, aligning with the coding guidelines.Here is a suggested addition:
+ /** + * Saves the last processed event for a specific chain. + * @param chainId - The chain ID. + * @param event - The new processed event to save. + * @param txConnection - Optional transaction connection for the operation. + */ async saveLastProcessedEvent( chainId: ChainId, event: NewProcessedEvent, txConnection?: KyselyTransaction, ): Promise<void> {packages/data-flow/src/data-loader/dataLoader.ts (1)
41-60: Add JSDoc comments to the constructor andhandlersmappingThe constructor and the
handlersmapping within theDataLoaderclass lack documentation. Adding JSDoc comments will improve code readability and help others understand the dependencies and the purpose of thehandlers.Suggested addition:
constructor( private readonly repositories: { project: IProjectRepository; round: IRoundRepository; application: IApplicationRepository; donation: IDonationRepository; applicationPayout: IApplicationPayoutRepository; + /** + * Repository for managing event registry. + */ eventRegistry: IEventRegistryRepository; }, private readonly transactionManager: ITransactionManager, private readonly logger: ILogger, ) { + /** + * Mapping of changeset types to their corresponding handler functions. + */ this.handlers = { ...createProjectHandlers(repositories.project), ...createRoundHandlers(repositories.round), ...createApplicationHandlers(repositories.application), ...createDonationHandlers(repositories.donation), ...createApplicationPayoutHandlers(repositories.applicationPayout), ...createProcessedEventHandlers(repositories.eventRegistry), }; }packages/data-flow/src/orchestrator.ts (1)
428-453: Clarify the return value ofhandleEventwhen skipping eventsThe
handleEventmethod returnsundefinedwhen an event is skipped due to an unsupported strategy ID. Ensure that the calling code properly handles this case to prevent unexpected behavior.Consider explicitly checking for
undefinedand documenting the behavior:return this.retryHandler.execute( async () => { const changesets = await this.handleEvent(event); if (changesets && changesets.length > 0) { await this.dataLoader.applyChanges([ ...changesets, { type: "InsertProcessedEvent", args: { chainId: this.chainId, processedEvent: { ...event, rawEvent: event, }, }, }, ]); - } + } else { + this.logger.debug("No changesets returned from handleEvent", { + event, + className: Orchestrator.name, + chainId: this.chainId, + }); + } }, { abortSignal: signal }, );packages/data-flow/test/unit/orchestrator.spec.ts (1)
253-278: Avoid duplication in error handling testsIn the test
"saves event outside transaction when processing fails"(lines 253-278), consider refactoring common setup code shared with other tests to improve maintainability and reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/processing/src/services/processing.service.ts(2 hunks)apps/processing/test/unit/processing.service.spec.ts(0 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/data-flow/test/data-loader/handlers/processedEvent.handlers.spec.ts(1 hunks)packages/data-flow/test/unit/orchestrator.spec.ts(9 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)
💤 Files with no reviewable changes (1)
- apps/processing/test/unit/processing.service.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.ts`: Review TypeScript files for adherence to the fo...
**/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoidany; useunknownwhen necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have anexternal.tsfile explicitly listing public exports.
- Usebigintas-is; cast toNumberonly 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.tspackages/data-flow/src/data-loader/handlers/index.tsapps/processing/src/services/processing.service.tspackages/data-flow/src/data-loader/dataLoader.tspackages/repository/src/types/changeset.types.tspackages/data-flow/src/data-loader/handlers/processedEvent.handlers.tspackages/data-flow/test/unit/orchestrator.spec.tspackages/data-flow/test/data-loader/handlers/processedEvent.handlers.spec.tspackages/repository/src/interfaces/eventsRepository.interface.tspackages/repository/src/repositories/kysely/eventRegistry.repository.tspackages/data-flow/test/data-loader/dataLoader.spec.tspackages/data-flow/src/orchestrator.tspackages/data-flow/src/retroactiveProcessor.ts
`**/services/**/*.ts`: Review service files with the followi...
**/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.,AggregatorServiceor domain-specific names likeMetricsService).
apps/processing/src/services/processing.service.ts
`**/*.spec.ts`: Review the unit test files with the followin...
**/*.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.
packages/data-flow/test/unit/orchestrator.spec.tspackages/data-flow/test/data-loader/handlers/processedEvent.handlers.spec.tspackages/data-flow/test/data-loader/dataLoader.spec.ts
🔇 Additional comments (29)
packages/repository/src/repositories/kysely/eventRegistry.repository.ts (1)
38-51: Ensure consistent use of transactions in database operationsWhile the optional
txConnectionparameter allows for transactional operations, it's important to ensure that all database interactions within the method respect the transaction context. Verify that any additional queries or operations within this method or future extensions usetxConnection || this.dbconsistently to maintain transactional integrity.packages/data-flow/src/data-loader/dataLoader.ts (1)
58-58: Confirm that thecreateProcessedEventHandlersfunction is correctly integratedVerify that the
createProcessedEventHandlersfunction fromprocessedEvent.handlers.jsis properly imported and integrated into thehandlersmapping. Ensure that it correctly registers the necessary handlers for processing events.packages/data-flow/src/orchestrator.ts (2)
80-82: Update theeventsRegistrytype to match new interfaceThe type of
eventsRegistryis changed fromIEventsRegistrytoIEventRegistryRepository. Ensure that all method calls and properties accessed oneventsRegistryare compatible with the new interface.
178-182: EnsuresaveLastProcessedEventcorrectly handles errorsIn the catch block, the code attempts to save the last processed event even when an error occurs. Confirm that this behavior is intentional and does not result in inconsistent state, especially if the error is due to issues with the event data.
packages/data-flow/test/unit/orchestrator.spec.ts (10)
13-13: Approve addition ofIEventRegistryRepositoryimportThe import of
IEventRegistryRepositoryat line 13 correctly brings in the necessary interface for the updated event registry usage in the tests.
18-18: Approve addition ofRoundNotFoundForIdimportIncluding
RoundNotFoundForIdin the imports allows the test suite to handle specific error cases related to rounds not found by ID.
39-39: Update import to includeInvalidEventThe import statement now includes
InvalidEvent, which is required for handling and testing invalid event scenarios within the orchestrator.
63-63: Update mock to useIEventRegistryRepositoryChanging
mockEventsRegistryto typeIEventRegistryRepositoryensures consistency with the updated interface and allows for accurate mocking of event registry behavior.
188-189: Verify thatapplyChangesis called the correct number of timesThe assertion at line 188 checks that
applyChangesis called twice, which aligns with the two events being processed. The additional assertion at line 189 confirms thatsaveLastProcessedEventis not called directly, ensuring that event saving is handled within the transaction.
212-251: Ensure test descriptions are clear and preciseThe test case
"includes InsertProcessedEvent changeset in transaction"(lines 212-251) clearly conveys the intent to check that theInsertProcessedEventchangeset is included within a transaction. The description aligns with best practices for test naming.
280-312: Approve transaction rollback test logicThe test
"rolls back transaction on error"(lines 280-312) effectively verifies that the transaction is rolled back when an error occurs duringapplyChanges. The mock implementations and assertions accurately simulate and confirm the expected behavior.
375-377: Useexpect.arrayContainingfor flexible matchingThe use of
expect.arrayContaining(changesets)(lines 375-377) allows the test to verify that the expected changesets are included in the array without requiring an exact match of the entire array, improving test robustness.
494-516: Consistent inclusion ofInsertProcessedEventin changesetsIn the tests starting at line 494, ensure that
InsertProcessedEventis correctly appended to the changesets when processing strategy events. This maintains consistency in how processed events are handled across different event types.
734-734: Correct verification ofsaveLastProcessedEventcall countThe assertion at line 734 confirms that
saveLastProcessedEventis called once, which is appropriate given the processing flow in the error handling test case.packages/data-flow/src/data-loader/handlers/index.ts (1)
6-6: IncludeprocessedEvent.handlers.jsin module exportsAdding
export * from "./processedEvent.handlers.js";ensures that theprocessedEventhandlers are exported and available for use, integrating the new handlers into the data loader module.packages/repository/src/interfaces/eventsRepository.interface.ts (3)
3-3: ImportTransactionConnectionfor transaction supportIncluding
TransactionConnectionin the imports (line 3) prepares the interface to handle transaction connections, which is necessary for the updated method signatures.
5-7: Enhance interface with genericTxConnectiontype parameterIntroducing a generic type parameter
TxConnection(lines 5-7) toIEventRegistryRepositoryallows for greater flexibility in specifying the transaction connection type, adhering to SOLID principles.
20-24: UpdatesaveLastProcessedEventto accept optional transaction connectionModifying the
saveLastProcessedEventmethod signature to include an optionaltxConnectionparameter (lines 20-24) enables the method to participate in transactions when provided, enhancing the repository's transactional capabilities.packages/data-flow/src/data-loader/handlers/processedEvent.handlers.ts (2)
12-14: LGTM! Well-structured type definition.The use of mapped types ensures type safety and maintainability.
22-32: LGTM! Clean and type-safe implementation.The implementation follows TypeScript best practices:
- Uses dependency injection for the repository
- Leverages the
satisfiesoperator for type safety- Properly handles the optional transaction connection
packages/data-flow/test/data-loader/handlers/processedEvent.handlers.spec.ts (2)
18-41: LGTM! Comprehensive test for successful event saving.The test properly verifies:
- Repository method call with correct parameters
- Transaction handling
43-67: LGTM! Thorough error handling test.Good coverage of error propagation scenario.
packages/repository/src/external.ts (1)
51-51: LGTM! Consistent export pattern.The new type export follows the established pattern for changeset types.
packages/repository/src/types/changeset.types.ts (3)
170-176: LGTM! Well-structured type definition.The new ProcessedEventChangeset type follows the established pattern and includes all necessary fields.
178-184: LGTM! Proper type composition.The Changeset union type is correctly updated to include the new ProcessedEventChangeset.
21-21: Consider addressing the TODO comment.The comment suggests a potential refactoring of changeset types. Consider creating a tracking issue for this technical debt.
Would you like me to create an issue to track this refactoring task?
packages/data-flow/test/data-loader/dataLoader.spec.ts (1)
45-47: 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
apps/processing/src/services/processing.service.ts (1)
80-81: LGTM! The changes improve reliability by removing the in-memory cache layer.The direct use of DatabaseEventRegistry ensures data consistency and reduces complexity.
Also applies to: 93-94
packages/data-flow/src/retroactiveProcessor.ts (1)
65-65: LGTM! The changes are consistent with the new event registry interface.The type changes from IEventsRegistry to IEventRegistryRepository are consistently applied across the class property, constructor parameter, and DataLoader integration.
Also applies to: 86-87, 109-110
| const changesets = await this.handleEvent(event!); | ||
| if (changesets) { | ||
| await this.dataLoader.applyChanges([ | ||
| ...changesets, | ||
| { | ||
| type: "InsertProcessedEvent", | ||
| args: { | ||
| chainId: this.chainId, | ||
| processedEvent: { | ||
| ...event!, | ||
| rawEvent: event, | ||
| }, | ||
| }, | ||
| }, | ||
| ]); | ||
| } | ||
| }, | ||
| { abortSignal: signal }, | ||
| ); |
There was a problem hiding this comment.
Handle potential null reference in event
There is a risk of a null or undefined reference when using event! in the async function passed to this.retryHandler.execute. Ensure that event is properly checked or handled to avoid runtime errors.
Apply this diff to add null checking:
await this.retryHandler.execute(
async () => {
- const changesets = await this.handleEvent(event!);
+ if (!event) {
+ throw new Error("Event is undefined or null");
+ }
+ const changesets = await this.handleEvent(event);
if (changesets) {
await this.dataLoader.applyChanges([
...changesets,
{
type: "InsertProcessedEvent",
args: {
chainId: this.chainId,
processedEvent: {
- ...event!,
+ ...event,
rawEvent: event,
},
},
},
]);
}
},
{ abortSignal: signal },
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const changesets = await this.handleEvent(event!); | |
| if (changesets) { | |
| await this.dataLoader.applyChanges([ | |
| ...changesets, | |
| { | |
| type: "InsertProcessedEvent", | |
| args: { | |
| chainId: this.chainId, | |
| processedEvent: { | |
| ...event!, | |
| rawEvent: event, | |
| }, | |
| }, | |
| }, | |
| ]); | |
| } | |
| }, | |
| { abortSignal: signal }, | |
| ); | |
| await this.retryHandler.execute( | |
| async () => { | |
| if (!event) { | |
| throw new Error("Event is undefined or null"); | |
| } | |
| const changesets = await this.handleEvent(event); | |
| if (changesets) { | |
| await this.dataLoader.applyChanges([ | |
| ...changesets, | |
| { | |
| type: "InsertProcessedEvent", | |
| args: { | |
| chainId: this.chainId, | |
| processedEvent: { | |
| ...event, | |
| rawEvent: event, | |
| }, | |
| }, | |
| }, | |
| ]); | |
| } | |
| }, | |
| { abortSignal: signal }, | |
| ); |
There was a problem hiding this comment.
look at the prior lines, if event is undefined or null, we continue the loop prior to the the retryHandler execute method. this doesn't need to be fixed
There was a problem hiding this comment.
@0xnigir1 You're absolutely right. I apologize for the unnecessary suggestion. The code already has a proper null check before the retryHandler.execute call, which ensures that event is never null or undefined at that point.
✏️ Learnings added
Learnt from: 0xnigir1
PR: defi-wonderland/grants-stack-indexer-v2#60
File: packages/data-flow/src/orchestrator.ts:157-175
Timestamp: 2025-02-06T15:32:39.661Z
Learning: In the Orchestrator class, when processing events in a loop, null checking is performed before the retryHandler.execute call. If the event is null/undefined, the loop continues, ensuring that event is always defined within the retry handler's execution block.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🤖 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