feat: bulk fetch metadata & prices for events batch #57
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant functional changes to the event processing workflow, particularly enhancing the capabilities for fetching metadata and token prices. Key modifications are made to the Changes
Assessment against linked issues
The changes comprehensively address the requirements outlined in GIT-241, implementing a robust mechanism for bulk fetching and processing events with efficient metadata and token price retrieval. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/shared/src/retry/retry.ts (1)
Line range hint
20-30: Update JSDoc to reflect the return value change.The method's JSDoc needs to be updated to accurately describe the new return type and possible undefined case.
* @param operation - Async operation to execute and potentially retry * @param params - Optional parameters * @param params.abortSignal - Optional AbortSignal to cancel retries -* @returns Promise that resolves when operation succeeds or max retries exceeded +* @returns Promise that resolves with the operation result when successful, or undefined * @throws RetriableError if max retries exceeded * @throws Error if operation aborted or non-retriable error occurspackages/shared/test/retry/retry.spec.ts (1)
32-32: Add test cases for undefined result scenarios.While the success case is now properly tested, we should add test cases for scenarios where the operation might return undefined.
Consider adding these test cases:
it("handles undefined result from operation", async () => { const handler = new RetryHandler(new ExponentialBackoff(), mockLogger); const operation = vi.fn().mockResolvedValue(undefined); const result = await handler.execute(operation); expect(result).toBeUndefined(); expect(operation).toHaveBeenCalledTimes(1); });Also applies to: 36-36
packages/pricing/src/providers/cachingProxy.provider.ts (2)
215-244: Consider returning the closest price instead of just the floor valueThe current implementation of
findClosestPricereturns the floor value (the largest timestamp less than or equal to the target timestamp). This may not always provide the closest price if a price with a slightly larger timestamp is closer to the target. Consider modifying the method to select the price with the minimal absolute difference to the target timestamp.You could adjust the method as follows:
// Return the price with the minimal absolute difference to the target timestamp - return prices[left]!; + if (Math.abs(prices[left]!.timestampMs - targetTimestamp) <= Math.abs(prices[right]!.timestampMs - targetTimestamp)) { + return prices[left]!; + } else { + return prices[right]!; + }
124-144: Add JSDoc comments to private methods for better documentationThe new private methods
getCachedPrices,getTimestampsToFetch,getClosestPricesWithCache, andbuildPriceMapare key to the functionality of the caching provider. Adding JSDoc comments to these methods would improve code readability and maintainability by providing clear explanations of their purpose and usage.For example:
/** * Retrieves cached prices for the given timestamps. * @param tokenCode - The token code. * @param timestamps - An array of timestamps. * @returns An array of settled promises with cached price results. */ private async getCachedPrices( tokenCode: TokenCode, timestamps: number[], ): Promise<PromiseSettledResult<CacheResult>[]> { // method implementation... }Also applies to: 146-155, 157-185, 187-206
packages/data-flow/src/orchestrator.ts (1)
213-223: Add JSDoc comments to new private methods for clarityThe methods
getMetadataFromEvents,getTokensFromEvents,bulkFetchMetadata, andbulkFetchTokensare crucial for the batch processing logic. Including JSDoc comments for these methods would enhance understanding for future maintainers and align with the coding guidelines.For example:
/** * Extracts unique metadata IDs from a list of events. * @param events - An array of indexer fetched events. * @returns A promise that resolves to an array of metadata IDs. */ private async getMetadataFromEvents(events: AnyIndexerFetchedEvent[]): Promise<string[]> { // method implementation... }Also applies to: 225-254, 338-355, 357-396
packages/pricing/src/interfaces/pricing.interface.ts (1)
29-36: Add error handling documentation.The JSDoc should document the exceptions that can be thrown, similar to the
getTokenPricemethod. This helps implementers handle errors consistently.Add the following to the JSDoc:
* @param tokenCode - The code of the token. * @param timestamps - Array of timestamps for which to retrieve prices. * @returns A promise that resolves to the prices of the token for each requested timestamp. * The returned prices may not have the exact timestamps requested. Depends on the implementation. + * @throws {UnsupportedToken} if the token is not supported by the pricing provider. + * @throws {NetworkException} if the network is not reachable. + * @throws {UnknownFetchException} if the pricing provider returns an unknown error. */packages/metadata/src/providers/ipfs.provider.ts (1)
40-44: Optimize gateway array creation.Creating a new array for each metadata fetch is inefficient, especially with high request volumes.
Consider caching the ordered gateways array and updating it only when
currentGatewayIndexwraps around:- // Create array of gateways starting from current index - const orderedGateways = Array.from({ length: this.gateways.length }, () => - this.getNextGateway(), - ); + // Use the next gateway directly + const startIndex = this.currentGatewayIndex; + for (let i = 0; i < this.gateways.length; i++) { + const gateway = this.gateways[(startIndex + i) % this.gateways.length]!;packages/pricing/src/providers/coingecko.provider.ts (1)
147-197: Consider rate limiting and performance optimizations.The implementation is solid but could benefit from a few enhancements:
- The error message shows "getTokenPrice" instead of "getTokenPrices"
- The interval parameter is hardcoded to "5m"
- No rate limiting consideration for bulk requests
Consider these improvements:
async getTokenPrices(tokenCode: TokenCode, timestamps: number[]): Promise<TokenPrice[]> { const tokenId = TokenMapping[tokenCode]; if (!tokenId) { throw new UnsupportedToken(tokenCode, { className: CoingeckoProvider.name, - methodName: "getTokenPrice", + methodName: "getTokenPrices", }); } + // Deduplicate timestamps to minimize API load + const uniqueTimestamps = Array.from(new Set(timestamps)); - if (timestamps.length === 0) { + if (uniqueTimestamps.length === 0) { return []; } - const effectiveMin = Math.min(...timestamps); - let effectiveMax = Math.max(...timestamps); + const effectiveMin = Math.min(...uniqueTimestamps); + let effectiveMax = Math.max(...uniqueTimestamps); if (effectiveMin === effectiveMax || effectiveMin > effectiveMax) { return []; } if (effectiveMax - effectiveMin < MIN_GRANULARITY_MS) { effectiveMax = effectiveMin + MIN_GRANULARITY_MS; } + // Calculate optimal interval based on time range + const timeRange = effectiveMax - effectiveMin; + const interval = timeRange > 30 * 24 * 60 * 60 * 1000 ? '1d' : '5m'; - const path = `/coins/${tokenId}/market_chart/range?vs_currency=usd&from=${effectiveMin}&to=${effectiveMax}&precision=full&interval=5m`; + const path = `/coins/${tokenId}/market_chart/range?vs_currency=usd&from=${effectiveMin}&to=${effectiveMax}&precision=full&interval=${interval}`; try { const { data } = await this.axios.get<CoingeckoPriceChartData>(path); - return data.prices.map(([timestampMs, priceUsd]) => ({ + const priceMap = new Map(data.prices.map(([ts, price]) => [ts, price])); + return timestamps.map(ts => ({ - timestampMs, - priceUsd, + timestampMs: ts, + priceUsd: priceMap.get(ts) ?? priceMap.get(findClosestTimestamp(ts, Array.from(priceMap.keys()))) ?? 0, })); } catch (error: unknown) { if (isAxiosError(error)) { this.handleAxiosError(error, path); } const errorMessage = `Unknown Coingecko API error: failed to fetch token price ` + stringify(error, Object.getOwnPropertyNames(error)); throw new UnknownPricingException(errorMessage, { className: CoingeckoProvider.name, - methodName: "getTokenPrice", + methodName: "getTokenPrices", additionalData: { path, }, }); } }Add this helper function:
function findClosestTimestamp(target: number, timestamps: number[]): number { return timestamps.reduce((prev, curr) => Math.abs(curr - target) < Math.abs(prev - target) ? curr : prev ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/data-flow/src/orchestrator.ts(9 hunks)packages/data-flow/test/unit/orchestrator.spec.ts(1 hunks)packages/data-flow/test/unit/retroactiveProcessor.spec.ts(1 hunks)packages/metadata/src/providers/ipfs.provider.ts(3 hunks)packages/pricing/src/constants/index.ts(1 hunks)packages/pricing/src/interfaces/pricing.interface.ts(1 hunks)packages/pricing/src/internal.ts(1 hunks)packages/pricing/src/providers/cachingProxy.provider.ts(3 hunks)packages/pricing/src/providers/coingecko.provider.ts(2 hunks)packages/pricing/src/providers/dummy.provider.ts(1 hunks)packages/processors/test/allo/handlers/poolCreated.handler.spec.ts(1 hunks)packages/processors/test/allo/handlers/poolFunded.handler.spec.ts(1 hunks)packages/processors/test/allo/handlers/poolMetadataUpdated.handler.spec.ts(1 hunks)packages/processors/test/strategy/directAllocation/handlers/directAllocated.handler.spec.ts(1 hunks)packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts(1 hunks)packages/processors/test/strategy/directGrantsLite/handlers/allocated.handler.spec.ts(1 hunks)packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/dvmdDirectTransfer.handler.spec.ts(1 hunks)packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.spec.ts(1 hunks)packages/shared/src/retry/retry.ts(2 hunks)packages/shared/test/retry/retry.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/pricing/src/constants/index.ts
🧰 Additional context used
📓 Path-based instructions (19)
packages/pricing/src/internal.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/processors/test/strategy/directAllocation/handlers/directAllocated.handler.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/processors/test/allo/handlers/poolFunded.handler.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/shared/test/retry/retry.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/dvmdDirectTransfer.handler.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/pricing/src/providers/dummy.provider.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/data-flow/test/unit/orchestrator.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/metadata/src/providers/ipfs.provider.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/shared/src/retry/retry.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/pricing/src/providers/coingecko.provider.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/processors/test/allo/handlers/poolMetadataUpdated.handler.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/processors/test/allo/handlers/poolCreated.handler.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/data-flow/src/orchestrator.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/processors/test/strategy/directGrantsLite/handlers/allocated.handler.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/pricing/src/interfaces/pricing.interface.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/data-flow/test/unit/retroactiveProcessor.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/pricing/src/providers/cachingProxy.provider.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
🔇 Additional comments (16)
packages/shared/src/retry/retry.ts (1)
31-32: LGTM! The changes enhance the method's functionality while maintaining type safety.The modifications to return the operation's result are well-implemented:
- Proper generic type usage
- Clear initialization of result variable
- Consistent error handling
- Clean return statement placement
Also applies to: 36-36, 65-66
packages/data-flow/src/orchestrator.ts (1)
305-309: Review the necessity of clearing caches before each batchIn
bulkFetchMetadataAndPricesForBatch, caches for the metadata and pricing providers are cleared before fetching data for each batch. This could lead to reduced caching benefits and potential performance issues, as cached data from previous batches is discarded. Consider evaluating whether it's necessary to clear the caches before each batch or if the caching strategy can be adjusted to retain useful cached data across batches.Would you like to review the caching strategy or run a verification to assess the impact of not clearing the caches before each batch?
packages/pricing/src/internal.ts (1)
2-2: Ensure consistent export patternsThe addition of
export * from "./constants/index.js";broadens the module's exports. Verify that this change does not introduce any unintended side effects, such as exposing internal constants that should remain private, and that it aligns with the project's module export practices.Please confirm that all exported constants are intended to be publicly available and do not conflict with existing exports.
packages/pricing/src/providers/dummy.provider.ts (1)
24-31: LGTM! Clean implementation of bulk price fetching.The implementation correctly maps each timestamp to a TokenPrice object, maintaining consistency with the single price method. The approach is suitable for testing purposes.
packages/metadata/src/providers/ipfs.provider.ts (1)
11-11: Consider thread safety in multi-instance scenarios.The
currentGatewayIndexcould lead to race conditions if multiple instances ofIpfsProviderare used concurrently.Consider using atomic operations or a more robust load balancing mechanism. For example:
- Use a connection pool
- Implement a stateless round-robin using request timestamp
- Use a distributed load balancer
packages/processors/test/allo/handlers/poolFunded.handler.spec.ts (1)
59-59: Add test coverage for bulk price fetching.The
getTokenPricesmock is added but not utilized in any test cases. This leaves the bulk price fetching functionality untested.Add test cases to verify:
- Bulk price fetching behavior
- Error handling for bulk requests
- Edge cases (empty timestamps array, invalid tokens)
packages/processors/test/allo/handlers/poolMetadataUpdated.handler.spec.ts (1)
71-71: LGTM!The mock provider correctly implements the updated interface.
packages/processors/test/strategy/directGrantsLite/directGrantsLite.handler.spec.ts (1)
87-87: LGTM!The mock provider correctly implements the updated interface.
packages/data-flow/test/unit/retroactiveProcessor.spec.ts (2)
147-147: LGTM! Test setup properly updated for bulk token price fetching.The test setup is correctly updated to include the new
getTokenPricesmethod in thepricingProvidermock, maintaining consistency with the existinggetTokenPricemethod.
147-147: Add test cases for bulk token price fetching.While the mock setup is correct, there are no test cases verifying the bulk token price fetching functionality. Consider adding test cases to cover:
- Successful bulk token price fetching
- Error handling scenarios
- Performance comparison with individual fetches
Would you like me to help generate comprehensive test cases for the bulk token price fetching functionality?
packages/data-flow/test/unit/orchestrator.spec.ts (1)
106-106: LGTM! Test setup properly updated for bulk token price fetching.The test setup is correctly updated to include the new
getTokenPricesmethod in thepricingProvidermock, maintaining consistency with the existinggetTokenPricemethod.packages/processors/test/strategy/directAllocation/handlers/directAllocated.handler.spec.ts (1)
44-44: Add tests for bulk price fetching functionality.The
getTokenPricesmock is added but not utilized in any test cases. Consider adding test coverage for bulk price fetching scenarios to ensure the new functionality works as expected.Would you like me to help generate test cases for bulk price fetching scenarios?
packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.spec.ts (1)
49-49: Add tests for bulk price fetching functionality.While the file has comprehensive test coverage for existing functionality, the newly added
getTokenPricesmock is not utilized in any test cases. Consider adding test coverage for bulk price fetching scenarios.Would you like me to help generate test cases for bulk price fetching scenarios that align with the existing test patterns in this file?
packages/processors/test/strategy/directGrantsLite/handlers/allocated.handler.spec.ts (1)
44-44: Add tests for bulk price fetching functionality.The file has thorough test coverage for various scenarios, but the newly added
getTokenPricesmock is not utilized. Consider adding test cases for bulk price fetching to maintain the same level of test coverage for the new functionality.Would you like me to help generate test cases for bulk price fetching that cover similar edge cases as the existing tests?
packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/dvmdDirectTransfer.handler.spec.ts (1)
115-115: Add tests for bulk price fetching functionality.While the file has thorough test coverage for
fetchMatchAmountand other functionality, the newly addedgetTokenPricesmock is not utilized. Consider adding test cases for bulk price fetching scenarios.Would you like me to help generate test cases for bulk price fetching that integrate with the existing
fetchMatchAmounttests?packages/processors/test/allo/handlers/poolCreated.handler.spec.ts (1)
59-59: Add test coverage for the newgetTokenPricesmethod.The newly added
getTokenPricesmethod inmockPricingProvideris not covered by any test cases. This method appears to be part of the bulk fetching feature mentioned in the PR objectives.Let's verify if there are any existing tests for bulk price fetching:
Would you like me to help generate test cases for bulk price fetching scenarios?
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
packages/shared/test/retry/retry.spec.ts (1)
39-46: Consider adding logger assertion for completeness.The test case effectively covers the undefined result scenario. Consider adding an assertion to verify logger behavior for consistency with other test cases:
expect(result).toBeUndefined(); expect(operation).toHaveBeenCalledTimes(1); +expect(mockLogger.debug).not.toHaveBeenCalled();packages/data-flow/src/orchestrator.ts (7)
37-40: Add JSDoc comments for the newTokenWithTimestampstype.To enhance code readability and maintainability, please include JSDoc comments explaining the purpose and usage of this new type.
212-223: Add JSDoc comments forgetMetadataFromEventsmethod.Including JSDoc comments will improve maintainability by documenting the method's purpose, parameters, and return value.
224-255: Add JSDoc comments forgetTokensFromEventsmethod.Including documentation will aid in understanding and maintaining the code by explaining the method's functionality, parameters, and return value.
Line range hint
284-299: Add JSDoc comments forgetNextEventsBatchmethod.To maintain consistency and improve readability, please document this method with JSDoc, explaining its purpose, parameters, and return value.
Line range hint
320-335: Add JSDoc comments forenqueueEventsmethod.To improve code maintainability, include JSDoc comments describing the purpose, parameters, and behavior of the
enqueueEventsmethod.
336-356: Add JSDoc comments forbulkFetchMetadatamethod.Including documentation will aid in understanding and maintaining the code by explaining the method's functionality, parameters, and return type.
358-382: Add JSDoc comments forbulkFetchTokensmethod.To maintain consistency and enhance readability, please add JSDoc comments outlining the method's purpose, parameters, and expected outcomes.
packages/pricing/test/providers/coingecko.provider.spec.ts (1)
183-222: Test suite needs additional edge cases.While the current test coverage is good, consider adding these test cases:
- Verify correct price mapping for multiple timestamps
- Test behavior when some timestamps are invalid
- Test handling of network errors (500 status)
Here's a suggested test case for multiple timestamps:
it("correctly maps prices for multiple timestamps", async () => { const timestamps = [1000, 2000, 3000]; mock.get.mockResolvedValue({ data: { prices: [ [1000, 1500], [2000, 1600], [3000, 1700], ], }, }); const result = await provider.getTokenPrices("ETH" as TokenCode, timestamps); expect(result).toEqual([ { timestampMs: 1000, priceUsd: 1500 }, { timestampMs: 2000, priceUsd: 1600 }, { timestampMs: 3000, priceUsd: 1700 }, ]); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/data-flow/src/orchestrator.ts(9 hunks)packages/data-flow/test/unit/orchestrator.spec.ts(6 hunks)packages/metadata/src/providers/ipfs.provider.ts(2 hunks)packages/pricing/src/providers/coingecko.provider.ts(2 hunks)packages/pricing/test/providers/cachingProxy.provider.spec.ts(1 hunks)packages/pricing/test/providers/coingecko.provider.spec.ts(1 hunks)packages/pricing/test/providers/dummy.provider.spec.ts(1 hunks)packages/shared/test/retry/retry.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
packages/metadata/src/providers/ipfs.provider.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/pricing/test/providers/dummy.provider.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/shared/test/retry/retry.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/pricing/test/providers/coingecko.provider.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/pricing/test/providers/cachingProxy.provider.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/pricing/src/providers/coingecko.provider.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/data-flow/test/unit/orchestrator.spec.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
packages/data-flow/src/orchestrator.ts (1)
Pattern **/*.ts: Review the TypeScript code for adherence to the following guidelines:
- Avoid over-abstraction that may lead to coupling; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid using any; prefer unknown where necessary.
- Use runtime type-checking for environment variables (e.g., using Zod).
- Follow the internal module pattern to prevent circular dependencies.
- When building libraries, ensure an external.ts file explicitly lists components/types/interfaces to expose, and these are imported/exported in the entry index.ts.
- Use bigint as-is when received from external APIs; only cast to Number if exposing the value through an API.
- Ensure all code components (functions, methods, classes, interfaces) are documented with JSDoc.
- Static Async Factory Functions (async constructors) are encouraged.
🔇 Additional comments (9)
packages/shared/test/retry/retry.spec.ts (1)
32-37: LGTM! Good test enhancement.The addition of result assertion improves test coverage by verifying both the behavior (operation call count) and the output value.
packages/data-flow/test/unit/orchestrator.spec.ts (3)
775-793: Good test coverage forgetMetadataFromEvents.The tests adequately cover scenarios including duplicates and events without metadata.
796-868: Comprehensive tests forgetTokensFromEvents.The test cases effectively cover duplicates, zero amounts, and invalid tokens, ensuring robust validation of the method's logic.
870-918: Effective testing ofbulkFetchMetadataAndPricesForBatchmethod.The tests verify both success and failure scenarios, ensuring the orchestrator handles fetch operations correctly even when some fetches fail.
packages/pricing/test/providers/dummy.provider.spec.ts (1)
19-38: Good addition of test cases forgetTokenPricesmethod.The new tests effectively verify the functionality for both populated and empty timestamps arrays.
packages/pricing/test/providers/cachingProxy.provider.spec.ts (2)
12-14: LGTM! Mock provider correctly implements new interface methods.The mock provider now implements both
getTokenPricesandclearCachemethods, aligning with theIPricingProvider & ICacheableinterface.
18-19: LGTM! Mock cache correctly implements async behavior.The mock cache's
setmethod now properly returns a Promise, accurately reflecting the asynchronous nature of cache operations.packages/pricing/src/providers/coingecko.provider.ts (2)
164-166: LGTM! Proper handling of minimum granularity.The code correctly adjusts the time range when it's smaller than MIN_GRANULARITY_MS.
151-154:⚠️ Potential issueFix incorrect method name in error context.
The error context references "getTokenPrice" instead of "getTokenPrices".
Apply this fix:
throw new UnsupportedToken(tokenCode, { className: CoingeckoProvider.name, - methodName: "getTokenPrice", + methodName: "getTokenPrices", });Likely invalid or redundant comment.
| /** | ||
| * Clear caches and fetch metadata and prices for the batch | ||
| */ | ||
| private async bulkFetchMetadataAndPricesForBatch( | ||
| events: AnyIndexerFetchedEvent[], | ||
| ): Promise<void> { | ||
| // Clear caches | ||
| if (this.dependencies.metadataProvider.clearCache) { | ||
| await this.dependencies.metadataProvider.clearCache(); | ||
| } | ||
|
|
||
| const metadataIds = await this.getMetadataFromEvents(events); | ||
| const tokens = await this.getTokensFromEvents(events); | ||
|
|
||
| await Promise.allSettled([ | ||
| this.bulkFetchMetadata(metadataIds), | ||
| this.bulkFetchTokens(tokens), | ||
| ]); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors from bulkFetchMetadata and bulkFetchTokens.
While using Promise.allSettled prevents unhandled promise rejections, you should inspect the results to handle any failures appropriately, such as logging errors or retrying failed operations.
Apply this diff to handle errors from bulkFetchMetadata and bulkFetchTokens:
- await Promise.allSettled([
+ const results = await Promise.allSettled([
this.bulkFetchMetadata(metadataIds),
this.bulkFetchTokens(tokens),
]);
+
+ results.forEach((result, index) => {
+ if (result.status === 'rejected') {
+ const operation = index === 0 ? 'bulkFetchMetadata' : 'bulkFetchTokens';
+ this.logger.error(`Error in ${operation}: ${result.reason}`, {
+ className: Orchestrator.name,
+ chainId: this.chainId,
+ });
+ }
+ });Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
we're fine without handling errors here, this is a performance optimization but in case some fetch fails, we would try one more time to fetch the data on the processor
There was a problem hiding this comment.
@0xnigir1 Understood, thanks for explaining the error handling strategy. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: 0xnigir1
PR: defi-wonderland/grants-stack-indexer-v2#57
File: packages/data-flow/src/orchestrator.ts:300-319
Timestamp: 2025-01-22T01:29:58.582Z
Learning: In the Orchestrator's bulkFetchMetadataAndPricesForBatch method, error handling is not required as it's a performance optimization. If the bulk fetch fails, the data will be fetched again during processing in the processor.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @@ -8,6 +8,7 @@ import { EmptyGatewaysUrlsException, InvalidContentException, isValidCid } from | |||
|
|
|||
| export class IpfsProvider implements IMetadataProvider { | |||
| private readonly axiosInstance: AxiosInstance; | |||
| private currentGatewayIndex: number = 0; | |||
There was a problem hiding this comment.
Consider thread safety for gateway index.
The currentGatewayIndex should be updated atomically to prevent race conditions in concurrent requests.
Consider using a synchronized method to update the index:
private synchronized updateGatewayIndex(): number {
this.currentGatewayIndex = (this.currentGatewayIndex + 1) % this.gateways.length;
return this.currentGatewayIndex;
}| const startIndex = this.currentGatewayIndex; | ||
| for (let i = 0; i < this.gateways.length; i++) { | ||
| const gateway = this.gateways[(startIndex + i) % this.gateways.length]!; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve gateway rotation logic.
The current implementation doesn't update currentGatewayIndex after successful requests, which could lead to uneven gateway utilization.
Consider updating the index after successful requests:
- const startIndex = this.currentGatewayIndex;
- for (let i = 0; i < this.gateways.length; i++) {
- const gateway = this.gateways[(startIndex + i) % this.gateways.length]!;
+ const totalGateways = this.gateways.length;
+ for (let attempt = 0; attempt < totalGateways; attempt++) {
+ const gateway = this.gateways[this.currentGatewayIndex]!;
+ this.currentGatewayIndex = (this.currentGatewayIndex + 1) % totalGateways;📝 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 startIndex = this.currentGatewayIndex; | |
| for (let i = 0; i < this.gateways.length; i++) { | |
| const gateway = this.gateways[(startIndex + i) % this.gateways.length]!; | |
| const totalGateways = this.gateways.length; | |
| for (let attempt = 0; attempt < totalGateways; attempt++) { | |
| const gateway = this.gateways[this.currentGatewayIndex]!; | |
| this.currentGatewayIndex = (this.currentGatewayIndex + 1) % totalGateways; |
| methodName: "getTokenPrice", | ||
| additionalData: { |
There was a problem hiding this comment.
Fix incorrect method name in error context.
Similar to the previous issue, the error context references the wrong method name.
Apply this fix:
className: CoingeckoProvider.name,
- methodName: "getTokenPrice",
+ methodName: "getTokenPrices",Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
The rabbit is right here ^
There was a problem hiding this comment.
@jahabeebs Thanks for confirming! Yes, it's important to maintain consistency in error contexts to help with debugging.
✏️ Learnings added
Learnt from: jahabeebs
PR: defi-wonderland/grants-stack-indexer-v2#57
File: packages/pricing/src/providers/coingecko.provider.ts:187-188
Timestamp: 2025-01-22T19:10:33.446Z
Learning: In TypeScript code, ensure that error contexts reference the correct method name where the error occurred, especially in catch blocks. For example, if an error occurs in method `getTokenPrices`, the error context should use "getTokenPrices" as the method name, not "getTokenPrice".
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
|
||
| for (const event of events) { | ||
| if ("metadata" in event.params) { | ||
| ids.add(event.params.metadata[1]); |
There was a problem hiding this comment.
| ids.add(event.params.metadata[1]); | |
| ids.add(event.params.metadata[1]); |
is metadata[1] safe ?
There was a problem hiding this comment.
yes is safe to use it this way
| } | ||
| } | ||
|
|
||
| return Array.from(ids); |
There was a problem hiding this comment.
are u using a set to avoid repeats ?
| @@ -197,6 +209,51 @@ export class Orchestrator { | |||
| }); | |||
| } | |||
|
|
|||
| private async getMetadataFromEvents(events: AnyIndexerFetchedEvent[]): Promise<string[]> { | |||
| return Array.from(ids); | ||
| } | ||
|
|
||
| private async getTokensFromEvents( |
| if ( | ||
| "token" in event.params && | ||
| "amount" in event.params && | ||
| BigInt(event.params.amount) > 0n |
There was a problem hiding this comment.
should we add runtime validation here ?
There was a problem hiding this comment.
sorry what validation are u specifically referring to?
There was a problem hiding this comment.
typings runtime validations , same as metadata
There was a problem hiding this comment.
will track in Linear to see where this validation fits the best in the system
| const ids = new Set<string>(); | ||
|
|
||
| for (const event of events) { | ||
| if ("metadata" in event.params) { |
There was a problem hiding this comment.
should we add an extra validation to check metadata structure
There was a problem hiding this comment.
here isn't relevant as different events have different structure, I'd leave that to the event processor
|
|
||
| /** | ||
| * Fetch all possible prices for the batch | ||
| */ |
| @@ -0,0 +1 @@ | |||
| export const MIN_GRANULARITY_MS = 300_000; // 5 minutes | |||
There was a problem hiding this comment.
add more context about coingecko and reference link pls 🙏
| ): Promise<void> { | ||
| // Clear caches | ||
| if (this.dependencies.metadataProvider.clearCache) { | ||
| await this.dependencies.metadataProvider.clearCache(); |
There was a problem hiding this comment.
token prices will live longer with a ttl , right ? i was wondering if a memory overflow could happen in this scenario, lets talk about this offline
There was a problem hiding this comment.
fixed the issue as per discussed offline
| @@ -31,6 +34,11 @@ import { EventsProcessor } from "./eventsProcessor.js"; | |||
| import { InvalidEvent } from "./exceptions/index.js"; | |||
| import { CoreDependencies, DataLoader, delay, IQueue, iStrategyAbi, Queue } from "./internal.js"; | |||
|
|
|||
| type TokenWithTimestamps = { | |||
| token: Token; | |||
| timestamps: number[]; | |||
There was a problem hiding this comment.
What is the exact type of the timestamps? Maybe we should define a type like NumberMs to prevent conversion confusion
There was a problem hiding this comment.
could be nice, i'll analyze how much impact it has on the codebase. if it's too big, i'll update the name to timestampsMs to be a bit clearer and leave the typing as tech debt
| * Enqueue new events from the events fetcher using the last processed event as a starting point | ||
| */ | ||
| private async enqueueEvents(): Promise<void> { | ||
| private async getNextEventsBatch(): Promise<AnyIndexerFetchedEvent[]> { |
| ), | ||
| ); | ||
|
|
||
| const metadata: unknown[] = []; |
There was a problem hiding this comment.
In the future we should try to parse or validate the metadata at sone basic level to be sure we can actually use it & make sure that we aren't typing it as unknown in the whole app
There was a problem hiding this comment.
metadata parsing (using Zod schemas) is done in the processors, is not Orchestrator responsibility. metadata has different shapes depending on the event but the Orchestrator doesn't know this
| @@ -755,6 +770,152 @@ describe("Orchestrator", { sequential: true }, () => { | |||
| expect(logger.error).not.toHaveBeenCalled(); | |||
| }); | |||
| }); | |||
|
|
|||
| describe("getMetadataFromEvents", () => { | |||
There was a problem hiding this comment.
What about a failure case? It's unknown so what if we can't parse it or can't get usable info from the metadata?
There was a problem hiding this comment.
this enhancement is related to speed and performance of the system. whether metadata is correct or not is task of the processors (which will eventually fail or not)
| .filter((price): price is TokenPrice => !!price); | ||
| } | ||
|
|
||
| private async getCachedPrices( |
There was a problem hiding this comment.
Docs (and for the rest of the file)
| methodName: "getTokenPrice", | ||
| additionalData: { |
There was a problem hiding this comment.
The rabbit is right here ^
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/data-flow/src/orchestrator.ts (3)
37-40: Consider using a more specific type for timestamps.Based on past discussions, consider either:
- Creating a
NumberMstype to prevent conversion confusion, or- Renaming the field to
timestampsMsto be more explicit about the unit.type TokenWithTimestamps = { token: Token; - timestamps: number[]; + timestampsMs: number[]; };
230-264: Token extraction logic is robust and well-documented.The implementation includes good practices:
- Filtering out zero amounts
- Deduplicating timestamps
- Sorting timestamps for consistency
- Proper null checks for token resolution
Consider extracting the magic number
0ninto a named constant for better readability.+const ZERO_AMOUNT = 0n; + private getTokensFromEvents(events: AnyIndexerFetchedEvent[]): TokenWithTimestamps[] { // ... if ( "token" in event.params && "amount" in event.params && - BigInt(event.params.amount) > 0n + BigInt(event.params.amount) > ZERO_AMOUNT ) {
320-334: Bulk fetching implementation is efficient and fault-tolerant.Good practices observed:
- Cache clearing before fetching
- Parallel execution using
Promise.allSettled- Graceful handling of partial failures
Consider adding debug logging for cache clearing operations to aid in troubleshooting.
// Clear caches if the provider supports it + this.logger.debug('Clearing metadata and pricing caches', { + className: Orchestrator.name, + chainId: this.chainId + }); await this.dependencies.metadataProvider.clearCache?.(); await this.dependencies.pricingProvider.clearCache?.();packages/pricing/src/providers/coingecko.provider.ts (2)
147-193: Consider optimizing the error message for bulk operations.The error message "failed to fetch token price" could be more specific since we're fetching multiple prices.
Apply this diff to improve the error message:
- `Unknown Coingecko API error: failed to fetch token price ` + + `Unknown Coingecko API error: failed to fetch token prices ` +
164-166: Consider adding a maximum time range limit.The code adjusts the minimum time range but doesn't limit the maximum range. Large time ranges could lead to excessive data fetching and potential rate limiting.
Consider adding a maximum time range check:
if (effectiveMax - effectiveMin < MIN_GRANULARITY_MS) { effectiveMax = effectiveMin + MIN_GRANULARITY_MS; } +const MAX_TIME_RANGE = 30 * 24 * 60 * 60 * 1000; // 30 days +if (effectiveMax - effectiveMin > MAX_TIME_RANGE) { + throw new NonRetriableError( + `Time range exceeds maximum allowed (${MAX_TIME_RANGE}ms)`, + { + className: CoingeckoProvider.name, + methodName: "getTokenPrices", + }, + ); +}packages/pricing/src/providers/cachingProxy.provider.ts (3)
54-68: Consider adding a retry mechanism for cache operations.The non-blocking cache operation is good for performance, but consider adding a retry mechanism for failed cache operations to improve cache hit rates.
Consider implementing a retry mechanism:
+ private async retryCache<T>( + operation: () => Promise<T>, + errorMessage: string, + retries = 3, + delay = 1000, + ): Promise<T> { + for (let i = 0; i < retries; i++) { + try { + return await operation(); + } catch (error) { + if (i === retries - 1) { + this.logger.debug(errorMessage, { error }); + throw error; + } + await new Promise((resolve) => setTimeout(resolve, delay)); + } + } + throw new Error('Unreachable'); + } // In getTokenPrice method: - this.cache - .set( + this.retryCache( + () => this.cache.set( { tokenCode, timestampMs: startTimestampMs, }, price, - ) - .catch((error) => { - this.logger.debug( - `Failed to cache price for token ${tokenCode} at ${startTimestampMs}`, - { error }, - ); - }); + ), + `Failed to cache price for token ${tokenCode} at ${startTimestampMs}`, + ).catch(() => {});
239-269: Optimize binary search implementation.The binary search implementation can be simplified and made more efficient.
Consider this optimized implementation:
private findClosestPrice(prices: TokenPrice[], targetTimestamp: number): TokenPrice | null { if (prices.length === 0) { return null; } - // Handle edge cases - if (targetTimestamp <= prices[0]!.timestampMs) return prices[0]!; - if (targetTimestamp >= prices[prices.length - 1]!.timestampMs) - return prices[prices.length - 1]!; - - // Binary search let left = 0; let right = prices.length - 1; - while (left + 1 < right) { + while (left <= right) { const mid = Math.floor((left + right) / 2); + const midPrice = prices[mid]!; - if (prices[mid]!.timestampMs === targetTimestamp) { - return prices[mid]!; + if (midPrice.timestampMs === targetTimestamp) { + return midPrice; } - if (prices[mid]!.timestampMs < targetTimestamp) { + if (midPrice.timestampMs < targetTimestamp) { left = mid; } else { right = mid; } } - // Return the floor value (largest timestamp <= target) - return prices[left]!; + // Find closest price + const leftDiff = Math.abs(prices[left]!.timestampMs - targetTimestamp); + const rightDiff = right < prices.length ? Math.abs(prices[right]!.timestampMs - targetTimestamp) : Infinity; + return leftDiff <= rightDiff ? prices[left]! : prices[right]!; }
191-192: Document the fire-and-forget pattern.The comment "Fire and forget cache operation" should be expanded to explain why this pattern is used.
Consider expanding the comment:
- // Fire and forget cache operation + // Fire and forget cache operation to avoid blocking the response + // Cache failures are logged but don't affect the price lookup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/data-flow/src/orchestrator.ts(10 hunks)packages/pricing/src/constants/index.ts(1 hunks)packages/pricing/src/providers/cachingProxy.provider.ts(3 hunks)packages/pricing/src/providers/coingecko.provider.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/pricing/src/constants/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
packages/pricing/src/providers/coingecko.provider.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/data-flow/src/orchestrator.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/pricing/src/providers/cachingProxy.provider.ts (1)
Pattern **/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoid any; use unknown when necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have an external.ts file explicitly listing public exports.
- Use bigint as-is; cast to Number only when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
📓 Learnings (1)
packages/pricing/src/providers/coingecko.provider.ts (1)
Learnt from: jahabeebs
PR: defi-wonderland/grants-stack-indexer-v2#57
File: packages/pricing/src/providers/coingecko.provider.ts:187-188
Timestamp: 2025-01-22T19:10:33.586Z
Learning: In TypeScript code, ensure that error contexts reference the correct method name where the error occurred, especially in catch blocks. For example, if an error occurs in method `getTokenPrices`, the error context should use "getTokenPrices" as the method name, not "getTokenPrice".
🔇 Additional comments (6)
packages/data-flow/src/orchestrator.ts (2)
Line range hint
42-69: Documentation is comprehensive and well-structured.The class documentation clearly explains the new bulk fetching step and its benefits.
128-132: Implementation aligns with performance optimization goals.The sequential flow of getting events, bulk fetching data, and then enqueueing is logically sound. As discussed in past reviews, error handling is intentionally omitted here since failed fetches will be retried during processing.
packages/pricing/src/providers/coingecko.provider.ts (3)
16-16: LGTM!The import of
MIN_GRANULARITY_MSis correctly added to support the new functionality.
130-130: LGTM!The error handling is correctly updated to use the new method name parameter.
195-195: LGTM!The method signature update correctly includes the new
methodNameparameter.packages/pricing/src/providers/cachingProxy.provider.ts (1)
6-9: LGTM!The
CacheResulttype is well-defined and appropriately captures both the timestamp and optional price.
| if ( | ||
| "token" in event.params && | ||
| "amount" in event.params && | ||
| BigInt(event.params.amount) > 0n |
There was a problem hiding this comment.
typings runtime validations , same as metadata
🤖 Linear
Closes GIT-241
Description
Currently, we are fetching price/metadata information when processing each event. This is sub-optimal, since we are having the external request delay for each event.
As a better solution, we're fetching all the token/metadata data for a given batch of events prior to start processing that batch. This way, we can query all this data in parallel and have it cached when required during event processing.
Note
This PR has the assumption that we will have really high rate limit API keys for Metadata and Pricing providers to make the most of this feature
Checklist before requesting a review
Summary by CodeRabbit
Release Notes
New Features
getTokenPricesmethod across multiple pricing providers.Improvements
Technical Enhancements