Conversation
📝 WalkthroughWalkthroughThis changeset integrates a caching mechanism for strategy timings throughout the codebase. A new cache class, Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
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:
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
Documentation and Community
|
![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 (5)
packages/repository/src/types/strategyTimings.types.ts (1)
1-6: Type definition for StrategyTimings looks good, but consider improving type safety for 'timings'The StrategyTimings type is well-structured with clear field definitions. However, using
unknownfor thetimingsproperty reduces type safety. Consider using a more specific type that represents the actual structure of the timing data that will be stored.export type StrategyTimings = { address: string; strategyId: string; - timings: unknown; + timings: Record<string, number>; // or a more specific interface that represents your timing data createdAt: Date; };scripts/bootstrap/src/strategyTimings.script.ts (1)
227-279: Consider removing the unused iteration variable.
The variablesreturned frompMapIterableis never used in thefor awaitloop, causing a lint warning. You can remove or rename it to_if it's intentionally unused.Apply this diff to remove the unused variable:
-for await (const s of pMapIterable(strategyAddressId, async (strategyAddressId) => { +for await (pMapIterable(strategyAddressId, async (strategyAddressId) => { try { // ... } catch (error) { // ... } })) { // ... }🧰 Tools
🪛 GitHub Check: lint / Run Linters
[failure] 229-229:
's' is assigned a value but never usedpackages/repository/src/kysely/repositories/strategyTimings.repository.ts (3)
20-42: Clarify “className” reference for better error context.
You’re usingclassName: KyselyMetadataCache.namein thehandlePostgresErrorcall, but the class isKyselyStrategyTimingsCache. Updating it would avoid confusion in logs.Here’s a suggested fix:
- className: KyselyMetadataCache.name, + className: KyselyStrategyTimingsCache.name,
20-42: Consider returning parsed JSON for thetimingsfield.
Thegetmethod returns a database row withtimingsas text. If you intendtimingsto be an object, parse it here for consistency. If not, consider renaming the field to clarify it remains unparsed JSON.
45-77: Avoid overwriting the creation timestamp on upsert.
In theonConflictclause, you updatecreatedAttonew Date(), which could erase the original creation time. Introduce a separateupdatedAtfield if you intend to track modifications while preserving the original creation date.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
apps/processing/src/services/sharedDependencies.service.ts(3 hunks)package.json(1 hunks)packages/data-flow/src/orchestrator.ts(1 hunks)packages/data-flow/src/types/index.ts(2 hunks)packages/data-flow/test/integration/helpers/dependencies.ts(2 hunks)packages/data-flow/test/unit/orchestrator.spec.ts(3 hunks)packages/data-flow/test/unit/retroactiveProcessor.spec.ts(2 hunks)packages/indexer-client/src/providers/envioIndexerClient.ts(1 hunks)packages/indexer-client/src/types/indexerClient.types.ts(1 hunks)packages/processors/src/processors/allo/handlers/poolCreated.handler.ts(3 hunks)packages/processors/src/types/processor.types.ts(2 hunks)packages/processors/test/allo/allo.processor.spec.ts(2 hunks)packages/processors/test/allo/handlers/poolCreated.handler.spec.ts(10 hunks)packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts(2 hunks)packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts(2 hunks)packages/processors/test/registry/handlers/profileCreated.handler.spec.ts(2 hunks)packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts(2 hunks)packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts(2 hunks)packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts(2 hunks)packages/processors/test/strategy/strategy.processor.spec.ts(2 hunks)packages/processors/test/strategy/strategyHandler.factory.spec.ts(3 hunks)packages/repository/src/db/connection.ts(2 hunks)packages/repository/src/external.ts(1 hunks)packages/repository/src/kysely/repositories/index.ts(1 hunks)packages/repository/src/kysely/repositories/strategyTimings.repository.ts(1 hunks)packages/repository/src/types/index.ts(1 hunks)packages/repository/src/types/strategyTimings.types.ts(1 hunks)scripts/bootstrap/package.json(2 hunks)scripts/bootstrap/src/schemas/index.ts(1 hunks)scripts/bootstrap/src/strategyTimings.script.ts(1 hunks)scripts/migrations/package.json(1 hunks)scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts(0 hunks)scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts(2 hunks)scripts/migrations/src/utils/parsing.ts(2 hunks)
💤 Files with no reviewable changes (1)
- scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts
🧰 Additional context used
📓 Path-based instructions (5)
`**/*.ts`:
**/*.ts:
packages/repository/src/kysely/repositories/index.tspackages/repository/src/types/index.tspackages/repository/src/external.tspackages/data-flow/test/integration/helpers/dependencies.tspackages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.tspackages/indexer-client/src/types/indexerClient.types.tspackages/data-flow/src/orchestrator.tspackages/repository/src/db/connection.tspackages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.tspackages/data-flow/test/unit/orchestrator.spec.tsscripts/migrations/src/utils/parsing.tsscripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.tspackages/repository/src/types/strategyTimings.types.tspackages/indexer-client/src/providers/envioIndexerClient.tsscripts/bootstrap/src/schemas/index.tspackages/processors/test/strategy/strategyHandler.factory.spec.tspackages/processors/test/registry/handlers/profileCreated.handler.spec.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.tspackages/data-flow/src/types/index.tspackages/processors/src/types/processor.types.tspackages/data-flow/test/unit/retroactiveProcessor.spec.tsscripts/bootstrap/src/strategyTimings.script.tspackages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.tspackages/processors/test/allo/allo.processor.spec.tsapps/processing/src/services/sharedDependencies.service.tspackages/processors/test/strategy/strategy.processor.spec.tspackages/processors/src/processors/allo/handlers/poolCreated.handler.tspackages/repository/src/kysely/repositories/strategyTimings.repository.tspackages/processors/test/strategy/directAllocation/directAllocation.handler.spec.tspackages/processors/test/allo/handlers/poolCreated.handler.spec.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/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.tspackages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.tspackages/data-flow/test/unit/orchestrator.spec.tspackages/processors/test/strategy/strategyHandler.factory.spec.tspackages/processors/test/registry/handlers/profileCreated.handler.spec.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.tspackages/data-flow/test/unit/retroactiveProcessor.spec.tspackages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.tspackages/processors/test/allo/allo.processor.spec.tspackages/processors/test/strategy/strategy.processor.spec.tspackages/processors/test/strategy/directAllocation/directAllocation.handler.spec.tspackages/processors/test/allo/handlers/poolCreated.handler.spec.ts
`scripts/**/*.ts`: Ensure scripts: - Use `process.cwd()` f...
scripts/**/*.ts: Ensure scripts:
- Use
process.cwd()for root references.- Follow folder conventions (
infra/for infra scripts,utilities/for utilities).- Are organized in
package.jsonwithscript:infra:{name}orscript:util:{name}.- Be concise and avoid overly nitpicky feedback outside of these best practices.
scripts/migrations/src/utils/parsing.tsscripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.tsscripts/bootstrap/src/schemas/index.tsscripts/bootstrap/src/strategyTimings.script.ts
`**/providers/**/*.ts`: Review provider files for the follow...
**/providers/**/*.ts: 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 theIMetadataProviderinterface and follow
naming conventions (e.g.,GithubProvider,JsonFileProvider).- Be concise and avoid overly nitpicky feedback outside of these best practices.
packages/indexer-client/src/providers/envioIndexerClient.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/sharedDependencies.service.ts
🪛 GitHub Check: lint / Run Linters
scripts/bootstrap/src/strategyTimings.script.ts
[failure] 167-167:
'strategy' is assigned a value but never used
[failure] 229-229:
's' is assigned a value but never used
🔇 Additional comments (71)
packages/repository/src/kysely/repositories/index.ts (1)
14-14: New strategy timings repository export added correctly.The addition of the export for the strategy timings repository follows the consistent pattern used for other repository exports in this file. This change correctly makes the new
strategyTimings.repository.jsmodule available to other parts of the application.packages/repository/src/types/index.ts (1)
15-15: Strategy timings types export added properly.The export statement for strategy timings types follows the established pattern in this file. This change correctly exposes the types defined in
strategyTimings.types.jsto other modules that import from this index.packages/indexer-client/src/types/indexerClient.types.ts (1)
36-39: Well-documented eventName filter parameter added.The addition of the optional
eventNameparameter to theGetEventsFilterstype is properly documented with a JSDoc comment. This enhancement allows for more precise filtering when fetching events, which aligns with the PR's goal of improving performance through better caching mechanisms.packages/repository/src/external.ts (2)
95-95: Strategy timings types exported correctly.The export statement adds three new types (
StrategyTimings,NewStrategyTimings, andPartialStrategyTimings) from the internal module, making them available for use in other parts of the application. This follows the established pattern for type exports in this file.
102-102: KyselyStrategyTimingsCache implementation exported properly.The export of
KyselyStrategyTimingsCacheis correctly added to the list of cache implementations, following the same pattern as other cache exports. This makes the new cache implementation available for use in the application, supporting the PR's objective of implementing a caching mechanism for strategy timestamps.packages/repository/src/db/connection.ts (2)
33-33: New table type import looks goodThis import adds the StrategyTimings table type needed for the database interface extension, supporting the caching mechanism for strategy timings.
87-87: Database interface extension is properly implementedThe addition of
strategyTimings: StrategyTimingsTableto the Database interface correctly establishes the relationship between the Kysely query builder and the new strategy timings table, allowing for typed database operations.packages/data-flow/test/integration/helpers/dependencies.ts (2)
26-26: Correctly updated the mock repository type signatureThis addition to the return type ensures type safety when using the mock repositories in tests.
91-94: Mock implementation follows best practicesThe implementation of the strategy timings repository mock with
getandsetmethods usingvi.fn()follows the established pattern in the file and provides the necessary methods for testing.scripts/migrations/package.json (1)
19-20: Migration folder path appropriately updatedThe change from
external_services_cachetoprocessing_cachefor both cache migration scripts reflects a more appropriate organization of migration scripts, aligning with the purpose of the strategy timings cache.apps/processing/src/services/sharedDependencies.service.ts (3)
24-24: Import added for new cache implementationThe import of KyselyStrategyTimingsCache is correctly added to access the new cache implementation.
140-143: Strategy timings repository instantiated with proper parametersThe instantiation of KyselyStrategyTimingsCache follows the established pattern for other repositories, passing the database connection and schema as required parameters.
173-173: Repository correctly exposed in core dependenciesThe strategy timings repository is properly added to the core object, making it accessible to other components that depend on the shared dependencies service.
packages/repository/src/types/strategyTimings.types.ts (1)
8-9: LGTM! Good pattern for type derivativesThe derived types for NewStrategyTimings and PartialStrategyTimings follow best practices by using TypeScript utility types like Omit and Partial.
scripts/migrations/src/utils/parsing.ts (2)
16-16: LGTM! Updated validation rule for migrations folderThe refinement condition has been correctly updated to include the new "processing_cache" folder option.
34-34: LGTM! Consistent update to CLI optionsThe choices array for command-line arguments has been updated to match the validation rule change, maintaining consistency.
packages/data-flow/src/types/index.ts (2)
8-9: LGTM! Appropriate imports for the new cache typesThe necessary imports have been added for the strategy timings cache implementation.
Also applies to: 14-16
39-39: LGTM! Well-typed repository addition to CoreDependenciesThe strategyTimingsRepository has been correctly added to CoreDependencies with proper typing as
ICache<Address, StrategyTimings>.package.json (2)
11-11: LGTM! Updated bootstrap:all script to include strategy timingsThe bootstrap:all script has been correctly updated to include the new strategy timings bootstrap process alongside metadata and pricing.
14-14: LGTM! Added dedicated bootstrap script for strategy timingsA specific script for bootstrapping strategy timings has been added, following the established naming pattern and implementation approach of other bootstrap scripts.
packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts (1)
7-9: Implementation of strategy timings cache looks goodThe addition of the
strategyTimingsRepositorydependency is correctly integrated into the test setup with proper typing usingICache<Address, StrategyTimings>.Also applies to: 11-12, 14-15, 68-69
packages/data-flow/src/orchestrator.ts (1)
199-204: Performance improvement in logging frequencyGood optimization to reduce logging frequency by only logging when
processedEventsis a multiple of 1000. This change aligns with the PR objective to improve processing speed by reducing overhead from excessive logging operations.packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts (1)
7-9: Strategy timings cache implementation correctly integratedThe test setup properly initializes the processor with the new
strategyTimingsRepositorydependency, ensuring the caching mechanism is available during testing.Also applies to: 11-12, 13-13, 57-58
packages/processors/src/types/processor.types.ts (1)
3-5: Core type definition for strategy timings cache looks goodThe
ProcessorDependenciestype is properly extended with thestrategyTimingsRepositoryproperty, ensuring consistent access to the caching mechanism across all processors. The type parametersICache<Address, StrategyTimings>clearly define the cache's key-value structure.Also applies to: 6-8, 11-11, 20-21
packages/data-flow/test/unit/orchestrator.spec.ts (2)
13-13: Appropriate imports for strategy timing cache.The additions of
ICacheandStrategyTimingsimports support the PR objective of implementing strategy timing caching.Also applies to: 22-22
128-128: Good implementation of strategy timings repository in test dependencies.The
strategyTimingsRepositoryis properly added to the dependencies object with the correct typing asICache<Address, StrategyTimings>, consistent with the PR's caching mechanism objective.packages/processors/test/strategy/strategyHandler.factory.spec.ts (2)
1-1: Necessary imports for strategy timing cache implementation.The addition of the
Addresstype import and repository types likeICacheandStrategyTimingsare correctly added to support the strategy timing cache feature.Also applies to: 9-10, 12-13
49-49: Correctly added strategyTimingsRepository to dependencies.The
strategyTimingsRepositoryis properly included in themockProcessorDependenciesobject, aligning with the caching implementation goal.scripts/bootstrap/package.json (2)
18-18: Added bootstrap script for strategy timings.The new
bootstrap:strategyTimingsscript enables running the strategy timings bootstrap process, which aligns with the PR objective of implementing a caching mechanism for strategy timestamps.
30-31: Added necessary dependencies for strategy timings implementation.The new dependencies (
@grants-stack-indexer/chain-providers,@grants-stack-indexer/processors, andviem) are correctly added to support the strategy timings feature.Also applies to: 35-36, 42-43
scripts/bootstrap/src/schemas/index.ts (2)
15-19: Well-structured schema for chain validation.The new
chainSchemadefines a clear structure for chain configuration with appropriate validations for RPC URLs, chain ID, and name.
28-32: Good implementation of chain ID uniqueness validation.The refinement to ensure chain IDs are unique is a valuable addition that prevents potential issues with duplicate chain configurations.
packages/data-flow/test/unit/retroactiveProcessor.spec.ts (2)
9-9: Import additions look good.The addition of
ICache,StrategyTimings, andAddressimports is appropriate for supporting the new caching mechanism.Also applies to: 17-17, 20-20
153-153: Well integrated dependency for strategy timings cache.The
strategyTimingsRepositoryis correctly added to the dependencies object, following the same pattern as other repositories. This implementation allows the RetroactiveProcessor to use the caching mechanism for strategy timings.packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts (1)
8-8: Import updates correctly structured.The inclusion of
ICacheandStrategyTimingstypes and the reorganization of imports from@grants-stack-indexer/sharedfollow good import structuring practices.Also applies to: 11-11, 13-19
packages/indexer-client/src/providers/envioIndexerClient.ts (2)
208-208: Variable declaration change looks good.Changing from
consttoletis necessary to allow for the additional conditional modification.
213-215:Details
✅ Verification successful
Event name filtering appropriately implemented.
The conditional block correctly appends the event_name filter when params.eventName is provided, enhancing query specificity for strategy timing events.
Let's verify if we have any unit tests that check for this new filtering capability:
🏁 Script executed:
#!/bin/bash # Find any test files that might test the EnvioIndexerClient fd -e ts -e js "envioIndexerClient" --exec grep -l "getEvents" {} \;Length of output: 194
Event name filtering verified and approved.
The filtering logic inpackages/indexer-client/src/providers/envioIndexerClient.ts(lines 213-215) correctly appends theevent_namefilter whenparams.eventNameis provided, and we confirmed that the corresponding unit tests inpackages/indexer-client/test/unit/envioIndexerClient.spec.tsexercise thegetEventsfunctionality.
- The conditional block appends the filter as expected.
- The existence of tests in the spec file ensures that this new filtering capability is covered.
scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts (3)
4-5: Good use of constants for type definitions.Using constants for
ADDRESS_TYPEandCHAIN_ID_TYPEimproves code maintainability and consistency.
46-47: Clean down migration implementation.The down migration correctly drops the newly added tables, ensuring a clean reversal of the migration.
23-41:Details
✅ Verification successful
Well structured table creation for caching strategy timings.
The schema for the new tables
strategy_timingsandstrategies_registryis well designed with appropriate columns and constraints for caching strategy timings.Let's verify if there's a corresponding repository implementation for these tables:
🏁 Script executed:
#!/bin/bash # Find repository implementation files for strategy timings fd -e ts "strategyTimings" --exec grep -l "Repository" {} \; fd -e ts "strategyTimings" --exec grep -l "Cache" {} \;Length of output: 294
Verified: Repository Implementation Confirmed for Caching Tables
The migration code for creating the
strategy_timingsandstrategies_registrytables is well structured. Verification via repository search shows that related implementations exist in both:
./scripts/bootstrap/src/strategyTimings.script.ts./packages/repository/src/kysely/repositories/strategyTimings.repository.tsNo further modifications are needed.
packages/processors/test/registry/handlers/profileCreated.handler.spec.ts (4)
1-1: Imports approved for usage in test file.
No issues found with the addition ofAddressandgetAddressimport.
9-9: Addition ofICacheimport is valid.
The new import from@grants-stack-indexer/repositoryis correctly placed.
12-12: Import ofStrategyTimingsis valid.
No concerns regarding this type import.
77-77: Mock dependency addition is fine.
ProvidingstrategyTimingsRepositoryin the test dependencies aligns with the new caching mechanism.packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts (4)
6-6: NewICacheimport is valid.
No issues adding this interface in the test context.
9-9:StrategyTimingsimport looks good.
Straightforward extension of existing type imports.
13-19: Block import changes are standard.
These imports from@grants-stack-indexer/sharedconform to typical test usage.
105-105: NewstrategyTimingsRepositorymock.
Introducing this cache mock dependency aligns with your new caching logic.scripts/bootstrap/src/strategyTimings.script.ts (5)
1-24: Initial imports and setup approved.
All necessary references for environment configuration and concurrency utilities are properly declared.
25-57: Excellent JSDoc documentation.
Thoroughly explains environment variables, script flow, and operational details.
59-164: Main function initialization and event fetching logic.
Overall, the structure for establishing the database connection, fetching events in a loop, and tracking checkpoints is clear. No issues detected.
165-217: Strategy registry lookup and contract reading logic.
The usage ofretryfor resilient fetching is sound. Despite static analysis suggesting the variablestrategyis unused, it is in fact used when referencingstrategy[0]andstrategy[1]. This appears to be a false positive.🧰 Tools
🪛 GitHub Check: lint / Run Linters
[failure] 167-167:
'strategy' is assigned a value but never used
319-323: Final script execution block.
Graceful handling of main function exceptions is appropriate.packages/processors/test/allo/handlers/poolCreated.handler.spec.ts (5)
7-12: New dependencies added correctly.The required imports for the caching mechanism (
ICacheandStrategyTimings) have been properly added to support the strategy timings feature.
13-20: Clean imports organization.The imports are now properly organized with
Addressand other types from the shared package, improving code readability.
102-103: Strategy timings repository properly injected.The new
strategyTimingsRepositorydependency has been correctly added to the handler constructor, maintaining test consistency with implementation changes.
130-131: Consistent dependency injection across test cases.The
strategyTimingsRepositoryis consistently added across all test scenarios, ensuring proper test coverage.
193-194: Properly maintained test for complex strategy.The strategy timings repository is correctly included in this test case which covers the DonationVotingMerkleDistributionDirectTransferStrategy.
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (3)
6-12: Cache interface correctly imported.The required interfaces for the strategy timings cache have been properly imported.
13-19: Improved import organization.The imports have been neatly reorganized to include the
Addresstype with other shared types.
87-88: Handler correctly updated with new dependency.The
strategyTimingsRepositoryis properly added to the handler constructor, correctly implementing the dependency injection pattern used throughout the codebase.packages/processors/test/allo/allo.processor.spec.ts (3)
6-12: Cache dependencies correctly imported.The
ICacheandStrategyTimingstypes are properly imported for the processor test.
13-19: Well-organized imports.The import structure is clean and organized, with appropriate types from the shared module.
61-62: Processor constructor updated properly.The
strategyTimingsRepositoryhas been correctly added to the AlloProcessor constructor, consistent with the implementation changes.packages/processors/test/strategy/strategy.processor.spec.ts (3)
6-12: Cache interface properly imported.The required cache interfaces for strategy timings have been correctly added to the imports.
13-19: Clean type imports organization.The shared type imports are well-organized, including the newly referenced
Addresstype.
49-50: Strategy processor correctly updated with caching dependency.The
strategyTimingsRepositoryis properly added to the processor's dependencies, consistent with the PR's objective of implementing a strategy timings cache.packages/processors/src/processors/allo/handlers/poolCreated.handler.ts (2)
14-18: Dependency injection looks appropriate.
Including"strategyTimingsRepository"into theDependenciestype is consistent with the usage in this file.
40-40: Destructuring dependencies is clear and consistent.
The destructuredstrategyTimingsRepositoryaligns with the newly added property in the dependencies interface.packages/repository/src/kysely/repositories/strategyTimings.repository.ts (2)
13-17: Constructor parameters look good.
The constructor correctly injects the database connection and schema name, facilitating flexible usage.
79-83: No-op clearAll is acceptable.
Assuming strategic reasons for never clearing this cache, this implementation looks fine.
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts
Show resolved
Hide resolved
| const strategyTimingsOnRegistry = await strategyTimingsRepository.get(strategyAddress); | ||
| if (strategyTimingsOnRegistry) { | ||
| strategyTimings = strategyTimingsOnRegistry.timings as unknown as StrategyTimings; | ||
| } else { | ||
| strategyTimings = await strategyHandler.fetchStrategyTimings(strategyAddress); | ||
| await strategyTimingsRepository.set(strategyAddress, { | ||
| address: strategyAddress, | ||
| timings: JSON.stringify(strategyTimings), | ||
| strategyId, | ||
| createdAt: new Date(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Parse the retrieved JSON to avoid incorrect type casting.
When retrieving stored timings from strategyTimingsOnRegistry.timings, you are force casting it to StrategyTimings. If it's stored as JSON text in the database, consider parsing it to avoid runtime errors.
Apply this diff to ensure correct parsing:
- strategyTimings = strategyTimingsOnRegistry.timings as unknown as StrategyTimings;
+ strategyTimings = JSON.parse(strategyTimingsOnRegistry.timings) as StrategyTimings;📝 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 strategyTimingsOnRegistry = await strategyTimingsRepository.get(strategyAddress); | |
| if (strategyTimingsOnRegistry) { | |
| strategyTimings = strategyTimingsOnRegistry.timings as unknown as StrategyTimings; | |
| } else { | |
| strategyTimings = await strategyHandler.fetchStrategyTimings(strategyAddress); | |
| await strategyTimingsRepository.set(strategyAddress, { | |
| address: strategyAddress, | |
| timings: JSON.stringify(strategyTimings), | |
| strategyId, | |
| createdAt: new Date(), | |
| }); | |
| } | |
| const strategyTimingsOnRegistry = await strategyTimingsRepository.get(strategyAddress); | |
| if (strategyTimingsOnRegistry) { | |
| strategyTimings = JSON.parse(strategyTimingsOnRegistry.timings) as StrategyTimings; | |
| } else { | |
| strategyTimings = await strategyHandler.fetchStrategyTimings(strategyAddress); | |
| await strategyTimingsRepository.set(strategyAddress, { | |
| address: strategyAddress, | |
| timings: JSON.stringify(strategyTimings), | |
| strategyId, | |
| createdAt: new Date(), | |
| }); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
scripts/bootstrap/src/strategyTimings.script.ts (5)
25-47: Adhere to script naming conventions if applicable.Your script is placed in
scripts/bootstrap/src/, which is generally acceptable. However, check if your repository’s coding guidelines or package.json scripts align with “infra” or “utilities” folders. Moving or renaming this script to comply with a standard location can improve discoverability.
78-81: Skip setting checkpoints when no events exist.Currently, if
eventsis an empty array for a given chain, you set a checkpoint using a chain ID of0. This might incorrectly overwrite or create entries incheckpointMapthat don’t reflect valid chain IDs. Consider skipping the checkpoint assignment whenevents.length === 0to avoid potential confusion or incorrect state.events.forEach((chainEvents) => { - checkpointMap.set(chainEvents[0]?.chainId ?? 0, { - blockNumber: chainEvents[chainEvents.length - 1]?.blockNumber ?? 0, - logIndex: chainEvents[chainEvents.length - 1]?.logIndex ?? 0, - }); + if (chainEvents.length > 0) { + checkpointMap.set(chainEvents[0].chainId, { + blockNumber: chainEvents[chainEvents.length - 1].blockNumber, + logIndex: chainEvents[chainEvents.length - 1].logIndex, + }); + } });
111-127: Use logger instead of console logs.While
loggeris already instantiated, lines 123-127 rely onconsole.log. For consistent logging and potential future enhancements (e.g., log levels, external log streams), consider usinglogger.infoorlogger.debug.
167-168: Remove unused parameters or disable the ESLint rule only when necessary.The
_strategyparameter in yourfor awaitloops is not used. You’re disabling the ESLint rule locally, but removing the parameter if not needed clarifies intent and cleans up the code.-for await (const _strategy of pMapIterable( +for await (const _ of pMapIterable(Also applies to: 234-235
238-244: Store null defaults when timings are undefined.Currently, if
fetchStrategyTimingsreturnsundefined, the code callsstrategyTimingsRepository.setwithtimings: undefined. As a result, the repository ends up with an undefined field for timings. Storing an explicit null-structure promotes clarity and consistent data shape.await strategyTimingsRepository.set(strategyAddressId[1] as Address, { address: strategyAddressId[1] as Address, strategyId: strategyAddressId[2] as `0x${string}`, - timings: strategyTimings, + timings: strategyTimings ?? { + applicationsStartTime: null, + applicationsEndTime: null, + donationsStartTime: null, + donationsEndTime: null + }, createdAt: new Date(), });Also applies to: 262-270
packages/repository/src/kysely/repositories/strategyTimings.repository.ts (1)
13-84: Add unit tests for coverage.To ensure reliability and future maintainability, consider adding unit tests (especially around error handling and the
timingsparse/format logic) to build confidence in this cache’s behavior.Would you like help generating these test suites (e.g., using Jest or another framework) to validate the get/set operations and ensure the cache logic works as expected?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/repository/src/kysely/repositories/strategyTimings.repository.ts(1 hunks)scripts/bootstrap/src/strategyTimings.script.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.ts`:
**/*.ts:
scripts/bootstrap/src/strategyTimings.script.tspackages/repository/src/kysely/repositories/strategyTimings.repository.ts
`scripts/**/*.ts`: Ensure scripts: - Use `process.cwd()` f...
scripts/**/*.ts: Ensure scripts:
- Use
process.cwd()for root references.- Follow folder conventions (
infra/for infra scripts,utilities/for utilities).- Are organized in
package.jsonwithscript:infra:{name}orscript:util:{name}.- Be concise and avoid overly nitpicky feedback outside of these best practices.
scripts/bootstrap/src/strategyTimings.script.ts
🔇 Additional comments (1)
scripts/bootstrap/src/strategyTimings.script.ts (1)
168-218: Great use of concurrency and retry logic.Your approach with
pMapIterableplusretryprovides a clean strategy for concurrent fetches with robust error handling, matching the script’s requirement for reindexing performance.Also applies to: 235-286
packages/repository/src/kysely/repositories/strategyTimings.repository.ts
Show resolved
Hide resolved
| if (!result) { | ||
| return undefined; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reparse the timings column to match the StrategyTimings interface.
Currently, if timings is stored as a JSON string, the get method returns it unparsed. Consider converting it back to an array or object for consistency with the interface.
Example fix:
- return result;
+ return {
+ ...result,
+ timings: typeof result.timings === "string"
+ ? JSON.parse(result.timings)
+ : result.timings,
+ };📝 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.
| if (!result) { | |
| return undefined; | |
| } | |
| return result; | |
| if (!result) { | |
| return undefined; | |
| } | |
| return { | |
| ...result, | |
| timings: typeof result.timings === "string" | |
| ? JSON.parse(result.timings) | |
| : result.timings, | |
| }; |
thelostone-mc
force-pushed
the
feat/strategy-timings-cache
branch
from
35c32e7 to
8ac9ff6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/bootstrap/package.json (1)
30-30: Dependency: @grants-stack-indexer/chain-providers
The dependency"@grants-stack-indexer/chain-providers": "workspace:*"has been added to support the new caching mechanism. Please verify that this package is correctly configured and available in your monorepo environment, ensuring consistent versioning and integration with the rest of the project.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
apps/processing/src/services/sharedDependencies.service.ts(3 hunks)package.json(1 hunks)packages/data-flow/src/orchestrator.ts(1 hunks)packages/data-flow/src/types/index.ts(2 hunks)packages/data-flow/test/integration/helpers/dependencies.ts(2 hunks)packages/data-flow/test/unit/orchestrator.spec.ts(3 hunks)packages/data-flow/test/unit/retroactiveProcessor.spec.ts(2 hunks)packages/indexer-client/src/providers/envioIndexerClient.ts(1 hunks)packages/indexer-client/src/types/indexerClient.types.ts(1 hunks)packages/processors/src/processors/allo/handlers/poolCreated.handler.ts(3 hunks)packages/processors/src/types/processor.types.ts(2 hunks)packages/processors/test/allo/allo.processor.spec.ts(2 hunks)packages/processors/test/allo/handlers/poolCreated.handler.spec.ts(10 hunks)packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts(2 hunks)packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts(2 hunks)packages/processors/test/registry/handlers/profileCreated.handler.spec.ts(2 hunks)packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts(2 hunks)packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts(2 hunks)packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts(2 hunks)packages/processors/test/strategy/strategy.processor.spec.ts(2 hunks)packages/processors/test/strategy/strategyHandler.factory.spec.ts(3 hunks)packages/repository/src/db/connection.ts(2 hunks)packages/repository/src/external.ts(1 hunks)packages/repository/src/kysely/repositories/index.ts(1 hunks)packages/repository/src/kysely/repositories/strategyTimings.repository.ts(1 hunks)packages/repository/src/types/index.ts(1 hunks)packages/repository/src/types/strategyTimings.types.ts(1 hunks)scripts/bootstrap/package.json(2 hunks)scripts/bootstrap/src/schemas/index.ts(1 hunks)scripts/bootstrap/src/strategyTimings.script.ts(1 hunks)scripts/migrations/package.json(1 hunks)scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts(0 hunks)scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts(2 hunks)scripts/migrations/src/utils/parsing.ts(2 hunks)
💤 Files with no reviewable changes (1)
- scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts
🚧 Files skipped from review as they are similar to previous changes (31)
- packages/repository/src/types/index.ts
- packages/repository/src/kysely/repositories/index.ts
- scripts/migrations/package.json
- packages/indexer-client/src/types/indexerClient.types.ts
- packages/data-flow/src/orchestrator.ts
- packages/repository/src/db/connection.ts
- scripts/migrations/src/utils/parsing.ts
- packages/repository/src/external.ts
- packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts
- packages/processors/test/strategy/strategyHandler.factory.spec.ts
- packages/processors/src/processors/allo/handlers/poolCreated.handler.ts
- apps/processing/src/services/sharedDependencies.service.ts
- packages/indexer-client/src/providers/envioIndexerClient.ts
- packages/repository/src/types/strategyTimings.types.ts
- packages/processors/src/types/processor.types.ts
- packages/data-flow/src/types/index.ts
- scripts/bootstrap/src/schemas/index.ts
- packages/data-flow/test/unit/orchestrator.spec.ts
- scripts/bootstrap/src/strategyTimings.script.ts
- packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts
- packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts
- packages/processors/test/registry/handlers/profileCreated.handler.spec.ts
- packages/processors/test/strategy/strategy.processor.spec.ts
- packages/processors/test/allo/handlers/poolCreated.handler.spec.ts
- packages/processors/test/allo/allo.processor.spec.ts
- packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
- packages/data-flow/test/unit/retroactiveProcessor.spec.ts
- package.json
- packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts
- packages/repository/src/kysely/repositories/strategyTimings.repository.ts
- scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`:
**/*.ts:
packages/data-flow/test/integration/helpers/dependencies.ts
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: Analysis
🔇 Additional comments (4)
packages/data-flow/test/integration/helpers/dependencies.ts (1)
26-26: Implementation of strategyTimingsRepository looks good!The addition of the strategyTimingsRepository to the mock repositories follows the established pattern in this file. The mock implementation with
getandsetmethods appropriately represents a cache interface, which aligns with the PR's goal of implementing a caching mechanism for strategy timings.Also applies to: 93-96
scripts/bootstrap/package.json (3)
16-19: New Bootstrap Script for Strategy Timings
The new script"bootstrap:strategyTimings": "tsx src/strategyTimings.script.ts"is added correctly to the scripts section. This addition aligns with the caching feature for strategy timings, so please double-check that the referenced file (src/strategyTimings.script.ts) properly implements the desired caching logic and is also referenced in the root bootstrap script (if applicable).
35-35: Dependency: @grants-stack-indexer/processors
The addition of the"@grants-stack-indexer/processors": "workspace:*"dependency provides necessary functionalities for processing strategy timings. Make sure that this module exposes the required interfaces and that its integration does not conflict with existing processors in other parts of the codebase.
42-42: Dependency: viem
The new dependency"viem": "2.19.6"is integrated for enhanced blockchain interactions. Please confirm that the version specified is compatible with other packages in your ecosystem and that there are no known security advisories for this version.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/production_operations.md (1)
86-86: Clarify the Purpose of the Strategy Timings Bootstrap CommandThe new command
pnpm bootstrap:strategyTimingsis clearly aligned with the PR objective to improve reindexing speed by leveraging cached strategy timestamps. To enhance usability and clarity, consider adding a brief inline comment or additional documentation that explains what this command does and why it is necessary. For example, you might update the line as follows:- 4. `pnpm bootstrap:strategyTimings` + 4. `pnpm bootstrap:strategyTimings # Initializes and caches strategy timestamps to speed up reindexing`This provides immediate context to the operator without requiring them to consult external documentation.
README.md (2)
91-98: Specify Code Block Language & Correct Typo in Command
The fenced code block does not specify a language, which triggers markdownlint warning MD040. For clarity and consistency, please add a language declaration (e.g.,bash) right after the opening triple backticks. Additionally, there is a typo on line 97: “pnpm boostrap:strategyTimings” should be “pnpm bootstrap:strategyTimings”.Suggested diff for line 97:
-pnpm boostrap:strategyTimings +pnpm bootstrap:strategyTimings
100-105: Review Migration Step Numbering
The migration step has been renumbered to “1. Apply migrations:” (previously step 8). While this reordering might be intentional to follow a reset in the bootstrap instructions, please verify that the numbering is clear to users and consistent with the rest of the README.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
102-102: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/production_operations.md(1 hunks)README.md(1 hunks)apps/processing/test/unit/sharedDependencies.service.spec.ts(1 hunks)packages/processors/src/processors/allo/handlers/poolCreated.handler.ts(3 hunks)packages/processors/test/allo/handlers/poolCreated.handler.spec.ts(13 hunks)packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts(3 hunks)packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts(5 hunks)packages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.ts(7 hunks)packages/repository/src/kysely/repositories/strategyTimings.repository.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/processors/src/processors/allo/handlers/poolCreated.handler.ts
- packages/processors/test/allo/handlers/poolCreated.handler.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.ts`:
**/*.ts:
apps/processing/test/unit/sharedDependencies.service.spec.tspackages/repository/src/kysely/repositories/strategyTimings.repository.tspackages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.tspackages/processors/test/strategy/directAllocation/directAllocation.handler.spec.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
`**/*.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 inten...
**/*.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/test/unit/sharedDependencies.service.spec.tspackages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.tspackages/processors/test/strategy/directAllocation/directAllocation.handler.spec.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
🧬 Code Definitions (2)
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts (2)
packages/repository/src/external.ts (2)
ICache(93-93)StrategyTimings(95-95)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (2)
packages/repository/src/external.ts (2)
ICache(93-93)StrategyTimings(95-95)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)
🪛 markdownlint-cli2 (0.17.2)
README.md
90-90: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 GitHub Check: lint / Run Linters
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts
[warning] 91-91:
Unsafe assignment of an any value
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
[warning] 116-116:
Unsafe assignment of an any value
[warning] 138-138:
Unsafe assignment of an any value
[warning] 160-160:
Unsafe assignment of an any value
🔇 Additional comments (16)
packages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.ts (2)
104-105: LGTM: Correctly added strategyTimingsRepository dependency to handler initializationThe addition of the strategyTimingsRepository parameter to the EasyRetroFundingStrategyHandler constructor is consistent with the PR objectives to implement a caching mechanism for strategy timestamps.
133-134: LGTM: Consistent implementation across all handler test casesThe strategyTimingsRepository dependency is correctly added to all handler instantiations in a consistent manner across all test cases, ensuring that each handler receives the necessary repository for strategy timing operations.
Also applies to: 155-156, 177-178, 199-200, 221-222, 243-244
README.md (1)
88-88: Verify Updated Bootstrap Instruction for StrategyTimings
The instruction now includes "StrategyTimings" as an optional component, which aligns with the new caching mechanism for strategy timings. Please double-check that all relevant documentation and downstream processes (e.g., migration scripts) have been updated accordingly.apps/processing/test/unit/sharedDependencies.service.spec.ts (1)
54-54: LGTM: Mock for strategy timings cache added correctlyThe
KyselyStrategyTimingsCachemock has been properly added to the repository mocks, consistent with the pattern used for other cache components.packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts (4)
7-12: Added required imports for strategy timings cacheThe
ICacheinterface andStrategyTimingstype are now properly imported to support the strategy timings caching capability.
13-19: Improved import organizationThe imports from
@grants-stack-indexer/sharedare now better organized with the addition of theAddresstype.
62-62: Added strategy timings repository to handler dependenciesThe handler now includes the strategy timings repository in its dependencies, which aligns with the PR objective of implementing a caching mechanism for strategy timestamps.
91-91: Verification for strategy timings repository added to testThe test now properly verifies that the strategy timings repository is passed to the handler, addressing a previous review comment.
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 91-91:
Unsafe assignment of ananyvaluepackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (4)
7-12: Added required imports for strategy timings cacheThe
ICacheinterface andStrategyTimingstype are now properly imported to support the strategy timings caching capability.
13-19: Improved import organizationThe imports from
@grants-stack-indexer/sharedare now better organized with the addition of theAddresstype.
87-87: Added strategy timings repository to handler dependenciesThe handler now includes the strategy timings repository in its dependencies, which aligns with the PR objective of implementing a caching mechanism for strategy timestamps.
116-116: Strategy timings repository verified in all handler testsThe tests now properly verify that the strategy timings repository is passed to all handler implementations.
Also applies to: 138-138, 160-160
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 116-116:
Unsafe assignment of ananyvaluepackages/repository/src/kysely/repositories/strategyTimings.repository.ts (4)
1-12: Implementation of KyselyStrategyTimingsCache class looks goodThe
KyselyStrategyTimingsCacheclass implements theICache<string>interface and is properly configured with dependency injection for the database instance and schema name.
13-36: Consider reparsing JSON timings in the get methodThe
getmethod currently returns the raw result from the database without parsing potential JSON strings in thetimingsfield.Apply this diff to ensure consistency between stored and retrieved data:
- return result; + return { + ...result, + timings: typeof result.timings === "string" + ? JSON.parse(result.timings) + : result.timings, + };
38-72: Proper implementation of the set method with conflict handlingThe
setmethod correctly formats the timings data and handles conflicts by updating existing entries. Error handling is also well-implemented.
74-78: Intentional no-op for clearAll with clear documentationThe decision to make
clearAlla no-op is clearly documented, indicating a design choice to maintain the cache data persistently.
thelostone-mc
force-pushed
the
feat/strategy-timings-cache
branch
2 times, most recently
from
77f5cca to
7550b06
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/processors/test/allo/handlers/poolCreated.handler.spec.ts (1)
248-248: Consider adding test coverage for the.setmethod.You are verifying
.getusage here; however, adding a scenario that invokes and tests.setcould enhance coverage and verify consistency of read/write operations onstrategyTimingsRepository.packages/repository/src/kysely/repositories/strategyTimings.repository.ts (1)
41-44: Standardize JSON storage fortimings.Currently, only arrays are
stringify’d, while other objects are written directly. Consider always serializingstrategyTimings.timingsfor consistent storage and retrieval.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.github/production_operations.md(1 hunks)README.md(1 hunks)apps/processing/src/services/sharedDependencies.service.ts(3 hunks)apps/processing/test/unit/sharedDependencies.service.spec.ts(1 hunks)package.json(1 hunks)packages/data-flow/src/orchestrator.ts(1 hunks)packages/data-flow/src/types/index.ts(2 hunks)packages/data-flow/test/integration/helpers/dependencies.ts(2 hunks)packages/data-flow/test/unit/orchestrator.spec.ts(3 hunks)packages/data-flow/test/unit/retroactiveProcessor.spec.ts(2 hunks)packages/indexer-client/src/providers/envioIndexerClient.ts(1 hunks)packages/indexer-client/src/types/indexerClient.types.ts(1 hunks)packages/processors/src/processors/allo/handlers/poolCreated.handler.ts(3 hunks)packages/processors/src/types/processor.types.ts(2 hunks)packages/processors/test/allo/allo.processor.spec.ts(2 hunks)packages/processors/test/allo/handlers/poolCreated.handler.spec.ts(13 hunks)packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts(2 hunks)packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts(2 hunks)packages/processors/test/registry/handlers/profileCreated.handler.spec.ts(2 hunks)packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts(3 hunks)packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts(2 hunks)packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts(5 hunks)packages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.ts(7 hunks)packages/processors/test/strategy/strategy.processor.spec.ts(2 hunks)packages/processors/test/strategy/strategyHandler.factory.spec.ts(3 hunks)packages/repository/src/db/connection.ts(2 hunks)packages/repository/src/external.ts(1 hunks)packages/repository/src/kysely/repositories/index.ts(1 hunks)packages/repository/src/kysely/repositories/strategyTimings.repository.ts(1 hunks)packages/repository/src/types/index.ts(1 hunks)packages/repository/src/types/strategyTimings.types.ts(1 hunks)scripts/bootstrap/.env.example(1 hunks)scripts/bootstrap/package.json(2 hunks)scripts/bootstrap/src/schemas/index.ts(1 hunks)scripts/bootstrap/src/strategyTimings.script.ts(1 hunks)scripts/migrations/package.json(1 hunks)scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts(0 hunks)scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts(2 hunks)scripts/migrations/src/utils/parsing.ts(2 hunks)
💤 Files with no reviewable changes (1)
- scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts
🚧 Files skipped from review as they are similar to previous changes (29)
- scripts/migrations/package.json
- packages/repository/src/kysely/repositories/index.ts
- packages/repository/src/types/index.ts
- packages/data-flow/test/integration/helpers/dependencies.ts
- packages/data-flow/src/types/index.ts
- .github/production_operations.md
- apps/processing/src/services/sharedDependencies.service.ts
- packages/repository/src/db/connection.ts
- packages/indexer-client/src/types/indexerClient.types.ts
- packages/repository/src/external.ts
- packages/data-flow/src/orchestrator.ts
- scripts/migrations/src/utils/parsing.ts
- apps/processing/test/unit/sharedDependencies.service.spec.ts
- scripts/bootstrap/src/schemas/index.ts
- packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts
- package.json
- packages/processors/test/strategy/strategyHandler.factory.spec.ts
- packages/processors/src/types/processor.types.ts
- packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts
- packages/indexer-client/src/providers/envioIndexerClient.ts
- packages/processors/test/allo/allo.processor.spec.ts
- packages/data-flow/test/unit/retroactiveProcessor.spec.ts
- packages/repository/src/types/strategyTimings.types.ts
- scripts/bootstrap/src/strategyTimings.script.ts
- packages/processors/test/strategy/strategy.processor.spec.ts
- packages/processors/src/processors/allo/handlers/poolCreated.handler.ts
- packages/processors/test/registry/handlers/profileCreated.handler.spec.ts
- packages/data-flow/test/unit/orchestrator.spec.ts
- packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.ts`:
**/*.ts:
packages/processors/test/allo/handlers/poolCreated.handler.spec.tsscripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.tspackages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.tspackages/repository/src/kysely/repositories/strategyTimings.repository.tspackages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts
`**/*.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 inten...
**/*.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/processors/test/allo/handlers/poolCreated.handler.spec.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.tspackages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.tspackages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts
`scripts/**/*.ts`: Ensure scripts: - Use `process.cwd()` for root references. - Follow folder conventions (`infra/` for infra scripts, `utilities/` for utilities). - Are orga...
scripts/**/*.ts: Ensure scripts:
- Use
process.cwd()for root references.- Follow folder conventions (
infra/for infra scripts,utilities/for utilities).- Are organized in
package.jsonwithscript:infra:{name}orscript:util:{name}.- Be concise and avoid overly nitpicky feedback outside of these best practices.
scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts
🧬 Code Definitions (3)
packages/processors/test/allo/handlers/poolCreated.handler.spec.ts (3)
packages/repository/src/external.ts (2)
ICache(93-93)StrategyTimings(95-95)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)packages/processors/src/types/strategy.types.ts (1)
StrategyTimings(17-22)
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (2)
packages/repository/src/external.ts (2)
ICache(93-93)StrategyTimings(95-95)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)
packages/repository/src/kysely/repositories/strategyTimings.repository.ts (2)
packages/repository/src/db/connection.ts (1)
Database(69-88)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)
🪛 GitHub Check: lint / Run Linters
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
[warning] 116-116:
Unsafe assignment of an any value
[warning] 138-138:
Unsafe assignment of an any value
[warning] 160-160:
Unsafe assignment of an any value
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts
[warning] 91-91:
Unsafe assignment of an any value
🪛 markdownlint-cli2 (0.17.2)
README.md
90-90: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (13)
packages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.ts (1)
104-105: Consistent integration of strategyTimingsRepository across all handlers.The
strategyTimingsRepositoryparameter has been properly added to all handler initializations. The tests now correctly verify that this new repository is passed to each handler, maintaining consistency across the test suite.Also applies to: 133-134, 155-156, 177-178, 199-200, 221-222, 243-244
scripts/bootstrap/.env.example (1)
5-5: Configuration looks good!The addition of the
CHAINSvariable with Optimism configuration provides the necessary RPC URLs for blockchain interaction, which aligns with implementing the strategy timings cache feature. The multiple RPC URLs offer good redundancy.README.md (1)
88-88: LGTM! Documentation updated to include the new feature.The updated heading clearly communicates that StrategyTimings is now part of the optional bootstrapping process.
scripts/bootstrap/package.json (3)
18-18: LGTM! New script added for strategy timings.The new script aligns with the PR objectives to implement a caching mechanism for strategy timestamps.
30-36: LGTM! Required dependencies added.The added dependencies are necessary for the new strategy timings functionality, providing access to chain providers and processors.
42-42: LGTM! Pinned viem dependency.Using a specific version (2.19.6) of the viem library ensures consistent builds and avoids unexpected breaking changes.
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts (4)
7-12: LGTM! Import new types for strategy timings cache.The addition of
ICacheandStrategyTimingsimports supports the integration of the strategy timings cache feature.
13-19: LGTM! Reorganized imports with added Address type.The refactoring to named imports is cleaner, and the addition of the
Addresstype is necessary for the cache implementation.
62-62: LGTM! Added strategyTimingsRepository dependency.The strategyTimingsRepository is properly initialized in the test setup as an empty object cast to the appropriate interface type.
91-91: LGTM! Updated test expectations for handler instantiation.The test now correctly verifies that strategyTimingsRepository is passed to the handler.
Note: The static analysis warning about "unsafe assignment of an
anyvalue" can be safely ignored asexpect.any(Object)is a standard pattern in testing.🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 91-91:
Unsafe assignment of ananyvaluepackages/processors/test/allo/handlers/poolCreated.handler.spec.ts (1)
61-79: Mock usage looks good.The introduction of a
mockStrategyTimingsRepositoryand its usage in the handler constructor is correct, ensuring that the handler’s new dependency is properly mocked in tests.packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (1)
87-88: Dependency injection ofstrategyTimingsRepositoryis well-integrated.Passing
{}as anICache<Address, StrategyTimings>mock is sufficient for these tests, and your structure aligns with existing dependencies.packages/repository/src/kysely/repositories/strategyTimings.repository.ts (1)
16-26: Revisit howtimingsis parsed on retrieval.If the database driver returns the JSONB column as a parsed object, this is fine. Otherwise, you may need to convert it back into a structured object for consistency with
StrategyTimings. This is a previously noted suggestion.
scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts
Show resolved
Hide resolved
scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts
Show resolved
Hide resolved
packages/repository/src/kysely/repositories/strategyTimings.repository.ts
Show resolved
Hide resolved
thelostone-mc
force-pushed
the
feat/strategy-timings-cache
branch
2 times, most recently
from
2571952 to
4e5aa55
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
scripts/bootstrap/package.json (1)
43-43: Pinned Viem Version Review
The dependency"viem": "2.19.6"is now explicitly pinned. This helps ensure stability for the caching feature implementation. It is recommended to periodically verify that this version remains secure and compatible with other chain-related libraries in your workspace.scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts (4)
23-31: Table naming convention differs from existing tablesI notice that the new
strategy_timingstable uses snake_case naming, while existing tables likepriceCacheandmetadataCacheuse camelCase. Consider adopting a consistent naming convention across all database tables for better maintainability.- .createTable("strategy_timings") + .createTable("strategyTimings") - .addPrimaryKeyConstraint("strategy_timings_cache_pkey", ["address"]) + .addPrimaryKeyConstraint("strategyTimingsCachePkey", ["address"])
33-40: Add default value for thehandledboolean columnThe
handledcolumn in thestrategies_registrytable is defined as a boolean without a default value. It's usually a good practice to provide a default value for boolean fields to avoid null values and simplify queries.- .addColumn("handled", "boolean") + .addColumn("handled", "boolean", (col) => col.defaultTo(false))
37-37: Consider adding an index for thechainIdcolumnThe
chainIdcolumn is part of the primary key and will likely be used in queries to filter strategies by chain. Adding an index on this column could improve query performance, especially if you'll be querying bychainIdalone withoutaddress..addColumn("chainId", CHAIN_ID_TYPE, (col) => col.notNull()) + .createIndex("strategies_registry_chain_id_idx").on("strategies_registry").column("chainId")
4-5: Consider using SQL type aliases for better type safetyUsing string constants for column types works, but for better type safety and consistency, consider using SQL type aliases (if supported by your database) or at least documenting the expected format/constraints for these types.
- const ADDRESS_TYPE = "text"; - const CHAIN_ID_TYPE = "integer"; + const ADDRESS_TYPE = sql`text`; // or potentially a domain type if you have one + const CHAIN_ID_TYPE = sql`integer`;README.md (3)
88-89: Clarify the Optionality in the Bootstrap Step
The updated instruction now reads:
7. Bootstrap DB with IPFS metadata and Pricing and StrategyTimings(optional):
Consider clarifying the optional nature of “StrategyTimings” (for example, by ensuring a clear separation with parentheses or additional note) so that users quickly recognize it isn’t mandatory.
91-93: Specify Language for the Fenced Code Block
The fenced code block starting with:# To run all of them pnpm bootstrap:alldoes not specify a language. Adding a language identifier (e.g.,
bash) would improve syntax highlighting and clarity.
94-97: Ensure Consistent Formatting and Verify Command Order
Similarly, the subsequent fenced code block that lists the individual bootstrap commands (for metadata, pricing, and strategyTimings) would benefit from a language specifier to maintain consistency. Also, please verify that the ordering of commands reflects the intended process—if the execution order is significant, a brief explanation might help readers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)package.json(1 hunks)packages/processors/src/processors/allo/handlers/poolCreated.handler.ts(3 hunks)packages/processors/test/gitcoinAttestationNetwork/handlers/onAttested.handler.spec.ts(3 hunks)scripts/bootstrap/package.json(2 hunks)scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- packages/processors/test/gitcoinAttestationNetwork/handlers/onAttested.handler.spec.ts
- packages/processors/src/processors/allo/handlers/poolCreated.handler.ts
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.ts`:
**/*.ts:
scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts
`scripts/**/*.ts`: Ensure scripts: - Use `process.cwd()` for root references. - Follow folder conventions (`infra/` for infra scripts, `utilities/` for utilities). - Are orga...
scripts/**/*.ts: Ensure scripts:
- Use
process.cwd()for root references.- Follow folder conventions (
infra/for infra scripts,utilities/for utilities).- Are organized in
package.jsonwithscript:infra:{name}orscript:util:{name}.- Be concise and avoid overly nitpicky feedback outside of these best practices.
scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts
🪛 markdownlint-cli2 (0.17.2)
README.md
90-90: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: e2e-tests / E2E Tests
🔇 Additional comments (5)
scripts/bootstrap/package.json (3)
15-19: Enhanced Bootstrap Script Commands
The addition of the"bootstrap:all"command to sequentially run the metadata, pricing, and new strategy timings scripts is well aligned with the PR’s objective for caching strategy timestamps. Moreover, introducing a dedicated"bootstrap:strategyTimings"script improves modularity, allowing targeted execution of strategy timing tasks. Ensure that any failures within an individual script propagate correctly so that the overall bootstrap process can handle errors gracefully.
31-31: New Dependency: Chain Providers
Adding"@grants-stack-indexer/chain-providers": "workspace:*"supports the new strategy caching mechanism and related chain operations. Confirm that the workspace configuration correctly resolves this dependency and that it is maintained with consistent versioning.
36-36: New Dependency: Processors Module
The inclusion of"@grants-stack-indexer/processors": "workspace:*"is appropriate, considering the new caching requirements may leverage processing functionalities. Please ensure this module provides the expected support for managing strategy timings across the system.scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts (1)
1-48: Using a future date in the migration filenameThe migration filename includes the date
20250127(January 27, 2025), which is in the future. This is unusual for migration files, which typically use the current date or a sequential numbering system. Using a future date might cause issues if other migrations need to be applied before this one but are created with current dates.Please verify if this is intentional and aligns with your team's migration versioning strategy.
README.md (1)
100-104: Ensure Consistent Instruction Numbering
The enriched summary mentioned that the migration step should be updated from "8. Apply migrations:" to "1. Apply migrations:" due to a reordering of steps. However, the current document still shows:8. Apply migrations: pnpm db:migratePlease verify and adjust the numbering so that the instructions remain consistent throughout the setup guide.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
102-102: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
thelostone-mc
force-pushed
the
feat/strategy-timings-cache
branch
from
4e5aa55 to
5b9fdbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts (2)
25-26: Inconsistent table naming convention.The existing tables use camelCase (
priceCache,metadataCache) while the new tables use snake_case (strategy_timings,strategies_registry). Consider adopting a consistent naming convention across all database tables to improve maintainability.
38-38: Consider setting a default value for thehandledboolean column.The
handledcolumn lacks both a NOT NULL constraint and a default value. This might lead to ambiguity when querying (having to check for bothfalseandnull). Consider adding a default value:-.addColumn("handled", "boolean") +.addColumn("handled", "boolean", (col) => col.defaultTo(false))packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts (1)
62-63: Consider adding mock methods for better coverage.Currently, the
strategyTimingsRepositoryis assigned to an empty object. If the handler eventually relies on calls togetorset, adding Jest/Vitest mock functions here can improve test coverage and help ensure the actual usage is being exercised.- strategyTimingsRepository: {} as ICache<Address, StrategyTimings>, + strategyTimingsRepository: { + get: vi.fn(), + set: vi.fn(), + } as ICache<Address, StrategyTimings>,scripts/bootstrap/src/strategyTimings.script.ts (3)
55-66: Initialize concurrency limits and check environment variables explicitly.The script structure is clear, and you’ve nicely documented the environment variables. However, consider validating concurrency settings (e.g., 10 as a default) and verifying that all required env variables, such as
CHAIN_IDS, are properly parsed before continuing. This helps avoid silent partial failures.
78-81: Proactively handle missing tables.Attempting database queries on
strategyTimingsandstrategiesRegistryis a valid pre-check. If these tables are missing, the script throws an error. Consider adding a clearer error message or fallback logic to guide the user on how to fix the missing tables (e.g., “Please run migrations before proceeding.”).
164-227: Ensure robust logging around retries.While the
retryusage prevents transient failures, additional structured logging about each retry attempt could help when debugging repeated failures or partial data issues. For instance, track the exact attempt number and relevant chain ID and strategy address in the logs.packages/processors/test/allo/handlers/poolCreated.handler.spec.ts (2)
76-79: Mock methods can be extended.You currently mock only
getandset. If further methods (e.g.,clearAll) are ever used, ensure they’re also mocked to avoid unintended undefined method calls.
107-108: Ensure each test scenario uses consistent mocks.Some tests pass a fully mocked
strategyTimingsRepository, while others pass an empty object. For consistent coverage and reduced duplication, consider using the same mock pattern across all tests unless a specific scenario requires otherwise.- strategyTimingsRepository: {} as ICache<Address, StrategyTimings>, + strategyTimingsRepository: mockStrategyTimingsRepository,Also applies to: 135-136, 198-199, 275-276, 345-346
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
.github/production_operations.md(1 hunks)README.md(1 hunks)apps/processing/src/services/sharedDependencies.service.ts(3 hunks)apps/processing/test/unit/sharedDependencies.service.spec.ts(1 hunks)package.json(1 hunks)packages/data-flow/src/orchestrator.ts(1 hunks)packages/data-flow/src/types/index.ts(2 hunks)packages/data-flow/test/integration/helpers/dependencies.ts(2 hunks)packages/data-flow/test/unit/orchestrator.spec.ts(3 hunks)packages/data-flow/test/unit/retroactiveProcessor.spec.ts(2 hunks)packages/indexer-client/src/providers/envioIndexerClient.ts(1 hunks)packages/indexer-client/src/types/indexerClient.types.ts(1 hunks)packages/processors/src/processors/allo/handlers/poolCreated.handler.ts(3 hunks)packages/processors/src/types/processor.types.ts(2 hunks)packages/processors/test/allo/allo.processor.spec.ts(2 hunks)packages/processors/test/allo/handlers/poolCreated.handler.spec.ts(13 hunks)packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts(2 hunks)packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts(2 hunks)packages/processors/test/gitcoinAttestationNetwork/handlers/onAttested.handler.spec.ts(3 hunks)packages/processors/test/registry/handlers/profileCreated.handler.spec.ts(2 hunks)packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts(3 hunks)packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts(2 hunks)packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts(5 hunks)packages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.ts(7 hunks)packages/processors/test/strategy/strategy.processor.spec.ts(2 hunks)packages/processors/test/strategy/strategyHandler.factory.spec.ts(3 hunks)packages/repository/src/db/connection.ts(2 hunks)packages/repository/src/external.ts(1 hunks)packages/repository/src/kysely/repositories/index.ts(1 hunks)packages/repository/src/kysely/repositories/strategyTimings.repository.ts(1 hunks)packages/repository/src/types/index.ts(1 hunks)packages/repository/src/types/strategyTimings.types.ts(1 hunks)scripts/bootstrap/.env.example(1 hunks)scripts/bootstrap/package.json(2 hunks)scripts/bootstrap/src/schemas/index.ts(1 hunks)scripts/bootstrap/src/strategyTimings.script.ts(1 hunks)scripts/migrations/package.json(1 hunks)scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts(0 hunks)scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts(2 hunks)scripts/migrations/src/utils/parsing.ts(2 hunks)
💤 Files with no reviewable changes (1)
- scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts
🚧 Files skipped from review as they are similar to previous changes (34)
- packages/repository/src/kysely/repositories/index.ts
- .github/production_operations.md
- scripts/migrations/package.json
- packages/indexer-client/src/types/indexerClient.types.ts
- apps/processing/src/services/sharedDependencies.service.ts
- packages/repository/src/external.ts
- packages/repository/src/db/connection.ts
- scripts/migrations/src/utils/parsing.ts
- apps/processing/test/unit/sharedDependencies.service.spec.ts
- packages/data-flow/test/integration/helpers/dependencies.ts
- scripts/bootstrap/.env.example
- README.md
- packages/repository/src/types/index.ts
- packages/data-flow/src/orchestrator.ts
- packages/data-flow/src/types/index.ts
- scripts/bootstrap/src/schemas/index.ts
- package.json
- packages/processors/src/types/processor.types.ts
- packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts
- packages/processors/test/gitcoinAttestationNetwork/handlers/onAttested.handler.spec.ts
- packages/repository/src/types/strategyTimings.types.ts
- packages/processors/test/strategy/strategyHandler.factory.spec.ts
- packages/processors/test/allo/allo.processor.spec.ts
- packages/data-flow/test/unit/retroactiveProcessor.spec.ts
- packages/processors/test/registry/handlers/profileCreated.handler.spec.ts
- packages/processors/test/strategy/strategy.processor.spec.ts
- packages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.ts
- packages/processors/src/processors/allo/handlers/poolCreated.handler.ts
- packages/indexer-client/src/providers/envioIndexerClient.ts
- packages/repository/src/kysely/repositories/strategyTimings.repository.ts
- packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts
- packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts
- packages/data-flow/test/unit/orchestrator.spec.ts
- scripts/bootstrap/package.json
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.ts`:
**/*.ts:
scripts/bootstrap/src/strategyTimings.script.tspackages/processors/test/allo/handlers/poolCreated.handler.spec.tsscripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.tspackages/processors/test/strategy/directAllocation/directAllocation.handler.spec.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
`scripts/**/*.ts`: Ensure scripts: - Use `process.cwd()` for root references. - Follow folder conventions (`infra/` for infra scripts, `utilities/` for utilities). - Are orga...
scripts/**/*.ts: Ensure scripts:
- Use
process.cwd()for root references.- Follow folder conventions (
infra/for infra scripts,utilities/for utilities).- Are organized in
package.jsonwithscript:infra:{name}orscript:util:{name}.- Be concise and avoid overly nitpicky feedback outside of these best practices.
scripts/bootstrap/src/strategyTimings.script.tsscripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts
`**/*.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 inten...
**/*.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/processors/test/allo/handlers/poolCreated.handler.spec.tspackages/processors/test/strategy/directAllocation/directAllocation.handler.spec.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
🧬 Code Definitions (4)
scripts/bootstrap/src/strategyTimings.script.ts (5)
scripts/bootstrap/src/schemas/index.ts (1)
getDatabaseConfigFromEnv(67-76)scripts/migrations/src/utils/parsing.ts (1)
parseArguments(22-44)packages/repository/src/kysely/repositories/strategyTimings.repository.ts (1)
KyselyStrategyTimingsCache(7-78)packages/repository/src/external.ts (2)
KyselyStrategyTimingsCache(102-102)KyselyStrategyRegistryRepository(73-73)packages/processors/src/types/processor.types.ts (1)
ProcessorDependencies(13-22)
packages/processors/test/allo/handlers/poolCreated.handler.spec.ts (3)
packages/repository/src/external.ts (2)
ICache(93-93)StrategyTimings(95-95)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)packages/processors/src/types/strategy.types.ts (1)
StrategyTimings(17-22)
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts (2)
packages/repository/src/external.ts (2)
ICache(93-93)StrategyTimings(95-95)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (2)
packages/repository/src/external.ts (2)
ICache(93-93)StrategyTimings(95-95)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)
🪛 GitHub Check: lint / Run Linters
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts
[warning] 91-91:
Unsafe assignment of an any value
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
[warning] 116-116:
Unsafe assignment of an any value
[warning] 138-138:
Unsafe assignment of an any value
[warning] 160-160:
Unsafe assignment of an any value
🔇 Additional comments (12)
scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts (4)
23-31: Well-structured strategy timings cache table with appropriate constraints.This implementation correctly sets up the
strategy_timingstable with proper column definitions and constraints. Theaddresscolumn is correctly marked as NOT NULL since it's used as the primary key, which prevents data integrity issues.
33-40: Well-implemented strategies registry table with proper primary key constraints.The implementation correctly marks both primary key columns (
addressandchainId) as NOT NULL, ensuring data integrity for the composite primary key.
46-47: Proper cleanup in down migration.The down migration correctly drops the newly created tables, ensuring a clean rollback process.
1-1: Migration file has a future date in the filename.The migration filename
20250127T000000_add_cache_tables.tsuses a date in 2025, which is far in the future. Migration systems typically rely on timestamps to determine execution order. Using a future date might cause issues if other migrations need to be added before this one is applied.Consider renaming the file to use a current date or follow your project's migration naming conventions.
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts (2)
8-19: Imports look aligned with recent caching enhancements.The introduced imports for
ICache,StrategyTimings, and shared types (e.g.,Address,ChainId, etc.) appear consistent with the newly added caching logic used elsewhere in the test suite. No immediate issues noted.
84-92: Good verification of constructor dependencies.Verifying the
strategyTimingsRepositoryin the constructor call ensures the parameter is correctly passed. To further strengthen this test, consider asserting thatgetand/orsetis invoked if the logic calls for it.🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 91-91:
Unsafe assignment of ananyvaluescripts/bootstrap/src/strategyTimings.script.ts (1)
289-292: Exit codes indicate success/failure cleanly.Terminating the script with
process.exit(1)on error andprocess.exit(0)on success is straightforward for a CLI script. Looks good.packages/processors/test/allo/handlers/poolCreated.handler.spec.ts (2)
8-12: Imports extended to accommodate caching logic.Adding
ICache,IRoundReadRepository,Round, andStrategyTimingsbetter prepares the test file for verifying strategy timing operations. No issues noted.
61-62: Mock repository for strategy timings.Defining
mockStrategyTimingsRepositoryensures the tests can track and validate the usage ofget/set. This pattern is consistent with other mock dependencies in the test suite.packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (3)
8-11: Imports are properly organized.The addition of
ICacheandStrategyTimingsimports and the restructuring of existing imports are well-organized and necessary for implementing the strategy timings cache feature.Also applies to: 13-19
87-87: Good implementation of strategyTimingsRepository.The handler initialization correctly includes the new strategyTimingsRepository with the appropriate type signature
ICache<Address, StrategyTimings>. This aligns with the PR objective of implementing a caching mechanism for strategy timestamps.
116-116: Test assertions are consistent with the new dependency.All test cases properly verify that the handler passes the strategyTimingsRepository to its handlers. While the static analyzer flags
expect.any(Object)as an unsafe assignment, this is a common and acceptable pattern in test assertions.Also applies to: 138-138, 160-160
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 116-116:
Unsafe assignment of ananyvalue
| let strategyTimingsCounter = 0; | ||
| let nullStrategyTimingsCounter = 0; | ||
|
|
||
| /** | ||
| * Cache strategy timings | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| for await (const _s of pMapIterable(strategyAddressId, async (strategyAddressId) => { | ||
| try { | ||
| //check if exists on strategyTimings | ||
| const strategyTimingsOnRegistry = await strategyTimingsRepository.get( | ||
| strategyAddressId[1] as Address, | ||
| ); | ||
| if (strategyTimingsOnRegistry) { | ||
| strategyTimingsCounter++; | ||
| return; | ||
| } | ||
| const strategyHandler = StrategyHandlerFactory.createHandler( | ||
| strategyAddressId[0] as ChainId, | ||
| { | ||
| logger, | ||
| evmProvider: evmProviders.get(strategyAddressId[0] as ChainId) as EvmProvider, | ||
| } as unknown as ProcessorDependencies, | ||
| strategyAddressId[2] as `0x${string}`, | ||
| ); | ||
| const strategyTimings = await strategyHandler?.fetchStrategyTimings( | ||
| strategyAddressId[1] as Address, | ||
| ); | ||
| await strategyTimingsRepository.set(strategyAddressId[1] as Address, { | ||
| address: strategyAddressId[1] as Address, | ||
| strategyId: strategyAddressId[2] as `0x${string}`, | ||
| timings: strategyTimings, | ||
| createdAt: new Date(), | ||
| }); | ||
| if (!strategyTimings) { | ||
| strategyTimingsCounter++; | ||
| return { | ||
| applicationsStartTime: null, | ||
| applicationsEndTime: null, | ||
| donationsStartTime: null, | ||
| donationsEndTime: null, | ||
| }; | ||
| } | ||
| strategyTimingsCounter++; | ||
| } catch (error) { | ||
| nullStrategyTimingsCounter++; | ||
| console.log(error); | ||
| } | ||
| })) { | ||
| console.log( | ||
| " StrategyTimings counter: ", | ||
| strategyTimingsCounter, | ||
| " out of ", | ||
| strategyAddressId.length, | ||
| " errors: ", | ||
| nullStrategyTimingsCounter, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for successful cache writes.
After calling strategyTimingsRepository.set, consider verifying the write’s success. If the set operation fails due to a transient DB error, it might silently skip caching for many strategies. Either re-throw or log more details upon failure to ensure easy debugging.
thelostone-mc
force-pushed
the
feat/strategy-timings-cache
branch
from
5b9fdbf to
071d9f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/processors/test/allo/handlers/poolCreated.handler.spec.ts (1)
275-275: Consider using the mockStrategyTimingsRepository consistently.Some handler instantiations use
{} as ICache<Address, StrategyTimings>while others use the more complete mock. For consistency, consider using the mockStrategyTimingsRepository in all test cases.- strategyTimingsRepository: {} as ICache<Address, StrategyTimings>, + strategyTimingsRepository: mockStrategyTimingsRepository,Also applies to: 345-345
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (1)
116-116: Consider using a more specific type matcher.The linter is warning about unsafe assignment of an
anyvalue when usingexpect.any(Object). While this is a common testing pattern, you could improve type safety by using a more specific expectation.- strategyTimingsRepository: expect.any(Object), + strategyTimingsRepository: expect.objectContaining({}),Also applies to: 138-138, 160-160
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 116-116:
Unsafe assignment of ananyvaluescripts/bootstrap/src/strategyTimings.script.ts (3)
18-24: Consider using relative imports for better portability.The import path using
../../../packages/processors/dist/src/internal.jscould cause issues if the directory structure changes. Consider using a relative import or adding a path mapping in the tsconfig.json file.-import { - ProcessorDependencies, - StrategyHandlerFactory, -} from "../../../packages/processors/dist/src/internal.js"; +import { + ProcessorDependencies, + StrategyHandlerFactory, +} from "@grants-stack-indexer/processors";
79-80: Error handling could be improved for table verification.The script attempts to verify table existence by executing select queries, but doesn't have specific error handling for cases where tables don't exist. Consider adding more informative error messages.
- //This is only to try in advance if we have the tables and db in sync - await db.selectFrom("strategyTimings").select("address").limit(1).execute(); - await db.selectFrom("strategiesRegistry").select("id").limit(1).execute(); + //This is only to try in advance if we have the tables and db in sync + try { + await db.selectFrom("strategyTimings").select("address").limit(1).execute(); + await db.selectFrom("strategiesRegistry").select("id").limit(1).execute(); + logger.info("Database tables verified successfully"); + } catch (error) { + logger.error("Failed to verify database tables", { error }); + throw new Error("Database tables verification failed. Please ensure migrations have been run."); + }
118-123: Consider using logger instead of console.log for consistency.The script uses a mix of console.log and logger. It would be more consistent to use the logger instance for all logging throughout the script.
if (flattedEvents.length === 0) { hasMoreEvents = false; // No more flattedEvents to process } else { poolCreatedEvents.push(...(flattedEvents as ProcessorEvent<"Allo", "PoolCreated">[])); } - console.log(flattedEvents, " events fetched"); - console.log("\n"); - console.log("Checkpoints by chainId:\r"); - console.log(checkpointMap); + logger.info("Events fetched", { count: flattedEvents.length, events: flattedEvents }); + logger.info("Checkpoints by chainId", { checkpoints: Object.fromEntries(checkpointMap) });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
.github/production_operations.md(1 hunks)README.md(1 hunks)apps/processing/src/services/sharedDependencies.service.ts(3 hunks)apps/processing/test/unit/sharedDependencies.service.spec.ts(1 hunks)package.json(1 hunks)packages/data-flow/src/orchestrator.ts(1 hunks)packages/data-flow/src/types/index.ts(2 hunks)packages/data-flow/test/integration/helpers/dependencies.ts(2 hunks)packages/data-flow/test/unit/orchestrator.spec.ts(3 hunks)packages/data-flow/test/unit/retroactiveProcessor.spec.ts(2 hunks)packages/indexer-client/src/providers/envioIndexerClient.ts(1 hunks)packages/indexer-client/src/types/indexerClient.types.ts(1 hunks)packages/processors/src/processors/allo/handlers/poolCreated.handler.ts(3 hunks)packages/processors/src/types/processor.types.ts(2 hunks)packages/processors/test/allo/allo.processor.spec.ts(2 hunks)packages/processors/test/allo/handlers/poolCreated.handler.spec.ts(13 hunks)packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts(2 hunks)packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts(2 hunks)packages/processors/test/gitcoinAttestationNetwork/handlers/onAttested.handler.spec.ts(3 hunks)packages/processors/test/registry/handlers/profileCreated.handler.spec.ts(2 hunks)packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts(3 hunks)packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts(2 hunks)packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts(5 hunks)packages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.ts(7 hunks)packages/processors/test/strategy/strategy.processor.spec.ts(2 hunks)packages/processors/test/strategy/strategyHandler.factory.spec.ts(3 hunks)packages/repository/src/db/connection.ts(2 hunks)packages/repository/src/external.ts(1 hunks)packages/repository/src/kysely/repositories/index.ts(1 hunks)packages/repository/src/kysely/repositories/strategyTimings.repository.ts(1 hunks)packages/repository/src/types/index.ts(1 hunks)packages/repository/src/types/strategyTimings.types.ts(1 hunks)scripts/bootstrap/.env.example(1 hunks)scripts/bootstrap/package.json(2 hunks)scripts/bootstrap/src/schemas/index.ts(1 hunks)scripts/bootstrap/src/strategyTimings.script.ts(1 hunks)scripts/migrations/package.json(1 hunks)scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts(0 hunks)scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts(2 hunks)scripts/migrations/src/utils/parsing.ts(2 hunks)
💤 Files with no reviewable changes (1)
- scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts
🚧 Files skipped from review as they are similar to previous changes (35)
- packages/indexer-client/src/types/indexerClient.types.ts
- README.md
- scripts/migrations/package.json
- packages/repository/src/types/index.ts
- packages/repository/src/kysely/repositories/index.ts
- apps/processing/test/unit/sharedDependencies.service.spec.ts
- .github/production_operations.md
- packages/data-flow/test/integration/helpers/dependencies.ts
- apps/processing/src/services/sharedDependencies.service.ts
- packages/processors/test/strategy/strategyHandler.factory.spec.ts
- packages/repository/src/external.ts
- package.json
- packages/repository/src/db/connection.ts
- scripts/bootstrap/.env.example
- packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts
- packages/repository/src/types/strategyTimings.types.ts
- packages/data-flow/test/unit/orchestrator.spec.ts
- packages/processors/test/gitcoinAttestationNetwork/handlers/onAttested.handler.spec.ts
- packages/data-flow/src/types/index.ts
- packages/data-flow/test/unit/retroactiveProcessor.spec.ts
- packages/processors/test/allo/allo.processor.spec.ts
- packages/processors/src/types/processor.types.ts
- scripts/migrations/src/utils/parsing.ts
- packages/processors/test/strategy/strategy.processor.spec.ts
- packages/processors/src/processors/allo/handlers/poolCreated.handler.ts
- scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts
- scripts/bootstrap/src/schemas/index.ts
- packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts
- packages/processors/test/registry/handlers/profileCreated.handler.spec.ts
- packages/repository/src/kysely/repositories/strategyTimings.repository.ts
- packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts
- packages/data-flow/src/orchestrator.ts
- scripts/bootstrap/package.json
- packages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.ts
- packages/indexer-client/src/providers/envioIndexerClient.ts
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.ts`:
**/*.ts:
packages/processors/test/allo/handlers/poolCreated.handler.spec.tsscripts/bootstrap/src/strategyTimings.script.tspackages/processors/test/strategy/directAllocation/directAllocation.handler.spec.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
`**/*.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 inten...
**/*.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/processors/test/allo/handlers/poolCreated.handler.spec.tspackages/processors/test/strategy/directAllocation/directAllocation.handler.spec.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
`scripts/**/*.ts`: Ensure scripts: - Use `process.cwd()` for root references. - Follow folder conventions (`infra/` for infra scripts, `utilities/` for utilities). - Are orga...
scripts/**/*.ts: Ensure scripts:
- Use
process.cwd()for root references.- Follow folder conventions (
infra/for infra scripts,utilities/for utilities).- Are organized in
package.jsonwithscript:infra:{name}orscript:util:{name}.- Be concise and avoid overly nitpicky feedback outside of these best practices.
scripts/bootstrap/src/strategyTimings.script.ts
🧬 Code Definitions (2)
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts (2)
packages/repository/src/external.ts (2)
ICache(93-93)StrategyTimings(95-95)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (2)
packages/repository/src/external.ts (2)
ICache(93-93)StrategyTimings(95-95)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)
🪛 GitHub Check: lint / Run Linters
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts
[warning] 91-91:
Unsafe assignment of an any value
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
[warning] 116-116:
Unsafe assignment of an any value
[warning] 138-138:
Unsafe assignment of an any value
[warning] 160-160:
Unsafe assignment of an any value
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: e2e-tests / E2E Tests
🔇 Additional comments (16)
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts (4)
8-8: LGTM! Added necessary imports for cache integration.The imports for
ICacheandStrategyTimingsare correctly added to support the strategy timings cache feature.Also applies to: 11-11
13-19: LGTM! Nicely structured imports.The refactoring of imports from separate lines to a structured import block improves code organization.
62-62: LGTM! Added strategyTimingsRepository dependency.The handler correctly incorporates the new strategyTimingsRepository as required by the caching feature.
91-91: Correctly added strategyTimingsRepository to handler verification.This addresses the previously reported missing verification, ensuring the handler instantiation test is complete.
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 91-91:
Unsafe assignment of ananyvaluepackages/processors/test/allo/handlers/poolCreated.handler.spec.ts (5)
8-12: LGTM! Added necessary imports for cache integration.The imports for
ICacheandStrategyTimingsare correctly added to support the strategy timings cache feature.
13-20: LGTM! Refactored imports into structured blocks.The reorganization of imports improves code readability.
61-61: LGTM! Added mockStrategyTimingsRepository with required methods.The test correctly initializes the mock repository with appropriate get/set methods.
Also applies to: 76-79
107-107: LGTM! Consistently incorporated strategyTimingsRepository.The strategyTimingsRepository is correctly added to each handler instantiation throughout the test file.
Also applies to: 135-135, 198-198, 275-275, 306-306, 345-345, 398-398, 483-483, 511-511
248-248: LGTM! Added verification for strategyTimingsRepository.get call.The test now verifies that the repository's get method is called as expected.
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (4)
8-8: LGTM! Added necessary imports for cache integration.The imports for
ICacheandStrategyTimingsare correctly added to support the strategy timings cache feature.Also applies to: 11-11
13-19: LGTM! Refactored imports into structured blocks.Organizing imports into a structured block improves code readability.
87-87: LGTM! Added strategyTimingsRepository to handler initialization.The strategyTimingsRepository dependency is correctly added to the handler constructor.
116-116: LGTM! Added strategyTimingsRepository to test expectations.All handler invocation test cases now correctly verify the presence of the strategyTimingsRepository.
Also applies to: 138-138, 160-160
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 116-116:
Unsafe assignment of ananyvaluescripts/bootstrap/src/strategyTimings.script.ts (3)
1-6: LGTM! Good choice of dependencies.The imports look appropriate for this script's functionality. Using
p-mapfor concurrency control andts-retryfor handling transient failures are good choices for a bootstrapping script that interacts with external APIs and databases.
256-261: Check for successful cache writes.After calling
strategyTimingsRepository.set, consider verifying the write's success. If the set operation fails due to a transient DB error, it might silently skip caching for many strategies. Either re-throw or log more details upon failure to ensure easy debugging.
289-292: LGTM! Good error handling in the main function call.The error handling for the main function is well-implemented, logging the error and exiting with the appropriate status code.
| if (!strategyTimings) { | ||
| strategyTimingsCounter++; | ||
| return { | ||
| applicationsStartTime: null, | ||
| applicationsEndTime: null, | ||
| donationsStartTime: null, | ||
| donationsEndTime: null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Inconsistent handling of null strategy timings.
When strategy timings are null, the code increments the counter and returns an object with null values, but doesn't actually cache this information. Consider either caching the null values or logging this as a warning.
if (!strategyTimings) {
strategyTimingsCounter++;
- return {
- applicationsStartTime: null,
- applicationsEndTime: null,
- donationsStartTime: null,
- donationsEndTime: null,
- };
+ logger.warn("Null strategy timings for address", {
+ address: strategyAddressId[1],
+ strategyId: strategyAddressId[2]
+ });
+ // Still cache the null timings for future reference
+ await strategyTimingsRepository.set(strategyAddressId[1] as Address, {
+ address: strategyAddressId[1] as Address,
+ strategyId: strategyAddressId[2] as `0x${string}`,
+ timings: {
+ applicationsStartTime: null,
+ applicationsEndTime: null,
+ donationsStartTime: null,
+ donationsEndTime: null,
+ },
+ createdAt: new Date(),
+ });
+ return;
}
strategyTimingsCounter++;| checkpointMap.set(events[0]?.chainId ?? 0, { | ||
| blockNumber: events[events.length - 1]?.blockNumber ?? 0, | ||
| logIndex: events[events.length - 1]?.logIndex ?? 0, | ||
| }); |
There was a problem hiding this comment.
Potential issue with checkpoint update logic.
The code unconditionally sets checkpoints even if the events array is empty, which could potentially overwrite valid checkpoints with zeros. Consider adding a check to ensure events array is not empty before updating checkpoints.
- events.forEach((events) => {
- checkpointMap.set(events[0]?.chainId ?? 0, {
- blockNumber: events[events.length - 1]?.blockNumber ?? 0,
- logIndex: events[events.length - 1]?.logIndex ?? 0,
- });
- });
+ events.forEach((chainEvents) => {
+ if (chainEvents.length > 0) {
+ checkpointMap.set(chainEvents[0].chainId, {
+ blockNumber: chainEvents[chainEvents.length - 1].blockNumber,
+ logIndex: chainEvents[chainEvents.length - 1].logIndex,
+ });
+ }
+ });📝 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.
| checkpointMap.set(events[0]?.chainId ?? 0, { | |
| blockNumber: events[events.length - 1]?.blockNumber ?? 0, | |
| logIndex: events[events.length - 1]?.logIndex ?? 0, | |
| }); | |
| events.forEach((chainEvents) => { | |
| if (chainEvents.length > 0) { | |
| checkpointMap.set(chainEvents[0].chainId, { | |
| blockNumber: chainEvents[chainEvents.length - 1].blockNumber, | |
| logIndex: chainEvents[chainEvents.length - 1].logIndex, | |
| }); | |
| } | |
| }); |
thelostone-mc
force-pushed
the
feat/strategy-timings-cache
branch
from
071d9f7 to
d12c6df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
scripts/bootstrap/src/strategyTimings.script.ts (3)
111-115:⚠️ Potential issuePotential issue with checkpoint update logic.
The code unconditionally sets checkpoints even if the events array is empty, which could potentially overwrite valid checkpoints with zeros.
-events.forEach((events) => { - checkpointMap.set(events[0]?.chainId ?? 0, { - blockNumber: events[events.length - 1]?.blockNumber ?? 0, - logIndex: events[events.length - 1]?.logIndex ?? 0, - }); -}); +events.forEach((chainEvents) => { + if (chainEvents.length > 0) { + checkpointMap.set(chainEvents[0].chainId, { + blockNumber: chainEvents[chainEvents.length - 1].blockNumber, + logIndex: chainEvents[chainEvents.length - 1].logIndex, + }); + } +});
256-261: 🛠️ Refactor suggestionCheck for successful cache writes.
After calling
strategyTimingsRepository.set, consider verifying the write's success. If the set operation fails due to a transient DB error, it might silently skip caching for many strategies.-await strategyTimingsRepository.set(strategyAddressId[1] as Address, { - address: strategyAddressId[1] as Address, - strategyId: strategyAddressId[2] as `0x${string}`, - timings: strategyTimings, - createdAt: new Date(), -}); +try { + await strategyTimingsRepository.set(strategyAddressId[1] as Address, { + address: strategyAddressId[1] as Address, + strategyId: strategyAddressId[2] as `0x${string}`, + timings: strategyTimings, + createdAt: new Date(), + }); + logger.info("Successfully cached strategy timings", { + address: strategyAddressId[1], + strategyId: strategyAddressId[2] + }); +} catch (error) { + logger.error("Failed to cache strategy timings", { + address: strategyAddressId[1], + strategyId: strategyAddressId[2], + error + }); + throw error; // Re-throw to be caught by the outer try-catch +}
262-270:⚠️ Potential issueInconsistent handling of null strategy timings.
When strategy timings are null, the code increments the counter and returns an object with null values, but doesn't actually cache this information.
if (!strategyTimings) { strategyTimingsCounter++; - return { - applicationsStartTime: null, - applicationsEndTime: null, - donationsStartTime: null, - donationsEndTime: null, - }; + logger.warn("Null strategy timings for address", { + address: strategyAddressId[1], + strategyId: strategyAddressId[2] + }); + // Still cache the null timings for future reference + await strategyTimingsRepository.set(strategyAddressId[1] as Address, { + address: strategyAddressId[1] as Address, + strategyId: strategyAddressId[2] as `0x${string}`, + timings: { + applicationsStartTime: null, + applicationsEndTime: null, + donationsStartTime: null, + donationsEndTime: null, + }, + createdAt: new Date(), + }); + return; }
🧹 Nitpick comments (1)
README.md (1)
90-92: Add language specifier to fenced code block.The code block should have a language specified for proper syntax highlighting.
-# To run all of them +# To run all of them +```bash pnpm bootstrap:all +``` # To run one specific one🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
90-90: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
.github/production_operations.md(1 hunks)README.md(1 hunks)apps/processing/src/services/sharedDependencies.service.ts(3 hunks)apps/processing/test/unit/sharedDependencies.service.spec.ts(1 hunks)package.json(1 hunks)packages/data-flow/src/orchestrator.ts(1 hunks)packages/data-flow/src/types/index.ts(2 hunks)packages/data-flow/test/integration/helpers/dependencies.ts(2 hunks)packages/data-flow/test/unit/orchestrator.spec.ts(3 hunks)packages/data-flow/test/unit/retroactiveProcessor.spec.ts(2 hunks)packages/indexer-client/src/providers/envioIndexerClient.ts(1 hunks)packages/indexer-client/src/types/indexerClient.types.ts(1 hunks)packages/processors/src/processors/allo/handlers/poolCreated.handler.ts(3 hunks)packages/processors/src/types/processor.types.ts(2 hunks)packages/processors/test/allo/allo.processor.spec.ts(2 hunks)packages/processors/test/allo/handlers/poolCreated.handler.spec.ts(13 hunks)packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts(2 hunks)packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts(2 hunks)packages/processors/test/gitcoinAttestationNetwork/handlers/onAttested.handler.spec.ts(3 hunks)packages/processors/test/registry/handlers/profileCreated.handler.spec.ts(2 hunks)packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts(3 hunks)packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts(2 hunks)packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts(5 hunks)packages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.ts(7 hunks)packages/processors/test/strategy/strategy.processor.spec.ts(2 hunks)packages/processors/test/strategy/strategyHandler.factory.spec.ts(3 hunks)packages/repository/src/db/connection.ts(2 hunks)packages/repository/src/external.ts(1 hunks)packages/repository/src/kysely/repositories/index.ts(1 hunks)packages/repository/src/kysely/repositories/strategyTimings.repository.ts(1 hunks)packages/repository/src/types/index.ts(1 hunks)packages/repository/src/types/strategyTimings.types.ts(1 hunks)scripts/bootstrap/.env.example(1 hunks)scripts/bootstrap/package.json(2 hunks)scripts/bootstrap/src/schemas/index.ts(1 hunks)scripts/bootstrap/src/strategyTimings.script.ts(1 hunks)scripts/migrations/package.json(1 hunks)scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts(0 hunks)scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts(2 hunks)scripts/migrations/src/utils/parsing.ts(2 hunks)
💤 Files with no reviewable changes (1)
- scripts/migrations/src/migrations/processing/20241210T175001_strategy_registry.ts
✅ Files skipped from review due to trivial changes (2)
- packages/processors/test/gitcoinAttestationNetwork/handlers/onAttested.handler.spec.ts
- packages/processors/test/registry/handlers/profileCreated.handler.spec.ts
🚧 Files skipped from review as they are similar to previous changes (33)
- packages/indexer-client/src/types/indexerClient.types.ts
- .github/production_operations.md
- scripts/bootstrap/package.json
- apps/processing/test/unit/sharedDependencies.service.spec.ts
- packages/data-flow/src/orchestrator.ts
- packages/repository/src/kysely/repositories/index.ts
- packages/data-flow/test/integration/helpers/dependencies.ts
- packages/processors/test/gitcoinAttestationNetwork/gitcoinAttestationNetwork.processor.spec.ts
- packages/data-flow/src/types/index.ts
- packages/repository/src/external.ts
- scripts/bootstrap/src/schemas/index.ts
- scripts/bootstrap/.env.example
- packages/repository/src/types/index.ts
- packages/repository/src/db/connection.ts
- packages/repository/src/types/strategyTimings.types.ts
- packages/indexer-client/src/providers/envioIndexerClient.ts
- scripts/migrations/src/utils/parsing.ts
- packages/processors/test/strategy/strategy.processor.spec.ts
- packages/processors/test/allo/allo.processor.spec.ts
- packages/processors/src/types/processor.types.ts
- packages/data-flow/test/unit/orchestrator.spec.ts
- packages/processors/src/processors/allo/handlers/poolCreated.handler.ts
- packages/processors/test/strategy/strategyHandler.factory.spec.ts
- apps/processing/src/services/sharedDependencies.service.ts
- package.json
- packages/processors/test/alloV1ToV2Migration/allo/alloV1ToV2Migration.processor.spec.ts
- packages/data-flow/test/unit/retroactiveProcessor.spec.ts
- scripts/migrations/src/migrations/processing_cache/20250127T000000_add_cache_tables.ts
- packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts
- packages/processors/test/strategy/easyRetroFunding/easyRetroFunding.handler.spec.ts
- packages/repository/src/kysely/repositories/strategyTimings.repository.ts
- scripts/migrations/package.json
- packages/processors/test/allo/handlers/poolCreated.handler.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.ts`:
**/*.ts:
scripts/bootstrap/src/strategyTimings.script.tspackages/processors/test/strategy/directAllocation/directAllocation.handler.spec.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
`scripts/**/*.ts`: Ensure scripts: - Use `process.cwd()` for root references. - Follow folder conventions (`infra/` for infra scripts, `utilities/` for utilities). - Are orga...
scripts/**/*.ts: Ensure scripts:
- Use
process.cwd()for root references.- Follow folder conventions (
infra/for infra scripts,utilities/for utilities).- Are organized in
package.jsonwithscript:infra:{name}orscript:util:{name}.- Be concise and avoid overly nitpicky feedback outside of these best practices.
scripts/bootstrap/src/strategyTimings.script.ts
`**/*.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 inten...
**/*.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/processors/test/strategy/directAllocation/directAllocation.handler.spec.tspackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
🧬 Code Graph Analysis (2)
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts (2)
packages/repository/src/external.ts (2)
ICache(93-93)StrategyTimings(95-95)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (2)
packages/repository/src/external.ts (2)
ICache(93-93)StrategyTimings(95-95)packages/repository/src/types/strategyTimings.types.ts (1)
StrategyTimings(1-6)
🪛 markdownlint-cli2 (0.17.2)
README.md
90-90: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 GitHub Check: lint / Run Linters
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts
[warning] 91-91:
Unsafe assignment of an any value
packages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts
[warning] 116-116:
Unsafe assignment of an any value
[warning] 138-138:
Unsafe assignment of an any value
[warning] 160-160:
Unsafe assignment of an any value
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: e2e-tests / E2E Tests
🔇 Additional comments (10)
README.md (1)
88-98: LGTM! Bootstrap instructions are clear and comprehensive.The instructions for bootstrapping the database with the new StrategyTimings functionality are well-documented and follow the same pattern as the existing instructions for metadata and pricing.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
90-90: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
packages/processors/test/strategy/directAllocation/directAllocation.handler.spec.ts (3)
7-12: New imports added correctly for strategyTimingsRepository.The imports for
ICacheandStrategyTimingstypes are properly added to support the new repository dependency.
62-62: LGTM! New strategyTimingsRepository dependency added to handler.The strategyTimingsRepository is correctly added to the handler's constructor options with proper typing.
91-91: LGTM! Test verification updated to check for strategyTimingsRepository.The test now properly verifies that DirectAllocatedHandler is instantiated with the strategyTimingsRepository dependency.
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 91-91:
Unsafe assignment of ananyvaluepackages/processors/test/strategy/directGrantsSimple/directGrantsSimple.handler.spec.ts (5)
7-12: New imports added correctly for strategyTimingsRepository.The imports for
ICacheandStrategyTimingstypes are properly added to support the new repository dependency.
87-87: LGTM! New strategyTimingsRepository dependency added to handler.The strategyTimingsRepository is correctly added to the handler's constructor options with proper typing.
116-117: LGTM! Test verification updated for TimestampsUpdated handler.The test now properly verifies that DGSimpleTimestampsUpdatedHandler is instantiated with the strategyTimingsRepository dependency.
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 116-116:
Unsafe assignment of ananyvalue
138-139: LGTM! Test verification updated for RegisteredWithSender handler.The test now properly verifies that DGSimpleRegisteredHandler is instantiated with the strategyTimingsRepository dependency.
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 138-138:
Unsafe assignment of ananyvalue
160-161: LGTM! Test verification updated for DistributedWithRecipientAddress handler.The test now properly verifies that BaseDistributedHandler is instantiated with the strategyTimingsRepository dependency.
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[warning] 160-160:
Unsafe assignment of ananyvaluescripts/bootstrap/src/strategyTimings.script.ts (1)
1-54: Well-documented bootstrap script for strategy timings.The script includes comprehensive documentation explaining its purpose, steps, required environment variables, and usage. This aligns with the PR objective of implementing a caching mechanism for strategy timestamps.
2 participants
Description
Saves strategy timestamps in cache to optimize reindex speed
Note
Since this involves messing around with the migration file , we'll have to be careful
git checkout -b bk-devdb:cache:resetdevand pull in the latest changesdb:cache:migrate(this should create the new cache table for timings)We will likely need repeat the same for the next deployment (blue / green) cause the migration files have been edited
Checklist before requesting a review
Summary by CodeRabbit
Summary by CodeRabbit
New Features
GetEventsFilterstype with an optionaleventNameproperty for improved query filtering.Refactor
Tests
Chores
https://www.loom.com/share/c22dfbde6567495bb66af0cf40df699d