feat: bootstrap scripts & metadata and pricing improvements #63
Conversation
|
@CodeRabbit pls review it :D |
🧩 Analysis chain🏁 Script executed: Length of output: 2005 🏁 Script executed: Length of output: 81973 🏁 Script executed: Length of output: 31 🏁 Script executed: Length of output: 3028 🏁 Script executed: Length of output: 12844 @0xkenj1 I've reviewed the changes and here's my feedback: The PR implements two main bootstrap scripts with solid functionality:
Suggestions:
The implementation looks solid, but as noted in the PR description "DO NOT MERGE, WE NEED TO TEST EVERYTHING", thorough testing should be performed before merging. Would you like me to help with updating the README or adding any specific tests? ✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe pull request encompasses a wide range of modifications across configuration files, application code, tests, and scripts. Environment variables have been updated—removing some obsolete ones and introducing new variables like NODE_ENV and METADATA_SOURCE—to better structure configuration settings. The Dockerfile now enables and prepares a specific pnpm version and alters deployment command parameters. The metadata provider refactoring replaces direct instantiation with a factory-based approach, and related tests have been updated accordingly. Several modules across data-flow, pricing, and repository now feature updated method signatures, enhanced caching logic, and refined error handling, including support for null values. New exception classes and type definitions are introduced to improve robustness. In addition, new scripts and configuration files for bootstrapping and database migrations have been added or revised to support environment-specific and domain-specific settings. Overall, these changes update dependency management, streamline environment validation, and enhance infrastructure consistency throughout the project. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (39)
scripts/bootstrap/src/utils/kysely.ts (1)
3-7: Add JSDoc documentation for the interface.The interface is well-structured but lacks JSDoc documentation. Please add documentation to describe the purpose of the interface and its generic type parameter.
+/** + * Configuration for database migrations. + * @template T The database schema type. + */ export interface MigrationConfig<T> { + /** The Kysely database instance */ db: Kysely<T>; + /** The database schema name */ schema: string; + /** Path to the migrations folder */ migrationsFolder: string; }scripts/bootstrap/src/schemas/index.ts (2)
6-13: Improve error context for invalid JSON.Currently, the custom issue logged is generic. Consider enhancing the error message to identify which specific environment variable failed JSON parsing for quicker debugging.
Example enhancement:
- ctx.addIssue({ code: "custom", message: "Invalid JSON" }); + ctx.addIssue({ + code: "custom", + message: `Invalid JSON in environment variable: "${str.slice(0, 20)}..."`, + });
56-74: Consolidate environment parsing.
getDatabaseConfigFromEnv()andgetEnv()both parseprocess.envwith separate schemas. This can inadvertently lead to inconsistencies if the environment changes between calls. Consider consolidating the environment parsing so the code only parses once and reuses results.scripts/bootstrap/src/pricing.script.ts (2)
22-44: Update documentation block to match actual script behavior.The docstring references database migrations exclusively, but this script primarily populates a pricing cache from external sources. Consider revising the doc block to accurately reflect token price fetching and storing processes.
102-125: Handle partial fetch failures more robustly.While unsupported tokens are logged, other errors are simply returned with a "rejected" status. Consider more explicit error reporting or partial retry strategies to avoid halting the entire process if one token fails repeatedly.
scripts/bootstrap/src/metadata.script.ts (3)
91-121: Persisting CIDs to an external file might create fragility.
Storing CIDs incids.txtis quick to implement, but it’s prone to data inconsistencies if the script crashes mid-process or runs in different environments. Consider storing checkpoint data in a more robust or centralized location (e.g., a database table).
130-131: Fix minor grammar in console output.
Currently the script logs "where fetched"; it should be "were fetched" for correct usage.- console.log(cids.length, " where fetched"); + console.log(cids.length, " were fetched");
150-151: Revisit high concurrency limit.
The concurrency limit is set to 1000, which may cause rate-limiting from external gateways. Consider making it configurable or lowering it to avoid throttling or socket exhaustion.- concurrency: 1000, + concurrency: 100,packages/data-flow/src/orchestrator.ts (1)
333-346: Make concurrency level configurable instead of hardcoding.
A concurrency of 10 is a reasonable start, but theFIXMEsuggests it’s meant to be environment-dependent or user-configurable. Keeping it hardcoded can limit flexibility and hamper performance tuning.packages/pricing/src/exceptions/noClosePriceFound.exception.ts (1)
1-5: Consider enhancing the error handling implementation.
- Consider extending a more specific error base class (e.g.,
NonRetriableError) for consistent error handling across the codebase.- Add JSDoc documentation to describe the purpose and usage of this exception.
+/** + * Exception thrown when no close price is found for a token. + * This is typically a non-retriable error that indicates the requested price data is unavailable. + */ -export class NoClosePriceFound extends Error { +export class NoClosePriceFound extends NonRetriableError { constructor() { super(`No close price found`); } }packages/metadata/src/exceptions/invalidMetadataSource.exception.ts (1)
1-7: Enhance error message and add documentation.
- Add JSDoc documentation to describe the purpose and usage of this exception.
- Consider including the provided source in the error message for better debugging.
+/** + * Exception thrown when an invalid or unsupported metadata source is provided. + * This is a non-retriable error that indicates a configuration issue. + */ export class InvalidMetadataSourceException extends NonRetriableError { - constructor() { - super(`Invalid metadata source`); + constructor(source?: string) { + super(`Invalid metadata source${source ? `: ${source}` : ''}`); } }packages/metadata/src/exceptions/missingDependencies.exception.ts (1)
1-7: Add documentation and consider type safety.
- Add JSDoc documentation to describe the purpose and usage of this exception.
- Consider adding type safety for the dependencies array.
+/** + * Exception thrown when required dependencies are missing. + * This is a non-retriable error that indicates a configuration issue. + */ export class MissingDependenciesException extends NonRetriableError { - constructor(dependencies: string[]) { + constructor(dependencies: readonly string[]) { super(`Missing dependencies: ${dependencies.join(", ")}`); } }packages/metadata/src/providers/dummy.provider.ts (1)
6-9: Enhance JSDoc documentation.The
@inheritdoctag alone is insufficient. Please add a comprehensive JSDoc comment describing the purpose of this dummy implementation and its behavior.- /* @inheritdoc */ + /** + * A dummy implementation that always returns undefined. + * @inheritdoc + * @param _ipfsCid - The IPFS CID to fetch metadata for (unused in this implementation) + * @returns A Promise that always resolves to undefined + */packages/metadata/src/interfaces/index.ts (1)
3-26: LGTM! Consider adding JSDoc comments.The type definitions are well-structured with good use of discriminated unions and conditional types for type safety. Consider adding JSDoc comments to document the purpose and usage of each type.
Add JSDoc comments like this:
+/** + * Supported metadata provider types + */ export type MetadataProvider = "pinata" | "public-gateway" | "dummy"; +/** + * Configuration for dummy metadata provider + */ export type DummyMetadataConfig = { metadataSource: "dummy"; };scripts/bootstrap/src/utils/parsing.ts (1)
9-14: Consider adding more schema validations.The schema could benefit from additional validations for the schema name.
Add these validations:
const zodSchema = z.object({ schema: z .string() + .min(1) + .regex(/^[a-z0-9_]+$/, 'Schema name must contain only lowercase letters, numbers, and underscores') .default(DEFAULT_SCHEMA) .describe("Database schema name where migrations are applied"), });packages/data-flow/src/helpers/index.ts (1)
16-33: Consider refactoring decode attempts for better maintainability.The current implementation has repetitive try-catch blocks. Consider refactoring to reduce duplication and improve maintainability.
for (const event of events) { if ("metadata" in event.params) { ids.add(event.params.metadata[1]); } else if ("data" in event.params) { - try { - const decoded = decodeDGApplicationData(event.params.data); - ids.add(decoded.metadata.pointer); - } catch (error) {} - try { - const decoded = decodeDVMDApplicationData(event.params.data); - ids.add(decoded.metadata.pointer); - } catch (error) {} - try { - const decoded = decodeDVMDExtendedApplicationData(event.params.data); - ids.add(decoded.metadata.pointer); - } catch (error) {} + const decoders = [ + decodeDGApplicationData, + decodeDVMDApplicationData, + decodeDVMDExtendedApplicationData + ]; + + for (const decoder of decoders) { + try { + const decoded = decoder(event.params.data); + ids.add(decoded.metadata.pointer); + break; // Stop if successful + } catch (error) { + console.warn(`Failed to decode with ${decoder.name}: ${error}`); + } + } } }packages/repository/src/repositories/memory/prices.repository.ts (1)
33-45: Add timestamp range validation.The getPricesByTimeRange method should validate that startTimestampMs is less than or equal to endTimestampMs.
async getPricesByTimeRange( tokenCode: TokenCode, startTimestampMs: TimestampMs, endTimestampMs: TimestampMs, ): Promise<TokenPrice[]> { + if (startTimestampMs > endTimestampMs) { + throw new Error('startTimestampMs must be less than or equal to endTimestampMs'); + } const keyString = `${tokenCode}`; const result = Array.from(this.cache.get(keyString)?.values() ?? []); return result.filter( (price) => price.timestampMs >= startTimestampMs && price.timestampMs <= endTimestampMs, ); }scripts/migrations/src/utils/parsing.ts (1)
14-19: Consider extracting folder names to constants.The migration folder names are hardcoded in multiple places. Consider extracting them to named constants for better maintainability.
+const MIGRATION_FOLDERS = { + PROCESSING: 'processing', + EXTERNAL_SERVICES_CACHE: 'external-services-cache' +} as const; + migrationsFolder: z .string() - .refine((value) => ["processing", "external-services-cache"].includes(value), { + .refine((value) => Object.values(MIGRATION_FOLDERS).includes(value), { message: "Invalid migrations folder", }) - .default("processing"), + .default(MIGRATION_FOLDERS.PROCESSING),packages/metadata/src/providers/pinata.provider.ts (3)
16-17: Remove commented out console.log statements.Remove debug console.log statements that were left commented out. These should not be committed to the codebase.
- // console.log("pinataJwt", this.pinataJwt); - // console.log("pinataGateway", this.pinataGateway); - // console.log("ipfsCid", ipfsCid); - // console.log("pinataClient", this.pinataClient.gateways); - // console.log("pinataResponse", pinataResponse);Also applies to: 30-31, 36-36
8-22: Add JSDoc documentation for class and constructor.Add comprehensive JSDoc documentation to improve code maintainability.
+/** + * Provider implementation for fetching metadata from Pinata IPFS gateway. + * @implements {IMetadataProvider} + */ export class PinataProvider implements IMetadataProvider { private readonly pinataClient: PinataSDK; + /** + * Creates a new PinataProvider instance. + * @param pinataJwt - JWT token for Pinata authentication + * @param pinataGateway - Pinata gateway URL + * @param logger - Logger instance for error and debug logging + */ constructor(
42-48: Specify error type in catch block.Add type annotation for the error parameter to ensure proper error handling.
- } catch (error) { + } catch (error: unknown) {packages/metadata/src/factory/index.ts (2)
14-16: Fix incorrect JSDoc comments.The JSDoc comments mention "pricing" instead of "metadata":
/** - * Factory class for creating pricing providers. + * Factory class for creating metadata providers. */ /** - * Creates a pricing provider based on the provided configuration. - * @param options - The pricing configuration. + * Creates a metadata provider based on the provided configuration. + * @param options - The metadata configuration. * @param deps - dependencies to inject into the pricing provider. - * @returns The created pricing provider. + * @returns The created metadata provider.Also applies to: 19-23
17-56: Consider using a namespace or functions instead of a static class.The class contains only static members. Consider using a namespace or simple functions instead.
-export class MetadataProviderFactory { - static create( +export const MetadataProviderFactory = { + create( options: MetadataConfig<MetadataProvider>, deps?: { logger?: ILogger; }, ): IMetadataProvider {🧰 Tools
🪛 Biome (1.9.4)
[error] 17-56: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/metadata/src/providers/publicGateway.provider.ts (1)
20-20: Consider making timeout configurable.The hardcoded timeout of 3000ms might not be suitable for all environments or network conditions. Consider making it configurable through constructor parameters with this as a default.
- this.axiosInstance = axios.create({ timeout: 3000 }); + this.axiosInstance = axios.create({ timeout: options?.timeout ?? 3000 });And update the constructor:
constructor( private readonly gateways: string[], private readonly logger: ILogger, + private readonly options?: { timeout?: number }, ) {packages/repository/src/repositories/kysely/prices.repository.ts (1)
69-99: Consider adding an index for query optimization.The
getPricesByTimeRangeimplementation is correct, but frequent queries might benefit from an index on(tokenCode, timestampMs).Consider adding the following index to improve query performance:
CREATE INDEX idx_price_cache_token_timestamp ON priceCache(tokenCode, timestampMs);packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.ts (1)
68-73: Consider using a constant for timestamp conversion.The multiplication by 1000 to convert seconds to milliseconds would be clearer with a named constant.
+const SECONDS_TO_MS = 1000; + const { amountInUsd } = await getTokenAmountInUsd( this.dependencies.pricingProvider, token, amount, this.event.blockTimestamp, ); // ... -timestamp: new Date(this.event.blockTimestamp * 1000), +timestamp: new Date(this.event.blockTimestamp * SECONDS_TO_MS),Also applies to: 103-103
apps/processing/src/services/sharedDependencies.service.ts (1)
52-58: Consider using an enum for environment types.The environment check could be more maintainable using an enum.
+enum Environment { + Development = 'development', + Staging = 'staging', + Production = 'production' +} + const kyselyDatabase = createKyselyDatabase( { connectionString: env.DATABASE_URL, - isProduction: env.NODE_ENV === "production" || env.NODE_ENV === "staging", + isProduction: [Environment.Production, Environment.Staging].includes(env.NODE_ENV as Environment), }, logger, );packages/pricing/src/providers/coingecko.provider.ts (1)
18-18: Remove commented-out code.These commented-out lines should be removed as they add noise to the codebase.
Also applies to: 40-40, 48-48, 59-59
packages/indexer-client/src/providers/envioIndexerClient.ts (1)
270-299: Add JSDoc comments for the new method.The new method
getBlockRangeTimestampByChainIdshould include JSDoc comments to document its purpose, parameters, and return type.Apply this diff to add JSDoc comments:
+/** + * Get the earliest and latest block timestamps for a specific chain ID. + * @param chainId - The chain ID to query. + * @returns A promise that resolves to an object containing the earliest (from) and latest (to) block timestamps. + * @throws {Error} If no block range is found for the specified chain ID. + */ async getBlockRangeTimestampByChainId(chainId: ChainId): Promise<{ from: number; to: number }> {packages/shared/src/tokens/tokens.ts (1)
26-36: Consider performance optimization for large token maps.The implementation is correct but could be optimized for large token maps by using a more efficient data structure.
Consider this alternative implementation using Object.values() with reduce:
-export const extractAllPriceSourceCodes = (tokensMap: TokensMap): TokenCode[] => { - const priceSourceCodes = new Set<TokenCode>(); - - Object.values(tokensMap).forEach((chainTokens) => { - Object.values(chainTokens).forEach((token) => { - priceSourceCodes.add(token.priceSourceCode); - }); - }); - - return Array.from(priceSourceCodes); -}; +export const extractAllPriceSourceCodes = (tokensMap: TokensMap): TokenCode[] => + Array.from( + new Set( + Object.values(tokensMap).reduce<TokenCode[]>( + (codes, chainTokens) => codes.concat( + Object.values(chainTokens).map(token => token.priceSourceCode) + ), + [] + ) + ) + );packages/indexer-client/test/unit/envioIndexerClient.spec.ts (1)
36-36: Fix typo in comment.The comment contains a typo: "datåa" should be "data".
- // Sample test datåa + // Sample test datapackages/data-flow/test/unit/orchestrator.spec.ts (1)
34-35: Clean up commented code.Large blocks of commented-out test code and imports should be removed if they are no longer needed, or documented if they are temporarily disabled.
If these tests are obsolete, please remove them. If they are temporarily disabled, add a TODO comment explaining why and when they will be re-enabled.
Also applies to: 775-869
scripts/migrations/package.json (1)
19-20: Added Cache Migration and Reset Scripts
The new scripts:
"db:cache:migrate": "tsx src/migrateDb.script.ts --migrationsFolder external-services-cache""db:cache:reset": "tsx src/resetDb.script.ts --migrationsFolder external-services-cache"
are introduced to handle cache-specific migrations. Please ensure these scripts are well documented and that environment variables (if any) are adequately configured for different environments.package.json (1)
11-12: Bootstrap Scripts Added for Metadata and Pricing
The addition of"bootstrap:metadata"and"bootstrap:pricing"scripts using the run filter is well-integrated. Please confirm that corresponding documentation (and any related tests) reflect these commands to avoid confusion during handoffs.scripts/bootstrap/README.md (2)
73-73: Typographical Issue in Documentation Link
The migration file path in the instructions ([packages/repository/src/migrations](../../packages//repository/migrations)) contains a double slash (//). Please correct it to a single slash (../../packages/repository/migrations) for clarity and correctness.
112-112: End-to-End Tests Reminder
The README includes a TODO to add E2E tests for the scripts. This is critical for ensuring reliability, so please prioritize adding these tests before merging.scripts/migrations/README.md (3)
57-64: Fenced Code Block Lacks Language Specification.
In the "Migrating Cache" section (lines 61–63), the fenced code block does not specify a language. This may trigger markdown lint warnings (see MD040). Consider adding the appropriate language (e.g.,bash) to improve readability and tooling support.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
61-61: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
86-92: Missing Language Specification in Reset Cache Code Block.
Similar to the "Migrating Cache" section, the "Resetting Cache" code block (lines 91–92) lacks a language designation. Addingbash(or the relevant shell) will help maintain consistency and clarity in documentation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
90-90: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
143-143: Reminder to Add E2E Tests.
The TODO note at line 143 reminds contributors to add end-to-end tests for these scripts. Please ensure E2E tests are implemented and validated prior to merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (70)
.env.example(0 hunks)Dockerfile(1 hunks)apps/processing/src/config/env.ts(4 hunks)apps/processing/src/services/sharedDependencies.service.ts(3 hunks)apps/processing/test/unit/sharedDependencies.service.spec.ts(3 hunks)docker-compose.yaml(3 hunks)package.json(2 hunks)packages/data-flow/package.json(1 hunks)packages/data-flow/src/external.ts(1 hunks)packages/data-flow/src/helpers/index.ts(1 hunks)packages/data-flow/src/internal.ts(1 hunks)packages/data-flow/src/orchestrator.ts(9 hunks)packages/data-flow/test/unit/eventsFetcher.spec.ts(1 hunks)packages/data-flow/test/unit/orchestrator.spec.ts(2 hunks)packages/indexer-client/src/interfaces/indexerClient.ts(1 hunks)packages/indexer-client/src/providers/envioIndexerClient.ts(2 hunks)packages/indexer-client/test/unit/envioIndexerClient.spec.ts(3 hunks)packages/metadata/package.json(1 hunks)packages/metadata/src/exceptions/invalidMetadataSource.exception.ts(1 hunks)packages/metadata/src/exceptions/missingDependencies.exception.ts(1 hunks)packages/metadata/src/external.ts(1 hunks)packages/metadata/src/factory/index.ts(1 hunks)packages/metadata/src/interfaces/index.ts(1 hunks)packages/metadata/src/interfaces/metadata.interface.ts(1 hunks)packages/metadata/src/internal.ts(1 hunks)packages/metadata/src/providers/cachingProxy.provider.ts(1 hunks)packages/metadata/src/providers/dummy.provider.ts(1 hunks)packages/metadata/src/providers/index.ts(1 hunks)packages/metadata/src/providers/pinata.provider.ts(1 hunks)packages/metadata/src/providers/publicGateway.provider.ts(4 hunks)packages/pricing/src/constants/index.ts(1 hunks)packages/pricing/src/exceptions/noClosePriceFound.exception.ts(1 hunks)packages/pricing/src/providers/cachingProxy.provider.ts(8 hunks)packages/pricing/src/providers/coingecko.provider.ts(5 hunks)packages/pricing/test/providers/cachingProxy.provider.spec.ts(2 hunks)packages/pricing/test/providers/coingecko.provider.spec.ts(2 hunks)packages/processors/src/external.ts(1 hunks)packages/processors/src/internal.ts(1 hunks)packages/processors/src/processors/allo/handlers/poolCreated.handler.ts(0 hunks)packages/processors/src/processors/strategy/directAllocation/handlers/directAllocated.handler.ts(2 hunks)packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.ts(2 hunks)packages/repository/src/db/connection.ts(2 hunks)packages/repository/src/external.ts(1 hunks)packages/repository/src/repositories/kysely/prices.repository.ts(2 hunks)packages/repository/src/repositories/memory/prices.repository.ts(2 hunks)packages/shared/src/external.ts(1 hunks)packages/shared/src/retry/exponentialBackoff.strategy.ts(1 hunks)packages/shared/src/tokens/tokens.ts(4 hunks)packages/shared/src/utils/testing.ts(1 hunks)scripts/bootstrap/.env.example(1 hunks)scripts/bootstrap/README.md(1 hunks)scripts/bootstrap/package.json(1 hunks)scripts/bootstrap/src/metadata.script.ts(1 hunks)scripts/bootstrap/src/pricing.script.ts(1 hunks)scripts/bootstrap/src/schemas/index.ts(1 hunks)scripts/bootstrap/src/utils/index.ts(1 hunks)scripts/bootstrap/src/utils/kysely.ts(1 hunks)scripts/bootstrap/src/utils/parsing.ts(1 hunks)scripts/bootstrap/tsconfig.build.json(1 hunks)scripts/bootstrap/tsconfig.json(1 hunks)scripts/bootstrap/vitest.config.ts(1 hunks)scripts/migrations/README.md(3 hunks)scripts/migrations/package.json(1 hunks)scripts/migrations/src/migrateDb.script.ts(2 hunks)scripts/migrations/src/migrations/external-services-cache/20250127T000000_add_cache_tables.ts(1 hunks)scripts/migrations/src/migrations/processing/20241029T120000_initial.ts(2 hunks)scripts/migrations/src/resetDb.script.ts(2 hunks)scripts/migrations/src/schemas/index.ts(1 hunks)scripts/migrations/src/utils/kysely.ts(3 hunks)scripts/migrations/src/utils/parsing.ts(2 hunks)
💤 Files with no reviewable changes (2)
- .env.example
- packages/processors/src/processors/allo/handlers/poolCreated.handler.ts
✅ Files skipped from review due to trivial changes (8)
- scripts/bootstrap/tsconfig.json
- packages/data-flow/src/internal.ts
- scripts/migrations/src/migrations/processing/20241029T120000_initial.ts
- scripts/bootstrap/tsconfig.build.json
- scripts/bootstrap/src/utils/index.ts
- scripts/bootstrap/.env.example
- packages/metadata/src/providers/index.ts
- scripts/bootstrap/package.json
🧰 Additional context used
📓 Path-based instructions (5)
`**/*.ts`: Review TypeScript files for adherence to the fo...
**/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoidany; useunknownwhen necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have anexternal.tsfile explicitly listing public exports.
- Usebigintas-is; cast toNumberonly when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/pricing/src/exceptions/noClosePriceFound.exception.tspackages/metadata/src/exceptions/missingDependencies.exception.tspackages/metadata/src/internal.tsscripts/migrations/src/schemas/index.tsscripts/migrations/src/migrations/external-services-cache/20250127T000000_add_cache_tables.tspackages/data-flow/test/unit/eventsFetcher.spec.tspackages/metadata/src/exceptions/invalidMetadataSource.exception.tspackages/pricing/src/constants/index.tspackages/metadata/src/providers/dummy.provider.tspackages/data-flow/src/external.tspackages/data-flow/test/unit/orchestrator.spec.tspackages/shared/src/utils/testing.tspackages/processors/src/external.tspackages/shared/src/retry/exponentialBackoff.strategy.tspackages/data-flow/src/helpers/index.tsapps/processing/test/unit/sharedDependencies.service.spec.tsscripts/migrations/src/utils/kysely.tspackages/repository/src/db/connection.tspackages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.tspackages/metadata/src/external.tspackages/pricing/test/providers/coingecko.provider.spec.tsscripts/migrations/src/migrateDb.script.tspackages/repository/src/external.tspackages/pricing/src/providers/coingecko.provider.tsscripts/migrations/src/utils/parsing.tspackages/metadata/src/interfaces/metadata.interface.tsscripts/bootstrap/src/pricing.script.tsscripts/migrations/src/resetDb.script.tspackages/metadata/src/factory/index.tspackages/shared/src/external.tspackages/processors/src/processors/strategy/directAllocation/handlers/directAllocated.handler.tsscripts/bootstrap/src/metadata.script.tspackages/pricing/test/providers/cachingProxy.provider.spec.tspackages/metadata/src/providers/pinata.provider.tsscripts/bootstrap/vitest.config.tspackages/metadata/src/interfaces/index.tspackages/metadata/src/providers/cachingProxy.provider.tsscripts/bootstrap/src/utils/parsing.tspackages/metadata/src/providers/publicGateway.provider.tspackages/indexer-client/test/unit/envioIndexerClient.spec.tsscripts/bootstrap/src/schemas/index.tspackages/repository/src/repositories/memory/prices.repository.tspackages/indexer-client/src/providers/envioIndexerClient.tspackages/repository/src/repositories/kysely/prices.repository.tspackages/data-flow/src/orchestrator.tsapps/processing/src/services/sharedDependencies.service.tspackages/processors/src/internal.tsscripts/bootstrap/src/utils/kysely.tsapps/processing/src/config/env.tspackages/pricing/src/providers/cachingProxy.provider.tspackages/indexer-client/src/interfaces/indexerClient.tspackages/shared/src/tokens/tokens.ts
`scripts/**/*.ts`: Ensure scripts: - Use `process.cw...
scripts/**/*.ts: Ensure scripts:
- Useprocess.cwd()for root references.
- Follow folder conventions (infra/for infra scripts,utilities/for utilities).
- Are organized inpackage.jsonwithscript:infra:{name}orscript:util:{name}.
- Be concise and avoid overly nitpicky feedback outside of these best practices.
scripts/migrations/src/schemas/index.tsscripts/migrations/src/migrations/external-services-cache/20250127T000000_add_cache_tables.tsscripts/migrations/src/utils/kysely.tsscripts/migrations/src/migrateDb.script.tsscripts/migrations/src/utils/parsing.tsscripts/bootstrap/src/pricing.script.tsscripts/migrations/src/resetDb.script.tsscripts/bootstrap/src/metadata.script.tsscripts/bootstrap/vitest.config.tsscripts/bootstrap/src/utils/parsing.tsscripts/bootstrap/src/schemas/index.tsscripts/bootstrap/src/utils/kysely.ts
`**/*.spec.ts`: Review the unit test files with the followin...
**/*.spec.ts: Review the unit test files with the following guidelines:
- Avoid using the word "should" in test descriptions.
- Ensure descriptive test names convey the intent of each test.
- Validate adherence to the Mocha/Chai/Jest test library best practices.
- Be concise and avoid overly nitpicky feedback outside of these best practices.
packages/data-flow/test/unit/eventsFetcher.spec.tspackages/data-flow/test/unit/orchestrator.spec.tsapps/processing/test/unit/sharedDependencies.service.spec.tspackages/pricing/test/providers/coingecko.provider.spec.tspackages/pricing/test/providers/cachingProxy.provider.spec.tspackages/indexer-client/test/unit/envioIndexerClient.spec.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 the
IMetadataProviderinterface and follow naming conventions (e.g.,GithubProvider,JsonFileProvider).- Be concise and avoid overly nitpicky feedback outside of these best practices.
packages/metadata/src/providers/dummy.provider.tspackages/pricing/test/providers/coingecko.provider.spec.tspackages/pricing/src/providers/coingecko.provider.tspackages/pricing/test/providers/cachingProxy.provider.spec.tspackages/metadata/src/providers/pinata.provider.tspackages/metadata/src/providers/cachingProxy.provider.tspackages/metadata/src/providers/publicGateway.provider.tspackages/indexer-client/src/providers/envioIndexerClient.tspackages/pricing/src/providers/cachingProxy.provider.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
🪛 Biome (1.9.4)
packages/metadata/src/providers/dummy.provider.ts
[error] 4-4: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/metadata/src/factory/index.ts
[error] 17-56: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🪛 markdownlint-cli2 (0.17.2)
scripts/migrations/README.md
61-61: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
90-90: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 LanguageTool
scripts/bootstrap/README.md
[grammar] ~74-~74: An auxiliary verb seems to be missing from this progressive structure. Did you mean “it's using”, “it is using”, or “it was using”?
Context: ...ackages//repository/migrations) 2. Name it using the format: `YYYYMMDDTHHmmss_descriptio...
(PRP_VBG)
🪛 GitHub Check: lint / Run Linters
packages/pricing/src/providers/cachingProxy.provider.ts
[failure] 7-7:
'CacheResult' is defined but never used
[failure] 99-99:
'fromTimestampMs' is never reassigned. Use 'const' instead
[failure] 100-100:
'toTimestampMs' is never reassigned. Use 'const' instead
🪛 ESLint
packages/pricing/src/providers/cachingProxy.provider.ts
[error] 7-7: 'CacheResult' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 99-99: 'fromTimestampMs' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 100-100: 'toTimestampMs' is never reassigned. Use 'const' instead.
(prefer-const)
🪛 GitHub Actions: Main Workflow
packages/pricing/src/providers/cachingProxy.provider.ts
[error] 7-7: 'CacheResult' is defined but never used
🔇 Additional comments (54)
packages/shared/src/external.ts (1)
12-12: LGTM! Well-organized exports.The changes maintain a clean public API surface by explicitly listing exports and grouping them logically.
Also applies to: 20-26
scripts/bootstrap/src/schemas/index.ts (1)
20-24: Verify required environment variables for production.You have optional and defaulted fields for development, but ensure that
PUBLIC_GATEWAY_URLS,CHAIN_IDS, etc., are properly tested or validated for production scenarios.Would you like to add stricter runtime checks for production mode to ensure no critical fields are left at defaults?
apps/processing/src/config/env.ts (1)
26-26: Ensure consistent usage of optional secrets and metadata source defaults.By making
INDEXER_ADMIN_SECREToptional and defaultingMETADATA_SOURCEto "dummy," you risk runtime misconfigurations if these values are unintentionally omitted in production. Review and confirm these defaults align with the intended deployment scenario.Also applies to: 35-37
scripts/bootstrap/src/metadata.script.ts (1)
67-71: Consider verifying table existence before inserting dummy rows.
This block attempts an insert into themetadataCachetable without validating that the table was created or whether migrations were applied. While this may work in dev environments, it could cause unexpected errors in production or CI pipelines if the table doesn’t exist yet.Would you like me to generate a shell script to confirm the table existence prior to these test inserts?
packages/data-flow/src/orchestrator.ts (1)
137-143: Verify the 30-minute threshold assumption carefully.
Here you only bulk-fetch metadata and prices if the first event in the batch is older than 30 minutes. Consider whether events near the 30-minute boundary might be missed. Confirm that this logic aligns with your indexing and data completeness goals.packages/metadata/src/internal.ts (1)
5-5: LGTM!The new export follows the established pattern and maintains consistency with the internal module pattern.
packages/pricing/src/constants/index.ts (1)
1-3: LGTM! Well-documented constant update.The change to
MIN_GRANULARITY_MSis properly documented with a reference to Coingecko's Enterprise plans, and the value is correctly formatted for readability.packages/metadata/src/external.ts (1)
1-6: LGTM! Well-organized exports.The exports are properly organized and follow the guideline of having an
external.tsfile for explicit public exports.packages/processors/src/internal.ts (1)
16-17: LGTM! Well-organized export addition.The new decoder export follows the established pattern and is properly categorized with a clear comment.
packages/data-flow/src/external.ts (1)
8-8: LGTM!The new export is properly added to the existing export block, following the TypeScript guideline of having an
external.tsfile explicitly listing public exports.packages/processors/src/external.ts (1)
10-14: LGTM!The new exports are properly added in a separate export block, following the TypeScript guideline of having an
external.tsfile explicitly listing public exports.packages/metadata/src/interfaces/metadata.interface.ts (1)
10-17: LGTM!The interface follows TypeScript best practices:
- Uses generics for type safety
- Uses Zod for runtime type-checking
- Has clear documentation explaining the return type changes
scripts/migrations/src/schemas/index.ts (1)
5-5: LGTM!The schema follows TypeScript best practices:
- Uses Zod for runtime type-checking of environment variables
- Has a default value for development
- Uses standard environment names
scripts/bootstrap/vitest.config.ts (1)
1-22: LGTM! Well-structured Vitest configuration.The configuration is comprehensive with:
- Proper test file patterns
- Multiple coverage reporters
- Path alias setup
- Clear exclusions for node_modules and dist
scripts/migrations/src/migrations/external-services-cache/20250127T000000_add_cache_tables.ts (1)
17-17: Verify the nullable metadata design.The
metadatacolumn is defined as nullable. This could lead to cache entries with no metadata, which might cause issues in the application logic. Consider:
- Whether this is intentional
- How the application handles null metadata
- If validation should be added at the application level
Let's check how the metadata column is used:
packages/indexer-client/src/interfaces/indexerClient.ts (1)
36-41: LGTM! Well-documented interface addition.The new method is properly documented and follows TypeScript best practices with clear type definitions.
packages/metadata/src/providers/pinata.provider.ts (1)
41-41: Add runtime type validation for metadata.The type casting
as Tis unsafe. Consider adding runtime type validation using Zod schema.- return pinataResponse.data as T; + if (validateContent) { + return validateContent.parse(pinataResponse.data); + } + return pinataResponse.data as T;✅ Verification successful
Error: No shell scripts or web queries found
Runtime Type Validation Update Needed
The original code performs an unsafe type assertion by casting the response data directly. The proposed diff correctly adds a runtime validation check using a schema validation function (e.g., one from Zod) when available:
- At
packages/metadata/src/providers/pinata.provider.ts(line 41), the suggestion is to:
- Check if a
validateContentfunction is provided.- If so, run
validateContent.parse(pinataResponse.data)to validate the incoming data.- Otherwise, fall back to returning the data with an unsafe cast.
This change improves the type safety of the metadata by ensuring that the runtime data conforms to expectations before being used.
packages/shared/src/retry/exponentialBackoff.strategy.ts (1)
24-26: Verify the impact of more aggressive retry settings.The retry strategy has been made more aggressive:
- baseDelay reduced from 5000ms to 200ms
- factor reduced from 2 to 1.5
- maxAttempts increased from 3 to 10
This could potentially lead to increased load on external services. Please ensure these values are appropriate for your use case.
packages/metadata/src/providers/cachingProxy.provider.ts (1)
25-52: LGTM! Robust null handling implementation.The changes properly handle null values in the cache, with appropriate type updates and conditional checks. The error handling and logging are well implemented.
packages/repository/src/external.ts (1)
79-79: LGTM!The addition of
IPricingCacheto the exports is well-organized, grouped with related pricing types.scripts/migrations/src/migrateDb.script.ts (2)
45-45: LGTM! Production safeguards added.Good addition of the
isProductionflag based on environment, which helps configure database connections appropriately for production/staging environments.
36-36: Verify NODE_ENV validation.Consider validating the NODE_ENV value to ensure it only accepts valid environments.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if NODE_ENV validation exists in the schema rg -A 5 'NODE_ENV.*z\.' ./scripts/migrations/src/schemas/Length of output: 517
NODE_ENV is properly validated in the schema.
- The
NODE_ENVfield is validated with a Zod enumeration in./scripts/migrations/src/schemas/index.tsusingz.enum(["development", "staging", "production"]).default("development").packages/metadata/src/providers/publicGateway.provider.ts (1)
41-44: LGTM! Improved error handling and validation.Good improvements:
- Added JSON validation before processing
- Clear error messages for invalid content
- Throwing error after exhausting all gateways instead of silent failure
Also applies to: 57-57
packages/data-flow/test/unit/eventsFetcher.spec.ts (1)
21-21: LGTM!The mock function addition is consistent with the interface changes and follows testing best practices.
packages/processors/src/processors/strategy/directAllocation/handlers/directAllocated.handler.ts (1)
60-65: LGTM! Improved timestamp handling.The changes correctly use the block timestamp for donation records instead of the price timestamp, which is more accurate for recording when the donation occurred. The conversion from seconds to milliseconds is properly handled.
Also applies to: 81-81
scripts/migrations/src/utils/kysely.ts (1)
19-19: LGTM! Enhanced database organization with domain-specific migration tables.The addition of the
domainproperty and its use in migration table names improves database organization by preventing conflicts when multiple domains share the same database.Also applies to: 56-57, 98-99
packages/repository/src/db/connection.ts (1)
111-115:⚠️ Potential issueReview SSL configuration security.
Setting
rejectUnauthorized: falsein production disables SSL certificate validation, which could expose the application to man-in-the-middle attacks. Consider:
- Using valid SSL certificates in production
- Setting
rejectUnauthorized: truefor proper certificate validation- Using environment variables to configure SSL options for different environments
packages/pricing/test/providers/cachingProxy.provider.spec.ts (1)
3-3: LGTM! Test updates align with interface changes.The changes correctly reflect the new
IPricingCacheinterface and include the newgetPricesByTimeRangemethod in the mock implementation.Also applies to: 19-19, 21-21
packages/repository/src/repositories/kysely/prices.repository.ts (1)
8-21: LGTM! Well-documented interface extension.The
IPricingCacheinterface is well-defined with clear JSDoc documentation.apps/processing/src/services/sharedDependencies.service.ts (1)
94-96: LGTM! Factory pattern improves provider initialization.The use of
MetadataProviderFactoryenhances maintainability and configuration flexibility.apps/processing/test/unit/sharedDependencies.service.spec.ts (3)
4-4: LGTM!The import statement correctly reflects the switch from
IpfsProvidertoPublicGatewayProvider.
109-110: LGTM!The
mockEnvtype and object correctly include the newMETADATA_SOURCEconfiguration.Also applies to: 116-116
134-134: LGTM!The test expectation correctly verifies the initialization of
PublicGatewayProviderwith the updated environment configuration.packages/pricing/test/providers/coingecko.provider.spec.ts (1)
224-224: LGTM!The test cases correctly assert timestamp parameters as
TimestampMs[]type, improving type safety.Also applies to: 229-231, 242-244
packages/pricing/src/providers/coingecko.provider.ts (2)
148-148: LGTM!The method signature correctly uses
TimestampMs[]type for better type safety.
157-202: LGTM!The implementation correctly handles:
- Empty timestamps array
- Minimum granularity for timestamps
- Segmentation of requests spanning more than 90 days
- Parallel processing of segments
packages/indexer-client/src/providers/envioIndexerClient.ts (1)
23-28: LGTM!The constructor correctly handles the optional
secretparameter.packages/shared/src/tokens/tokens.ts (2)
20-24: LGTM! Well-structured type definition.The
TokensMaptype provides a clear and type-safe way to represent the token mapping structure.
96-96: Verify the impact of changing the price source code.The price source code for token at address
0x93A5347036f69BC6f37Ed2b59CBcDDa927719217is changed from "DAI" to "GIST".Run this script to check for any dependencies on the old price source code:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any hardcoded references to the DAI price source code for this token rg -l "0x93A5347036f69BC6f37Ed2b59CBcDDa927719217.*DAI"Length of output: 57
I'll run additional searches to gather more context around this token in the codebase. One moment while I execute a shell script to check for any remaining references to the hardcoded "DAI" price source for the token.
Please run this script so we can verify the surrounding context and ensure that there are no remaining dependencies on the old "DAI" string for the token.
🏁 Script executed:
#!/bin/bash # Checking for any references to the token's address and its price source code context in tokens.ts echo "Checking token address context in packages/shared/src/tokens/tokens.ts:" rg -n "0x93A5347036f69BC6f37Ed2b59CBcDDa927719217" packages/shared/src/tokens/tokens.ts -C 3 echo "Searching for any assignment of the price source code 'DAI' in tokens.ts:" rg -n 'priceSourceCode:\s*"DAI"' packages/shared/src/tokens/tokens.ts -C 3Length of output: 3492
Impact Verification: Price source update localized and confirmed
The investigation shows that the token data for address
0x93A5347036f69BC6f37Ed2b59CBcDDa927719217uses"GIST"for bothcodeandpriceSourceCode, and there are no detected dependencies or hardcoded references combining this token’s address with"DAI". The change appears to be isolated to this token entry.packages/indexer-client/test/unit/envioIndexerClient.spec.ts (1)
112-114: Document the reason for removing the header test.The test for x-hasura-admin-secret header is commented out without explanation.
Please add a comment explaining why this test was removed or if it should be deleted entirely.
Dockerfile (2)
5-5: LGTM! Explicit version pinning improves reproducibility.Pinning pnpm to version 9.12.0 ensures consistent builds across environments.
12-12: Verify the impact of removing --prod flag.The --prod flag has been removed from the deploy command which might affect the production dependencies.
Please confirm if this change is intentional and that all required dependencies will be included in the production build.
packages/metadata/package.json (1)
31-36: New Dependency Added: "pinata"
The new"pinata": "1.10.1"dependency is added correctly under the dependencies section. Ensure that the corresponding provider (e.g.,PinataProvider) properly integrates this package for IPFS-related functionality.packages/data-flow/package.json (1)
37-39: New Dependency "p-map" Introduced
The addition of"p-map": "7.0.3"enhances the package’s ability to manage concurrent asynchronous operations. Verify that its usage in orchestration logic (e.g., inbulkFetchMetadataAndPricesForBatch) aligns with expected concurrency patterns.package.json (2)
16-17: Cache Migration Scripts Reflected at Repository Root
The inclusion of"db:cache:migrate"and"db:cache:reset"at the root level mirrors the updates in the migrations package. Ensure consistency of behavior between these wrappers and the underlying migration scripts.
56-56: Package Manager Version Update
The update to"packageManager": "[email protected]"helps standardize the development environment. Verify that all CI/CD pipelines and local development setups are compatible with this version upgrade.scripts/bootstrap/README.md (1)
7-11: Well-Structured Script Overview
The inclusion of a table that outlines available scripts (db:migrateanddb:reset) and their descriptions provides clear guidance for users.scripts/migrations/README.md (3)
7-12: Updated Scripts Table is clear and informative.
The table now cleanly displays the available migration and cache management scripts, ensuring users quickly understand the available commands.
65-71: Migrating Cache Instructions are Well-Structured.
The steps outlining the cache migration process (loading environment variables, connecting to the database, running migrations, and logging results) are clear.
93-99: Reset Cache Section Instructions Are Clear.
The instructions for resetting the cache schema are well outlined and mirror the structure of the migrating cache section.docker-compose.yaml (4)
41-47: Commented Console Assets Setting – Verify Intent.
The commented-out line forHASURA_GRAPHQL_CONSOLE_ASSETS_DIR(line 46) is retained with an inline note. Please confirm that leaving it commented is intentional for the offline console assets usage scenario, or consider removing it if no longer needed.
58-67: New Processing Service Configuration: Consider Networking Needs.
The newly addedprocessingservice (lines 58–67) is configured with build context, restart policy, dependency, and environment file. However, it does not specify any networks. If this service is expected to communicate with other services (e.g., the database or other internal services), it may be advisable to explicitly join one or more networks (such asdatalayeror a dedicated network).
121-122: Indexer-GraphQL-API Service Network Update.
The addition of thedatalayernetwork to theindexer-graphql-apiservice (lines 121–122) appears intended for enhanced inter-service communication. Please verify that this change meets your networking and security requirements without inadvertently exposing the service to unwanted internal traffic.
123-128: Indexer Service Network Adjustment is Consistent.
The removal of thedatalayernetwork from theindexerservice is consistent with the intended network segmentation. Just ensure that this change does not impact any inter-service communication patterns that might require theindexerservice to interact with components on thedatalayernetwork.
| export const getSchemaName = (schema: SchemaModule): string => { | ||
| let name = "public"; | ||
| schema.createTable("test").$call((b) => { | ||
| name = b.toOperationNode().table.table.schema?.name ?? "public"; | ||
| }); | ||
| return name; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider cleanup and error handling for schema name extraction.
While the workaround is clever, creating a temporary table without cleanup could have side effects. Consider:
- Using a more unique temporary table name to avoid conflicts
- Adding cleanup logic to drop the temporary table
- Adding error handling for table creation failures
export const getSchemaName = (schema: SchemaModule): string => {
let name = "public";
- schema.createTable("test").$call((b) => {
+ const tempTableName = `_temp_schema_check_${Date.now()}`;
+ try {
+ schema.createTable(tempTableName).$call((b) => {
name = b.toOperationNode().table.table.schema?.name ?? "public";
- });
+ });
+ } finally {
+ // Clean up the temporary table
+ schema.dropTable(tempTableName).$call(() => {});
+ }
return name;
};📝 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.
| export const getSchemaName = (schema: SchemaModule): string => { | |
| let name = "public"; | |
| schema.createTable("test").$call((b) => { | |
| name = b.toOperationNode().table.table.schema?.name ?? "public"; | |
| }); | |
| return name; | |
| }; | |
| export const getSchemaName = (schema: SchemaModule): string => { | |
| let name = "public"; | |
| const tempTableName = `_temp_schema_check_${Date.now()}`; | |
| try { | |
| schema.createTable(tempTableName).$call((b) => { | |
| name = b.toOperationNode().table.table.schema?.name ?? "public"; | |
| }); | |
| } finally { | |
| // Clean up the temporary table | |
| schema.dropTable(tempTableName).$call(() => {}); | |
| } | |
| return name; | |
| }; |
There was a problem hiding this comment.
the table is not actually created since .execute is not called, so the current approach is fine
There was a problem hiding this comment.
@0xnigir1 Ah, you're absolutely right! Thank you for the clarification.
✏️ Learnings added
Learnt from: 0xnigir1
PR: defi-wonderland/grants-stack-indexer-v2#63
File: scripts/bootstrap/src/utils/kysely.ts:13-19
Timestamp: 2025-02-05T17:41:48.691Z
Learning: In Kysely, using `$call()` on a query builder only constructs the operation node for inspection without executing the actual SQL query. The query is only executed when `.execute()` is called.
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!
packages/shared/src/utils/testing.ts
Outdated
| export function isJSON(item: unknown): item is ObjectType { | ||
| try { | ||
| JSON.parse(item as string); | ||
| return true; | ||
| } catch (e) { | ||
| console.log(e); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix type guard implementation and add documentation.
The current implementation has several issues:
- The type guard is incorrect as it returns
truefor any valid JSON, not just objects. - Console logging should be removed from utility functions.
- Missing JSDoc documentation.
Apply this diff to fix the issues:
+/**
+ * Type guard to check if a value is a valid JSON object string
+ * @param item The value to check
+ * @returns True if the value is a valid JSON object string, false otherwise
+ */
-export function isJSON(item: unknown): item is ObjectType {
+export function isJSON(item: unknown): item is string {
try {
- JSON.parse(item as string);
- return true;
+ const parsed = JSON.parse(item as string);
+ return isObject(parsed);
} catch (e) {
- console.log(e);
return false;
}
}Committable suggestion skipped: line range outside the PR's diff.
apps/processing/src/config/env.ts
Outdated
| }) | ||
| .and( | ||
| z | ||
| .discriminatedUnion("METADATA_SOURCE", [ | ||
| dummyMetadataSchema, | ||
| pinataMetadataSchema, | ||
| publicGatewayMetadataSchema, | ||
| ]) | ||
| .transform((val) => { | ||
| if (val.METADATA_SOURCE === "dummy") { | ||
| return { metadataSource: val.METADATA_SOURCE, ...val }; | ||
| } | ||
| if (val.METADATA_SOURCE === "pinata") { | ||
| return { | ||
| metadataSource: val.METADATA_SOURCE, | ||
| jwt: val.PINATA_JWT, | ||
| gateway: val.PINATA_GATEWAY_URL, | ||
| ...val, | ||
| }; | ||
| } | ||
| if (val.METADATA_SOURCE === "public-gateway") { | ||
| return { | ||
| metadataSource: val.METADATA_SOURCE, | ||
| gateways: val.PUBLIC_GATEWAY_URLS, | ||
| ...val, | ||
| }; | ||
| } | ||
| throw new Error("Invalid metadata source"); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify chained discriminated unions for maintainability.
The combination of two .discriminatedUnion(...) calls using .and(...) can be tricky to maintain. If you add or remove a source, you must carefully update each chain of transforms. Consider extracting smaller schema parts or adopting a single union structure with refined branching.
There was a problem hiding this comment.
would be interesting to see what solution or apporach you suggest on this?
There was a problem hiding this comment.
@0xnigir1 Here's a suggested approach to simplify the schema using a single discriminated union:
// Define all possible combinations of sources
const sourceSchema = z.object({
sourceType: z.enum([
'dummy-all',
'dummy-pricing-coingecko-metadata',
'coingecko-pricing-dummy-metadata',
'coingecko-pricing-pinata-metadata',
'coingecko-pricing-public-gateway-metadata',
]),
});
// Define source-specific configurations
const sourcesConfig = {
'dummy-all': baseSchema.extend({
sourceType: z.literal('dummy-all'),
DUMMY_PRICE: z.coerce.number().optional().default(1),
}),
'dummy-pricing-coingecko-metadata': baseSchema.extend({
sourceType: z.literal('dummy-pricing-coingecko-metadata'),
DUMMY_PRICE: z.coerce.number().optional().default(1),
COINGECKO_API_KEY: z.string().min(1),
COINGECKO_API_TYPE: z.enum(['demo', 'pro']).default('pro'),
}),
// ... other combinations
};
const validationSchema = z
.discriminatedUnion('sourceType', Object.values(sourcesConfig))
.transform((val) => {
const config: Record<string, unknown> = {
pricingSource: val.sourceType.includes('dummy-pricing') ? 'dummy' : 'coingecko',
metadataSource: val.sourceType.includes('dummy-metadata') ? 'dummy' :
val.sourceType.includes('pinata') ? 'pinata' :
val.sourceType.includes('public-gateway') ? 'public-gateway' : 'dummy',
};
// Add source-specific configurations
if (config.pricingSource === 'dummy') {
config.dummyPrice = val.DUMMY_PRICE;
}
if (config.pricingSource === 'coingecko') {
config.apiKey = val.COINGECKO_API_KEY;
config.apiType = val.COINGECKO_API_TYPE;
}
// ... handle metadata sources similarly
return { ...val, ...config };
});This approach:
- Uses a single
sourceTypediscriminator that combines both pricing and metadata sources - Defines all possible valid combinations upfront
- Simplifies the transform logic by deriving configurations from the source type
- Makes it easier to add new source combinations by just extending the
sourcesConfig
Let me know if you'd like me to explain any part in more detail!
| let fromTimestampMs = timestamps[0] as TimestampMs; | ||
| let toTimestampMs = timestamps[timestamps.length - 1] as TimestampMs; |
There was a problem hiding this comment.
Use constant declarations for non-reassigned variables.
These variables are never reassigned and can cause lint failures.
Apply this change to fix the lint warnings:
- let fromTimestampMs = timestamps[0] as TimestampMs;
- let toTimestampMs = timestamps[timestamps.length - 1] as TimestampMs;
+ const fromTimestampMs = timestamps[0] as TimestampMs;
+ const toTimestampMs = timestamps[timestamps.length - 1] as TimestampMs;📝 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.
| let fromTimestampMs = timestamps[0] as TimestampMs; | |
| let toTimestampMs = timestamps[timestamps.length - 1] as TimestampMs; | |
| const fromTimestampMs = timestamps[0] as TimestampMs; | |
| const toTimestampMs = timestamps[timestamps.length - 1] as TimestampMs; |
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[failure] 99-99:
'fromTimestampMs' is never reassigned. Use 'const' instead
[failure] 100-100:
'toTimestampMs' is never reassigned. Use 'const' instead
🪛 ESLint
[error] 99-99: 'fromTimestampMs' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 100-100: 'toTimestampMs' is never reassigned. Use 'const' instead.
(prefer-const)
| type CacheResult = { | ||
| timestampMs: TimestampMs; | ||
| price: TokenPrice | undefined; | ||
| }; |
There was a problem hiding this comment.
Remove unused type to fix pipeline failure.
The type CacheResult is declared but never used, triggering lint errors.
Apply this diff to remove the unused type and resolve the lint issue:
-type CacheResult = {
- timestampMs: TimestampMs;
- price: TokenPrice | undefined;
-};📝 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.
| type CacheResult = { | |
| timestampMs: TimestampMs; | |
| price: TokenPrice | undefined; | |
| }; | |
| // (No code remains from this snippet as the unused type has been removed) |
🧰 Tools
🪛 GitHub Check: lint / Run Linters
[failure] 7-7:
'CacheResult' is defined but never used
🪛 ESLint
[error] 7-7: 'CacheResult' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Actions: Main Workflow
[error] 7-7: 'CacheResult' is defined but never used
| try { | ||
| const decoded = decodeDGApplicationData(event.params.data); | ||
| ids.add(decoded.metadata.pointer); | ||
| } catch (error) {} |
There was a problem hiding this comment.
Add error logging in catch blocks.
Empty catch blocks silently swallow errors, making it difficult to debug issues with metadata decoding. Consider logging these errors or implementing proper error handling.
try {
const decoded = decodeDGApplicationData(event.params.data);
ids.add(decoded.metadata.pointer);
-} catch (error) {}
+} catch (error) {
+ console.warn(`Failed to decode DG application data: ${error}`);
+}Also applies to: 24-27, 28-31
| async set(key: PriceCacheKey, value: TokenPrice): Promise<void> { | ||
| const { tokenCode, timestampMs } = key; | ||
|
|
||
| const keyString = `${tokenCode}-${timestampMs}`; | ||
| const keyString = `${tokenCode}`; | ||
|
|
||
| this.cache.get(keyString)?.set(timestampMs, value); | ||
| } |
There was a problem hiding this comment.
Fix potential undefined error in set method.
The set method assumes the nested map exists but doesn't initialize it if it doesn't, which could lead to runtime errors.
async set(key: PriceCacheKey, value: TokenPrice): Promise<void> {
const { tokenCode, timestampMs } = key;
const keyString = `${tokenCode}`;
- this.cache.get(keyString)?.set(timestampMs, value);
+ if (!this.cache.has(keyString)) {
+ this.cache.set(keyString, new Map());
+ }
+ this.cache.get(keyString)!.set(timestampMs, value);
}📝 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.
| async set(key: PriceCacheKey, value: TokenPrice): Promise<void> { | |
| const { tokenCode, timestampMs } = key; | |
| const keyString = `${tokenCode}-${timestampMs}`; | |
| const keyString = `${tokenCode}`; | |
| this.cache.get(keyString)?.set(timestampMs, value); | |
| } | |
| async set(key: PriceCacheKey, value: TokenPrice): Promise<void> { | |
| const { tokenCode, timestampMs } = key; | |
| const keyString = `${tokenCode}`; | |
| if (!this.cache.has(keyString)) { | |
| this.cache.set(keyString, new Map()); | |
| } | |
| this.cache.get(keyString)!.set(timestampMs, value); | |
| } |
There was a problem hiding this comment.
| export const getMigrationsFolder = (type: string): string => { | ||
| const migrationsFolder = path.join( | ||
| path.dirname(new URL(import.meta.url).pathname), | ||
| `../migrations`, | ||
| `../migrations/${type}`, | ||
| ); |
There was a problem hiding this comment.
Use process.cwd() for root references.
The current path construction using URL pathname might not work correctly across different platforms. Consider using process.cwd() as per the coding guidelines.
export const getMigrationsFolder = (type: string): string => {
const migrationsFolder = path.join(
- path.dirname(new URL(import.meta.url).pathname),
+ process.cwd(),
`../migrations/${type}`,
);📝 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.
| export const getMigrationsFolder = (type: string): string => { | |
| const migrationsFolder = path.join( | |
| path.dirname(new URL(import.meta.url).pathname), | |
| `../migrations`, | |
| `../migrations/${type}`, | |
| ); | |
| export const getMigrationsFolder = (type: string): string => { | |
| const migrationsFolder = path.join( | |
| process.cwd(), | |
| `../migrations/${type}`, | |
| ); | |
| // Further processing may follow... | |
| } |
There was a problem hiding this comment.
replace '-' with '_' in names
There was a problem hiding this comment.
@0xkenj1 I'll update the choices array to use underscores instead of hyphens:
.options("migrationsFolder", {
alias: "m",
type: "string",
choices: ["processing", "external_services_cache"],
demandOption: true,
description: "Migrations folder",
default: "processing",
})|
|
||
| const logger = Logger.getInstance(); | ||
|
|
||
| const db = createKyselyDatabase( | ||
| { | ||
| connectionString: DATABASE_URL, | ||
| withSchema: schema, | ||
| isProduction: NODE_ENV === "production" || NODE_ENV === "staging", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add additional safeguard for destructive operations in production.
Since this is a destructive operation that drops the schema, consider adding an additional confirmation step or environment check before proceeding in production/staging environments.
- isProduction: NODE_ENV === "production" || NODE_ENV === "staging",
+ isProduction: NODE_ENV === "production" || NODE_ENV === "staging",
+ },
+ logger,
+ );
+
+ if (NODE_ENV === "production" || NODE_ENV === "staging") {
+ const confirmation = process.env.CONFIRM_DESTRUCTIVE_ACTION;
+ if (confirmation !== "YES_I_KNOW_THIS_WILL_DELETE_ALL_DATA") {
+ throw new Error("Destructive action not confirmed for production environment");
+ }📝 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.
| isProduction: NODE_ENV === "production" || NODE_ENV === "staging", | |
| isProduction: NODE_ENV === "production" || NODE_ENV === "staging", | |
| }, | |
| logger, | |
| ); | |
| if (NODE_ENV === "production" || NODE_ENV === "staging") { | |
| const confirmation = process.env.CONFIRM_DESTRUCTIVE_ACTION; | |
| if (confirmation !== "YES_I_KNOW_THIS_WILL_DELETE_ALL_DATA") { | |
| throw new Error("Destructive action not confirmed for production environment"); | |
| } | |
| } |
There was a problem hiding this comment.
this is just a nice to have for now , will tackle it on future improvements
There was a problem hiding this comment.
@0xkenj1 Understood! Feel free to implement these safeguards when you tackle the improvements later.
scripts/bootstrap/README.md
Outdated
| # grants-stack-indexer: migrations scripts | ||
|
|
||
| This package contains scripts for managing the database schema and migrations. |
There was a problem hiding this comment.
Title and Directory Consistency Check
The README begins with # grants-stack-indexer: migrations scripts while it resides in the scripts/bootstrap directory. If this document is intended to cover bootstrap scripts (e.g., metadata and pricing bootstrapping as per the PR objectives), please update the title to reflect that. Otherwise, if it should document migration scripts, consider moving it to the appropriate directory for clarity.
Docstrings generation was requested by @0xkenj1. * #63 (comment) The following files were modified: * `packages/shared/src/utils/testing.ts` * `scripts/bootstrap/src/schemas/index.ts` * `scripts/migrations/src/migrations/external-services-cache/20250127T000000_add_cache_tables.ts` * `scripts/migrations/src/migrations/processing/20241029T120000_initial.ts` * `scripts/migrations/src/utils/kysely.ts`
|
Note Generated docstrings for this pull request at #65 |
| @@ -0,0 +1,5 @@ | |||
| export class NoClosePriceFound extends Error { | |||
There was a problem hiding this comment.
this should extend RetriableError or NonRetriableError if we consider that there's no chance that a correct response will ever be returned
| import { ICacheable, ILogger, TimestampMs, TokenCode } from "@grants-stack-indexer/shared"; | ||
|
|
||
| import { IPricingProvider, TokenPrice } from "../internal.js"; | ||
| import { NoClosePriceFound } from "../exceptions/noClosePriceFound.exception.js"; |
There was a problem hiding this comment.
export error in exceptions/index.ts and import from ..internal.js
|
|
||
| type CacheResult = { | ||
| timestampMs: TimestampMs; | ||
| price: TokenPrice | undefined; | ||
| }; | ||
|
|
||
| const PROXIMITY_GRANULARITY_MS_FACTOR = 5; |
There was a problem hiding this comment.
move this to the constants file
| let fromTimestampMs = timestamps[0] as TimestampMs; | ||
| let toTimestampMs = timestamps[timestamps.length - 1] as TimestampMs; |
There was a problem hiding this comment.
should we sort the array first or we trust that input is sorted?
| const cachedPrices = ( | ||
| await this.getCachedPrices( | ||
| tokenCode, | ||
| ((fromTimestampMs as number) - MIN_GRANULARITY_MS) as TimestampMs, |
There was a problem hiding this comment.
why here isn't (fromTimestampMs as number) - MIN_GRANULARITY_MS * PROXIMITY_GRANULARITY_MS_FACTOR ?
probably we want a private method calculateLowerBoundTimestamp
| @@ -112,11 +113,9 @@ export class CoingeckoProvider implements IPricingProvider { | |||
| return undefined; | |||
| } | |||
|
|
|||
| const path = `/coins/${tokenId}/market_chart/range?vs_currency=usd&from=${startTimestampMs}&to=${endTimestampMs}&precision=full`; | |||
|
|
|||
| const path = `/coins/${tokenId}/market_chart/range?vs_currency=usd&from=${startTimestampMs / 1000}&to=${endTimestampMs / 1000}&precision=full`; | |||
There was a problem hiding this comment.
are we sure that we have to send timestamp in seconds?
| const oneDayMs = 24 * 60 * 60 * 1000; // 1 day in milliseconds | ||
| const ninetyDaysMs = 90 * oneDayMs; // 90 days in milliseconds |
| // Log if the difference is greater than 90 days | ||
| if (effectiveMax - effectiveMin > ninetyDaysMs) { | ||
| const segments: Promise<TokenPrice[]>[] = []; | ||
| const segmentDuration = 88 * oneDayMs; // 88 days in milliseconds | ||
| let currentStart = effectiveMin; | ||
|
|
There was a problem hiding this comment.
why it talks about 90 days but uses 88 days? also, it's not logging anything
There was a problem hiding this comment.
is this logic related to smth on the Coingecko API or what's the reason behind the 90 days segmentation?
There was a problem hiding this comment.
yes, its related to coingecko, 88 is to make sure we return data in samples of 1h
There was a problem hiding this comment.
remove log comment
There was a problem hiding this comment.
niceee 💯
just delete commented out lines
| const gateway = this.gateways[(startIndex + i) % this.gateways.length]!; | ||
| const gateway = this.gateways[i]; |
There was a problem hiding this comment.
you think this isn't necessary? i've done it to avoid always start fetching with the same Gateway which leads to rate-limiting it faster (from what i tested locally)
apps/processing/src/config/env.ts
Outdated
| }) | ||
| .and( | ||
| z | ||
| .discriminatedUnion("METADATA_SOURCE", [ | ||
| dummyMetadataSchema, | ||
| pinataMetadataSchema, | ||
| publicGatewayMetadataSchema, | ||
| ]) | ||
| .transform((val) => { | ||
| if (val.METADATA_SOURCE === "dummy") { | ||
| return { metadataSource: val.METADATA_SOURCE, ...val }; | ||
| } | ||
| if (val.METADATA_SOURCE === "pinata") { | ||
| return { | ||
| metadataSource: val.METADATA_SOURCE, | ||
| jwt: val.PINATA_JWT, | ||
| gateway: val.PINATA_GATEWAY_URL, | ||
| ...val, | ||
| }; | ||
| } | ||
| if (val.METADATA_SOURCE === "public-gateway") { | ||
| return { | ||
| metadataSource: val.METADATA_SOURCE, | ||
| gateways: val.PUBLIC_GATEWAY_URLS, | ||
| ...val, | ||
| }; | ||
| } | ||
| throw new Error("Invalid metadata source"); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
would be interesting to see what solution or apporach you suggest on this?
| if ( | ||
| events[0] && | ||
| Math.abs(new Date().getTime() - events[0].blockTimestamp!) > | ||
| 1000 * 60 * 60 * 0.5 // 30 minutes |
There was a problem hiding this comment.
move this to a constant
also, why is this condition? for under 30 minutes from the present you've seen any sub-optimization on the bulk fetch?
| this.logger.info(`Processed events: ${processedEvents}/${totalEvents}`, { | ||
| className: Orchestrator.name, | ||
| chainId: this.chainId, | ||
| }); |
| from: raw_events( | ||
| where: { chain_id: { _eq: $chainId } } | ||
| order_by: { block_timestamp: asc } | ||
| limit: 1 | ||
| ) { | ||
| block_timestamp | ||
| } |
There was a problem hiding this comment.
from can be cached right? it's always the same
There was a problem hiding this comment.
Yes, but wont be fixed for now, since it is only used by scripts
| //This is only to try in advance if we have the tables and db in sync | ||
| await db | ||
| .insertInto("metadataCache") | ||
| .values({ id: "asd", metadata: { asd: "asd" }, createdAt: new Date() }) | ||
| .execute(); |
There was a problem hiding this comment.
didn't understand this, why we need to make an insert to test if table exists? can't we make a select?
There was a problem hiding this comment.
await db.selectFrom("metadataCache").selectAll().execute();
| /** | ||
| * This script handles database reset for the grants-stack-indexer project. | ||
| * | ||
| * It performs the following steps: | ||
| * 1. Loads environment variables from .env file | ||
| * 2. Gets database configuration (URL and schema name) from environment | ||
| * 3. Creates a Kysely database connection with the specified schema | ||
| * 4. Drops and recreates the database schema | ||
| * 5. Reports success/failure of reset operation | ||
| * 6. Closes database connection and exits | ||
| * | ||
| * Environment variables required: | ||
| * - DATABASE_URL: PostgreSQL connection string | ||
| * | ||
| * Script arguments: | ||
| * - schema: Database schema name where migrations are applied | ||
| * | ||
| * The script will: | ||
| * - Drop the schema if it exists | ||
| * - Recreate an empty schema | ||
| * - Log results of the reset operation | ||
| * - Exit with code 0 on success, 1 on failure | ||
| * | ||
| * WARNING: This is a destructive operation that will delete all data in the schema. | ||
| * Make sure you have backups if needed before running this script. | ||
| */ |
There was a problem hiding this comment.
jsdocs doesn't match script's task
| if (fs.existsSync("cids.txt")) { | ||
| cids.push(...fs.readFileSync("cids.txt", "utf-8").split("\n")); | ||
| } else { |
There was a problem hiding this comment.
we should add a TODO to validate the txt file content
| ); | ||
|
|
||
| // This is only to try in advance if we have the tables and db in sync | ||
| await db |
| delay: 1000, | ||
| }; | ||
| for await (const tokenCode of pMapIterable( | ||
| TOKENS_SOURCE_CODES, |
There was a problem hiding this comment.
is effective to search for all tokens?
| export const getMigrationsFolder = (type: string): string => { | ||
| const migrationsFolder = path.join( | ||
| path.dirname(new URL(import.meta.url).pathname), | ||
| `../migrations`, | ||
| `../migrations/${type}`, | ||
| ); |
There was a problem hiding this comment.
replace '-' with '_' in names
| export const getMigrationsFolder = (): string => { | ||
| const migrationsFolder = path.join( | ||
| path.dirname(new URL(import.meta.url).pathname), | ||
| `../migrations`, | ||
| ); | ||
|
|
||
| if (!existsSync(migrationsFolder)) { | ||
| throw new Error(`Migrations folder not found`); | ||
| } | ||
|
|
||
| return migrationsFolder; | ||
| }; |
There was a problem hiding this comment.
we dont need to be compliant with windows os
|
|
||
| const logger = Logger.getInstance(); | ||
|
|
||
| const db = createKyselyDatabase( | ||
| { | ||
| connectionString: DATABASE_URL, | ||
| withSchema: schema, | ||
| isProduction: NODE_ENV === "production" || NODE_ENV === "staging", |
There was a problem hiding this comment.
this is just a nice to have for now , will tackle it on future improvements
| //This is only to try in advance if we have the tables and db in sync | ||
| await db | ||
| .insertInto("metadataCache") | ||
| .values({ id: "asd", metadata: { asd: "asd" }, createdAt: new Date() }) | ||
| .execute(); |
| /** | ||
| * This script handles database reset for the grants-stack-indexer project. | ||
| * | ||
| * It performs the following steps: | ||
| * 1. Loads environment variables from .env file | ||
| * 2. Gets database configuration (URL and schema name) from environment | ||
| * 3. Creates a Kysely database connection with the specified schema | ||
| * 4. Drops and recreates the database schema | ||
| * 5. Reports success/failure of reset operation | ||
| * 6. Closes database connection and exits | ||
| * | ||
| * Environment variables required: | ||
| * - DATABASE_URL: PostgreSQL connection string | ||
| * | ||
| * Script arguments: | ||
| * - schema: Database schema name where migrations are applied | ||
| * | ||
| * The script will: | ||
| * - Drop the schema if it exists | ||
| * - Recreate an empty schema | ||
| * - Log results of the reset operation | ||
| * - Exit with code 0 on success, 1 on failure | ||
| * | ||
| * WARNING: This is a destructive operation that will delete all data in the schema. | ||
| * Make sure you have backups if needed before running this script. | ||
| */ |
| export interface IPricingCache extends ICache<PriceCacheKey, TokenPrice> { | ||
| /** | ||
| * Get the prices for a given token and time range. | ||
| * @param tokenCode - The code of the token. | ||
| * @param startTimestampMs - The start timestamp. | ||
| * @param endTimestampMs - The end timestamp. | ||
| * @returns The prices for the given token and time range. | ||
| */ | ||
| getPricesByTimeRange( | ||
| tokenCode: TokenCode, | ||
| startTimestampMs: TimestampMs, | ||
| endTimestampMs: TimestampMs, | ||
| ): Promise<TokenPrice[]>; | ||
| } |
| @@ -53,6 +66,38 @@ export class KyselyPricingCache implements ICache<PriceCacheKey, TokenPrice> { | |||
| } | |||
| } | |||
|
|
|||
| async getPricesByTimeRange( | |||
| import { ICacheable, ILogger, TimestampMs, TokenCode } from "@grants-stack-indexer/shared"; | ||
|
|
||
| import { IPricingProvider, TokenPrice } from "../internal.js"; | ||
| import { NoClosePriceFound } from "../exceptions/noClosePriceFound.exception.js"; |
|
|
||
| type CacheResult = { | ||
| timestampMs: TimestampMs; | ||
| price: TokenPrice | undefined; | ||
| }; | ||
|
|
||
| const PROXIMITY_GRANULARITY_MS_FACTOR = 5; |
| @@ -0,0 +1,5 @@ | |||
| export class NoClosePriceFound extends Error { | |||
0xkenj1
changed the title
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (37)
packages/shared/src/retry/exponentialBackoff.strategy.ts (1)
23-27: LGTM! Consider documenting the rationale for the new retry values.The new default values make the retry strategy more suitable for bootstrap operations:
- Faster initial retry (200ms) helps with transient failures
- Gentler backoff (1.5x) prevents excessive delays
- More attempts (10) improve resilience for flaky services
Consider updating the JSDoc to reflect these new defaults and their rationale:
/** * @param options - Configuration options - * @param options.baseDelay - Initial delay in milliseconds (default: 5000) - * @param options.factor - Multiplier for exponential increase (default: 2) - * @param options.maxAttempts - Maximum number of retry attempts (default: 3) + * @param options.baseDelay - Initial delay in milliseconds (default: 200) + * @param options.factor - Multiplier for exponential increase (default: 1.5) + * @param options.maxAttempts - Maximum number of retry attempts (default: 10) * @param options.maxDelay - Optional maximum delay cap in milliseconds + * + * Note: Default values are optimized for bootstrap operations that need to handle + * many concurrent requests to potentially flaky services (IPFS, pricing APIs). */packages/indexer-client/src/interfaces/indexerClient.ts (1)
36-41: Enhance JSDoc documentation for return value format.The JSDoc comments should specify that the returned timestamps are in seconds.
/** * Get the block range timestamp by chain id from the indexer service * @param chainId Id of the chain - * @returns Block range from the indexer service + * @returns Block range from the indexer service with timestamps in seconds + * {from: number, to: number} where from is the earliest block timestamp + * and to is the latest block timestamp */packages/indexer-client/src/providers/envioIndexerClient.ts (1)
270-306: Consider optimizing the GraphQL query.The current implementation uses two separate queries to fetch the earliest and latest timestamps. This could be optimized into a single aggregate query.
-async getBlockRangeTimestampByChainId(chainId: ChainId): Promise<{ from: number; to: number }> { - try { - const response = (await this.client.request( - gql` - query getBlockRangeByChainId($chainId: Int!) { - from: raw_events( - where: { chain_id: { _eq: $chainId } } - order_by: { block_timestamp: asc } - limit: 1 - ) { - block_timestamp - } - to: raw_events( - where: { chain_id: { _eq: $chainId } } - order_by: { block_timestamp: desc } - limit: 1 - ) { - block_timestamp - } - } - `, - { chainId }, - )) as { from: { block_timestamp: number }[]; to: { block_timestamp: number }[] }; - if (!response.from[0] || !response.to[0]) { - throw new IndexerClientError("No block range found", { - className: EnvioIndexerClient.name, - methodName: "getBlockRangeTimestampByChainId", - }); - } - return { - from: response.from[0].block_timestamp, - to: response.to[0].block_timestamp, - }; +async getBlockRangeTimestampByChainId(chainId: ChainId): Promise<{ from: number; to: number }> { + try { + const response = (await this.client.request( + gql` + query getBlockRangeByChainId($chainId: Int!) { + raw_events_aggregate(where: { chain_id: { _eq: $chainId } }) { + min { + block_timestamp + } + max { + block_timestamp + } + } + } + `, + { chainId }, + )) as { raw_events_aggregate: { min: { block_timestamp: number }; max: { block_timestamp: number } } }; + const { min, max } = response.raw_events_aggregate; + if (!min.block_timestamp || !max.block_timestamp) { + throw new IndexerClientError("No block range found", { + className: EnvioIndexerClient.name, + methodName: "getBlockRangeTimestampByChainId", + }); + } + return { + from: min.block_timestamp, + to: max.block_timestamp, + }; } catch (error) { throw this.handleError(error, "getBlockRangeTimestampByChainId"); } }packages/indexer-client/test/unit/envioIndexerClient.spec.ts (1)
711-738: Add edge case tests for the block range timestamps.The test suite covers basic scenarios but could benefit from additional edge cases.
Add these test cases:
it("handles single block in range", async () => { graphqlClient.request.mockResolvedValue({ from: [{ block_timestamp: 123123123000 }], to: [{ block_timestamp: 123123123000 }], }); const result = await envioIndexerClient.getBlockRangeTimestampByChainId(1 as ChainId); expect(result).toEqual({ from: 123123123000, to: 123123123000, }); }); it("handles partial response with missing to timestamp", async () => { graphqlClient.request.mockResolvedValue({ from: [{ block_timestamp: 123123123000 }], to: [], }); await expect( envioIndexerClient.getBlockRangeTimestampByChainId(1 as ChainId), ).rejects.toThrow(IndexerClientError); });packages/repository/src/repositories/memory/prices.repository.ts (1)
32-45: Consider optimizing time-range lookups.For large timestamp sets, converting everything to an array and filtering can become costly. A sorted structure (like a sorted list of timestamps) or an interval-based lookup strategy could reduce the complexity of filtering.
packages/metadata/src/factory/index.ts (1)
16-49: Avoid classes with only static members.Static analysis warns about classes containing only static methods. You can replace this class with a simple function to reduce complexity:
-export class MetadataProviderFactory { - static create( - options: MetadataConfig<MetadataProvider>, - deps?: { logger?: ILogger }, - ): IMetadataProvider { - // ... - } -} +export function createMetadataProvider( + options: MetadataConfig<MetadataProvider>, + deps?: { logger?: ILogger }, +): IMetadataProvider { + // ... +}🧰 Tools
🪛 Biome (1.9.4)
[error] 16-49: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/metadata/src/providers/publicGateway.provider.ts (4)
20-20: Check if the 3-second timeout is appropriate.
A 3-second timeout may be too short in some network conditions or too long in congested environments. Ensure this value is tested in staging or with real-world loads.
30-38: Consider adding a retry mechanism for each gateway.
There's a TODO comment about retrying each gateway request. A standard approach is exponential backoff or retry-with-jitter. This would improve the resiliency of your fetch process.
41-44: Consolidate the invalid JSON check.
Currently, you're verifying bothtypeof data === "object"andisJSON(data). Ifdatais not already parsed JSON, it might pass one check while failing the other; consider streamlining into a single validation step for clarity.
57-57: Use a custom error instead of a genericError.
Throwing a domain-specific exception (e.g.,MetadataFetchException) would allow upstream error handling to differentiate between IPFS failures vs. other errors.- throw new Error(`Failed to fetch IPFS data for CID ${ipfsCid} from all gateways.`); + import { MetadataFetchException } from "../exceptions/metadataFetch.exception.js"; + + throw new MetadataFetchException(`Failed to fetch IPFS data for CID ${ipfsCid} from all gateways.`);apps/processing/src/config/env.ts (2)
55-57:dummyMetadataSchemaintroduced.
This schema is minimal, which is fine for tests or placeholder usage. Make sure you have test coverage to confirm it doesn't break with future expansions.
77-97: Consider consolidating the double discriminated union into a single schema.
Chaining discriminated unions can complicate future expansions. A single union with refined branching logic may be more maintainable.packages/repository/src/repositories/kysely/prices.repository.ts (1)
54-85: NewgetPricesByTimeRangemethod is well-structured.
This method helps handle queries for historical pricing data efficiently. Note that if concurrency is a major concern, consider adding an index on(tokenCode, timestampMs)for faster lookups, if not already present.packages/pricing/src/providers/cachingProxy.provider.ts (2)
31-41: Extract threshold calculation into a helper function.
You repeatedly cast(startTimestampMs as number) - MIN_GRANULARITY_MS * PROXIMITY_FACTORtoTimestampMs. For cleaner code, consider extracting this offset calculation into a helper or constant.- cachedPrices = ( - await this.cache.getPricesByTimeRange( - tokenCode, - ((startTimestampMs as number) - MIN_GRANULARITY_MS * PROXIMITY_FACTOR) as TimestampMs, - startTimestampMs, - ) - ).sort((a, b) => a.timestampMs - b.timestampMs); + const fromTs = ((startTimestampMs as number) - MIN_GRANULARITY_MS * PROXIMITY_FACTOR) as TimestampMs; + cachedPrices = (await this.cache.getPricesByTimeRange( + tokenCode, + fromTs, + startTimestampMs, + )).sort((a, b) => a.timestampMs - b.timestampMs);
185-193: Centralize boundary checks for clarity.
The boundary checks for timestamps vs.prices[0]andprices[prices.length - 1]are correct, but you may wish to unify them or store thethreshold = MIN_GRANULARITY_MS * PROXIMITY_FACTORin a single local variable to enhance readability.packages/pricing/test/providers/cachingProxy.provider.spec.ts (1)
136-214: Avoid using "should" in test descriptions.
Per the coding guidelines, rephrase test descriptions to use more declarative phrasing. For example, replace"should return ..."with"returns ...".- it("should return an empty array when no timestamps are provided", ... + it("returns an empty array when no timestamps are provided", ...packages/data-flow/src/orchestrator.ts (4)
108-108: Typed environment
Explicitly declaring the environment as"development" | "staging" | "production"is a good approach. Consider adding runtime validation (e.g., Zod) if you anticipate unexpected values.
182-186: Frequent info logging
Logging on every event might be too verbose with large batches. Consider switching todebugif logs get cluttered, or batch these logs less frequently.
292-292: Unresolved TODO
A TODO comment references discussingallowPartialLastBlock. If needed, I can help open a new issue to track this question.
347-359: Hardcoded concurrency
ThepMapconcurrency is currently hardcoded at 10. Consider making it configurable via environment variables or constructor arguments to make the script more flexible.packages/metadata/src/exceptions/invalidMetadataSource.exception.ts (1)
3-7: Enhance error message with more details.Consider making the error message more descriptive by including the invalid source value to help with debugging.
export class InvalidMetadataSourceException extends NonRetriableError { - constructor() { - super(`Invalid metadata source`); + constructor(source: string) { + super(`Invalid metadata source: ${source}`); } }packages/metadata/src/providers/dummy.provider.ts (1)
4-4: Add proper JSDoc documentation.Replace
@inheritdocwith proper JSDoc documentation to describe the method's purpose and parameters.- /* @inheritdoc */ + /** + * Returns undefined for any IPFS CID, as this is a dummy implementation. + * @param ipfsCid - The IPFS CID to fetch metadata for + * @returns A Promise that resolves to undefined + */packages/metadata/src/exceptions/missingDependencies.exception.ts (1)
3-7: Add JSDoc documentation to the exception class.As per our TypeScript guidelines, please add JSDoc documentation to describe the purpose of this exception class and its constructor parameter.
Add the following documentation:
+/** + * Exception thrown when required dependencies are missing during initialization. + */ export class MissingDependenciesException extends NonRetriableError { + /** + * Creates a new instance of MissingDependenciesException. + * @param dependencies - Array of missing dependency names. + */ constructor(dependencies: string[]) { super(`Missing dependencies: ${dependencies.join(", ")}`); } }packages/metadata/src/interfaces/index.ts (1)
5-18: Consider adding runtime validation for gateway URLs.While the types provide good compile-time safety, consider adding runtime validation using Zod to ensure that the
gatewaysarray contains valid URLs.Example implementation:
import { z } from 'zod'; const gatewayMetadataConfigSchema = z.object({ metadataSource: z.literal('public-gateway'), gateways: z.array(z.string().url()) }); type GatewayMetadataConfig = z.infer<typeof gatewayMetadataConfigSchema>;packages/data-flow/test/unit/helpers/index.spec.ts (1)
7-38: Add test cases for better coverage.While the current test cases cover basic functionality, consider adding tests for:
- Events with invalid data to verify error handling.
- Events with multiple metadata CIDs to verify uniqueness.
- Events with different types of data (DG, DVMD, DVMDExtended) to verify all decoders.
packages/data-flow/src/helpers/index.ts (1)
13-44: Optimize the function for better readability.Consider refactoring the function to reduce nesting and improve readability:
export const getMetadataCidsFromEvents = ( events: AnyIndexerFetchedEvent[], logger: ILogger, ): string[] => { const ids = new Set<string>(); for (const event of events) { - if ("metadata" in event.params) { - ids.add(event.params.metadata[1]); - } else if ("data" in event.params) { - try { - const decoded = decodeDGApplicationData(event.params.data); - ids.add(decoded.metadata.pointer); - } catch (error) {} - try { - const decoded = decodeDVMDApplicationData(event.params.data); - ids.add(decoded.metadata.pointer); - } catch (error) {} - try { - const decoded = decodeDVMDExtendedApplicationData(event.params.data); - ids.add(decoded.metadata.pointer); - } catch (error) { - logger.warn("Failed to decode DVMD extended application data", { - error, - event, - }); - } - } + if (!("metadata" in event.params) && !("data" in event.params)) { + continue; + } + + if ("metadata" in event.params) { + ids.add(event.params.metadata[1]); + continue; + } + + const decoders = [ + { fn: decodeDGApplicationData, name: "DG" }, + { fn: decodeDVMDApplicationData, name: "DVMD" }, + { fn: decodeDVMDExtendedApplicationData, name: "DVMDExtended" }, + ]; + + for (const { fn, name } of decoders) { + try { + const decoded = fn(event.params.data); + ids.add(decoded.metadata.pointer); + } catch (error) { + logger.warn(`Failed to decode ${name} application data`, { + error, + event, + }); + } + } } return Array.from(ids); };scripts/migrations/src/utils/parsing.ts (1)
46-61: Improve error handling with specific error messages.The error messages in the try-catch block are identical. Consider providing more specific error messages to help with debugging.
export const getMigrationsFolder = (type: string): string => { try { const migrationsFolder = path.join( path.dirname(new URL(import.meta.url).pathname), `../migrations/${type}`, ); if (!existsSync(migrationsFolder)) { - throw new Error(`Migrations folder not found`); + throw new Error(`Migrations folder not found at path: ${migrationsFolder}`); } return migrationsFolder; } catch (error) { - throw new Error(`Migrations folder not found`); + throw new Error(`Failed to resolve migrations folder for type '${type}': ${error.message}`); } };packages/metadata/test/factory/metadata.factory.spec.ts (2)
30-32: Use a proper logger mock instead of an empty object.An empty object as logger mock might hide potential issues. Consider using a proper mock implementation.
- const metadataProvider = MetadataProviderFactory.create(options, { - logger: {} as ILogger, - }); + const loggerMock: ILogger = { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }; + const metadataProvider = MetadataProviderFactory.create(options, { + logger: loggerMock, + });
48-51: Avoid type assertion for invalid metadata source test.Using type assertion to create invalid options might hide type errors. Consider using a more type-safe approach.
- const options = { - metadataSource: "invalid", - } as unknown as MetadataConfig<"dummy">; + const options: MetadataConfig<"dummy" | "public-gateway"> & { metadataSource: string } = { + metadataSource: "invalid", + };packages/metadata/src/providers/cachingProxy.provider.ts (2)
8-13: Update documentation to reflect null handling.The class documentation should be updated to explain when and why null values might be returned or cached.
/** * A metadata provider that caches metadata lookups from the underlying provider. * When a metadata is requested, it first checks the cache. If found, returns the cached metadata. * If not found in cache, fetches from the underlying provider and caches the result before returning. * Cache failures (both reads and writes) are logged but do not prevent the provider from functioning. + * + * @remarks + * The provider can return null values, which are treated as valid cached results. + * This allows distinguishing between "not found" (null) and "error fetching" (undefined) cases. */
17-17: Consider using a more specific type for cache value.The cache type could be more specific to match the actual values being stored.
- private readonly cache: ICache<string, unknown> & Partial<ICacheable>, + private readonly cache: ICache<string, T | null> & Partial<ICacheable>,scripts/bootstrap/src/pricing.script.ts (2)
94-97: Consider making retry and concurrency options configurable.The retry options and concurrency limit are hardcoded. Consider making these configurable through environment variables for better flexibility.
+const RETRY_MAX_ATTEMPTS = process.env.RETRY_MAX_ATTEMPTS ? parseInt(process.env.RETRY_MAX_ATTEMPTS) : 10; +const RETRY_DELAY_MS = process.env.RETRY_DELAY_MS ? parseInt(process.env.RETRY_DELAY_MS) : 1000; +const CONCURRENCY_LIMIT = process.env.CONCURRENCY_LIMIT ? parseInt(process.env.CONCURRENCY_LIMIT) : 10; const retryOptions: RetryOptions = { - maxTry: 10, - delay: 1000, + maxTry: RETRY_MAX_ATTEMPTS, + delay: RETRY_DELAY_MS, };Update the concurrency limit:
- { concurrency: 10 }, + { concurrency: CONCURRENCY_LIMIT },Also applies to: 118-118
111-116: Enhance error logging for better debugging.The error handling could provide more detailed information about the failure.
if (error instanceof UnsupportedToken) { - console.log(`${tokenCode} is not supported `); + console.log(`Token ${tokenCode} is not supported: ${error.message}`); +} else { + console.error(`Failed to fetch prices for token ${tokenCode}:`, error); }scripts/bootstrap/src/metadata.script.ts (1)
144-146: Enhance error logging for better debugging.The error handling could provide more context about the failure.
} catch (error) { + console.error(`Failed to fetch metadata for CID ${cid}:`, error); return { status: "rejected", error }; }scripts/bootstrap/README.md (1)
61-66: Add language identifier to code blocks.Add language identifiers to the code blocks for better syntax highlighting.
-``` +```bash pnpm bootstrap:metadata --schema=schema_nameAlso applies to: 71-72 </blockquote></details> <details> <summary>scripts/migrations/README.md (2)</summary><blockquote> `61-63`: **Add language identifier to code blocks.** Add language identifiers to the code blocks for better syntax highlighting. ```diff -``` +```bash pnpm db:cache:migrate --schema=schema_nameAlso applies to: 90-92 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 61-61: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `143-143`: **Address TODO for E2E tests.** The TODO comment indicates that E2E tests are needed for the scripts. Would you like me to help generate E2E tests for the migration scripts? </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 6ead1ef14cd90699d115f3b97482e6c6270787a2 and 17bcc8b4c4d2070c03737b6955f7947f10ffac35. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (79)</summary> * `.env.example` (0 hunks) * `Dockerfile` (1 hunks) * `apps/processing/src/config/env.ts` (4 hunks) * `apps/processing/src/services/sharedDependencies.service.ts` (3 hunks) * `apps/processing/test/unit/sharedDependencies.service.spec.ts` (5 hunks) * `docker-compose.yaml` (3 hunks) * `package.json` (2 hunks) * `packages/data-flow/package.json` (1 hunks) * `packages/data-flow/src/external.ts` (1 hunks) * `packages/data-flow/src/helpers/index.ts` (1 hunks) * `packages/data-flow/src/internal.ts` (1 hunks) * `packages/data-flow/src/orchestrator.ts` (9 hunks) * `packages/data-flow/test/unit/eventsFetcher.spec.ts` (1 hunks) * `packages/data-flow/test/unit/helpers/index.spec.ts` (1 hunks) * `packages/data-flow/test/unit/orchestrator.spec.ts` (1 hunks) * `packages/indexer-client/src/interfaces/indexerClient.ts` (1 hunks) * `packages/indexer-client/src/providers/envioIndexerClient.ts` (2 hunks) * `packages/indexer-client/test/unit/envioIndexerClient.spec.ts` (4 hunks) * `packages/metadata/package.json` (1 hunks) * `packages/metadata/src/exceptions/index.ts` (1 hunks) * `packages/metadata/src/exceptions/invalidMetadataSource.exception.ts` (1 hunks) * `packages/metadata/src/exceptions/missingDependencies.exception.ts` (1 hunks) * `packages/metadata/src/external.ts` (1 hunks) * `packages/metadata/src/factory/index.ts` (1 hunks) * `packages/metadata/src/interfaces/index.ts` (1 hunks) * `packages/metadata/src/interfaces/metadata.interface.ts` (1 hunks) * `packages/metadata/src/internal.ts` (1 hunks) * `packages/metadata/src/providers/cachingProxy.provider.ts` (1 hunks) * `packages/metadata/src/providers/dummy.provider.ts` (1 hunks) * `packages/metadata/src/providers/index.ts` (1 hunks) * `packages/metadata/src/providers/publicGateway.provider.ts` (4 hunks) * `packages/metadata/test/factory/metadata.factory.spec.ts` (1 hunks) * `packages/metadata/test/providers/publicGateway.provider.spec.ts` (3 hunks) * `packages/pricing/src/constants/index.ts` (1 hunks) * `packages/pricing/src/exceptions/index.ts` (1 hunks) * `packages/pricing/src/exceptions/noClosePriceFound.exception.ts` (1 hunks) * `packages/pricing/src/providers/cachingProxy.provider.ts` (7 hunks) * `packages/pricing/src/providers/coingecko.provider.ts` (5 hunks) * `packages/pricing/test/providers/cachingProxy.provider.spec.ts` (4 hunks) * `packages/pricing/test/providers/coingecko.provider.spec.ts` (5 hunks) * `packages/processors/src/external.ts` (1 hunks) * `packages/processors/src/internal.ts` (1 hunks) * `packages/processors/src/processors/allo/handlers/poolCreated.handler.ts` (0 hunks) * `packages/processors/src/processors/strategy/directAllocation/handlers/directAllocated.handler.ts` (2 hunks) * `packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.ts` (2 hunks) * `packages/processors/test/mocks/event.mock.ts` (1 hunks) * `packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.spec.ts` (2 hunks) * `packages/repository/src/db/connection.ts` (2 hunks) * `packages/repository/src/external.ts` (1 hunks) * `packages/repository/src/interfaces/index.ts` (1 hunks) * `packages/repository/src/interfaces/pricingCache.interface.ts` (1 hunks) * `packages/repository/src/repositories/kysely/prices.repository.ts` (2 hunks) * `packages/repository/src/repositories/memory/prices.repository.ts` (2 hunks) * `packages/shared/src/external.ts` (1 hunks) * `packages/shared/src/retry/exponentialBackoff.strategy.ts` (1 hunks) * `packages/shared/src/tokens/tokens.ts` (4 hunks) * `packages/shared/src/utils/testing.ts` (1 hunks) * `packages/shared/test/retry/exponentialBackoff.strategy.spec.ts` (1 hunks) * `scripts/bootstrap/.env.example` (1 hunks) * `scripts/bootstrap/README.md` (1 hunks) * `scripts/bootstrap/package.json` (1 hunks) * `scripts/bootstrap/src/metadata.script.ts` (1 hunks) * `scripts/bootstrap/src/pricing.script.ts` (1 hunks) * `scripts/bootstrap/src/schemas/index.ts` (1 hunks) * `scripts/bootstrap/src/utils/index.ts` (1 hunks) * `scripts/bootstrap/src/utils/kysely.ts` (1 hunks) * `scripts/bootstrap/src/utils/parsing.ts` (1 hunks) * `scripts/bootstrap/tsconfig.build.json` (1 hunks) * `scripts/bootstrap/tsconfig.json` (1 hunks) * `scripts/bootstrap/vitest.config.ts` (1 hunks) * `scripts/migrations/README.md` (3 hunks) * `scripts/migrations/package.json` (1 hunks) * `scripts/migrations/src/migrateDb.script.ts` (2 hunks) * `scripts/migrations/src/migrations/external_services_cache/20250127T000000_add_cache_tables.ts` (1 hunks) * `scripts/migrations/src/migrations/processing/20241029T120000_initial.ts` (2 hunks) * `scripts/migrations/src/resetDb.script.ts` (2 hunks) * `scripts/migrations/src/schemas/index.ts` (1 hunks) * `scripts/migrations/src/utils/kysely.ts` (3 hunks) * `scripts/migrations/src/utils/parsing.ts` (2 hunks) </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * .env.example * packages/processors/src/processors/allo/handlers/poolCreated.handler.ts </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (5)</summary> <details> <summary>`**/*.ts`: Review TypeScript files for adherence to the fo...</summary> > `**/*.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/internal.ts` - `packages/metadata/src/internal.ts` - `packages/repository/src/interfaces/index.ts` - `scripts/bootstrap/src/utils/index.ts` - `packages/metadata/src/exceptions/missingDependencies.exception.ts` - `packages/metadata/src/providers/dummy.provider.ts` - `packages/data-flow/test/unit/helpers/index.spec.ts` - `packages/data-flow/src/external.ts` - `packages/metadata/src/exceptions/invalidMetadataSource.exception.ts` - `packages/processors/src/internal.ts` - `packages/pricing/src/exceptions/noClosePriceFound.exception.ts` - `packages/metadata/src/exceptions/index.ts` - `packages/processors/test/mocks/event.mock.ts` - `packages/metadata/src/external.ts` - `scripts/migrations/src/migrations/external_services_cache/20250127T000000_add_cache_tables.ts` - `packages/shared/src/external.ts` - `apps/processing/test/unit/sharedDependencies.service.spec.ts` - `packages/repository/src/external.ts` - `packages/shared/src/utils/testing.ts` - `packages/metadata/test/factory/metadata.factory.spec.ts` - `scripts/bootstrap/src/utils/parsing.ts` - `packages/processors/src/processors/strategy/directAllocation/handlers/directAllocated.handler.ts` - `packages/metadata/src/providers/index.ts` - `packages/processors/src/external.ts` - `scripts/bootstrap/vitest.config.ts` - `scripts/migrations/src/schemas/index.ts` - `packages/pricing/src/exceptions/index.ts` - `packages/indexer-client/src/interfaces/indexerClient.ts` - `packages/indexer-client/src/providers/envioIndexerClient.ts` - `scripts/migrations/src/migrations/processing/20241029T120000_initial.ts` - `packages/shared/test/retry/exponentialBackoff.strategy.spec.ts` - `packages/data-flow/src/helpers/index.ts` - `packages/repository/src/repositories/memory/prices.repository.ts` - `scripts/migrations/src/migrateDb.script.ts` - `packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.spec.ts` - `packages/shared/src/retry/exponentialBackoff.strategy.ts` - `packages/data-flow/test/unit/orchestrator.spec.ts` - `scripts/migrations/src/resetDb.script.ts` - `packages/repository/src/interfaces/pricingCache.interface.ts` - `scripts/bootstrap/src/metadata.script.ts` - `packages/metadata/src/interfaces/metadata.interface.ts` - `packages/metadata/src/factory/index.ts` - `packages/indexer-client/test/unit/envioIndexerClient.spec.ts` - `packages/metadata/test/providers/publicGateway.provider.spec.ts` - `packages/repository/src/repositories/kysely/prices.repository.ts` - `packages/pricing/test/providers/coingecko.provider.spec.ts` - `packages/pricing/src/providers/coingecko.provider.ts` - `packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.ts` - `packages/data-flow/src/orchestrator.ts` - `apps/processing/src/services/sharedDependencies.service.ts` - `scripts/migrations/src/utils/kysely.ts` - `scripts/bootstrap/src/utils/kysely.ts` - `scripts/migrations/src/utils/parsing.ts` - `scripts/bootstrap/src/schemas/index.ts` - `packages/data-flow/test/unit/eventsFetcher.spec.ts` - `packages/metadata/src/providers/publicGateway.provider.ts` - `packages/pricing/test/providers/cachingProxy.provider.spec.ts` - `packages/pricing/src/providers/cachingProxy.provider.ts` - `packages/repository/src/db/connection.ts` - `apps/processing/src/config/env.ts` - `packages/pricing/src/constants/index.ts` - `packages/shared/src/tokens/tokens.ts` - `scripts/bootstrap/src/pricing.script.ts` - `packages/metadata/src/providers/cachingProxy.provider.ts` - `packages/metadata/src/interfaces/index.ts` </details> <details> <summary>`scripts/**/*.ts`: Ensure scripts: - Use `process.cw...</summary> > `scripts/**/*.ts`: Ensure scripts: > - Use `process.cwd()` for root references. > - Follow folder conventions (`infra/` for infra scripts, `utilities/` for utilities). > - Are organized in `package.json` with `script:infra:{name}` or `script:util:{name}`. > - Be concise and avoid overly nitpicky feedback outside of these best practices. - `scripts/bootstrap/src/utils/index.ts` - `scripts/migrations/src/migrations/external_services_cache/20250127T000000_add_cache_tables.ts` - `scripts/bootstrap/src/utils/parsing.ts` - `scripts/bootstrap/vitest.config.ts` - `scripts/migrations/src/schemas/index.ts` - `scripts/migrations/src/migrations/processing/20241029T120000_initial.ts` - `scripts/migrations/src/migrateDb.script.ts` - `scripts/migrations/src/resetDb.script.ts` - `scripts/bootstrap/src/metadata.script.ts` - `scripts/migrations/src/utils/kysely.ts` - `scripts/bootstrap/src/utils/kysely.ts` - `scripts/migrations/src/utils/parsing.ts` - `scripts/bootstrap/src/schemas/index.ts` - `scripts/bootstrap/src/pricing.script.ts` </details> <details> <summary>`**/providers/**/*.ts`: Review provider files for the follow...</summary> > `**/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 the `IMetadataProvider` interface and follow naming conventions (e.g., `GithubProvider`, `JsonFileProvider`). > - Be concise and avoid overly nitpicky feedback outside of these best practices. - `packages/metadata/src/providers/dummy.provider.ts` - `packages/metadata/src/providers/index.ts` - `packages/indexer-client/src/providers/envioIndexerClient.ts` - `packages/metadata/test/providers/publicGateway.provider.spec.ts` - `packages/pricing/test/providers/coingecko.provider.spec.ts` - `packages/pricing/src/providers/coingecko.provider.ts` - `packages/metadata/src/providers/publicGateway.provider.ts` - `packages/pricing/test/providers/cachingProxy.provider.spec.ts` - `packages/pricing/src/providers/cachingProxy.provider.ts` - `packages/metadata/src/providers/cachingProxy.provider.ts` </details> <details> <summary>`**/*.spec.ts`: Review the unit test files with the followin...</summary> > `**/*.spec.ts`: Review the unit test files with the following guidelines: > - Avoid using the word "should" in test descriptions. > - Ensure descriptive test names convey the intent of each test. > - Validate adherence to the Mocha/Chai/Jest test library best practices. > - Be concise and avoid overly nitpicky feedback outside of these best practices. - `packages/data-flow/test/unit/helpers/index.spec.ts` - `apps/processing/test/unit/sharedDependencies.service.spec.ts` - `packages/metadata/test/factory/metadata.factory.spec.ts` - `packages/shared/test/retry/exponentialBackoff.strategy.spec.ts` - `packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.spec.ts` - `packages/data-flow/test/unit/orchestrator.spec.ts` - `packages/indexer-client/test/unit/envioIndexerClient.spec.ts` - `packages/metadata/test/providers/publicGateway.provider.spec.ts` - `packages/pricing/test/providers/coingecko.provider.spec.ts` - `packages/data-flow/test/unit/eventsFetcher.spec.ts` - `packages/pricing/test/providers/cachingProxy.provider.spec.ts` </details> <details> <summary>`**/services/**/*.ts`: Review service files with the followi...</summary> > `**/services/**/*.ts`: Review service files with the following considerations: > - A Service encapsulates a broader business workflow and might orchestrate various components or interact with multiple data sources or APIs. > - Ensure proper composability: Services should use Providers for data/resource access and apply business/orchestration logic. > - Validate naming conventions for aggregating services (e.g., `AggregatorService` or domain-specific names like `MetricsService`). - `apps/processing/src/services/sharedDependencies.service.ts` </details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>scripts/bootstrap/README.md</summary> [style] ~15-~15: Consider using a different verb for a more formal wording. Context: ... | Runs formatter and automatically fixes issues | | `lint` ... (FIX_RESOLVE) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>scripts/migrations/README.md</summary> 61-61: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 90-90: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 Biome (1.9.4)</summary> <details> <summary>packages/metadata/src/factory/index.ts</summary> [error] 16-49: Avoid classes that contain only static members. Prefer using simple functions instead of classes with only static members. (lint/complexity/noStaticOnlyClass) </details> </details> </details> <details> <summary>🔇 Additional comments (103)</summary><blockquote> <details> <summary>packages/shared/test/retry/exponentialBackoff.strategy.spec.ts (2)</summary> `7-12`: **LGTM! Default options test case is updated correctly.** The test case properly validates the new default options for the ExponentialBackoff strategy. The changes reflect more reasonable defaults: - Shorter base delay (200ms) for faster initial retries - Gentler growth factor (1.5) for more gradual backoff - More retry attempts (10) for better resilience --- `1-108`: **LGTM! Test suite follows best practices.** The test suite is well-structured with: - Clear describe blocks for logical grouping - Descriptive test names that convey intent - Comprehensive coverage of all strategy behaviors - Good use of mocking for deterministic tests - Proper validation of edge cases </details> <details> <summary>packages/processors/test/mocks/event.mock.ts (1)</summary> `45-45`: **LGTM!** The timestamp adjustment maintains consistency by rounding to the nearest second, which is a good practice for testing. </details> <details> <summary>packages/processors/src/processors/strategy/directAllocation/handlers/directAllocated.handler.ts (1)</summary> `60-81`: **LGTM! Improved timestamp accuracy.** Using the block timestamp directly for the donation timestamp is more accurate as it represents when the transaction was actually mined on-chain, rather than when the price was fetched. </details> <details> <summary>packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.ts (1)</summary> `68-103`: **LGTM! Consistent timestamp handling.** The changes align with the DirectAllocatedHandler implementation, ensuring consistent timestamp handling across different handlers by using the block timestamp. </details> <details> <summary>packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.spec.ts (1)</summary> `111-111`: **LGTM! Tests updated to match implementation.** The test assertions now correctly verify that donations use the block timestamp from the event, ensuring the tests match the actual implementation. Also applies to: 174-174 </details> <details> <summary>packages/data-flow/test/unit/eventsFetcher.spec.ts (1)</summary> `21-21`: **LGTM!** The mock method is correctly added to match the interface. </details> <details> <summary>packages/indexer-client/src/providers/envioIndexerClient.ts (1)</summary> `23-28`: **LGTM!** The constructor changes safely handle optional secret parameter. </details> <details> <summary>packages/indexer-client/test/unit/envioIndexerClient.spec.ts (1)</summary> `115-119`: **LGTM!** The test correctly verifies that the header is not set when secret is omitted. </details> <details> <summary>packages/repository/src/repositories/memory/prices.repository.ts (2)</summary> `1-1`: **Looks good.** No issues spotted in these lines. Also applies to: 3-3, 9-9, 10-10, 16-16, 18-18, 23-23 --- `24-30`: **Initialize nested map to prevent potential runtime errors.** This was addressed in a past review but remains unresolved. The set method will fail silently if the nested map is missing. Ensure it’s initialized: ```diff async set(key: PriceCacheKey, value: TokenPrice): Promise<void> { const { tokenCode, timestampMs } = key; const keyString = `${tokenCode}`; - this.cache.get(keyString)?.set(timestampMs, value); + if (!this.cache.has(keyString)) { + this.cache.set(keyString, new Map()); + } + this.cache.get(keyString)!.set(timestampMs, value); }scripts/bootstrap/src/schemas/index.ts (1)
1-58: No further issues found.The schema-based validation approach with Zod is well-structured and helps ensure environment correctness.
Also applies to: 62-68, 72-75
packages/metadata/src/providers/publicGateway.provider.ts (3)
4-4: No concerns regarding the updated import.
ImportingILogger,isJSON, andstringifyfrom the shared module is appropriate and consistent with the rest of the code.
9-9: RenamingIpfsProvidertoPublicGatewayProvideris clear and consistent.
This change better reflects the broader functionality of fetching from multiple gateways rather than a single IPFS implementation.
26-26: Verify the necessity of returningnullin addition toundefined.
WithPromise<T | undefined | null>, you now have three potential "empty" states (undefined,null, and valid data). Confirm that each of these states is distinctly handled throughout the codebase.apps/processing/src/config/env.ts (5)
26-26: Proper usage ofNODE_ENVwith a default.
Providing a default dev environment is a good practice to avoid unexpected behavior in CI or locally.
35-35: MakingINDEXER_ADMIN_SECREToptional.
Confirm downstream usage to ensure no runtime errors occur if this secret is missing.
37-37: NewMETADATA_SOURCEintroduced.
This allows flexibility for various metadata-fetching providers. Ensure that all references toMETADATA_SOURCEare updated accordingly.
40-40: Check growth of exponential backoff factor.
WithRETRY_FACTORdefaulting to 2, exponential backoff grows quickly. Verify that this aligns with your infrastructure requirements to avoid prolonged waiting times.
59-63:publicGatewayMetadataSchemaensures URL validation.
Good approach usingstringToJSONSchemaplus.array(z.string().url()). This robustly validates that all provided gateways are valid URLs.packages/repository/src/repositories/kysely/prices.repository.ts (2)
5-6: ReplacingICachewithIPricingCacheis suitable.
This clarifies that the repository is specifically intended for pricing operations instead of a generic cache.
14-14: Renaming class toKyselyPricingCacheis clear.
Keeping the class name aligned with its function makes the code more readable and discoverable for future maintainers.apps/processing/src/services/sharedDependencies.service.ts (3)
3-3: Dependency injection looks consistent.
Good job replacing direct imports (IpfsProvider, etc.) withMetadataProviderFactory, which helps maintain flexibility and keep the code aligned with SOLID.
55-55: Confirm environment equivalence for staging and production.
Using|| env.NODE_ENV === "staging"for a production-like setting may be intentional, but please verify if staging needs distinctive cloning or migration steps.
93-96: Kudos on using the metadata factory.
Creating the metadata provider via a factory with logger injection supports maintainability. No issues found here.packages/pricing/src/providers/cachingProxy.provider.ts (1)
203-210: Direct match logic is handled properly.
The direct equality check(prices[mid]!.timestampMs === targetTimestamp)followed by a proximity check is well-structured for ensuring accurate closeness detection.packages/data-flow/src/orchestrator.ts (8)
2-2: Usage ofp-mapis a good choice for concurrency.
This import looks fine. No issues identified.
35-35: Reference toTOKENS_SOURCE_CODES
Reusing this constant for enumerating tokens is a clean solution.
42-42: Helper import recognized
ImportinggetMetadataCidsFromEventscentralizes this logic and promotes maintainability.
46-46: Narrower type fortoken
Definingtokenas{ priceSourceCode: Token["priceSourceCode"] }neatly restricts usage.
135-137: Local event counters
Maintaining counters locally in-memory may lead to inaccuracies in multi-instance scenarios. Verify if your deployment model might need external or persistent tracking. Otherwise, this is acceptable for single-instance usage.
145-145: IncrementingtotalEvents
UpdatingtotalEventswith the batch size is correct. No issues found.
305-305: Early return when no events
Returning early is a clean pattern and prevents unnecessary calls. Looks good.
310-316: Constructing tokens list
Creating a tokens array fromTOKENS_SOURCE_CODESis clear and straightforward.scripts/bootstrap/src/utils/index.ts (1)
1-2: Re-exporting utility modules
These exports make it convenient for consumers to import from a central location. No issues found.packages/metadata/src/providers/index.ts (1)
1-2: Consolidated metadata provider exports
ExportingpublicGateway.provider.jsanddummy.provider.jsprovides clear, flexible options for metadata retrieval.packages/metadata/src/internal.ts (1)
5-5: LGTM!The new export statement follows the internal module pattern and aligns with the PR objectives for metadata improvements.
packages/pricing/src/exceptions/noClosePriceFound.exception.ts (1)
1-7: LGTM!The implementation correctly extends
NonRetriableErroras suggested in the past review, with a clear error message.packages/metadata/src/providers/dummy.provider.ts (1)
5-7: LGTM!The implementation correctly follows the interface contract and TypeScript best practices.
packages/pricing/src/exceptions/index.ts (1)
6-6: LGTM!The new export follows the established pattern and maintains consistency with other exports.
packages/metadata/src/external.ts (1)
1-5: LGTM!The exports follow our TypeScript guidelines by explicitly listing public exports in the
external.tsfile.packages/processors/src/internal.ts (1)
16-17: LGTM!The new decoder exports are well-organized with a clear category comment and follow the established pattern.
packages/data-flow/src/external.ts (1)
1-9: LGTM! Clean export structure.The new export
getMetadataCidsFromEventsis properly added to the existing export list, maintaining a clean separation between type exports and value exports.packages/data-flow/src/internal.ts (1)
1-13: LGTM! Clean modular structure.The new re-export from "./helpers/index.js" is properly added, maintaining a clean separation between different module exports.
packages/processors/src/external.ts (1)
9-14: LGTM! Clean export structure.The new decoding function exports are properly grouped together, maintaining a clean separation between type exports and value exports.
packages/metadata/src/interfaces/index.ts (1)
3-3: LGTM! Well-defined union type.The
MetadataProvidertype effectively constrains the possible values to "public-gateway" or "dummy".packages/metadata/src/interfaces/metadata.interface.ts (1)
10-17: LGTM! Clear documentation and improved type safety.The changes improve type safety by explicitly handling invalid CID cases with
nulland provide clear documentation about the return values.scripts/migrations/src/schemas/index.ts (1)
5-5: LGTM! Well-structured environment validation.The addition of
NODE_ENVwith proper enum values and a default follows best practices for environment variable validation using Zod.packages/pricing/src/constants/index.ts (1)
1-9: LGTM! Well-documented time constants.The changes:
- Align with Coingecko's Enterprise plan limitations
- Improve code readability with descriptive constant names
- Use accurate time calculations
scripts/bootstrap/src/utils/kysely.ts (2)
3-7: LGTM! Well-structured migration configuration interface.The interface follows TypeScript best practices with clear type definitions.
13-19: LGTM! Valid workaround for Kysely schema name extraction.The implementation is correct as it only constructs the operation node without executing the SQL query (no
.execute()call).packages/repository/src/interfaces/index.ts (1)
12-12: LGTM! Export statement follows consistent pattern.The new export for pricingCache.interface.js maintains consistency with other interface exports and correctly uses the .js extension for ESM compatibility.
packages/repository/src/interfaces/pricingCache.interface.ts (1)
1-20: LGTM! Well-structured interface with comprehensive documentation.The interface is well-designed with:
- Clear type definitions using shared package types
- Proper extension of the base cache interface
- Complete JSDoc documentation for the method
- Strong typing for all parameters
scripts/bootstrap/src/utils/parsing.ts (2)
7-12: LGTM! Well-structured schema definition.The zod schema is well-defined with:
- Appropriate type constraints
- Default value handling
- Clear description for the schema parameter
14-28: LGTM! Robust argument parsing implementation.The parseArguments function is well-implemented with:
- Proper yargs configuration
- Schema validation through zod
- Appropriate error handling
- Synchronous parsing which is suitable for bootstrap scripts
scripts/bootstrap/vitest.config.ts (1)
4-22: LGTM! Comprehensive test configuration.The Vitest configuration is well-structured with:
- Appropriate test environment settings
- Comprehensive coverage configuration with multiple reporters
- Clear file inclusion/exclusion patterns
- Proper path alias setup matching TypeScript configuration
scripts/migrations/src/migrations/external_services_cache/20250127T000000_add_cache_tables.ts (1)
17-17: Verify the intent of making the metadata column nullable.The
metadatacolumn in themetadataCachetable is now nullable. Please confirm if this is intentional to handle cases where metadata is temporarily unavailable or failed to fetch.packages/shared/src/external.ts (1)
12-12: LGTM!The changes to exports are well-organized and properly typed, following TypeScript best practices.
Also applies to: 20-26
packages/data-flow/src/helpers/index.ts (1)
23-31: Add error logging in catch blocks.Empty catch blocks silently swallow errors, making it difficult to debug issues with metadata decoding. Consider logging these errors or implementing proper error handling.
scripts/migrations/src/utils/parsing.ts (1)
46-61: Use process.cwd() for root references.The current path construction using URL pathname might not work correctly across different platforms.
packages/repository/src/external.ts (1)
80-80: LGTM! Clean addition of IPricingCache export.The export is properly grouped with related pricing types and follows TypeScript best practices.
scripts/migrations/src/migrateDb.script.ts (2)
36-37: LGTM! Environment-aware database configuration.The addition of NODE_ENV and isProduction flag properly handles different environments for database connections.
Also applies to: 45-45
56-58: Verify migration folder structure.Ensure that using migrationsFolder as the domain name aligns with your migration organization strategy, as it will affect the migration table names.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check migration folder structure fd -t d . "migrations" -x echo "Found migrations folder: {}"Length of output: 158
Manual Verification of Migration Folder Structure
The current automated search command produced errors because its arguments cause FD to treat "migrations" as a search path rather than a pattern. This means we cannot conclusively verify that a proper migrations folder exists and is consistently used as the domain name in the migration strategy. Please manually verify that the migrations folder is correctly organized in your project and that setting the domain to the migrations folder aligns with your migration naming conventions.
scripts/migrations/src/resetDb.script.ts (2)
39-40: LGTM! Consistent environment handling with migrateDb.script.The changes maintain consistency with the migration script's environment handling approach.
Also applies to: 48-48
48-48: Add additional safeguard for destructive operations in production.Since this is a destructive operation that drops the schema, consider adding an additional confirmation step or environment check before proceeding in production/staging environments.
scripts/migrations/src/utils/kysely.ts (2)
19-19: LGTM! Domain support for migration configuration.The addition of the domain property enhances migration organization capabilities.
56-57: LGTM! Consistent domain-based table naming.The implementation consistently uses the domain for both migration and lock table names across migrate and reset operations.
Also applies to: 98-99
packages/metadata/test/providers/publicGateway.provider.spec.ts (2)
10-10: LGTM! Consistent renaming fromIpfsProvidertoPublicGatewayProvider.The renaming is consistently applied across the test suite, improving clarity about the provider's purpose.
Also applies to: 13-13, 21-21, 26-26, 36-36
68-74: LGTM! Updated error handling test case.The test case now correctly asserts that an error is thrown when all gateways fail, which is a more robust error handling approach compared to returning undefined.
packages/repository/src/db/connection.ts (2)
36-41: LGTM! Well-documentedisProductionproperty.The JSDoc comments clearly explain the purpose and default value of the
isProductionproperty.
111-115: LGTM! Secure SSL configuration for production environments.The SSL configuration is properly set based on the environment:
- Production/staging: SSL enabled with
rejectUnauthorized: false- Other environments: SSL disabled
scripts/bootstrap/src/metadata.script.ts (1)
149-149: Review the high concurrency limit.A concurrency limit of 1000 might be too aggressive and could lead to resource exhaustion or rate limiting issues with the IPFS gateways.
Consider reducing the concurrency limit and making it configurable:
- concurrency: 1000, + concurrency: process.env.METADATA_CONCURRENCY ? parseInt(process.env.METADATA_CONCURRENCY) : 100,apps/processing/test/unit/sharedDependencies.service.spec.ts (4)
4-4: LGTM! Import changes reflect the shift to factory pattern.The change from direct
IpfsProviderimport toMetadataProviderFactoryaligns with the PR's objective of improving metadata handling through a factory pattern.Also applies to: 13-17
111-112: LGTM! Environment variables updated for metadata source and environment type.The addition of
METADATA_SOURCEandNODE_ENVtomockEnvproperly supports the new metadata provider factory pattern and environment-aware configuration.Also applies to: 119-120
127-131: LGTM! Database initialization updated with production awareness.The addition of
isProductioncheck in database initialization enhances environment-specific configuration handling.
140-142: LGTM! Test assertions updated for metadata provider factory.The test assertions have been correctly updated to verify the initialization of the metadata provider factory with the appropriate environment and logger parameters.
packages/pricing/src/providers/coingecko.provider.ts (4)
18-19: LGTM! Time interval constants added.The addition of
ninetyDaysMsandoneDayMsconstants improves code readability and maintainability.
41-42: LGTM! Unsupported tokens commented out.The commented-out tokens (
GIST,GcV,MTK) are clearly marked as unsupported.Also applies to: 49-50, 60-61
117-117: LGTM! Timestamp conversion to seconds.The division by 1000 correctly converts timestamps from milliseconds to seconds for the Coingecko API.
172-198: LGTM! Enhanced handling of large date ranges.The implementation correctly segments requests for date ranges exceeding 90 days into 88-day intervals, processes them in parallel, and combines the results.
packages/pricing/test/providers/coingecko.provider.spec.ts (4)
13-17: LGTM! Import structure improved.The change to multi-line imports enhances code readability and maintainability.
236-254: LGTM! Test coverage added for valid timestamps.The test case thoroughly verifies the behavior of
getTokenPriceswith valid timestamps.
256-274: LGTM! Test coverage added for approximate matches.The test case properly verifies that the closest available prices are returned when exact timestamps are not available.
276-282: LGTM! Error handling test added.The test case properly verifies that unknown pricing errors are handled correctly.
scripts/migrations/src/migrations/processing/20241029T120000_initial.ts (2)
3-3: LGTM! Import path updated.The import path for
getSchemaNamehas been correctly updated to reflect the new directory structure.
22-22: LGTM! Removed debug logging.The schema logging has been commented out, which is appropriate for production code.
packages/shared/src/tokens/tokens.ts (3)
20-24: LGTM! Well-structured type definition.The
TokensMaptype provides a clear and type-safe way to map chain IDs to token addresses and their details.
26-36: LGTM! Efficient implementation of price source code extraction.The function uses a
Setto ensure uniqueness and provides a clean way to extract all price source codes.
94-100: Verify the impact of price source code change.The token's price source code has been changed from "DAI" to "GIST". This change might affect price lookups.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any direct references to the old price source code # Search for "DAI" price source code references rg -l "priceSourceCode.*DAI" --type tsLength of output: 77
Price Source Code Change Impact Verified
- The search shows that the only occurrence of a reference to an old "DAI" price source is within the tokens file itself.
- The updated token entry (address "0x93A5347036f69BC6f37Ed2b59CBcDDa927719217") now uses
"GIST"for itspriceSourceCode, while other token entries in the same file still use"DAI".- There is no evidence that any logic (e.g., price lookup or conditional checks) relies on a constant string
"DAI". This suggests that the change to"GIST"is isolated to this token definition and should not adversely affect price lookups.packages/data-flow/test/unit/orchestrator.spec.ts (1)
35-36: Verify if these imports are still needed.The
TokenandTokenCodeimports are commented out but not removed. If they're no longer needed, they should be removed to maintain code cleanliness.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if these types are used in the file # Search for Token and TokenCode usage rg -w "Token|TokenCode" "packages/data-flow/test/unit/orchestrator.spec.ts"Length of output: 107
Clean up unused commented imports in orchestrator.spec.ts
The grep output confirms that "Token" and "TokenCode" appear only in commented lines and aren’t used in any active code. If these imports are not required for upcoming changes or tests, it is best to remove them to clean up the file.
packages/metadata/src/exceptions/index.ts (1)
4-5: LGTM! Consistent exception exports.The new exception exports follow the established pattern and maintain consistency with existing exports.
scripts/bootstrap/tsconfig.build.json (1)
1-11: LGTM! Well-structured TypeScript configuration.The configuration follows best practices by:
- Extending the base configuration
- Enabling declaration files and source maps
- Setting up proper output directory
- Including appropriate files while excluding test and build artifacts
Dockerfile (2)
5-5: LGTM! Version locking pnpm improves build reproducibility.Explicitly preparing [email protected] ensures consistent builds across environments.
12-12: Verify impact of removing --prod flag.The removal of
--prodflag from the deploy command might include development dependencies in the production build, potentially increasing the image size.Consider keeping the
--prodflag to ensure a lean production build:-RUN pnpm deploy --filter=./apps/processing /prod/processing +RUN pnpm deploy --filter=./apps/processing --prod /prod/processingscripts/bootstrap/.env.example (1)
4-4: LGTM! Redundant gateway configuration improves reliability.The multiple public gateway URLs provide good fallback options if some gateways are unavailable.
packages/metadata/package.json (1)
34-34: LGTM! Adding Pinata enhances IPFS capabilities.The addition of the Pinata dependency will improve IPFS interaction capabilities. The version is appropriately locked for stability.
packages/data-flow/package.json (1)
38-38: LGTM! p-map addition will improve concurrent processing.Adding p-map is a good choice for handling concurrent operations in metadata and pricing processing. The version is appropriately locked for stability.
scripts/migrations/package.json (1)
19-20: LGTM! Well-structured cache migration scripts.The new cache migration scripts follow the existing pattern and maintain good separation of concerns between cache and main database migrations.
scripts/bootstrap/package.json (1)
1-46: LGTM! Well-structured package configuration.The package is well-organized with appropriate dependencies and comprehensive scripts for metadata and pricing bootstrap operations.
package.json (1)
11-12: LGTM! New scripts and package manager update.The new bootstrap and cache-related scripts enhance the project's functionality, and the package manager update is appropriate.
Also applies to: 16-17, 56-56
docker-compose.yaml (3)
7-9: Address TODO about processing service persistence.The TODO comment indicates that volume mounting is pending until the processing service persists the latest event processed.
Please confirm if this feature has been implemented in the processing service.
58-67: LGTM! Well-configured processing service.The processing service is properly configured with appropriate build context, restart policy, and dependencies.
120-121: Verify network configuration change impact.Adding the datalayer network to indexer-graphql-api might affect service communication.
Please confirm that this network configuration change is intentional and required for the service to function properly.
scripts/bootstrap/tsconfig.json
Outdated
| { | ||
| "extends": "../../tsconfig.json", | ||
| "include": ["src/**/*", "test/**/*"] | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Configuration extends root tsconfig and includes relevant paths.
The configuration correctly extends the root tsconfig.json and includes both source and test files, which aligns with the bootstrap scripts implementation mentioned in the PR objectives.
Let's verify the paths and structure:
🏁 Script executed:
#!/bin/bash
# Description: Verify the TypeScript configuration structure
# Check if root tsconfig exists
if [ ! -f "tsconfig.json" ]; then
echo "Error: Root tsconfig.json not found"
exit 1
fi
# Check if src and test directories exist in scripts/bootstrap
if [ ! -d "scripts/bootstrap/src" ] || [ ! -d "scripts/bootstrap/test" ]; then
echo "Warning: src or test directories not found in scripts/bootstrap"
fi
# Compare with other similar configurations
echo "Similar TypeScript configurations in the project:"
fd tsconfig.json --exclude node_modulesLength of output: 833
Review Update: Incorrect Include Paths in TSConfig
Our verification indicates that the paths specified in the "include" property ("src/**/*" and "test/**/*") do not exist in the scripts/bootstrap directory. Please confirm whether these directories should be created or if the configuration needs to be updated to reference the correct locations.
- Missing directories:
scripts/bootstrap/srcand/orscripts/bootstrap/testdo not exist. - Action: Adjust the include paths to point to the correct directories, or create them if intended.
| if (!result.success) { | ||
| console.error("❌ Invalid environment variables:", result.error.format()); | ||
| throw new Error("Invalid environment variables"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent potential secret leakage in logs.
Using console.error with result.error.format() might reveal sensitive environment variables (e.g., keys). Restrict logs to non-sensitive details:
- console.error("❌ Invalid environment variables:", result.error.format());
+ console.error("❌ Invalid environment variables. Please check the logs carefully for details.");Also applies to: 69-71
| /** | ||
| * Type guard to check if a value is a valid JSON object string | ||
| * @param item The value to check | ||
| * @returns True if the value is a valid JSON object string, false otherwise | ||
| */ | ||
| export function isJSON(item: unknown): item is string { | ||
| try { | ||
| JSON.parse(item as string); | ||
| return true; | ||
| } catch (e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix type guard implementation to properly validate JSON objects.
The current implementation has several issues:
- It returns true for any valid JSON (including arrays, numbers, strings)
- The type guard claims to return
item is stringbut should returnitem is ObjectType - It doesn't utilize the existing
isObjectfunction
/**
* Type guard to check if a value is a valid JSON object string
* @param item The value to check
* @returns True if the value is a valid JSON object string, false otherwise
*/
-export function isJSON(item: unknown): item is string {
+export function isJSON(item: unknown): item is ObjectType {
try {
- JSON.parse(item as string);
- return true;
+ const parsed = JSON.parse(item as string);
+ return isObject(parsed);
} catch (e) {
return false;
}
}📝 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.
| /** | |
| * Type guard to check if a value is a valid JSON object string | |
| * @param item The value to check | |
| * @returns True if the value is a valid JSON object string, false otherwise | |
| */ | |
| export function isJSON(item: unknown): item is string { | |
| try { | |
| JSON.parse(item as string); | |
| return true; | |
| } catch (e) { | |
| return false; | |
| } | |
| } | |
| /** | |
| * Type guard to check if a value is a valid JSON object string | |
| * @param item The value to check | |
| * @returns True if the value is a valid JSON object string, false otherwise | |
| */ | |
| export function isJSON(item: unknown): item is ObjectType { | |
| try { | |
| const parsed = JSON.parse(item as string); | |
| return isObject(parsed); | |
| } catch (e) { | |
| return false; | |
| } | |
| } |
| cids.push(...getMetadataCidsFromEvents(flattedEvents, { ...console })); | ||
| // Save checkpoint logic here (e.g., save cids or event data) | ||
| } | ||
| fs.writeFileSync("cids.txt", cids.join("\n")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations.
The file write operation lacks error handling and could fail silently.
- fs.writeFileSync("cids.txt", cids.join("\n"));
+ try {
+ fs.writeFileSync("cids.txt", cids.join("\n"));
+ } catch (error) {
+ console.error("Failed to write CIDs to file:", error);
+ process.exit(1);
+ }📝 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.
| fs.writeFileSync("cids.txt", cids.join("\n")); | |
| try { | |
| fs.writeFileSync("cids.txt", cids.join("\n")); | |
| } catch (error) { | |
| console.error("Failed to write CIDs to file:", error); | |
| process.exit(1); | |
| } |
| @@ -0,0 +1,9 @@ | |||
| DATABASE_URL=postgre://postgres:testing@localhost:5434/datalayer-postgres-db | |||
There was a problem hiding this comment.
Use placeholder for database credentials.
The database URL contains hardcoded credentials. For security, use placeholders in the example file:
-DATABASE_URL=postgre://postgres:testing@localhost:5434/datalayer-postgres-db
+DATABASE_URL=postgresql://${DB_USER}:${DB_PASSWORD}@${DB_HOST}:${DB_PORT}/${DB_NAME}📝 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.
| DATABASE_URL=postgre://postgres:testing@localhost:5434/datalayer-postgres-db | |
| DATABASE_URL=postgresql://${DB_USER}:${DB_PASSWORD}@${DB_HOST}:${DB_PORT}/${DB_NAME} |
scripts/bootstrap/.env.example
Outdated
| COINGECKO_API_KEY=CG-9B9jasdasdasd | ||
| COINGECKO_API_TYPE=pro |
There was a problem hiding this comment.
Remove API key from example file.
The example file contains what appears to be a real CoinGecko API key. Replace it with a placeholder:
-COINGECKO_API_KEY=CG-9B9jasdasdasd
+COINGECKO_API_KEY=your-api-key-here📝 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.
| COINGECKO_API_KEY=CG-9B9jasdasdasd | |
| COINGECKO_API_TYPE=pro | |
| COINGECKO_API_KEY=your-api-key-here | |
| COINGECKO_API_TYPE=pro |
| # Pricing source | ||
| PRICING_SOURCE=coingecko | ||
| # Coingecko API key | ||
| COINGECKO_API_KEY=CG-9B9jasdasdasd |
There was a problem hiding this comment.
Replace example API key with a placeholder.
The example Coingecko API key should use a placeholder value like your-api-key-here to avoid potential security concerns.
- COINGECKO_API_KEY=CG-9B9jasdasdasd
+ COINGECKO_API_KEY=your-api-key-here📝 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.
| COINGECKO_API_KEY=CG-9B9jasdasdasd | |
| COINGECKO_API_KEY=your-api-key-here |
| try { | ||
| const decoded = decodeDGApplicationData(event.params.data); | ||
| ids.add(decoded.metadata.pointer); | ||
| } catch (error) {} | ||
| try { | ||
| const decoded = decodeDVMDApplicationData(event.params.data); | ||
| ids.add(decoded.metadata.pointer); | ||
| } catch (error) {} | ||
| try { | ||
| const decoded = decodeDVMDExtendedApplicationData(event.params.data); | ||
| ids.add(decoded.metadata.pointer); | ||
| } catch (error) { | ||
| logger.warn("Failed to decode DVMD extended application data", { | ||
| error, | ||
| event, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
now im thinking, we could write a decode method that doesn't throw but instead returns [boolean, result | undefined] so we don't have this nested try/catch blocks. i think it would make it a bit cleaner
There was a problem hiding this comment.
i would leave it as it is, it doesn't bother imo
| expect(result).toEqual(["bafkreihrjyu5tney6wia2hmkertc74nzfpsgxw2epvnxm72bxj6ifnd4ku"]); | ||
| }); | ||
|
|
||
| it("should skip events without metadata or data", () => { |
There was a problem hiding this comment.
would be nice to add test cases when metadata is extracted from data using the decode functions
There was a problem hiding this comment.
yeaaa, i will pick some event from envio :)
| // Token, | ||
| // TokenCode, |
There was a problem hiding this comment.
| // Token, | |
| // TokenCode, |
| @@ -33,7 +33,7 @@ describe("EnvioIndexerClient", () => { | |||
| let testEvents: EnvioEvent[]; | |||
|
|
|||
| beforeEach(() => { | |||
| // Sample test data | |||
| // Sample test datåa | |||
There was a problem hiding this comment.
| // Sample test datåa |
| /** | ||
| * Factory class for creating pricing providers. | ||
| */ | ||
| export class MetadataProviderFactory { | ||
| /** | ||
| * Creates a pricing provider based on the provided configuration. | ||
| * @param options - The pricing configuration. | ||
| * @param deps - dependencies to inject into the pricing provider. | ||
| * @returns The created pricing provider. | ||
| * @throws {InvalidMetadataSource} if the pricing source is invalid. | ||
| * @throws {MissingDependencies} if the dependencies are missing. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * Factory class for creating pricing providers. | |
| */ | |
| export class MetadataProviderFactory { | |
| /** | |
| * Creates a pricing provider based on the provided configuration. | |
| * @param options - The pricing configuration. | |
| * @param deps - dependencies to inject into the pricing provider. | |
| * @returns The created pricing provider. | |
| * @throws {InvalidMetadataSource} if the pricing source is invalid. | |
| * @throws {MissingDependencies} if the dependencies are missing. | |
| */ | |
| /** | |
| * Factory class for creating metadata providers. | |
| */ | |
| export class MetadataProviderFactory { | |
| /** | |
| * Creates a metadata provider based on the provided configuration. | |
| * @param options - The metadata configuration. | |
| * @param deps - dependencies to inject into the metadata provider. | |
| * @returns The created metadata provider. | |
| * @throws {InvalidMetadataSource} if the metadata source is invalid. | |
| * @throws {MissingDependencies} if the dependencies are missing. | |
| */ |
| ...val, | ||
| }; | ||
| } | ||
| throw new Error("Invalid metadata source"); |
There was a problem hiding this comment.
Something to have in mind that could avoid throwing errors:
https://github.com/colinhacks/zod?tab=readme-ov-file#validating-during-transform
🤖 Linear
Closes GIT-XXX
Description
Checklist before requesting a review
Summary by CodeRabbit
New Features
Enhancements
Documentation