Refactor VCR #116
Refactor VCR #116
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several changes across various files, primarily focusing on the addition of new JSON metadata files for the Changes
Possibly related PRs
Poem
Warning Rate limit exceeded@fracek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (22)
examples/cli/apibara.config.ts (1)
8-12: Consider using a more secure and persistent location for the dev database.Using
/tmpdirectory for database storage in development:
- Could be cleared by system cleanup processes
- Might have permissions issues in some environments
- Could pose security risks in shared environments
Consider using a more appropriate location. Here's a suggested change:
dev: { runtimeConfig: { - databasePath: "/tmp/my-db.sqlite", + databasePath: "./data/dev.sqlite", }, },This change:
- Keeps the database with your project files
- Provides better persistence
- Can be easily added to
.gitignore- Follows common development practices
packages/indexer/src/vcr/replay.ts (3)
11-17: Add JSDoc documentation for the replay function.The function implementation looks good and is well-focused. However, adding documentation would help users understand its purpose and parameters better.
Consider adding documentation like this:
+/** + * Replays a recorded cassette through the indexer for testing purposes. + * @param vcrConfig - Configuration for the VCR functionality + * @param indexer - The indexer to test + * @param cassetteName - Name of the cassette file to replay + */ export async function replay<TFilter, TBlock, TTxnParams>(
Line range hint
19-41: Add file existence check before reading.The function should validate that the cassette file exists before attempting to read it.
Consider adding a file check:
export function loadCassette<TFilter, TBlock>( vcrConfig: VcrConfig, cassetteName: string, ): Client<TFilter, TBlock> { const filePath = path.join(vcrConfig.cassetteDir, `${cassetteName}.json`); + if (!fs.existsSync(filePath)) { + throw new Error(`Cassette file not found: ${filePath}`); + } + const data = fs.readFileSync(filePath, "utf8");
Line range hint
1-41: Good architectural direction with the VCR refactor.The simplification of the VCR module and removal of sink-related functionality aligns well with the PR objectives. The changes make the testing infrastructure more maintainable and focused.
Consider documenting these architectural decisions in the module's README or documentation to help future contributors understand the testing strategy.
packages/cli/src/core/build/types.ts (1)
Line range hint
8-45: Consider enhancing error handling and loggingThe
writeTypesfunction could benefit from:
- Try-catch blocks around file operations
- More detailed success message including the number of files written
Here's a suggested improvement:
export async function writeTypes(apibara: Apibara) { const typesDir = resolve(apibara.options.buildDir, "types"); + try { const config = [ // ... existing code ... ]; const buildFiles: { path: string; contents: string }[] = []; buildFiles.push({ path: join(typesDir, "apibara-config.d.ts"), contents: config.join("\n"), }); await Promise.all( buildFiles.map(async (file) => { const _file = resolve(apibara.options.buildDir, file.path); await fsp.mkdir(dirname(_file), { recursive: true }); await fsp.writeFile(_file, file.contents); }), ); - apibara.logger.success(`Types written to ${prettyPath(typesDir)}`); + apibara.logger.success( + `${buildFiles.length} type files written to ${prettyPath(typesDir)}` + ); + } catch (error) { + apibara.logger.error(`Failed to write types: ${error.message}`); + throw error; + } }packages/indexer/src/sink.ts (2)
40-43: Consider enhancing the documentationWhile the current documentation is clear, it would be helpful to add more context about:
- When to use this default sink
- Its role in testing scenarios
- Any limitations or considerations when using it
Example enhancement:
-/** A default sink that does nothing. */ +/** + * A default sink that does nothing. Useful for: + * - Testing scenarios where sink operations can be ignored + * - Providing a no-op implementation during development + * - Cases where data persistence is not required + */
45-56: Implementation looks good with minor enhancement opportunitiesThe function is well-typed and handles the basic error case, but consider these improvements:
- Make the error message more descriptive about when/why this might occur
- Add validation for the returned transaction object
export function useSink<TTxnParams>({ context, }: { context: IndexerContext<TTxnParams>; }) { if (!context.sinkTransaction) { - throw new Error("Transaction context doesn't exist!"); + throw new Error( + "Transaction context is missing. Ensure useSink is called within a valid transaction scope." + ); } + // Validate the transaction object + if (typeof context.sinkTransaction !== 'object') { + throw new Error("Invalid transaction context type"); + } return context.sinkTransaction; }packages/indexer/src/vcr/record.ts (2)
Line range hint
1-49: Consider enhancing the VCR infrastructure.The current implementation could benefit from additional features to make it more robust and maintainable:
- Add metadata to cassettes (timestamp, environment, API version)
- Support compression for large response data
- Make serialization format configurable
- Add cleanup mechanisms for old cassettes
This would improve maintainability and help manage storage requirements.
Would you like me to propose a detailed implementation for any of these enhancements?
Line range hint
13-26: Add JSDoc documentation for the record function.Since this is part of a testing infrastructure refactor, comprehensive documentation would help users understand the function's purpose and usage.
Add documentation like this:
+/** + * Records API responses to a cassette file for use in tests. + * @param vcrConfig - Configuration for the VCR system + * @param client - The API client to record responses from + * @param indexerArg - The indexer instance to use + * @param cassetteOptions - Options for the recording (name, cursors) + * @throws {Error} If filesystem operations fail + * @example + * ```typescript + * await record(vcrConfig, client, indexer, { + * name: 'test-cassette', + * startingCursor: '0', + * endingCursor: '100' + * }); + * ``` + */ export async function record<TFilter, TBlock, TTxnParams>(packages/indexer/src/testing/index.ts (2)
9-15: Consider simplifying the function structure.The function returns an object with a single method, which could be simplified by directly exporting the async function.
Consider this alternative structure:
-export function createVcr() { - return { - async run<TFilter, TBlock, TTxnParams>( - cassetteName: string, - indexerConfig: IndexerWithStreamConfig<TFilter, TBlock, TTxnParams>, - range: { fromBlock: bigint; toBlock: bigint }, - ) { +export async function runVcr<TFilter, TBlock, TTxnParams>( + cassetteName: string, + indexerConfig: IndexerWithStreamConfig<TFilter, TBlock, TTxnParams>, + range: { fromBlock: bigint; toBlock: bigint }, +) {
16-28: Make cassette directory configurable.The cassette directory is hardcoded to "cassettes", which might not be suitable for all test environments or configurations.
Consider making it configurable:
- const vcrConfig: VcrConfig = { - cassetteDir: "cassettes", - }; + const vcrConfig: VcrConfig = { + cassetteDir: process.env.VCR_CASSETTE_DIR ?? "cassettes", + };packages/indexer/src/sinks/sqlite.test.ts (1)
Line range hint
19-26: Consider enhancing test robustness and maintainability.While the test works correctly, consider these improvements:
- Parameterize the SQL queries to prevent potential SQL injection
- Extract the table schema to a constant for reusability
- Use
afterEachto ensure proper cleanup even if tests failHere's a suggested refactor:
+ const TEST_TABLE_SCHEMA = ` + CREATE TABLE IF NOT EXISTS test ( + data BLOB, + _cursor BIGINT + ) + `; describe("Run Test", () => { + let db: Database.Database; + + afterEach(() => { + db?.close(); + }); it("should store in sqlite db via sqlitesink", async () => { - const db = new Database(":memory:"); + db = new Database(":memory:"); - db.prepare("DROP TABLE IF EXISTS test;").run(); + db.prepare("DROP TABLE IF EXISTS ?").run("test"); - db.prepare( - ` - CREATE TABLE IF NOT EXISTS test ( - data BLOB, - _cursor BIGINT - );`, - ).run(); + db.prepare(TEST_TABLE_SCHEMA).run(); // ... rest of the test - db.close(); }); });packages/indexer/src/internal/testing.ts (4)
13-23: Consider making mock data generation more configurableThe function could be more flexible and reusable with the following improvements:
- Make the base value (5_000_000) configurable
- Allow customization of finality status
- Support more complex data structures for testing edge cases
Here's a suggested improvement:
-export function generateMockMessages(count = 10): MockStreamResponse[] { +export function generateMockMessages( + count = 10, + options?: { + baseOrderKey?: number; + finality?: string; + dataGenerator?: (index: number) => any; + } +): MockStreamResponse[] { + const { + baseOrderKey = 5_000_000, + finality = "accepted", + dataGenerator = (i) => `${baseOrderKey + i}`, + } = options ?? {}; return [...Array(count)].map((_, i) => ({ _tag: "data", data: { - cursor: { orderKey: BigInt(5_000_000 - 1) }, - finality: "accepted", - data: [{ data: `${5_000_000 + i}` }], - endCursor: { orderKey: BigInt(5_000_000 + i) }, + cursor: { orderKey: BigInt(baseOrderKey - 1) }, + finality, + data: [{ data: dataGenerator(i) }], + endCursor: { orderKey: BigInt(baseOrderKey + i) }, }, })); }
36-37: Consider making configuration values configurableThe streamUrl and finality values are currently hard-coded. Consider making these configurable through the override parameter for better testing flexibility.
- streamUrl: "https://sepolia.ethereum.a5a.ch", - finality: "accepted", + streamUrl: override?.streamUrl ?? "https://sepolia.ethereum.a5a.ch", + finality: override?.finality ?? "accepted",
71-73: Add JSDoc documentation for MockSink classConsider adding documentation to explain the purpose and usage patterns of this mock sink implementation.
+/** + * A mock implementation of the Sink class for testing purposes. + * Captures written data and cursors for verification in tests. + * + * @example + * ```typescript + * const sink = mockSink(); + * // Use in tests + * expect(sink.result).toHaveLength(1); + * ``` + */ export class MockSink extends Sink {
63-69: Enhance helper functions with proper typing and documentationThe helper functions would benefit from explicit return types and documentation.
+/** Creates a writer object for managing transaction data */ -const transactionHelper = (context: TxnContext) => { +const transactionHelper = (context: TxnContext): TxnParams['writer'] => { return { insert: (data: SinkData[]) => { context.buffer.push(...data); }, }; }; +/** Creates a new instance of MockSink for testing */ -export function mockSink() { +export function mockSink(): MockSink { return new MockSink(); }Also applies to: 105-107
packages/indexer/src/plugins/kv.ts (1)
107-113: Consider enhancing error handling.While the implementation is solid, consider these improvements:
- Make the error message more actionable by explaining how to resolve the issue
- Add separate checks for
ctxandctx.kvfor more precise error messagesexport function useKVStore(): UseKVStoreResult { const ctx = useIndexerContext(); - - if (!ctx?.kv) throw new Error("KV Plugin is not available in context!"); + + if (!ctx) { + throw new Error("Indexer context not found. Ensure useKVStore is called within an indexer context."); + } + + if (!ctx.kv) { + throw new Error("KV Plugin not found in context. Ensure the KV plugin is properly initialized using the kv() plugin factory."); + } return ctx.kv; }packages/indexer/src/sinks/drizzle/drizzle.test.ts (4)
Line range hint
25-27: Consider using environment variables for database configurationThe database connection currently uses hardcoded credentials. Consider using environment variables for better security and flexibility across different environments.
const client = new Client({ - connectionString: "postgres://postgres:postgres@localhost:5432/postgres", + connectionString: process.env.DATABASE_URL || "postgres://postgres:postgres@localhost:5432/postgres", });
Line range hint
171-173: Improve type safety by removing 'any' assertionsThe code uses multiple
anytype assertions with biome-ignore comments. Consider creating a proper type for the result that includes the_cursorfield.type TestTableRow = typeof testTable.$inferSelect & { _cursor: Int8Range; }; // Then use this type instead of any assertions: const result = await db.select().from(testTable).orderBy(asc(testTable.id)) as TestTableRow[]; expect(result[0]._cursor.range.upper).toBe(null);Also applies to: 176-177, 179-180
Line range hint
44-93: Consider extracting common test setup logicThe mock message generation and sink setup is duplicated across test cases. Consider extracting this into a helper function to improve maintainability.
function setupTestEnvironment(transformFn: TransformFn) { const client = new MockClient<MockFilter, MockBlock>((request, options) => { return generateMockMessages(5); }); const sink = drizzleSink({ database: db, tables: [testTable] }); const indexer = getMockIndexer({ sink, override: { transform: transformFn }, }); return { client, sink, indexer }; } // Usage in tests: it("should insert data", async () => { const { client, indexer } = setupTestEnvironment(async ({ context, endCursor, block: { data } }) => { const { db } = useSink({ context }); await db.insert(testTable).values([{ id: Number(endCursor?.orderKey), data }]); }); await run(client, indexer); // ... rest of the test });
Line range hint
156-170: Improve test data setup readabilityThe test data setup in the invalidate test uses a ts-ignore comment and could be more readable. Consider using a helper function to create test data with proper typing.
interface TestData { id: number; data: string; cursorRange: bigint | [bigint, bigint]; } function createTestData(data: TestData[]) { return data.map(({ id, data, cursorRange }) => ({ id, data, _cursor: getDrizzleCursor(cursorRange), })); } // Usage: await db.insert(testTable).values(createTestData([ { id: 1, data: "data1", cursorRange: [1n, 5n] }, { id: 2, data: "data2", cursorRange: [2n, 5n] }, // ... ]));examples/cli/indexers/2-starknet.indexer.ts (1)
14-15: Ensure correct data types in table creationThe
numbercolumn is defined asTEXT. If it's meant to store numerical values, consider usingINTEGERfor better performance and type safety.Apply this diff to adjust the data type:
- "CREATE TABLE IF NOT EXISTS test (number TEXT, hash TEXT, _cursor BIGINT)", + "CREATE TABLE IF NOT EXISTS test (number INTEGER, hash TEXT, _cursor BIGINT)",
🛑 Comments failed to post (10)
examples/cli/tsconfig.json (1)
15-16: 💡 Codebase verification
Remove
.apibara/typesfrom includes as it's not a valid types directoryThe
.apibara/typesdirectory is not part of the project's type generation system. Based on the codebase analysis:
- No type generation scripts or configurations reference this directory
- The CLI package manages its types in
distdirectory, as seen in its package.json- The example CLI project doesn't require this directory for type definitions
- All required types are provided by the workspace packages (
@apibara/*)Additionally, removing "dist" from excludes could cause issues as the CLI package and other workspace packages use it for compiled output. Consider keeping both the correct includes and excludes:
{ "include": ["."], "exclude": ["node_modules", "dist"] }🔗 Analysis chain
Verify .apibara/types directory setup
The addition of "./.apibara/types" to includes suggests type definitions are expected in this location. Please ensure:
- This directory is properly created/managed
- Consider documenting the purpose of these types
- Verify if removing "dist" from excludes won't cause issues with build output files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if .apibara/types exists and what it contains fd -t d .apibara # Look for type generation scripts or configuration rg -A 5 "generateTypes|typeGen|declaration" package.jsonLength of output: 77
Script:
#!/bin/bash # Check if .apibara directory exists anywhere in the codebase fd apibara # Look for any TypeScript configuration files fd tsconfig.json # Check package.json for any type-related configurations cat package.json # Look for any documentation about types fd README.md -x cat {}Length of output: 7235
Script:
#!/bin/bash # Check the CLI package specifically for type generation or build scripts cat packages/cli/package.json # Check the CLI's tsconfig.json for any type-related configurations cat packages/cli/tsconfig.json # Look for any type generation or build scripts in the example CLI directory cat examples/cli/package.json # Check for any apibara config files that might explain type generation cat examples/cli/apibara.config.tsLength of output: 4835
examples/cli/test/starknet.test.ts (2)
1-7: 🛠️ Refactor suggestion
Improve test setup and resource management
The current setup has several potential issues:
- The VCR instance is created globally, which could lead to state sharing between tests
- No cleanup of the database file is implemented
- The database path is hardcoded
Consider refactoring the setup as follows:
import { createVcr } from "@apibara/indexer/testing"; import { describe, expect, it } from "vitest"; +import { tmpdir } from "os"; +import { join } from "path"; +import { unlinkSync } from "fs"; import Database from "better-sqlite3"; import starknetIndexer from "../indexers/2-starknet.indexer"; -const vcr = createVcr(); +describe("Starknet indexer", () => { + let vcr; + let databasePath; + + beforeEach(() => { + vcr = createVcr(); + databasePath = join(tmpdir(), `test-${Date.now()}.sqlite`); + }); + + afterEach(() => { + try { + unlinkSync(databasePath); + } catch (error) { + // Ignore if file doesn't exist + } + });📝 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.import { createVcr } from "@apibara/indexer/testing"; import { describe, expect, it } from "vitest"; import { tmpdir } from "os"; import { join } from "path"; import { unlinkSync } from "fs"; import Database from "better-sqlite3"; import starknetIndexer from "../indexers/2-starknet.indexer"; describe("Starknet indexer", () => { let vcr; let databasePath; beforeEach(() => { vcr = createVcr(); databasePath = join(tmpdir(), `test-${Date.now()}.sqlite`); }); afterEach(() => { try { unlinkSync(databasePath); } catch (error) { // Ignore if file doesn't exist } });
9-23: 🛠️ Refactor suggestion
Enhance test implementation and error handling
The test implementation needs several improvements:
- Test description is too vague
- Database connection isn't properly closed
- Missing error handling
- Test seems incomplete (TODO comment)
Consider refactoring the test as follows:
-describe("Starknet indexer", () => { - it("should work", async () => { - const databasePath = "/tmp/my-db.sqlite"; - const indexer = starknetIndexer({ databasePath }); - await vcr.run("simple-test", indexer, { - fromBlock: 800_000n, - toBlock: 800_005n, - }); - - const database = new Database(databasePath); - const rows = database.prepare("SELECT * FROM test").all(); - // TODO: update indexer to insert data - expect(rows).toMatchInlineSnapshot("[]"); - }); +describe("Starknet indexer", () => { + it("should process blocks and store data in the database", async () => { + let database; + try { + const indexer = starknetIndexer({ databasePath }); + + // Validate indexer configuration + expect(indexer.config).toBeDefined(); + + await vcr.run("process-blocks", indexer, { + fromBlock: 800_000n, + toBlock: 800_005n, + }); + + database = new Database(databasePath); + const rows = database.prepare("SELECT * FROM test").all(); + expect(rows).toMatchInlineSnapshot("[]"); + } finally { + database?.close(); + } + });The TODO comment suggests incomplete implementation. Would you like assistance in implementing the data insertion test cases?
📝 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.describe("Starknet indexer", () => { it("should process blocks and store data in the database", async () => { let database; try { const indexer = starknetIndexer({ databasePath }); // Validate indexer configuration expect(indexer.config).toBeDefined(); await vcr.run("process-blocks", indexer, { fromBlock: 800_000n, toBlock: 800_005n, }); database = new Database(databasePath); const rows = database.prepare("SELECT * FROM test").all(); expect(rows).toMatchInlineSnapshot("[]"); } finally { database?.close(); } }); });packages/cli/src/core/build/types.ts (1)
13-13: 💡 Codebase verification
⚠️ Potential issueModule path change requires coordinated updates across the codebase
The verification reveals multiple issues that need to be addressed:
- Multiple files are still importing from "apibara/types"
- Only two references to the new "apibara/runtime-config" path exist
- The path mapping in tsconfig.json still references the old path
- Several core components and CLI commands depend on the old import path
Required updates:
- Update tsconfig.json path mappings to include the new module path
- Migrate all imports from "apibara/types" to "apibara/runtime-config" in:
- packages/cli/src/hooks/useRuntimeConfig.ts
- packages/cli/playground/indexers/starknet.indexer.ts
- packages/cli/src/types/virtual/indexers.d.ts
- Other files importing ApibaraRuntimeConfig
- Update examples and documentation to reflect the new import path
🔗 Analysis chain
Breaking Change: Module path has been updated
The change from
"apibara/types"to"apibara/runtime-config"better reflects the purpose of these types, but it's a breaking change that requires careful migration.Let's verify the impact of this change:
Let's gather more specific information about the usage of these imports to better understand the impact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to the old module path # and identify potential areas needing updates # Check for any remaining references to the old module path echo "Checking for references to old module path..." rg -l "apibara/types" # Look for potential type imports that might need updating echo "Checking for potential type imports..." ast-grep --pattern 'import { $_ } from "apibara/types"'Length of output: 1306
Script:
#!/bin/bash # Get detailed context of the references to see actual usage echo "Checking detailed usage of apibara/types..." rg "apibara/types" -B 2 -A 2 # Check if there are any references to the new module path echo -e "\nChecking references to new module path..." rg "apibara/runtime-config" # Check package.json and tsconfig references echo -e "\nChecking configuration files..." echo "package.json:" cat package.json | grep -A 5 -B 5 "apibara/types" echo -e "\ntsconfig.json:" cat packages/cli/tsconfig.json | grep -A 5 -B 5 "apibara/types"Length of output: 9565
packages/indexer/src/vcr/record.ts (1)
36-43:
⚠️ Potential issueAdd error handling and improve file operations safety.
The filesystem operations need additional safeguards:
- Error handling for filesystem operations
- File existence check before overwriting
- Path sanitization for cassette names
Consider applying these improvements:
async "run:after"() { const output: CassetteDataType<TFilter, TBlock> = { filter: indexer.options.filter, messages: messages, }; - await fs.mkdir(vcrConfig.cassetteDir, { recursive: true }); + try { + await fs.mkdir(vcrConfig.cassetteDir, { recursive: true }); + + const sanitizedName = path.basename(cassetteOptions.name).replace(/[^a-z0-9-_]/gi, '_'); + const filePath = path.join(vcrConfig.cassetteDir, `${sanitizedName}.json`); + + // Check if file exists + try { + await fs.access(filePath); + console.warn(`Warning: Overwriting existing cassette file: ${filePath}`); + } catch (err) { + // File doesn't exist, which is fine + } + + await fs.writeFile(filePath, serialize(output), { + mode: 0o644, // rw-r--r-- + flag: 'w' + }); + } catch (err) { + throw new Error(`Failed to save cassette: ${err.message}`); + }📝 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.await fs.mkdir(vcrConfig.cassetteDir, { recursive: true }); try { await fs.mkdir(vcrConfig.cassetteDir, { recursive: true }); const sanitizedName = path.basename(cassetteOptions.name).replace(/[^a-z0-9-_]/gi, '_'); const filePath = path.join(vcrConfig.cassetteDir, `${sanitizedName}.json`); // Check if file exists try { await fs.access(filePath); console.warn(`Warning: Overwriting existing cassette file: ${filePath}`); } catch (err) { // File doesn't exist, which is fine } await fs.writeFile(filePath, serialize(output), { mode: 0o644, // rw-r--r-- flag: 'w' }); } catch (err) { throw new Error(`Failed to save cassette: ${err.message}`); }packages/indexer/src/testing/index.ts (1)
30-44:
⚠️ Potential issueAdd error handling for client creation and network operations.
The code should handle potential errors that might occur during client creation and network operations.
Consider adding try-catch blocks and proper error handling:
if (!isCassetteAvailable(vcrConfig, cassetteName)) { if (ci.isCI) { throw new Error("Cannot record cassette in CI"); } - const client = createClient( - indexer.streamConfig, - indexer.options.streamUrl, - ); - await record(vcrConfig, client, indexer, cassetteOptions); + try { + const client = createClient( + indexer.streamConfig, + indexer.options.streamUrl, + ); + await record(vcrConfig, client, indexer, cassetteOptions); + } catch (error) { + throw new Error(`Failed to record cassette: ${error.message}`); + } } else { - await replay(vcrConfig, indexer, cassetteName); + try { + await replay(vcrConfig, indexer, cassetteName); + } catch (error) { + throw new Error(`Failed to replay cassette: ${error.message}`); + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const indexer = createIndexer(indexerConfig); if (!isCassetteAvailable(vcrConfig, cassetteName)) { if (ci.isCI) { throw new Error("Cannot record cassette in CI"); } try { const client = createClient( indexer.streamConfig, indexer.options.streamUrl, ); await record(vcrConfig, client, indexer, cassetteOptions); } catch (error) { throw new Error(`Failed to record cassette: ${error.message}`); } } else { try { await replay(vcrConfig, indexer, cassetteName); } catch (error) { throw new Error(`Failed to replay cassette: ${error.message}`); } }packages/indexer/src/internal/testing.ts (2)
39-41:
⚠️ Potential issueImplement the TODO in transform function
The transform function is currently empty with a TODO comment. This could lead to unexpected behavior in tests.
Would you like me to help implement a basic transform function or create a GitHub issue to track this task?
94-102:
⚠️ Potential issueImplement invalidate and finalize methods
Both methods are currently throwing "Not implemented" errors. This could cause tests to fail unexpectedly if these methods are called.
Consider implementing basic versions:
async invalidate(cursor?: Cursor) { - // TODO: Implement - throw new Error("Not implemented"); + this.result = this.result.filter( + (item) => !item.endCursor || item.endCursor.orderKey > cursor?.orderKey + ); } async finalize(cursor?: Cursor) { - // TODO: Implement - throw new Error("Not implemented"); + // No-op implementation for testing + return; }Committable suggestion skipped: line range outside the PR's diff.
examples/cli/indexers/2-starknet.indexer.ts (2)
10-12:
⚠️ Potential issueAvoid dropping the database table on each run
Dropping the
testtable every time the indexer starts will result in loss of all previously indexed data. Consider removing theDROP TABLE IF EXISTS teststatement to preserve existing data between runs.Apply this diff to remove the drop table command:
- database.exec("DROP TABLE IF EXISTS test");If this behavior is intended for a testing environment, ensure it's clearly documented.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const database = new Database(runtimeConfig.databasePath);
34-40: 🛠️ Refactor suggestion
Uncomment and verify the data insertion logic
The data insertion code within the
transformfunction is commented out, preventing events from being written to the database. If data persistence is intended, uncomment thewriter.insertblock and ensure the data aligns with the table schema.Apply this diff to uncomment the code:
- // writer.insert([{ - // number: header?.blockNumber.toString(), - // hash: header?.blockHash, - // }]) + writer.insert([{ + number: header?.blockNumber, + hash: header?.blockHash, + }]);Also, consider removing the optional chaining operator (
?) ifheaderis guaranteed to be defined.Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (13)
packages/indexer/src/vcr/replay.ts (2)
10-17: Consider adding JSDoc documentation.The function has been nicely simplified to focus on its core responsibility. However, adding JSDoc documentation would help users understand:
- The purpose of the replay functionality
- The expected parameters
- The function's behavior
Example documentation:
+/** + * Replays a recorded cassette for testing an indexer. + * @param vcrConfig - Configuration for the Virtual Cassette Recorder + * @param indexer - The indexer to test + * @param cassetteName - Name of the cassette file to replay + */ export async function replay<TFilter, TBlock, TTxnParams>(
Line range hint
19-41: Consider enhancing type safety for cassette data validation.While the function handles basic validation through
assert.deepStrictEqual, consider adding runtime type validation for the cassette data to catch potential JSON parsing issues early.Example enhancement:
function isCassetteData<TFilter, TBlock>( data: unknown ): data is CassetteDataType<TFilter, TBlock> { return ( typeof data === 'object' && data !== null && 'filter' in data && 'messages' in data && Array.isArray((data as any).messages) ); } // Use in loadCassette: const cassetteData = deserialize(data); if (!isCassetteData<TFilter, TBlock>(cassetteData)) { throw new Error('Invalid cassette data format'); }packages/cli/src/core/build/types.ts (2)
Line range hint
36-43: Consider enhancing error handling for file operations.The file operations could benefit from more robust error handling to gracefully handle potential issues like permission errors or disk space limitations.
Consider wrapping the file operations in a try-catch block:
await Promise.all( buildFiles.map(async (file) => { + try { const _file = resolve(apibara.options.buildDir, file.path); await fsp.mkdir(dirname(_file), { recursive: true }); await fsp.writeFile(_file, file.contents); + } catch (error) { + apibara.logger.error( + `Failed to write types to ${prettyPath(file.path)}: ${error.message}` + ); + throw error; + } }), );
Line range hint
15-24: Consider adding runtime config validation.The type generation relies on the structure of
apibara.options.runtimeConfig, but there's no validation to ensure the config matches expected schema.Consider adding validation before type generation:
+ const runtimeConfig = Object.fromEntries( + Object.entries(apibara.options.runtimeConfig), + ) as Record<string, JSValue>; + + // Validate runtime config structure + if (Object.keys(runtimeConfig).length === 0) { + apibara.logger.warn('Runtime config is empty'); + } + apibara.options.typescript.generateRuntimeConfigTypes ? generateTypes( - await resolveSchema( - Object.fromEntries( - Object.entries(apibara.options.runtimeConfig), - ) as Record<string, JSValue>, - ), + await resolveSchema(runtimeConfig),examples/cli/indexers/2-starknet.indexer.ts (1)
8-9: Add configuration validationConsider adding validation for the runtime configuration at the start of the function to ensure all required parameters are present and valid before proceeding with initialization.
Example implementation:
function validateConfig(config: ApibaraRuntimeConfig) { if (!config.databasePath) { throw new Error('Database path is required'); } // Add more validation as needed } export default function (runtimeConfig: ApibaraRuntimeConfig) { validateConfig(runtimeConfig); console.log("--> Starknet Indexer Runtime Config: ", runtimeConfig); // ... rest of the code }packages/indexer/src/testing/index.ts (3)
11-15: Add JSDoc documentation for the run methodConsider adding JSDoc documentation to describe the purpose of the method, its parameters, and return type. This would improve developer experience and code maintainability.
+/** + * Runs the indexer with VCR (Virtual Cassette Recorder) capabilities + * @param cassetteName - The name of the cassette file to record/replay + * @param indexerConfig - The indexer configuration with stream settings + * @param range - The block range to process {fromBlock, toBlock} + */ async run<TFilter, TBlock, TTxnParams>(
16-28: Make cassette directory configurableThe cassette directory is currently hardcoded to "cassettes". Consider making this configurable through the factory function or environment variables for better flexibility.
-export function createVcr() { +export function createVcr(config?: { cassetteDir?: string }) { return { async run<TFilter, TBlock, TTxnParams>( cassetteName: string, indexerConfig: IndexerWithStreamConfig<TFilter, TBlock, TTxnParams>, range: { fromBlock: bigint; toBlock: bigint }, ) { const vcrConfig: VcrConfig = { - cassetteDir: "cassettes", + cassetteDir: config?.cassetteDir ?? "cassettes", };
1-47: Consider architectural improvements for better testing flexibilityA few architectural suggestions for future iterations:
- Consider implementing a proper VCR interface to allow different storage backends
- Add support for partial recording/replaying of cassettes
- Consider adding cassette transformation capabilities for test data manipulation
Example interface structure:
interface VcrStorage { save(name: string, data: unknown): Promise<void>; load(name: string): Promise<unknown>; exists(name: string): Promise<boolean>; } interface VcrTransformer { transform(data: unknown): unknown; }packages/indexer/src/internal/testing.ts (4)
13-23: Consider enhancing mock data generation for better test coverageWhile the implementation is functional, consider these improvements:
- Add bounds checking for the count parameter to prevent BigInt overflow
- Consider parameterizing the finality status to test different scenarios
- Allow for more complex mock data structures to test edge cases
-export function generateMockMessages(count = 10): MockStreamResponse[] { +export function generateMockMessages( + count = 10, + options?: { + baseOrderKey?: bigint; + finality?: 'accepted' | 'pending'; + generateData?: (index: number) => string; + } +): MockStreamResponse[] { + if (count < 0 || count > 1000) { + throw new Error('Count must be between 0 and 1000'); + } + const { + baseOrderKey = BigInt(5_000_000), + finality = 'accepted', + generateData = (i) => `${Number(baseOrderKey) + i}`, + } = options ?? {}; return [...Array(count)].map((_, i) => ({ _tag: "data", data: { - cursor: { orderKey: BigInt(5_000_000 - 1) }, - finality: "accepted", - data: [{ data: `${5_000_000 + i}` }], - endCursor: { orderKey: BigInt(5_000_000 + i) }, + cursor: { orderKey: baseOrderKey - BigInt(1) }, + finality, + data: [{ data: generateData(i) }], + endCursor: { orderKey: baseOrderKey + BigInt(i) }, }, })); }
39-41: Implement the TODO in transform functionThe transform function is currently empty with a TODO comment. This could lead to unexpected behavior in tests.
Would you like me to help implement a basic transform function or create an issue to track this task?
94-102: Document unimplemented methods and their expected behaviorThe
invalidateandfinalizemethods are marked as TODO and throw errors. This should be documented in the class JSDoc to prevent confusion.export class MockSink extends Sink { + /** + * Mock implementation of the Sink class for testing purposes. + * Note: invalidate and finalize methods are not implemented in this mock. + * They will throw errors if called. + */
80-92: Consider adding parameter validation in transaction methodThe transaction method should validate its cursor parameters before proceeding.
async transaction( { cursor, endCursor, finality }: SinkCursorParams, cb: (params: TxnParams) => Promise<void>, ) { + if (!cursor || !endCursor) { + throw new Error('Cursor and endCursor are required'); + } const context: TxnContext = { buffer: [], };packages/indexer/src/plugins/kv.ts (1)
105-113: Good architectural direction with plugin-centric organization.The relocation of the KV store hook from a separate hooks directory directly into the plugin file aligns well with the VCR refactoring objectives. This organization:
- Improves code locality
- Makes dependencies more explicit
- Simplifies the plugin's API surface
Consider documenting this pattern for other plugin authors to follow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
change/@apibara-indexer-6b02cd7d-9389-44a1-9d26-667585103372.json(1 hunks)change/apibara-4f51164c-fe53-45f7-be69-3c9e0ea43b4c.json(1 hunks)examples/cli/apibara.config.ts(1 hunks)examples/cli/indexers/2-starknet.indexer.ts(2 hunks)examples/cli/package.json(1 hunks)examples/cli/test/starknet.test.ts(1 hunks)examples/cli/tsconfig.json(1 hunks)packages/cli/src/core/build/types.ts(2 hunks)packages/cli/src/types/config.ts(0 hunks)packages/indexer/package.json(1 hunks)packages/indexer/src/hooks/index.ts(0 hunks)packages/indexer/src/hooks/useKVStore.ts(0 hunks)packages/indexer/src/hooks/useSink.ts(0 hunks)packages/indexer/src/index.ts(0 hunks)packages/indexer/src/indexer.test.ts(3 hunks)packages/indexer/src/internal/testing.ts(1 hunks)packages/indexer/src/plugins/kv.ts(1 hunks)packages/indexer/src/plugins/persistence.test.ts(1 hunks)packages/indexer/src/sink.ts(2 hunks)packages/indexer/src/sinks/csv.test.ts(1 hunks)packages/indexer/src/sinks/drizzle/drizzle.test.ts(1 hunks)packages/indexer/src/sinks/sqlite.test.ts(1 hunks)packages/indexer/src/testing/helper.ts(0 hunks)packages/indexer/src/testing/index.ts(1 hunks)packages/indexer/src/testing/indexer.ts(0 hunks)packages/indexer/src/testing/setup.ts(0 hunks)packages/indexer/src/testing/vcr.ts(0 hunks)packages/indexer/src/vcr/record.ts(1 hunks)packages/indexer/src/vcr/replay.ts(2 hunks)
💤 Files with no reviewable changes (9)
- packages/cli/src/types/config.ts
- packages/indexer/src/hooks/index.ts
- packages/indexer/src/hooks/useKVStore.ts
- packages/indexer/src/hooks/useSink.ts
- packages/indexer/src/index.ts
- packages/indexer/src/testing/helper.ts
- packages/indexer/src/testing/indexer.ts
- packages/indexer/src/testing/setup.ts
- packages/indexer/src/testing/vcr.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- change/@apibara-indexer-6b02cd7d-9389-44a1-9d26-667585103372.json
- change/apibara-4f51164c-fe53-45f7-be69-3c9e0ea43b4c.json
- examples/cli/apibara.config.ts
- examples/cli/package.json
- examples/cli/test/starknet.test.ts
- examples/cli/tsconfig.json
- packages/indexer/package.json
- packages/indexer/src/indexer.test.ts
- packages/indexer/src/plugins/persistence.test.ts
- packages/indexer/src/sink.ts
- packages/indexer/src/sinks/csv.test.ts
- packages/indexer/src/sinks/drizzle/drizzle.test.ts
- packages/indexer/src/sinks/sqlite.test.ts
- packages/indexer/src/vcr/record.ts
🔇 Additional comments (12)
packages/indexer/src/vcr/replay.ts (2)
1-9: LGTM! Well-organized imports.
The imports are logically structured, starting with node built-ins followed by external dependencies, and the types are properly imported for TypeScript support.
31-37: Great improvement to error messaging and documentation!
The updated error message with the hint to delete and rerun the cassette provides better guidance to users. The added comments also clarify the filter array wrapping behavior.
packages/cli/src/core/build/types.ts (1)
13-13: LGTM: Module declaration change aligns with runtime configuration focus.
The change from "apibara/types" to "apibara/runtime-config" better reflects the specific purpose of these generated types.
examples/cli/indexers/2-starknet.indexer.ts (2)
1-2: LGTM: Import statements are correctly updated
The new imports align with the refactoring objectives, properly importing the necessary functions for database handling.
23-23: Verify sink configuration usage across the codebase
The change from plugins to sink looks correct, but let's ensure this pattern is consistently applied.
✅ Verification successful
Both sink and plugins configurations are valid
The codebase shows two different but valid indexer configurations:
examples/cli/indexers/2-starknet.indexer.tsuses the newsinkproperty with SQLiteexamples/indexer/src/indexer.tsuses bothsinkandpluginsproperties with Drizzle ORM
The presence of both configurations is correct because:
- The
sinkproperty is used to define the primary data sink (SQLite/Drizzle) - The empty
pluginsarray in the second file is optional and doesn't conflict with the sink configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other indexer configurations to ensure consistent sink usage
rg -l "defineIndexer.*\{" | xargs rg "plugins:|sink:"
Length of output: 199
Script:
#!/bin/bash
# Let's check the content of both files to understand the context better
echo "=== examples/cli/indexers/2-starknet.indexer.ts ==="
cat examples/cli/indexers/2-starknet.indexer.ts
echo -e "\n=== examples/indexer/src/indexer.ts ==="
cat examples/indexer/src/indexer.ts
Length of output: 3520
packages/indexer/src/testing/index.ts (2)
1-8: LGTM! Well-organized imports
The imports are properly organized and each import is utilized within the implementation.
9-11: LGTM! Clean factory function pattern
The factory function follows a clean pattern, returning an object with a single async method. This design allows for future extensibility while maintaining a clear API.
packages/indexer/src/internal/testing.ts (4)
1-12: LGTM! Well-organized imports
The imports are properly structured and typed, with clear separation between external and internal dependencies.
49-61: LGTM! Clear and purposeful type definitions
The types are well-structured and provide good type safety for the mock implementations.
63-69: LGTM! Clean transaction helper implementation
The helper function is well-implemented with proper context management.
105-107: LGTM! Clean factory function implementation
The factory function follows good practices and provides a clean way to create MockSink instances.
packages/indexer/src/plugins/kv.ts (1)
105-106: LGTM! Well-defined type alias.
The UseKVStoreResult type alias provides a clean and type-safe way to reference the KVStore instance type.
| const database = new Database(runtimeConfig.databasePath); | ||
|
|
||
| database.exec("DROP TABLE IF EXISTS test"); | ||
| database.exec( | ||
| "CREATE TABLE IF NOT EXISTS test (number TEXT, hash TEXT, _cursor BIGINT)", | ||
| ); |
There was a problem hiding this comment.
Consider improving database initialization safety and constraints
Several concerns with the current implementation:
- Dropping the table on every initialization will lead to data loss. Consider using migrations instead.
- Database operations lack error handling.
- The schema could benefit from constraints (e.g., NOT NULL, UNIQUE).
Consider this safer implementation:
- database.exec("DROP TABLE IF EXISTS test");
- database.exec(
- "CREATE TABLE IF NOT EXISTS test (number TEXT, hash TEXT, _cursor BIGINT)",
- );
+ try {
+ database.exec(`
+ CREATE TABLE IF NOT EXISTS test (
+ number TEXT NOT NULL,
+ hash TEXT NOT NULL,
+ _cursor BIGINT NOT NULL,
+ created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP
+ )
+ `);
+ } catch (error) {
+ console.error("Failed to initialize database:", error);
+ throw error;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const database = new Database(runtimeConfig.databasePath); | |
| database.exec("DROP TABLE IF EXISTS test"); | |
| database.exec( | |
| "CREATE TABLE IF NOT EXISTS test (number TEXT, hash TEXT, _cursor BIGINT)", | |
| ); | |
| const database = new Database(runtimeConfig.databasePath); | |
| try { | |
| database.exec(` | |
| CREATE TABLE IF NOT EXISTS test ( | |
| number TEXT NOT NULL, | |
| hash TEXT NOT NULL, | |
| _cursor BIGINT NOT NULL, | |
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP | |
| ) | |
| `); | |
| } catch (error) { | |
| console.error("Failed to initialize database:", error); | |
| throw error; | |
| } |
| const { writer } = useSink({ context }); | ||
|
|
||
| // writer.insert([{ | ||
| // number: header?.blockNumber.toString(), | ||
| // hash: header?.blockHash, | ||
| // }]) | ||
| }, |
There was a problem hiding this comment.
Uncomment and implement the writer.insert logic
The insertion logic is currently commented out, which means the indexer isn't storing any data. This needs to be implemented for the indexer to function properly.
Additionally, consider adding error handling for the insertion operation.
Here's the suggested implementation:
async transform({ block: { header }, context }) {
const { writer } = useSink({ context });
- // writer.insert([{
- // number: header?.blockNumber.toString(),
- // hash: header?.blockHash,
- // }])
+ try {
+ if (!header?.blockNumber || !header?.blockHash) {
+ throw new Error('Missing required block data');
+ }
+ await writer.insert([{
+ number: header.blockNumber.toString(),
+ hash: header.blockHash,
+ }]);
+ } catch (error) {
+ console.error('Failed to insert block data:', error);
+ throw error;
+ }
}📝 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 transform({ block: { header }, context }) { | |
| const { writer } = useSink({ context }); | |
| // writer.insert([{ | |
| // number: header?.blockNumber.toString(), | |
| // hash: header?.blockHash, | |
| // }]) | |
| async transform({ block: { header }, context }) { | |
| const { writer } = useSink({ context }); | |
| try { | |
| if (!header?.blockNumber || !header?.blockHash) { | |
| throw new Error('Missing required block data'); | |
| } | |
| await writer.insert([{ | |
| number: header.blockNumber.toString(), | |
| hash: header.blockHash, | |
| }]); | |
| } catch (error) { | |
| console.error('Failed to insert block data:', error); | |
| throw error; | |
| } |
| if (!isCassetteAvailable(vcrConfig, cassetteName)) { | ||
| if (ci.isCI) { | ||
| throw new Error("Cannot record cassette in CI"); | ||
| } | ||
|
|
||
| const client = createClient( | ||
| indexer.streamConfig, | ||
| indexer.options.streamUrl, | ||
| ); | ||
| await record(vcrConfig, client, indexer, cassetteOptions); | ||
| } else { | ||
| await replay(vcrConfig, indexer, cassetteName); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and type safety
The error handling could be improved in several ways:
- Add specific error types for different failure scenarios
- Include more descriptive error messages
- Add error handling for the client creation and recording process
+class VcrError extends Error {
+ constructor(message: string) {
+ super(message);
+ this.name = 'VcrError';
+ }
+}
if (!isCassetteAvailable(vcrConfig, cassetteName)) {
if (ci.isCI) {
- throw new Error("Cannot record cassette in CI");
+ throw new VcrError(
+ `Cannot record cassette '${cassetteName}' in CI environment`
+ );
}
const client = createClient(
indexer.streamConfig,
indexer.options.streamUrl,
);
- await record(vcrConfig, client, indexer, cassetteOptions);
+ try {
+ await record(vcrConfig, client, indexer, cassetteOptions);
+ } catch (error) {
+ throw new VcrError(
+ `Failed to record cassette '${cassetteName}': ${error.message}`
+ );
+ }
} else {
- await replay(vcrConfig, indexer, cassetteName);
+ try {
+ await replay(vcrConfig, indexer, cassetteName);
+ } catch (error) {
+ throw new VcrError(
+ `Failed to replay cassette '${cassetteName}': ${error.message}`
+ );
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!isCassetteAvailable(vcrConfig, cassetteName)) { | |
| if (ci.isCI) { | |
| throw new Error("Cannot record cassette in CI"); | |
| } | |
| const client = createClient( | |
| indexer.streamConfig, | |
| indexer.options.streamUrl, | |
| ); | |
| await record(vcrConfig, client, indexer, cassetteOptions); | |
| } else { | |
| await replay(vcrConfig, indexer, cassetteName); | |
| } | |
| class VcrError extends Error { | |
| constructor(message: string) { | |
| super(message); | |
| this.name = 'VcrError'; | |
| } | |
| } | |
| if (!isCassetteAvailable(vcrConfig, cassetteName)) { | |
| if (ci.isCI) { | |
| throw new VcrError( | |
| `Cannot record cassette '${cassetteName}' in CI environment` | |
| ); | |
| } | |
| const client = createClient( | |
| indexer.streamConfig, | |
| indexer.options.streamUrl, | |
| ); | |
| try { | |
| await record(vcrConfig, client, indexer, cassetteOptions); | |
| } catch (error) { | |
| throw new VcrError( | |
| `Failed to record cassette '${cassetteName}': ${error.message}` | |
| ); | |
| } | |
| } else { | |
| try { | |
| await replay(vcrConfig, indexer, cassetteName); | |
| } catch (error) { | |
| throw new VcrError( | |
| `Failed to replay cassette '${cassetteName}': ${error.message}` | |
| ); | |
| } | |
| } |
| export function getMockIndexer<TTxnParams>({ | ||
| plugins, | ||
| sink, | ||
| override, | ||
| }: { | ||
| plugins?: ReadonlyArray<IndexerPlugin<MockFilter, MockBlock, TTxnParams>>; | ||
| sink?: Sink<TTxnParams>; | ||
| override?: Partial<IndexerConfig<MockFilter, MockBlock, TTxnParams>>; | ||
| } = {}) { | ||
| return createIndexer( | ||
| defineIndexer(MockStream)({ | ||
| streamUrl: "https://sepolia.ethereum.a5a.ch", | ||
| finality: "accepted", | ||
| filter: {}, | ||
| async transform({ block: { data }, context }) { | ||
| // TODO | ||
| }, | ||
| sink, | ||
| plugins, | ||
| ...override, | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making streamUrl configurable
The streamUrl is hardcoded to "https://sepolia.ethereum.a5a.ch". Consider making it configurable through the override parameter for better flexibility across different environments.
return createIndexer(
defineIndexer(MockStream)({
- streamUrl: "https://sepolia.ethereum.a5a.ch",
+ streamUrl: override?.streamUrl ?? "https://sepolia.ethereum.a5a.ch",
finality: "accepted",
filter: {},📝 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 function getMockIndexer<TTxnParams>({ | |
| plugins, | |
| sink, | |
| override, | |
| }: { | |
| plugins?: ReadonlyArray<IndexerPlugin<MockFilter, MockBlock, TTxnParams>>; | |
| sink?: Sink<TTxnParams>; | |
| override?: Partial<IndexerConfig<MockFilter, MockBlock, TTxnParams>>; | |
| } = {}) { | |
| return createIndexer( | |
| defineIndexer(MockStream)({ | |
| streamUrl: "https://sepolia.ethereum.a5a.ch", | |
| finality: "accepted", | |
| filter: {}, | |
| async transform({ block: { data }, context }) { | |
| // TODO | |
| }, | |
| sink, | |
| plugins, | |
| ...override, | |
| }), | |
| ); | |
| } | |
| export function getMockIndexer<TTxnParams>({ | |
| plugins, | |
| sink, | |
| override, | |
| }: { | |
| plugins?: ReadonlyArray<IndexerPlugin<MockFilter, MockBlock, TTxnParams>>; | |
| sink?: Sink<TTxnParams>; | |
| override?: Partial<IndexerConfig<MockFilter, MockBlock, TTxnParams>>; | |
| } = {}) { | |
| return createIndexer( | |
| defineIndexer(MockStream)({ | |
| streamUrl: override?.streamUrl ?? "https://sepolia.ethereum.a5a.ch", | |
| finality: "accepted", | |
| filter: {}, | |
| async transform({ block: { data }, context }) { | |
| // TODO | |
| }, | |
| sink, | |
| plugins, | |
| ...override, | |
| }), | |
| ); | |
| } |
| export function useKVStore(): UseKVStoreResult { | ||
| const ctx = useIndexerContext(); | ||
|
|
||
| if (!ctx?.kv) throw new Error("KV Plugin is not available in context!"); | ||
|
|
||
| return ctx.kv; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and type safety in the useKVStore hook.
While the implementation is functional, consider these improvements:
- The error message should guide users on how to fix the issue
- The context existence check should be explicit
Consider this improved implementation:
export function useKVStore(): UseKVStoreResult {
const ctx = useIndexerContext();
- if (!ctx?.kv) throw new Error("KV Plugin is not available in context!");
+ if (!ctx) {
+ throw new Error("Indexer context is not available. Ensure useKVStore is called within an indexer context.");
+ }
+
+ if (!ctx.kv) {
+ throw new Error(
+ "KV Plugin is not available in context. Ensure the KV plugin is properly configured using the kv() function."
+ );
+ }
return ctx.kv;
}📝 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 function useKVStore(): UseKVStoreResult { | |
| const ctx = useIndexerContext(); | |
| if (!ctx?.kv) throw new Error("KV Plugin is not available in context!"); | |
| return ctx.kv; | |
| } | |
| export function useKVStore(): UseKVStoreResult { | |
| const ctx = useIndexerContext(); | |
| if (!ctx) { | |
| throw new Error("Indexer context is not available. Ensure useKVStore is called within an indexer context."); | |
| } | |
| if (!ctx.kv) { | |
| throw new Error( | |
| "KV Plugin is not available in context. Ensure the KV plugin is properly configured using the kv() function." | |
| ); | |
| } | |
| return ctx.kv; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
packages/cli/src/core/build/types.ts (2)
Line range hint
11-27: Consider enhancing type generation robustness and documentation.The type generation logic looks good, but consider these improvements:
- Add error handling for
resolveSchema- Add JSDoc comments to the generated types for better documentation
Here's a suggested improvement:
const config = [ "// Generated by apibara", + "/**", + " * @packageDocumentation", + " * Runtime configuration types for Apibara", + " */", ` declare module "apibara/types" {`, apibara.options.typescript.generateRuntimeConfigTypes - ? generateTypes( - await resolveSchema( - Object.fromEntries( - Object.entries(apibara.options.runtimeConfig), - ) as Record<string, JSValue>, - ), + ? await Promise.resolve() + .then(() => + resolveSchema( + Object.fromEntries( + Object.entries(apibara.options.runtimeConfig), + ) as Record<string, JSValue>, + ) + ) + .then((schema) => + generateTypes(schema, { + interfaceName: "ApibaraRuntimeConfig", + addExport: false, + addDefaults: false, + allowExtraKeys: false, + indentation: 2, + }) + ) + .catch((error) => { + throw new Error(`Failed to generate runtime config types: ${error.message}`); + }) - { - interfaceName: "ApibaraRuntimeConfig", - addExport: false, - addDefaults: false, - allowExtraKeys: false, - indentation: 2, - }, - ) : "",
Line range hint
34-43: Add error handling for file operations.The file writing logic should include proper error handling and file existence checks.
Here's a suggested improvement:
- await Promise.all( - buildFiles.map(async (file) => { - const _file = resolve(apibara.options.buildDir, file.path); - await fsp.mkdir(dirname(_file), { recursive: true }); - await fsp.writeFile(_file, file.contents); - }), - ); + try { + await Promise.all( + buildFiles.map(async (file) => { + const _file = resolve(apibara.options.buildDir, file.path); + const dir = dirname(_file); + + // Ensure directory exists + await fsp.mkdir(dir, { recursive: true }); + + // Check if file exists + const exists = await fsp + .access(_file) + .then(() => true) + .catch(() => false); + + if (exists) { + apibara.logger.warn(`Overwriting existing file: ${prettyPath(_file)}`); + } + + await fsp.writeFile(_file, file.contents); + }), + ); + + apibara.logger.success(`Types written to ${prettyPath(typesDir)}`); + } catch (error) { + throw new Error(`Failed to write type definitions: ${error.message}`); + } - apibara.logger.success(`Types written to ${prettyPath(typesDir)}`);examples/cli/indexers/2-starknet.indexer.ts (1)
34-40: Consider adding debug logging for e2e testingSince this indexer will be used for e2e testing, consider adding detailed debug logging to help troubleshoot test failures.
async transform({ block: { header }, context }) { const { writer } = useSink({ context }); + console.debug('Processing block:', { + blockNumber: header?.blockNumber, + blockHash: header?.blockHash + });packages/indexer/src/internal/testing.ts (4)
13-23: Consider enhancing mock data generationThe mock data generation could be improved in several ways:
- The orderKey calculation (
5_000_000 - 1and5_000_000 + i) lacks clear intent.- The data format (
data: \${5_000_000 + i}``) is oversimplified.Consider this enhancement:
export function generateMockMessages(count = 10): MockStreamResponse[] { + const BASE_ORDER_KEY = 5_000_000; return [...Array(count)].map((_, i) => ({ _tag: "data", data: { - cursor: { orderKey: BigInt(5_000_000 - 1) }, + cursor: { orderKey: BigInt(BASE_ORDER_KEY + i) }, finality: "accepted", - data: [{ data: `${5_000_000 + i}` }], + data: [{ + blockNumber: BASE_ORDER_KEY + i, + timestamp: Date.now(), + data: `mock_data_${i}` + }], - endCursor: { orderKey: BigInt(5_000_000 + i) }, + endCursor: { orderKey: BigInt(BASE_ORDER_KEY + i + 1) }, }, })); }
39-41: Implement the TODO in transform functionThe transform function is currently empty with a TODO comment. This could lead to unexpected behavior in tests.
Would you like me to help implement a basic transform function or create an issue to track this?
63-69: Add error handling to transactionHelperConsider adding validation for the input parameters and error handling for edge cases.
const transactionHelper = (context: TxnContext) => { + if (!context || !Array.isArray(context.buffer)) { + throw new Error('Invalid transaction context'); + } return { insert: (data: SinkData[]) => { + if (!Array.isArray(data)) { + throw new Error('Invalid data format'); + } context.buffer.push(...data); }, }; };
94-102: Implement invalidate and finalize methodsBoth methods are currently throwing "Not implemented" errors. This could cause issues in tests that rely on these methods.
Would you like me to help implement these methods or create issues to track their implementation?
packages/indexer/src/plugins/kv.ts (2)
107-113: Consider adding transaction state validation.The hook is used within transaction contexts, but there's no runtime validation to ensure it's being used correctly.
Consider adding transaction state tracking:
export class KVStore { + private _inTransaction = false; + beginTransaction() { + if (this._inTransaction) { + throw new Error('Transaction already in progress'); + } this._beginTxnQuery.run(); + this._inTransaction = true; } commitTransaction() { + if (!this._inTransaction) { + throw new Error('No transaction in progress'); + } this._commitTxnQuery.run(); + this._inTransaction = false; } get<T>(key: string): T { + if (!this._inTransaction) { + throw new Error('Operation must be performed within a transaction'); + } // ... rest of the method } }
Line range hint
114-162: Consider parameterizing SQL table name.The SQL statements use a hardcoded table name 'kvs'. Consider making it configurable to support multiple KV stores or different naming schemes.
Example approach:
const createTableTemplate = (tableName: string) => ` CREATE TABLE IF NOT EXISTS ${tableName} ( from_block INTEGER NOT NULL, to_block INTEGER, k TEXT NOT NULL, v BLOB NOT NULL, PRIMARY KEY (from_block, k) ); `; // Usage in constructor constructor( private _db: SqliteDatabase, private _finality: DataFinality, private _endCursor: Cursor, private _tableName: string = 'kvs', ) { // ... prepare statements using _tableName }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
change/@apibara-indexer-6b02cd7d-9389-44a1-9d26-667585103372.json(1 hunks)change/apibara-4f51164c-fe53-45f7-be69-3c9e0ea43b4c.json(1 hunks)examples/cli/apibara.config.ts(1 hunks)examples/cli/indexers/2-starknet.indexer.ts(2 hunks)examples/cli/package.json(1 hunks)examples/cli/test/starknet.test.ts(1 hunks)examples/cli/tsconfig.json(1 hunks)packages/cli/src/core/build/types.ts(1 hunks)packages/cli/src/types/config.ts(0 hunks)packages/indexer/package.json(1 hunks)packages/indexer/src/hooks/index.ts(0 hunks)packages/indexer/src/hooks/useKVStore.ts(0 hunks)packages/indexer/src/hooks/useSink.ts(0 hunks)packages/indexer/src/index.ts(0 hunks)packages/indexer/src/indexer.test.ts(3 hunks)packages/indexer/src/internal/testing.ts(1 hunks)packages/indexer/src/plugins/kv.ts(1 hunks)packages/indexer/src/plugins/persistence.test.ts(1 hunks)packages/indexer/src/sink.ts(2 hunks)packages/indexer/src/sinks/csv.test.ts(1 hunks)packages/indexer/src/sinks/drizzle/drizzle.test.ts(1 hunks)packages/indexer/src/sinks/sqlite.test.ts(1 hunks)packages/indexer/src/testing/helper.ts(0 hunks)packages/indexer/src/testing/index.ts(1 hunks)packages/indexer/src/testing/indexer.ts(0 hunks)packages/indexer/src/testing/setup.ts(0 hunks)packages/indexer/src/testing/vcr.ts(0 hunks)packages/indexer/src/vcr/record.ts(1 hunks)packages/indexer/src/vcr/replay.ts(2 hunks)
💤 Files with no reviewable changes (9)
- packages/cli/src/types/config.ts
- packages/indexer/src/hooks/index.ts
- packages/indexer/src/hooks/useKVStore.ts
- packages/indexer/src/hooks/useSink.ts
- packages/indexer/src/index.ts
- packages/indexer/src/testing/helper.ts
- packages/indexer/src/testing/indexer.ts
- packages/indexer/src/testing/setup.ts
- packages/indexer/src/testing/vcr.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- change/@apibara-indexer-6b02cd7d-9389-44a1-9d26-667585103372.json
- change/apibara-4f51164c-fe53-45f7-be69-3c9e0ea43b4c.json
- examples/cli/apibara.config.ts
- examples/cli/package.json
- examples/cli/test/starknet.test.ts
- examples/cli/tsconfig.json
- packages/indexer/package.json
- packages/indexer/src/indexer.test.ts
- packages/indexer/src/plugins/persistence.test.ts
- packages/indexer/src/sink.ts
- packages/indexer/src/sinks/csv.test.ts
- packages/indexer/src/sinks/drizzle/drizzle.test.ts
- packages/indexer/src/sinks/sqlite.test.ts
- packages/indexer/src/testing/index.ts
- packages/indexer/src/vcr/record.ts
- packages/indexer/src/vcr/replay.ts
🔇 Additional comments (11)
packages/cli/src/core/build/types.ts (1)
2-2: Verify the module import path.
The import from "apibara/types" seems inconsistent with the AI summary which mentions changing it to "apibara/runtime-config". Please verify if this import path needs to be updated.
examples/cli/indexers/2-starknet.indexer.ts (4)
1-2: LGTM! Import changes align with the sink-based architecture
The addition of useSink and sqlite imports reflects a good architectural decision to standardize data persistence through the sink pattern.
23-23: LGTM! Proper sink configuration
The sink configuration correctly uses the new sqlite sink pattern, which aligns with the refactoring objectives.
10-15:
Previous concerns about database initialization safety remain valid
The implementation still has the same issues regarding table dropping, lack of error handling, and missing constraints that were identified in the previous review.
34-40:
Previous concerns about implementing writer.insert logic remain valid
The insertion logic is still commented out, preventing the indexer from storing data. The previous review's recommendations for implementing proper error handling and data validation remain applicable.
packages/indexer/src/internal/testing.ts (4)
1-11: LGTM! Clean import organization
The imports are well-structured, properly typed, and follow best practices.
36-36: Consider making streamUrl configurable
The streamUrl is hardcoded to "https://sepolia.ethereum.a5a.ch". Consider making it configurable through the override parameter for better flexibility across different environments.
49-61: LGTM! Clear type definitions
The types are well-defined and serve their intended purpose.
105-107: LGTM! Clean factory function implementation
The factory function follows best practices and serves its purpose well.
packages/indexer/src/plugins/kv.ts (2)
105-105: LGTM! Clean type alias definition.
The type alias provides a clear and type-safe way to reference KVStore instances.
107-113: 🛠️ Refactor suggestion
Enhance error handling and type safety.
The current implementation could benefit from improved error handling and type safety.
Additionally, consider adding type assertions or type guards:
export function useKVStore(): UseKVStoreResult {
const ctx = useIndexerContext();
- if (!ctx?.kv) throw new Error("KV Plugin is not available in context!");
+ if (!ctx) {
+ throw new Error(
+ "Indexer context is not available. Ensure useKVStore is called within an indexer context."
+ );
+ }
+
+ if (!ctx.kv) {
+ throw new Error(
+ "KV Plugin is not available in context. Ensure the KV plugin is properly configured using the kv() function."
+ );
+ }
+
+ // Type assertion to ensure type safety
+ return ctx.kv as UseKVStoreResult;
}| write({ data, endCursor }: { data: SinkData[]; endCursor?: Cursor }) { | ||
| if (data.length === 0) return; | ||
|
|
||
| this.result.push({ data, endCursor }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation in write method
The write method should validate the input parameters.
write({ data, endCursor }: { data: SinkData[]; endCursor?: Cursor }) {
+ if (!Array.isArray(data)) {
+ throw new Error('Invalid data format');
+ }
if (data.length === 0) return;
this.result.push({ data, endCursor });
}📝 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.
| write({ data, endCursor }: { data: SinkData[]; endCursor?: Cursor }) { | |
| if (data.length === 0) return; | |
| this.result.push({ data, endCursor }); | |
| } | |
| write({ data, endCursor }: { data: SinkData[]; endCursor?: Cursor }) { | |
| if (!Array.isArray(data)) { | |
| throw new Error('Invalid data format'); | |
| } | |
| if (data.length === 0) return; | |
| this.result.push({ data, endCursor }); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/cli/src/core/build/types.ts (3)
Line range hint
12-14: Module declaration doesn't match the import pathThe module declaration creates types for "apibara/types" but according to the AI summary, this should be "apibara/runtime-config".
Apply this diff:
-declare module "apibara/types" {`, +declare module "apibara/runtime-config" {`,
Line range hint
15-27: Consider extracting type generation configurationThe type generation configuration is embedded within the template literal. Consider extracting it to a separate configuration object for better maintainability.
Example refactor:
+const typeGenConfig = { + interfaceName: "ApibaraRuntimeConfig", + addExport: false, + addDefaults: false, + allowExtraKeys: false, + indentation: 2, +}; apibara.options.typescript.generateRuntimeConfigTypes ? generateTypes( await resolveSchema( Object.fromEntries( Object.entries(apibara.options.runtimeConfig), ) as Record<string, JSValue>, ), - { - interfaceName: "ApibaraRuntimeConfig", - addExport: false, - addDefaults: false, - allowExtraKeys: false, - indentation: 2, - }, + typeGenConfig, ) : "",
Line range hint
39-46: Add error handling for file operationsThe file operations could fail for various reasons (permissions, disk space, etc.). Consider adding proper error handling.
Example implementation:
- await Promise.all( - buildFiles.map(async (file) => { - const _file = resolve(apibara.options.buildDir, file.path); - await fsp.mkdir(dirname(_file), { recursive: true }); - await fsp.writeFile(_file, file.contents); - }), - ); + try { + await Promise.all( + buildFiles.map(async (file) => { + const _file = resolve(apibara.options.buildDir, file.path); + await fsp.mkdir(dirname(_file), { recursive: true }); + await fsp.writeFile(_file, file.contents); + }), + ); + } catch (error) { + apibara.logger.error(`Failed to write types: ${error.message}`); + throw error; + }examples/cli/indexers/2-starknet.indexer.ts (1)
Line range hint
1-43: Consider enhancing the example with comprehensive testing patternsSince this is an example indexer that will be used for e2e testing:
- Consider adding comments explaining the testing strategy
- Add error handling examples for network issues, invalid data, etc.
- Include examples of common testing patterns (mocking, VCR usage)
This would make the example more valuable for users implementing their own indexers.
packages/indexer/src/plugins/kv.ts (1)
Line range hint
1-164: Consider architectural improvements for better maintainability and flexibility.A few suggestions to enhance the architecture:
- Consider extracting an interface for the KV store to allow different implementations (e.g., Redis, LevelDB).
- Document the serialization format and constraints for stored values.
- Consider making the transaction handling more abstract to support different storage backends.
Example interface:
interface IKVStore<T> { beginTransaction(): void; commitTransaction(): void; rollbackTransaction(): void; get<T>(key: string): T; put<T>(key: string, value: T): void; del(key: string): void; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
change/@apibara-indexer-6b02cd7d-9389-44a1-9d26-667585103372.json(1 hunks)change/apibara-4f51164c-fe53-45f7-be69-3c9e0ea43b4c.json(1 hunks)examples/cli/apibara.config.ts(1 hunks)examples/cli/indexers/2-starknet.indexer.ts(2 hunks)examples/cli/package.json(1 hunks)examples/cli/test/starknet.test.ts(1 hunks)examples/cli/tsconfig.json(1 hunks)packages/cli/src/core/build/types.ts(1 hunks)packages/cli/src/types/config.ts(0 hunks)packages/indexer/package.json(1 hunks)packages/indexer/src/hooks/index.ts(0 hunks)packages/indexer/src/hooks/useKVStore.ts(0 hunks)packages/indexer/src/hooks/useSink.ts(0 hunks)packages/indexer/src/index.ts(0 hunks)packages/indexer/src/indexer.test.ts(3 hunks)packages/indexer/src/internal/testing.ts(1 hunks)packages/indexer/src/plugins/kv.ts(1 hunks)packages/indexer/src/plugins/persistence.test.ts(1 hunks)packages/indexer/src/sink.ts(2 hunks)packages/indexer/src/sinks/csv.test.ts(1 hunks)packages/indexer/src/sinks/drizzle/drizzle.test.ts(1 hunks)packages/indexer/src/sinks/sqlite.test.ts(1 hunks)packages/indexer/src/testing/helper.ts(0 hunks)packages/indexer/src/testing/index.ts(1 hunks)packages/indexer/src/testing/indexer.ts(0 hunks)packages/indexer/src/testing/setup.ts(0 hunks)packages/indexer/src/testing/vcr.ts(0 hunks)packages/indexer/src/vcr/record.ts(1 hunks)packages/indexer/src/vcr/replay.ts(2 hunks)
💤 Files with no reviewable changes (9)
- packages/cli/src/types/config.ts
- packages/indexer/src/hooks/index.ts
- packages/indexer/src/hooks/useKVStore.ts
- packages/indexer/src/hooks/useSink.ts
- packages/indexer/src/index.ts
- packages/indexer/src/testing/helper.ts
- packages/indexer/src/testing/indexer.ts
- packages/indexer/src/testing/setup.ts
- packages/indexer/src/testing/vcr.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- change/@apibara-indexer-6b02cd7d-9389-44a1-9d26-667585103372.json
- change/apibara-4f51164c-fe53-45f7-be69-3c9e0ea43b4c.json
- examples/cli/apibara.config.ts
- examples/cli/package.json
- examples/cli/test/starknet.test.ts
- examples/cli/tsconfig.json
- packages/indexer/package.json
- packages/indexer/src/indexer.test.ts
- packages/indexer/src/internal/testing.ts
- packages/indexer/src/plugins/persistence.test.ts
- packages/indexer/src/sink.ts
- packages/indexer/src/sinks/csv.test.ts
- packages/indexer/src/sinks/drizzle/drizzle.test.ts
- packages/indexer/src/sinks/sqlite.test.ts
- packages/indexer/src/testing/index.ts
- packages/indexer/src/vcr/record.ts
- packages/indexer/src/vcr/replay.ts
🔇 Additional comments (7)
packages/cli/src/core/build/types.ts (1)
Line range hint 15-27: Verify the impact of runtime config type changes
The changes to runtime config type generation might affect existing consumers.
Let's check for existing usages:
✅ Verification successful
Let me analyze the runtime config type usage further to understand the potential impact of changes.
Runtime config type changes are safe to proceed
The verification shows that the runtime config type changes are safe because:
- The base type
ApibaraRuntimeConfigis defined asRecord<string, unknown>, which is a flexible type that allows any string keys with unknown values - The type is used primarily in indexer function signatures and the generated type will maintain backward compatibility as it allows the same flexibility
- Existing indexers are using the runtime config in a type-safe manner, accessing specific properties after type checking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for imports of runtime config types
rg -t ts -t js "from ['\"]apibara/(types|runtime-config)['\"]"
# Search for direct references to ApibaraRuntimeConfig
rg -t ts -t js "ApibaraRuntimeConfig"
Length of output: 3351
Script:
#!/bin/bash
# Check the actual implementation of runtime config type
ast-grep --pattern 'type ApibaraRuntimeConfig = $_'
# Check how the runtime config is being used in indexers
rg -A 5 -B 5 'runtimeConfig: ApibaraRuntimeConfig'
# Check the schema resolution implementation
ast-grep --pattern 'function resolveSchema($_) {
$$$
}'
Length of output: 3094
examples/cli/indexers/2-starknet.indexer.ts (4)
1-2: LGTM! Import changes align with persistent storage approach
The changes to import useSink and sqlite appropriately support the transition to persistent storage.
10-15: Consider improving database initialization safety and constraints
The previous concerns about database initialization safety remain valid:
- Dropping the table on initialization risks data loss
- Missing error handling for database operations
- Schema lacks important constraints
34-40: Uncomment and implement the writer.insert logic
The insertion logic remains commented out, preventing the indexer from storing data.
23-23: Verify sink configuration across different environments
The sink configuration looks good, but ensure it works consistently across different environments.
✅ Verification successful
Let me search for sink configurations in these files to complete the verification.
Let me try a broader search to find sink configurations.
Sink configuration is consistent with the codebase patterns
The verification shows that the sink configuration in examples/cli/indexers/2-starknet.indexer.ts follows the established patterns in the codebase. The codebase supports multiple sink types including:
- SQLite sink (as used in the file under review)
- Drizzle sink (for PostgreSQL)
- CSV sink
- Default sink
The configuration syntax sink: sqlite({ database, tableName: "test" }) is correct and matches the expected usage patterns shown in the sink implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other sink configurations that might need similar updates
# Search for other indexer configurations
rg -A 5 "defineIndexer\(" --type ts
Length of output: 2519
Script:
#!/bin/bash
# Search for sink configurations in the identified indexer files
rg -A 5 "sink:" examples/starknet-indexer/src/indexer.ts examples/indexer/src/indexer.ts examples/cli/indexers/1-evm.indexer.ts packages/cli/playground/indexers/starknet.indexer.ts packages/indexer/src/internal/testing.ts
Length of output: 221
Script:
#!/bin/bash
# Search for sink configurations with more context and broader pattern
rg -B 2 -A 5 "sink[: ]" --type ts
Length of output: 11505
packages/indexer/src/plugins/kv.ts (2)
105-106: LGTM! Type alias enhances type safety.
The UseKVStoreResult type alias provides proper type safety for KVStore instances and follows TypeScript best practices.
107-113: 🛠️ Refactor suggestion
Enhance error handling in the useKVStore hook.
The current implementation would benefit from more descriptive error messages and explicit context checks, as previously suggested.
Consider this improved implementation:
export function useKVStore(): UseKVStoreResult {
const ctx = useIndexerContext();
- if (!ctx?.kv) throw new Error("KV Plugin is not available in context!");
+ if (!ctx) {
+ throw new Error("Indexer context is not available. Ensure useKVStore is called within an indexer context.");
+ }
+
+ if (!ctx.kv) {
+ throw new Error(
+ "KV Plugin is not available in context. Ensure the KV plugin is properly configured using the kv() function."
+ );
+ }
return ctx.kv;
}| @@ -1,8 +1,6 @@ | |||
| import fsp from "node:fs/promises"; | |||
| import type { Apibara } from "apibara/types"; | |||
There was a problem hiding this comment.
Update import to match module declaration changes
Based on the AI summary, the module declaration was changed from "apibara/types" to "apibara/runtime-config", but the import statement still references "apibara/types".
Apply this diff to maintain consistency:
-import type { Apibara } from "apibara/types";
+import type { Apibara } from "apibara/runtime-config";📝 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.
| import type { Apibara } from "apibara/types"; | |
| import type { Apibara } from "apibara/runtime-config"; |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
packages/cli/src/core/build/types.ts (2)
Line range hint
33-42: Enhance error handling for file operationsThe file operations could benefit from proper error handling to gracefully handle potential issues like permission errors or disk space problems.
Consider wrapping the file operations in try-catch blocks:
await Promise.all( buildFiles.map(async (file) => { + try { const _file = resolve(apibara.options.buildDir, file.path); await fsp.mkdir(dirname(_file), { recursive: true }); await fsp.writeFile(_file, file.contents); + } catch (error) { + apibara.logger.error( + `Failed to write types to ${prettyPath(file.path)}: ${error.message}` + ); + throw error; + } }), );
Line range hint
13-26: Add runtime config validationConsider adding validation for the runtime configuration object before generating types to ensure type safety and prevent potential runtime errors.
Consider adding validation:
+ const runtimeConfig = Object.fromEntries( + Object.entries(apibara.options.runtimeConfig), + ) as Record<string, JSValue>; + + // Validate runtime config structure + if (Object.keys(runtimeConfig).length === 0) { + apibara.logger.warn('Runtime configuration is empty'); + } + apibara.options.typescript.generateRuntimeConfigTypes ? generateTypes( - await resolveSchema( - Object.fromEntries( - Object.entries(apibara.options.runtimeConfig), - ) as Record<string, JSValue>, - ), + await resolveSchema(runtimeConfig),packages/indexer/src/internal/testing.ts (2)
13-23: Consider making mock data more configurableThe function could be more flexible for different test scenarios.
Consider this improvement:
-export function generateMockMessages(count = 10): MockStreamResponse[] { +export function generateMockMessages( + count = 10, + options = { startBlock: 5_000_000, finality: "accepted" as const } +): MockStreamResponse[] { + if (count < 0) throw new Error("Count must be non-negative"); return [...Array(count)].map((_, i) => ({ _tag: "data", data: { - cursor: { orderKey: BigInt(5_000_000 - 1) }, + cursor: { orderKey: BigInt(options.startBlock - 1) }, finality: "accepted", - data: [{ data: `${5_000_000 + i}` }], + data: [{ data: `${options.startBlock + i}` }], - endCursor: { orderKey: BigInt(5_000_000 + i) }, + endCursor: { orderKey: BigInt(options.startBlock + i) }, }, })); }
39-41: Implement the TODO in transform functionThe transform function contains a TODO comment without implementation. This could affect test coverage and functionality.
Would you like me to help implement a basic transform function or create an issue to track this?
packages/indexer/src/plugins/kv.ts (1)
Line range hint
1-113: Consider architectural improvements for better maintainability.A few suggestions to enhance the codebase:
- Consider moving SQL statements to a separate file (e.g.,
kv.sql.ts) to improve maintainability and separation of concerns.- Consider implementing a transaction manager class to handle the transaction lifecycle, which could provide:
- Automatic rollback on errors using a try-finally pattern
- Transaction isolation level controls
- Nested transaction support
Would you like me to provide an example implementation of these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
change/@apibara-indexer-6b02cd7d-9389-44a1-9d26-667585103372.json(1 hunks)change/apibara-4f51164c-fe53-45f7-be69-3c9e0ea43b4c.json(1 hunks)examples/cli/apibara.config.ts(1 hunks)examples/cli/indexers/2-starknet.indexer.ts(2 hunks)examples/cli/package.json(1 hunks)examples/cli/test/starknet.test.ts(1 hunks)examples/cli/tsconfig.json(1 hunks)packages/cli/src/core/build/types.ts(1 hunks)packages/cli/src/types/config.ts(0 hunks)packages/indexer/package.json(1 hunks)packages/indexer/src/hooks/index.ts(0 hunks)packages/indexer/src/hooks/useKVStore.ts(0 hunks)packages/indexer/src/hooks/useSink.ts(0 hunks)packages/indexer/src/index.ts(0 hunks)packages/indexer/src/indexer.test.ts(3 hunks)packages/indexer/src/internal/testing.ts(1 hunks)packages/indexer/src/plugins/kv.ts(1 hunks)packages/indexer/src/plugins/persistence.test.ts(1 hunks)packages/indexer/src/sink.ts(2 hunks)packages/indexer/src/sinks/csv.test.ts(1 hunks)packages/indexer/src/sinks/drizzle/drizzle.test.ts(1 hunks)packages/indexer/src/sinks/sqlite.test.ts(1 hunks)packages/indexer/src/testing/helper.ts(0 hunks)packages/indexer/src/testing/index.ts(1 hunks)packages/indexer/src/testing/indexer.ts(0 hunks)packages/indexer/src/testing/setup.ts(0 hunks)packages/indexer/src/testing/vcr.ts(0 hunks)packages/indexer/src/vcr/record.ts(1 hunks)packages/indexer/src/vcr/replay.ts(2 hunks)
💤 Files with no reviewable changes (9)
- packages/cli/src/types/config.ts
- packages/indexer/src/hooks/index.ts
- packages/indexer/src/hooks/useKVStore.ts
- packages/indexer/src/hooks/useSink.ts
- packages/indexer/src/index.ts
- packages/indexer/src/testing/helper.ts
- packages/indexer/src/testing/indexer.ts
- packages/indexer/src/testing/setup.ts
- packages/indexer/src/testing/vcr.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- change/@apibara-indexer-6b02cd7d-9389-44a1-9d26-667585103372.json
- change/apibara-4f51164c-fe53-45f7-be69-3c9e0ea43b4c.json
- examples/cli/apibara.config.ts
- examples/cli/indexers/2-starknet.indexer.ts
- examples/cli/package.json
- examples/cli/test/starknet.test.ts
- examples/cli/tsconfig.json
- packages/indexer/package.json
- packages/indexer/src/indexer.test.ts
- packages/indexer/src/plugins/persistence.test.ts
- packages/indexer/src/sink.ts
- packages/indexer/src/sinks/csv.test.ts
- packages/indexer/src/sinks/drizzle/drizzle.test.ts
- packages/indexer/src/sinks/sqlite.test.ts
- packages/indexer/src/testing/index.ts
- packages/indexer/src/vcr/record.ts
- packages/indexer/src/vcr/replay.ts
🔇 Additional comments (8)
packages/cli/src/core/build/types.ts (2)
2-2: Update import to match module declaration changes
The import statement needs to be updated to reflect the module declaration change from "apibara/types" to "apibara/runtime-config".
3-3: LGTM!
The pathe import is correctly structured and includes necessary path manipulation utilities.
packages/indexer/src/internal/testing.ts (4)
1-12: LGTM! Well-organized imports
The imports are properly structured and all necessary types and components are correctly imported.
49-69: LGTM! Well-structured helper functions and types
The type definitions are clear and the helper functions are properly implemented with good separation of concerns.
Also applies to: 105-107
36-38: 🛠️ Refactor suggestion
Consider making streamUrl configurable
The streamUrl is hardcoded to "https://sepolia.ethereum.a5a.ch". Consider making it configurable through the override parameter for better flexibility across different environments.
74-78: 🛠️ Refactor suggestion
Add validation in write method
The write method should validate the input parameters.
packages/indexer/src/plugins/kv.ts (2)
105-105: LGTM! Clean type alias definition.
The type alias provides good type safety and improves code readability by creating a clear reference to the KVStore instance type.
107-113: 🛠️ Refactor suggestion
Enhance error handling in the useKVStore hook.
The current implementation would benefit from more descriptive error messages and explicit context checks, as previously suggested.
Consider implementing the previously suggested improvements:
export function useKVStore(): UseKVStoreResult {
const ctx = useIndexerContext();
- if (!ctx?.kv) throw new Error("KV Plugin is not available in context!");
+ if (!ctx) {
+ throw new Error("Indexer context is not available. Ensure useKVStore is called within an indexer context.");
+ }
+
+ if (!ctx.kv) {
+ throw new Error(
+ "KV Plugin is not available in context. Ensure the KV plugin is properly configured using the kv() function."
+ );
+ }
return ctx.kv;
}| async invalidate(cursor?: Cursor) { | ||
| // TODO: Implement | ||
| throw new Error("Not implemented"); | ||
| } | ||
|
|
||
| async finalize(cursor?: Cursor) { | ||
| // TODO: Implement | ||
| throw new Error("Not implemented"); | ||
| } |
There was a problem hiding this comment.
Implement invalidate and finalize methods
Both methods are currently throwing "Not implemented" errors. This could cause issues in tests that rely on these methods.
Consider implementing these methods or providing mock implementations that track their calls:
async invalidate(cursor?: Cursor) {
- // TODO: Implement
- throw new Error("Not implemented");
+ this.result = this.result.filter(
+ (item) => !item.endCursor || item.endCursor.orderKey > cursor?.orderKey
+ );
}
async finalize(cursor?: Cursor) {
- // TODO: Implement
- throw new Error("Not implemented");
+ // Track that finalize was called with this cursor
+ this.result.push({ data: [], endCursor: cursor });
}📝 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 invalidate(cursor?: Cursor) { | |
| // TODO: Implement | |
| throw new Error("Not implemented"); | |
| } | |
| async finalize(cursor?: Cursor) { | |
| // TODO: Implement | |
| throw new Error("Not implemented"); | |
| } | |
| async invalidate(cursor?: Cursor) { | |
| this.result = this.result.filter( | |
| (item) => !item.endCursor || item.endCursor.orderKey > cursor?.orderKey | |
| ); | |
| } | |
| async finalize(cursor?: Cursor) { | |
| // Track that finalize was called with this cursor | |
| this.result.push({ data: [], endCursor: cursor }); | |
| } |
| async transaction( | ||
| { cursor, endCursor, finality }: SinkCursorParams, | ||
| cb: (params: TxnParams) => Promise<void>, | ||
| ) { | ||
| const context: TxnContext = { | ||
| buffer: [], | ||
| }; | ||
|
|
||
| const writer = transactionHelper(context); | ||
|
|
||
| await cb({ writer }); | ||
| this.write({ data: context.buffer, endCursor }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to transaction method
The transaction method should handle potential errors from the callback.
Consider this improvement:
async transaction(
{ cursor, endCursor, finality }: SinkCursorParams,
cb: (params: TxnParams) => Promise<void>,
) {
const context: TxnContext = {
buffer: [],
};
const writer = transactionHelper(context);
- await cb({ writer });
- this.write({ data: context.buffer, endCursor });
+ try {
+ await cb({ writer });
+ this.write({ data: context.buffer, endCursor });
+ } catch (error) {
+ // Rollback by clearing the buffer
+ context.buffer = [];
+ throw error;
+ }
}📝 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 transaction( | |
| { cursor, endCursor, finality }: SinkCursorParams, | |
| cb: (params: TxnParams) => Promise<void>, | |
| ) { | |
| const context: TxnContext = { | |
| buffer: [], | |
| }; | |
| const writer = transactionHelper(context); | |
| await cb({ writer }); | |
| this.write({ data: context.buffer, endCursor }); | |
| } | |
| async transaction( | |
| { cursor, endCursor, finality }: SinkCursorParams, | |
| cb: (params: TxnParams) => Promise<void>, | |
| ) { | |
| const context: TxnContext = { | |
| buffer: [], | |
| }; | |
| const writer = transactionHelper(context); | |
| try { | |
| await cb({ writer }); | |
| this.write({ data: context.buffer, endCursor }); | |
| } catch (error) { | |
| // Rollback by clearing the buffer | |
| context.buffer = []; | |
| throw error; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/cli/src/core/build/types.ts (2)
Line range hint
11-13: Update module declaration in generated typesThe module declaration in the generated types should be updated to match the new module name.
Apply this diff:
declare module "apibara/types" { +declare module "apibara/runtime-config" {
Line range hint
32-35: Update type definition filename for consistencyConsider updating the filename to match the new module name for better consistency.
Apply this diff:
- path: join(typesDir, "apibara-config.d.ts"), + path: join(typesDir, "apibara-runtime-config.d.ts"),examples/cli/indexers/2-starknet.indexer.ts (1)
Test coverage needs improvement
The current test implementation is minimal and incomplete:
- The test case is a placeholder with no actual data verification (
expect(rows).toMatchInlineSnapshot("[]"))- The indexer's
transformfunction is commented out, making the test ineffective- No test cases for error scenarios or edge cases
- Missing verification of event filtering and data transformation logic
Required improvements:
- Implement the commented-out
transformfunction- Add test cases to verify event filtering configuration
- Include tests for data transformation and storage
- Add error handling test scenarios
🔗 Analysis chain
Line range hint
1-43: Verify test coverage for the indexer implementationLet's ensure that the indexer implementation is properly tested.
Let me gather more information about the test implementation to provide a comprehensive verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test files covering this indexer implementation # Look for test files related to this indexer echo "Looking for test files..." fd -e test.ts -e spec.ts starknet # Check for actual test cases in the found files echo "Checking test content..." rg -A 5 "describe|it\(" $(fd -e test.ts -e spec.ts starknet)Length of output: 734
Script:
#!/bin/bash # Check the complete test implementation echo "Complete test implementation:" cat examples/cli/test/starknet.test.ts # Check if there are any mock data or fixtures echo -e "\nLooking for test fixtures:" fd fixture starknetLength of output: 1014
packages/indexer/src/internal/testing.ts (3)
13-23: Consider adding input validation and configuration optionsThe function could be more robust with:
- Input validation for negative count values
- Configurable base value instead of hardcoded 5_000_000
-export function generateMockMessages(count = 10): MockStreamResponse[] { +export function generateMockMessages( + count = 10, + baseValue = 5_000_000 +): MockStreamResponse[] { + if (count < 0) { + throw new Error('Count must be non-negative'); + } return [...Array(count)].map((_, i) => ({ _tag: "data", data: { - cursor: { orderKey: BigInt(5_000_000 - 1) }, + cursor: { orderKey: BigInt(baseValue - 1) }, finality: "accepted", - data: [{ data: `${5_000_000 + i}` }], - endCursor: { orderKey: BigInt(5_000_000 + i) }, + data: [{ data: `${baseValue + i}` }], + endCursor: { orderKey: BigInt(baseValue + i) }, }, })); }
39-41: Implement the transform functionThe transform function contains a TODO comment without implementation. This could cause issues in tests that rely on block transformation.
Would you like me to help implement a basic transform function or create an issue to track this task?
49-61: Consider improving type definitions
MockRettype seems too specific with just a string data field. Consider making it generic or expanding it for broader use cases.TxnParams.writer.insertcould benefit from stronger typing for the data parameter.-export type MockRet = { - data: string; +export type MockRet<T = string> = { + data: T; }; type TxnParams = { writer: { - insert: (data: SinkData[]) => void; + insert: (data: ReadonlyArray<SinkData>) => void; }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
change/@apibara-indexer-6b02cd7d-9389-44a1-9d26-667585103372.json(1 hunks)change/apibara-4f51164c-fe53-45f7-be69-3c9e0ea43b4c.json(1 hunks)examples/cli/apibara.config.ts(1 hunks)examples/cli/indexers/2-starknet.indexer.ts(2 hunks)examples/cli/package.json(1 hunks)examples/cli/test/starknet.test.ts(1 hunks)examples/cli/tsconfig.json(1 hunks)packages/cli/src/core/build/types.ts(1 hunks)packages/cli/src/core/config/defaults.ts(0 hunks)packages/cli/src/types/config.ts(0 hunks)packages/indexer/package.json(1 hunks)packages/indexer/src/hooks/index.ts(0 hunks)packages/indexer/src/hooks/useKVStore.ts(0 hunks)packages/indexer/src/hooks/useSink.ts(0 hunks)packages/indexer/src/index.ts(0 hunks)packages/indexer/src/indexer.test.ts(3 hunks)packages/indexer/src/internal/testing.ts(1 hunks)packages/indexer/src/plugins/kv.ts(1 hunks)packages/indexer/src/plugins/persistence.test.ts(1 hunks)packages/indexer/src/sink.ts(2 hunks)packages/indexer/src/sinks/csv.test.ts(1 hunks)packages/indexer/src/sinks/drizzle/drizzle.test.ts(1 hunks)packages/indexer/src/sinks/sqlite.test.ts(1 hunks)packages/indexer/src/testing/helper.ts(0 hunks)packages/indexer/src/testing/index.ts(1 hunks)packages/indexer/src/testing/indexer.ts(0 hunks)packages/indexer/src/testing/setup.ts(0 hunks)packages/indexer/src/testing/vcr.ts(0 hunks)packages/indexer/src/vcr/record.ts(1 hunks)packages/indexer/src/vcr/replay.ts(2 hunks)
💤 Files with no reviewable changes (10)
- packages/cli/src/core/config/defaults.ts
- packages/cli/src/types/config.ts
- packages/indexer/src/hooks/index.ts
- packages/indexer/src/hooks/useKVStore.ts
- packages/indexer/src/hooks/useSink.ts
- packages/indexer/src/index.ts
- packages/indexer/src/testing/helper.ts
- packages/indexer/src/testing/indexer.ts
- packages/indexer/src/testing/setup.ts
- packages/indexer/src/testing/vcr.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- change/@apibara-indexer-6b02cd7d-9389-44a1-9d26-667585103372.json
- change/apibara-4f51164c-fe53-45f7-be69-3c9e0ea43b4c.json
- examples/cli/apibara.config.ts
- examples/cli/package.json
- examples/cli/test/starknet.test.ts
- examples/cli/tsconfig.json
- packages/indexer/package.json
- packages/indexer/src/indexer.test.ts
- packages/indexer/src/plugins/persistence.test.ts
- packages/indexer/src/sink.ts
- packages/indexer/src/sinks/csv.test.ts
- packages/indexer/src/sinks/drizzle/drizzle.test.ts
- packages/indexer/src/sinks/sqlite.test.ts
- packages/indexer/src/testing/index.ts
- packages/indexer/src/vcr/record.ts
- packages/indexer/src/vcr/replay.ts
🔇 Additional comments (17)
packages/cli/src/core/build/types.ts (2)
2-2:
Update import to match module declaration changes
The import statement needs to be updated to match the new module declaration.
Apply this diff to maintain consistency:
-import type { Apibara } from "apibara/types";
+import type { Apibara } from "apibara/runtime-config";Line range hint 1-45: Verify all imports are updated across the codebase
The module name change from "apibara/types" to "apibara/runtime-config" requires verification of all import statements across the codebase.
examples/cli/indexers/2-starknet.indexer.ts (5)
1-2: LGTM: Import statements are correctly structured
The new imports for useSink and sqlite are appropriate for the VCR refactoring.
10-15: Database initialization concerns remain unaddressed
The current implementation still has safety and robustness issues that were previously identified.
23-23: LGTM: Sink configuration is properly implemented
The sink configuration correctly uses the new sqlite implementation with the specified table name.
34-35: LGTM: Context and sink usage is correct
The transform function correctly uses the new context-based sink pattern.
37-40: Implement the commented-out insertion logic
The insertion logic is still commented out, preventing the indexer from storing data.
packages/indexer/src/internal/testing.ts (7)
1-12: LGTM! Well-organized imports
The imports are properly structured and typed, with clear separation between external and internal dependencies.
36-37: Consider making streamUrl configurable
The streamUrl is hardcoded to "https://sepolia.ethereum.a5a.ch". Consider making it configurable through the override parameter for better flexibility across different environments.
63-69: LGTM! Clean helper function implementation
The transactionHelper function is well-structured and maintains a clear single responsibility.
74-78: Add validation in write method
The write method should validate the input parameters.
80-92: Add error handling to transaction method
The transaction method should handle potential errors from the callback.
94-102: Implement invalidate and finalize methods
Both methods are currently throwing "Not implemented" errors. This could cause issues in tests that rely on these methods.
105-107: LGTM! Clean factory function
The mockSink factory function follows good practices and maintains simplicity.
packages/indexer/src/plugins/kv.ts (3)
105-105: LGTM! Type alias enhances type safety.
The UseKVStoreResult type alias provides clear typing for KVStore instances, improving type inference and developer experience.
107-113: Enhance error handling in the useKVStore hook.
The current implementation could benefit from more descriptive error messages and explicit type checking.
Consider implementing the previously suggested improvements:
export function useKVStore(): UseKVStoreResult {
const ctx = useIndexerContext();
- if (!ctx?.kv) throw new Error("KV Plugin is not available in context!");
+ if (!ctx) {
+ throw new Error("Indexer context is not available. Ensure useKVStore is called within an indexer context.");
+ }
+
+ if (!ctx.kv) {
+ throw new Error(
+ "KV Plugin is not available in context. Ensure the KV plugin is properly configured using the kv() function."
+ );
+ }
return ctx.kv;
}107-113: Verify plugin initialization before hook usage.
The hook assumes the KV plugin is initialized, but there's no guarantee of initialization order. Consider adding runtime checks or documentation about proper initialization sequence.
Let's check for potential initialization-related issues:
jaipaljadeja
left a comment
jaipaljadeja
left a comment
There was a problem hiding this comment.
This is so good. Thanks a lot!
2 participants
Refactor the testing and VCR module. Also start testing the example indexer to have e2e tests of the user workflow.