feat: implemented dynamic datasource removing themseleves and adding them at end blocks #2904
Conversation
stwiname
left a comment
stwiname
left a comment
There was a problem hiding this comment.
This is on the right track but it is flawed currently.
With dynamic datasources there can be multiple instances from the same template, the only differences being the start block and arguments. So there needs to be more information passed through to remove the correct datasource.
I think the best solution for this is to provide the ability to list dynamic datasources for a template. Then the user can select the appropriate one to remove by providing the index.
|
@stwiname just implemented listing of dynamic datasources and to remove it by index no |
| async destroyDynamicDatasource( | ||
| templateName: string, | ||
| currentBlockHeight: number, | ||
| index?: number, |
There was a problem hiding this comment.
Index should be required, it then simplifies this code and avoids users accidentally removing all dynamic datasources.
| await this.dynamicDsService.destroyDynamicDatasource(templateName, blockHeight, index); | ||
|
|
||
| // Mark datasources with this template for removal from current processing | ||
| filteredDataSources.forEach((fds) => { |
There was a problem hiding this comment.
The destroyed dynamic datasources should be removed from filteredDatasources, at this point it shouldn't be used anymore.
…ces should be removed from filteredDatasources
Gmin2
force-pushed
the
dynamic-datasource
branch
from
c580346 to
4971c3d
Compare
| .filter(({params}) => params.templateName === templateName && params.endBlock === undefined); | ||
|
|
||
| return matchingDatasources.map(({globalIndex, params}, index) => ({ | ||
| index, |
There was a problem hiding this comment.
Should this be the global index?
| const globalIndex = this._datasourceParams.findIndex( | ||
| (p) => p.templateName === dsInfo.templateName && p.startBlock === dsInfo.startBlock && p.endBlock === undefined | ||
| ); |
There was a problem hiding this comment.
Why not just have the index on DynamicDatasourceInfo then there's no need to filter by templateName, find the index, then find the global index.
| const dsParam = (this.dynamicDsService as any)._datasourceParams?.find( | ||
| (p: any) => | ||
| p.templateName === (fds as any).mapping?.file?.split('/').pop()?.replace('.js', '') || | ||
| (p.startBlock === (fds as any).startBlock && p.templateName === templateName) |
There was a problem hiding this comment.
I don't think this is sufficient, there could be multiple instances of a dynamic datasource with the same startBlock but different args.
| filteredDataSources.length = 0; | ||
| filteredDataSources.push(...updatedFilteredDataSources); |
There was a problem hiding this comment.
Why not just filteredDatasources = updatedFilteredDataSources?
WalkthroughAdds endBlock tracking and runtime destructor/getter APIs for dynamic datasources: new types and global VM bindings; DynamicDsService gains endBlock-aware creation, querying and destruction; worker host functions and indexer manager propagate blockHeight and re-filter datasources after destruction; tests expanded for endBlock and destructor behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Handler as Handler
participant VM as VM Sandbox
participant Manager as Indexer Manager
participant Service as DynamicDsService
participant Store as Metadata Store
Handler->>VM: destroyDynamicDatasource(templateName, index)
VM->>Manager: destroyDynamicDatasource(templateName, blockHeight, index)
Manager->>Service: destroyDynamicDatasource(templateName, blockHeight, index)
Service->>Service: getDatasourceParamByIndex(index)
alt datasource found & active
Service->>Service: set param.endBlock = blockHeight
Service->>Service: update in-memory _datasourceParams/_datasources
Service->>Store: persist updated params (metadata)
Service-->>Manager: success
Manager->>Manager: re-filter filteredDataSources (apply endBlock)
Manager-->>VM: ack
VM-->>Handler: continue (datasource excluded for future handlers)
else not found / already destroyed
Service-->>Manager: throw error
Manager-->>VM: propagate error
VM-->>Handler: error returned
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/node-core/src/indexer/indexer.manager.ts (1)
140-149: Consider a more robust comparison for args.The current implementation uses
JSON.stringifyto compare datasource arguments (lines 144-145), which can produce false negatives if object keys are in different orders. Additionally, the fallback chain for retrieving args fromprocessor.options || optionsis fragile and may not correctly locate args across different datasource types.Consider using a deep-equality utility (e.g., lodash's
isEqual) or normalizing the comparison by sorting object keys before stringification.Apply this approach:
+import {isEqual} from 'lodash'; // Filter out the destroyed datasource by matching startBlock and args // Note: Reassigning filteredDataSources is intentional - subsequent handlers // within the same block will see the updated filtered list filteredDataSources = filteredDataSources.filter((fds) => { const fdsStartBlock = (fds as any).startBlock; // For custom datasources, args are stored in processor.options // For runtime datasources, they may be stored differently - const fdsArgs = JSON.stringify((fds as any).processor?.options || (fds as any).options || {}); - const paramArgs = JSON.stringify(destroyedDsParam.args || {}); + const fdsArgs = (fds as any).processor?.options || (fds as any).options || {}; + const paramArgs = destroyedDsParam.args || {}; // Keep datasource if it doesn't match the destroyed one - return !(fdsStartBlock === destroyedDsParam.startBlock && fdsArgs === paramArgs); + return !(fdsStartBlock === destroyedDsParam.startBlock && isEqual(fdsArgs, paramArgs)); });packages/node-core/src/indexer/dynamic-ds.service.ts (1)
137-183: Consider type-safe endBlock assignment.The destruction logic is comprehensive with good validation. However, line 178 uses
as anyto assignendBlockto the datasource object, which bypasses TypeScript's type safety.If the datasource type doesn't include
endBlock, consider:
- Updating the base datasource types to include an optional
endBlockproperty, or- Documenting why the type assertion is necessary (e.g., if datasource types are dynamically extended at runtime)
This would improve type safety and make the code's intent clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/node-core/src/indexer/dynamic-ds.service.spec.ts(6 hunks)packages/node-core/src/indexer/dynamic-ds.service.ts(4 hunks)packages/node-core/src/indexer/indexer.manager.ts(2 hunks)packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts(2 hunks)packages/types-core/src/global.ts(2 hunks)packages/types-core/src/interfaces.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/types-core/src/global.ts (3)
packages/node-core/src/indexer/dynamic-ds.service.ts (2)
destroyDynamicDatasource(137-183)getDynamicDatasources(186-196)packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts (2)
destroyDynamicDatasource(40-42)getDynamicDatasources(44-46)packages/types-core/src/interfaces.ts (2)
DynamicDatasourceDestructor(5-5)DynamicDatasourceGetter(26-26)
packages/node-core/src/indexer/indexer.manager.ts (1)
packages/node-core/logger.js (1)
logger(5-5)
packages/node-core/src/indexer/dynamic-ds.service.ts (1)
packages/types-core/src/interfaces.ts (1)
DynamicDatasourceInfo(10-24)
packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts (2)
packages/node-core/src/indexer/dynamic-ds.service.ts (2)
DatasourceParams(18-23)IDynamicDsService(25-32)packages/types-core/src/interfaces.ts (1)
DynamicDatasourceInfo(10-24)
🔇 Additional comments (9)
packages/node-core/src/indexer/indexer.manager.ts (2)
92-92: LGTM: Correct mutability change.Changing from
consttoletis necessary to support the runtime filtering of destroyed datasources at line 140.
119-122: LGTM: Clean injection of getDynamicDatasources.The function properly delegates to the dynamic datasource service and follows the established pattern for VM injections.
packages/types-core/src/global.ts (1)
5-5: LGTM: Clean addition of global bindings.The new imports and global declarations properly expose the dynamic datasource destruction and querying APIs, aligning with the interfaces defined in
interfaces.ts.Also applies to: 15-16
packages/types-core/src/interfaces.ts (1)
5-26: LGTM: Well-designed public API types.The new type definitions are clear, well-documented, and provide a clean public API surface for dynamic datasource lifecycle management. The
DynamicDatasourceInfointerface appropriately includes the global index, making destruction straightforward.packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts (1)
6-21: LGTM: Proper worker-host bridge implementation.The new methods correctly extend the worker-host communication bridge for dynamic datasource destruction and template-based querying, maintaining consistency with the existing delegation pattern.
Also applies to: 40-50, 56-59
packages/node-core/src/indexer/dynamic-ds.service.ts (3)
98-122: LGTM: Clean template-based datasource querying.The implementation correctly filters datasources by template name and excludes destroyed ones. Using the global index in the returned
DynamicDatasourceInfoobjects is the right approach for enabling destruction by index.
124-135: LGTM: Simple and safe bounds-checked getter.The implementation provides straightforward access to datasource parameters by global index with appropriate bounds checking.
213-219: LGTM: Template construction properly includes endBlock.The signature update and implementation correctly handle the optional
endBlockparameter, ensuring it's properly propagated to constructed datasources.packages/node-core/src/indexer/dynamic-ds.service.spec.ts (1)
18-20: LGTM: Comprehensive test coverage for new functionality.The test suite thoroughly covers:
- Happy path: creation, destruction, and querying
- Error cases: bounds checking, already destroyed, template mismatches
- Edge cases: global indexing after multiple destructions, multiple templates, reset behavior
- Integration: metadata persistence and in-memory consistency
This meets the PR objective of achieving at least 80% test coverage on new code.
Also applies to: 27-27, 44-44, 74-438
| if (!this._datasourceParams || index < 0 || index >= this._datasourceParams.length) { | ||
| return undefined; | ||
| } | ||
| return this._datasourceParams[index]; |
There was a problem hiding this comment.
| if (!this._datasourceParams || index < 0 || index >= this._datasourceParams.length) { | |
| return undefined; | |
| } | |
| return this._datasourceParams[index]; | |
| return this._datasourceParams?.[index]; |
| // Validate the global index is within bounds | ||
| if (index < 0 || index >= this._datasourceParams.length) { | ||
| throw new Error( | ||
| `Index ${index} is out of bounds. There are ${this._datasourceParams.length} datasource(s) in total` | ||
| ); |
There was a problem hiding this comment.
I think this can be removed and instead check that dsParam below is not undefined.
| // For custom datasources, args are stored in processor.options | ||
| // For runtime datasources, they may be stored differently | ||
| const fdsArgs = JSON.stringify((fds as any).processor?.options || (fds as any).options || {}); | ||
| const paramArgs = JSON.stringify(destroyedDsParam.args || {}); |
There was a problem hiding this comment.
I think a better option might be to call filterDataSources again on the filtered datasources.
| // Note: Reassigning filteredDataSources is intentional - subsequent handlers | ||
| // within the same block will see the updated filtered list | ||
| filteredDataSources = filteredDataSources.filter((fds) => { | ||
| const fdsStartBlock = (fds as any).startBlock; |
There was a problem hiding this comment.
It would be good to remove this any cast
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/node-core/src/indexer/indexer.manager.ts (1)
119-123: Optional: return immutable copies from getDynamicDatasources.Handlers could accidentally mutate returned info objects. Consider returning Object.freeze()-d copies to harden the VM API. No behavior change required.
packages/node-core/src/indexer/dynamic-ds.service.spec.ts (1)
262-333: Add tests for identical startBlock with different args + param lookup.
- Add a case with two 'Test' datasources sharing startBlock but different args; destroy by global index should affect only the targeted one.
- Add a small unit for getDatasourceParamByIndex (valid index, OOB).
Example:
it('destroys by global index when startBlock collides but args differ', async () => { const a = {templateName:'Test', startBlock: 10, args:{id: 'A'}}; const b = {templateName:'Test', startBlock: 10, args:{id: 'B'}}; const meta = mockMetadata([a, b]); await service.init(meta); await service.destroyDynamicDatasource('Test', 20, 1); // destroy 'B' const params = (service as any)._datasourceParams; expect(params[0]).toEqual(a); expect(params[1]).toEqual({...b, endBlock: 20}); }); it('getDatasourceParamByIndex returns undefined when OOB', async () => { const meta = mockMetadata([testParam1]); await service.init(meta); expect(service.getDatasourceParamByIndex(5)).toBeUndefined(); expect(service.getDatasourceParamByIndex(0)).toEqual(testParam1); });packages/node-core/src/indexer/dynamic-ds.service.ts (1)
134-181: Add a guard to prevent destruction before startBlock.Defensive check avoids setting endBlock earlier than the datasource starts.
Apply this diff:
// Validate it matches the template name and is not already destroyed if (dsParam.templateName !== templateName) { throw new Error( `Datasource at index ${index} has template name "${dsParam.templateName}", not "${templateName}"` ); } if (dsParam.endBlock !== undefined) { throw new Error(`Dynamic datasource at index ${index} is already destroyed`); } + + // Prevent destroying before the datasource has started + if (currentBlockHeight < dsParam.startBlock) { + throw new Error( + `Cannot destroy datasource at index ${index} before its startBlock (${dsParam.startBlock})` + ); + }Optionally, expose a helper for the manager to remove items in-place without array scans:
+ /** Returns the DS object by global index (active or destroyed). */ + getDatasourceByIndex(index: number): DS | undefined { + return this._datasources?.[index]; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/node-core/src/indexer/dynamic-ds.service.spec.ts(6 hunks)packages/node-core/src/indexer/dynamic-ds.service.ts(4 hunks)packages/node-core/src/indexer/indexer.manager.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/node-core/src/indexer/dynamic-ds.service.ts (1)
packages/types-core/src/interfaces.ts (1)
DynamicDatasourceInfo(10-24)
🔇 Additional comments (6)
packages/node-core/src/indexer/indexer.manager.ts (1)
92-92: LGTM on making filteredDataSources mutable.Using let here is required for subsequent in-block updates.
packages/node-core/src/indexer/dynamic-ds.service.spec.ts (2)
18-20: LGTM: exposing getTemplate for tests.Keeps production visibility intact while enabling endBlock assertions.
74-87: Destroy lifecycle tests look solid.Good coverage of success paths, OOB, double-destroy, cross-template, and metadata sync.
Also applies to: 194-217, 334-437
packages/node-core/src/indexer/dynamic-ds.service.ts (3)
5-5: LGTM: types import and endBlock in params.endBlock on DatasourceParams and DynamicDatasourceInfo usage align with the new lifecycle.
Also applies to: 22-22
98-122: Getter by template with global indices looks good.Clear contract: returns active items and exposes global index for destruction.
210-217: LGTM: endBlock propagation into templates and ds objects.Ensures destroyed state is reflected in memory and persisted via updateDynamicDs.
Also applies to: 219-221
| // Inject function to destroy ds into vm | ||
| vm.freeze(async (templateName: string, index: number) => { | ||
| await this.dynamicDsService.destroyDynamicDatasource(templateName, blockHeight, index); | ||
|
|
||
| // Re-filter datasources to exclude the destroyed one | ||
| // The destroyed datasource now has endBlock set, so filterDataSources will exclude it | ||
| // Note: Reassigning filteredDataSources is intentional - subsequent handlers | ||
| // within the same block will see the updated filtered list | ||
| filteredDataSources = this.filterDataSources(blockHeight, filteredDataSources); | ||
| }, 'destroyDynamicDatasource'); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
In-block destroy isn’t effective: reassign doesn’t affect current iteration; filter logic won’t exclude endBlock === current block.
- Reassigning filteredDataSources won’t change the array that indexBlockData is iterating.
- filterDataSources includes ds where endBlock >= height, so a destroyed ds with endBlock === blockHeight will still be included for this block.
Fix by removing the destroyed DS from filteredDataSources in place using object identity.
Apply this diff:
vm.freeze(async (templateName: string, index: number) => {
await this.dynamicDsService.destroyDynamicDatasource(templateName, blockHeight, index);
- // Re-filter datasources to exclude the destroyed one
- // The destroyed datasource now has endBlock set, so filterDataSources will exclude it
- // Note: Reassigning filteredDataSources is intentional - subsequent handlers
- // within the same block will see the updated filtered list
- filteredDataSources = this.filterDataSources(blockHeight, filteredDataSources);
+ // Remove the destroyed DS from the current list IN PLACE so the rest of this block
+ // no longer schedules it. EndBlock handles exclusion from subsequent blocks.
+ const allDynamic = await this.dynamicDsService.getDynamicDatasources();
+ const destroyed = allDynamic[index] as DS | undefined;
+ if (destroyed) {
+ const pos = filteredDataSources.indexOf(destroyed);
+ if (pos !== -1) filteredDataSources.splice(pos, 1);
+ }
}, 'destroyDynamicDatasource');Run this script to confirm indexBlockData iterates the original array (so reassigning the variable is ineffective):
🏁 Script executed:
#!/bin/bash
# Look for indexBlockData implementations and how 'dataSources' is iterated
rg -nP -C3 '(?:async\s+)?indexBlockData\s*\(.*dataSources\s*:\s*[^\)]*\)' --type ts
rg -nP -C3 '\bfor\s*\(\s*const\s+\w+\s+of\s+dataSources\b|\bdataSources\.forEach\(' --type tsLength of output: 2595
I need to gather more information about the indexing flow. Let me search for the complete context.
I need to examine the indexBlockData implementation and the filterDataSources logic to verify the claims.
Let me search directly in the codebase for the relevant implementations.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find indexBlockData implementation with context
rg -nA20 'indexBlockData' packages/node-core/src/indexer/indexer.manager.ts --type ts
Length of output: 2063
🏁 Script executed:
#!/bin/bash
# Find filterDataSources implementation with endBlock logic
rg -nA15 'filterDataSources' packages/node-core/src/indexer/indexer.manager.ts --type tsLength of output: 2146
🏁 Script executed:
#!/bin/bash
# Get the full context around lines 100-160 in indexer.manager.ts
sed -n '100,160p' packages/node-core/src/indexer/indexer.manager.tsLength of output: 2380
Let me check for all implementations of indexBlockData to ensure the pattern is consistent:
🏁 Script executed:
#!/bin/bash
# Find all indexBlockData implementations
rg -nB2 -A10 'indexBlockData\s*\(' packages/ --type ts -g '!*.spec.ts' | grep -A10 'for\|forEach'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify when vm.freeze callbacks are executed - search for where handlers use injected functions
rg -nC5 'destroyDynamicDatasource|createDynamicDatasource' packages/ --type ts -g '!*.spec.ts' | head -60Length of output: 4941
The review comment is verified as correct and identifies a real bug.
Based on my analysis of the codebase:
-
Reassignment ineffectiveness confirmed: The
indexBlockDatamethod iterates overfilteredDataSourceswithfor...of(line 193). Reassigning the variable inside the callback doesn't affect the ongoing iteration—only in-place modifications (likepushat line 115) would work during iteration. -
Filter logic issue confirmed: The
filterDataSourcescondition at line 158 is(ds.endBlock ?? Number.MAX_SAFE_INTEGER) >= nextProcessingHeight. When a DS is destroyed withendBlock === blockHeight, this condition evaluates to true, so the destroyed DS is NOT excluded for the current block. -
Fix is sound: Using
splice()withindexOf()to remove the destroyed DS in-place would correctly remove it from the array during iteration, affecting subsequent handler executions in the same block.
🤖 Prompt for AI Agents
In packages/node-core/src/indexer/indexer.manager.ts around lines 124-134, the
current callback reassigns filteredDataSources after destroying a dynamic
datasource which doesn't affect an ongoing for...of iteration; replace that
reassignment with an in-place removal: locate the destroyed datasource in
filteredDataSources by matching templateName and index (or the datasource
identifier used elsewhere) and call filteredDataSources.splice(foundIndex, 1) so
subsequent handlers in the same block see it removed; do not reassign the
variable. Also verify and, if necessary, fix filterDataSources' condition so a
datasource with endBlock === blockHeight is excluded from current processing
(e.g., use > nextProcessingHeight or adjust the comparison) to avoid future
inconsistencies.
✅ Addressed in commit 1382702
There was a problem hiding this comment.
@Gmin2 it would be good to address this and to have a test that confirms the correct behaviour
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@Gmin2 this is still an issue, heres a simplified example.
Gmin2
force-pushed
the
dynamic-datasource
branch
from
2905c79 to
1382702
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/node-core/src/indexer/indexer.manager.ts (1)
124-143: Confirmed: The matching logic is redundant and fragile. Use object reference tracking instead.The review comment correctly identifies a real issue. The
DynamicDsServicemaintains synchronized_datasourcesand_datasourceParamsarrays indexed by global index. WhendestroyDynamicDatasource(templateName, blockHeight, index)is called, theindexparameter directly identifies the datasource viathis._datasourceParams[index].The current code at lines 132–138 unnecessarily re-matches datasources in
filteredDataSourcesusing property comparison (startBlock + serialized options/args), which:
- Is redundant: The index already identifies the exact datasource
- Fails with property mismatch: Compares
(fds as any).options || (fds as any).processor?.optionsagainstargs— these are distinct properties with unclear relationship- Is brittle: Multiple datasources could share the same
startBlockandargs; JSON.stringify is order-dependentSolution: Store the created datasource reference when adding to
filteredDataSources, then use it for removal:// Inject function to create ds into vm vm.freeze(async (templateName: string, args?: Record<string, unknown>) => { const newDs = await this.dynamicDsService.createDynamicDatasource({ templateName, args, startBlock: blockHeight, }); // Push the newly created dynamic ds to be processed this block on any future extrinsics/events filteredDataSources.push(newDs); dynamicDsCreated = true; }, 'createDynamicDatasource'); // Inject function to destroy ds into vm - vm.freeze(async (templateName: string, index: number) => { + const createdDsMap = new Map<number, DS>(); + vm.freeze(async (templateName: string, index: number) => { await this.dynamicDsService.destroyDynamicDatasource(templateName, blockHeight, index); - // Remove the destroyed datasource from the current processing array - // Find the datasource by matching the global index stored in the service - const destroyedDsParam = this.dynamicDsService.getDatasourceParamByIndex(index); - if (destroyedDsParam) { - const dsIndex = filteredDataSources.findIndex((fds) => { - return ( - fds.startBlock === destroyedDsParam.startBlock && - JSON.stringify((fds as any).options || (fds as any).processor?.options || {}) === - JSON.stringify(destroyedDsParam.args || {}) - ); - }); - if (dsIndex !== -1) { - filteredDataSources.splice(dsIndex, 1); - } - } + // Remove by reference (stored during creation) + const destroyed = createdDsMap.get(index); + if (destroyed) { + const pos = filteredDataSources.indexOf(destroyed); + if (pos !== -1) filteredDataSources.splice(pos, 1); + } }, 'destroyDynamicDatasource'); // Store reference mapping during creation - vm.freeze(async (templateName: string, args?: Record<string, unknown>) => { + const originalCreate = async (templateName: string, args?: Record<string, unknown>) => { const newDs = await this.dynamicDsService.createDynamicDatasource({ templateName, args, startBlock: blockHeight, }); filteredDataSources.push(newDs); + const currentIndex = this.dynamicDsService.dynamicDatasources.length - 1; + createdDsMap.set(currentIndex, newDs); dynamicDsCreated = true; - }, 'createDynamicDatasource'); + }; + vm.freeze(originalCreate, 'createDynamicDatasource');
🧹 Nitpick comments (1)
packages/node-core/src/indexer/dynamic-ds.service.ts (1)
18-23: Consider adding JSDoc for the endBlock field.While the implementation is correct, adding documentation would help users understand when and how
endBlockis set.export interface DatasourceParams { templateName: string; args?: Record<string, unknown>; startBlock: number; + /** Block height where this datasource stops processing (set when destroyed) */ endBlock?: number; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/node-core/src/indexer/dynamic-ds.service.spec.ts(6 hunks)packages/node-core/src/indexer/dynamic-ds.service.ts(4 hunks)packages/node-core/src/indexer/indexer.manager.ts(1 hunks)packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts(2 hunks)packages/types-core/src/global.ts(2 hunks)packages/types-core/src/interfaces.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/types-core/src/interfaces.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/node-core/src/indexer/dynamic-ds.service.ts (1)
packages/types-core/src/interfaces.ts (1)
DynamicDatasourceInfo(10-24)
packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts (2)
packages/node-core/src/indexer/dynamic-ds.service.ts (2)
DatasourceParams(18-23)IDynamicDsService(25-32)packages/types-core/src/interfaces.ts (1)
DynamicDatasourceInfo(10-24)
packages/types-core/src/global.ts (3)
packages/node-core/src/indexer/dynamic-ds.service.ts (2)
destroyDynamicDatasource(134-180)getDynamicDatasources(183-193)packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts (2)
destroyDynamicDatasource(40-42)getDynamicDatasources(44-46)packages/types-core/src/interfaces.ts (2)
DynamicDatasourceDestructor(5-5)DynamicDatasourceGetter(26-26)
🔇 Additional comments (13)
packages/node-core/src/indexer/indexer.manager.ts (1)
119-122: LGTM: Clean VM injection for template-based datasource queries.The function correctly delegates to the dynamic datasource service to retrieve datasources by template name.
packages/types-core/src/global.ts (2)
5-5: LGTM: Import statement updated correctly.Properly imports the new
DynamicDatasourceDestructorandDynamicDatasourceGettertypes.
15-16: LGTM: Global bindings for dynamic datasource lifecycle.The new global functions align with the destructor and getter interfaces defined in
interfaces.ts.packages/node-core/src/indexer/dynamic-ds.service.spec.ts (2)
18-20: LGTM: Test helper updated for endBlock support.The
getTemplatemethod now correctly accepts anendBlockparameter, aligning with the production implementation.
74-457: Excellent test coverage for dynamic datasource destruction.The test suite comprehensively covers:
- Basic destruction scenarios
- Error handling (non-existent, already-destroyed, out-of-bounds indices)
- Template name validation
- Global index tracking after destructions
- Multi-template scenarios
- EndBlock propagation and metadata persistence
- In-memory state synchronization
This achieves the ≥80% coverage requirement from issue #2099.
packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts (3)
6-21: LGTM: Worker-host interface extended correctly.The
HostDynamicDSinterface andhostDynamicDsKeysarray properly include the newdestroyDynamicDatasourceandgetDynamicDatasourcesByTemplatebindings.
40-50: LGTM: Worker methods delegate properly to host.Both
destroyDynamicDatasourceandgetDynamicDatasourcesByTemplatecorrectly forward calls to the host implementation.
56-58: LGTM: Host function bindings complete.The
dynamicDsHostFunctionscorrectly binds the new service methods for cross-thread communication.packages/node-core/src/indexer/dynamic-ds.service.ts (5)
106-122: LGTM: Clean implementation of template-based datasource queries.The method correctly:
- Filters by template name and active status (no endBlock)
- Preserves global indices for destruction calls
- Returns well-structured DynamicDatasourceInfo objects
130-132: LGTM: Simple and effective index-based lookup.The optional chaining correctly returns
undefinedfor out-of-bounds indices.
134-180: LGTM: Robust destruction implementation with excellent validation.The method includes comprehensive checks:
- Initialization state
- Index bounds with clear error messages
- Template name matching
- Already-destroyed prevention
- Array synchronization validation
The error messages are clear and actionable. The metadata persistence ensures durability across restarts.
210-217: LGTM: Template construction updated for endBlock support.The method correctly:
- Accepts optional
endBlockparameter- Spreads it into the cloned template
- Maintains the name removal to avoid pollution
219-229: LGTM: Datasource construction propagates endBlock correctly.The
endBlockis properly passed fromparamstogetTemplate, ensuring consistency throughout the datasource lifecycle.
| if (!this._datasources[index]) { | ||
| throw new Error(`Datasources array out of sync with params at index ${index}`); | ||
| } | ||
| (this._datasources[index] as any).endBlock = currentBlockHeight; |
There was a problem hiding this comment.
Can the any cast be removed here please
| // Inject function to destroy ds into vm | ||
| vm.freeze(async (templateName: string, index: number) => { | ||
| await this.dynamicDsService.destroyDynamicDatasource(templateName, blockHeight, index); | ||
|
|
||
| // Re-filter datasources to exclude the destroyed one | ||
| // The destroyed datasource now has endBlock set, so filterDataSources will exclude it | ||
| // Note: Reassigning filteredDataSources is intentional - subsequent handlers | ||
| // within the same block will see the updated filtered list | ||
| filteredDataSources = this.filterDataSources(blockHeight, filteredDataSources); | ||
| }, 'destroyDynamicDatasource'); | ||
|
|
There was a problem hiding this comment.
@Gmin2 this is still an issue, heres a simplified example.
| // Remove the destroyed datasource from the current processing array | ||
| // Find the datasource by matching the global index stored in the service | ||
| const destroyedDsParam = this.dynamicDsService.getDatasourceParamByIndex(index); | ||
| if (destroyedDsParam) { | ||
| const dsIndex = filteredDataSources.findIndex((fds) => { | ||
| return ( | ||
| fds.startBlock === destroyedDsParam.startBlock && | ||
| JSON.stringify((fds as any).options || (fds as any).processor?.options || {}) === | ||
| JSON.stringify(destroyedDsParam.args || {}) | ||
| ); | ||
| }); | ||
| if (dsIndex !== -1) { | ||
| filteredDataSources.splice(dsIndex, 1); | ||
| } |
There was a problem hiding this comment.
This can be moved into its own function.
| const dsIndex = filteredDataSources.findIndex((fds) => { | ||
| return ( | ||
| fds.startBlock === destroyedDsParam.startBlock && | ||
| JSON.stringify((fds as any).options || (fds as any).processor?.options || {}) === |
There was a problem hiding this comment.
This is quite fragile, there's no requirement that args maps to options or processor.options which would mean this fails. A better option would be to get the dynamic datasource rather than just the params and use that to compare.
The as any checks should also be removed.
Gmin2
force-pushed
the
dynamic-datasource
branch
from
d2a9845 to
87dca65
Compare
|
Take a look @stwiname |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/node-core/src/indexer/indexer.manager.ts (1)
124-129: Destroyed datasource still processes remaining handlers in the same block.Per the past review discussion with @stwiname, setting
endBlockalone doesn't prevent the destroyed datasource from processing remaining handlers within the same block. ThefilterDataSourcescondition at line 154 uses>= nextProcessingHeight, so a datasource withendBlock === blockHeightis still included.The comment indicates child classes should check
ds.endBlock, but this requires coordination across all implementations. Consider:
- Removing the destroyed DS in-place from
filteredDataSourcesusingsplice()after destruction, or- Adding a test that confirms and documents the intended behavior (that destruction takes effect on the next block, not immediately)
#!/bin/bash # Check if child implementations check endBlock before processing handlers rg -nC5 'indexBlockData|endBlock' packages/node/src/indexer/ packages/node-core/src/indexer/ --type ts -g '!*.spec.ts' | head -80
🧹 Nitpick comments (3)
packages/node-core/src/indexer/dynamic-ds.service.ts (1)
176-178: Consider addingendBlocktoBaseDataSourcetype for cleaner typing.The
Object.assignworkaround is functional but the comment acknowledges the type mismatch. IfBaseDataSourcein@subql/types-coredoesn't includeendBlock, consider extending the type or using a union type to avoid runtime property assignment that bypasses TypeScript's type system.For now, the implementation is correct - just a suggestion for future type safety improvements.
packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts (1)
42-56: Consider serialization consistency for cross-thread communication.
createDynamicDatasource(line 39) usesJSON.parse(JSON.stringify(params))for serialization over the worker bridge. The new methods don't apply the same pattern. While this may be fine if the worker thread uses a shared memory bridge or if the data is already serializable, consider whethergetDatasourceParamByIndexshould also ensure its return value is properly serialized.If the bridge handles serialization automatically, this is fine as-is.
packages/node/src/indexer/indexer.manager.ts (1)
201-202: AddendBlockproperty to type definition for better type safety.The type assertion
(ds as { endBlock?: number })at lines 201-202 bypasses TypeScript type checking. SinceendBlockis being dynamically set and accessed in this lifecycle management feature, consider addingendBlock?: numberto theSubstrateProjectDstype definition to maintain type safety throughout the codebase. Alternatively, use a type guard function instead of type assertion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/node-core/src/indexer/dynamic-ds.service.spec.ts(6 hunks)packages/node-core/src/indexer/dynamic-ds.service.ts(4 hunks)packages/node-core/src/indexer/indexer.manager.ts(1 hunks)packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts(2 hunks)packages/node/src/indexer/indexer.manager.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript throughout with strict configuration in the monorepo
Use Ethers.js for Ethereum integration
Files:
packages/node/src/indexer/indexer.manager.tspackages/node-core/src/indexer/indexer.manager.tspackages/node-core/src/indexer/dynamic-ds.service.spec.tspackages/node-core/src/indexer/dynamic-ds.service.tspackages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Run ESLint with TypeScript support, Prettier, and various plugins across all TypeScript files using configured husky pre-commit hooks and lint-staged
Files:
packages/node/src/indexer/indexer.manager.tspackages/node-core/src/indexer/indexer.manager.tspackages/node-core/src/indexer/dynamic-ds.service.spec.tspackages/node-core/src/indexer/dynamic-ds.service.tspackages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use custom Jest configuration with module name mapping for workspace packages, enforce UTC timezone for tests, and handle Polkadot packages in transform ignore patterns
Files:
packages/node-core/src/indexer/dynamic-ds.service.spec.ts
🧬 Code graph analysis (3)
packages/node/src/indexer/indexer.manager.ts (2)
packages/node/src/indexer/types.ts (1)
isFullBlock(31-35)packages/common-substrate/src/project/versioned/ProjectManifestVersioned.ts (1)
dataSources(54-56)
packages/node-core/src/indexer/dynamic-ds.service.spec.ts (1)
packages/node-core/src/indexer/dynamic-ds.service.ts (1)
DatasourceParams(18-23)
packages/node-core/src/indexer/dynamic-ds.service.ts (1)
packages/types-core/src/interfaces.ts (1)
DynamicDatasourceInfo(10-24)
🔇 Additional comments (13)
packages/node-core/src/indexer/indexer.manager.ts (1)
119-122: LGTM!The
getDynamicDatasourcesinjection follows the established pattern and correctly delegates to the service method.packages/node-core/src/indexer/dynamic-ds.service.spec.ts (5)
18-27: LGTM!The test helper correctly mirrors the updated
getTemplatesignature, andtestParamOtherenables thorough multi-template testing.
74-136: Good test coverage for destruction scenarios.The tests thoroughly verify:
- EndBlock is correctly set on both params and datasource objects
- Error handling for non-existent templates and already-destroyed datasources
- Lifecycle flow of destroy-then-create
150-168: LGTM!The test correctly verifies that
resetDynamicDatasourcepreserves theendBlockon already-destroyed datasources while removing those created after the target height.
263-333: Comprehensive test coverage forgetDynamicDatasourcesByTemplate.The tests properly validate:
- Global index preservation after destruction
- Destroyed datasource exclusion
- Args propagation
- Initialization requirements
335-458: Thorough test coverage for index-based destruction.The tests comprehensively cover:
- Boundary validation (out of bounds, negative indices)
- Global index semantics after partial destruction
- Cross-template independence
- Template name validation at specific indices
- EndBlock propagation to both params and datasource objects
packages/node-core/src/indexer/dynamic-ds.service.ts (4)
98-122: LGTM!Clean implementation that correctly filters active datasources by template and provides global indices for subsequent destruction operations.
124-132: LGTM!Concise implementation with proper optional chaining.
213-219: LGTM!Clean backward-compatible extension that includes
endBlockin the datasource creation.
222-224: LGTM!Correctly propagates
endBlockfrom params during datasource reconstruction.packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts (2)
9-23: LGTM!The host interface and keys correctly mirror the new
IDynamicDsServicemethods.
59-66: LGTM!Host functions correctly bind all new service methods.
packages/node/src/indexer/indexer.manager.ts (1)
203-204: Clarify the destruction timing semantics.The condition
endBlock <= blockHeightwill skip a datasource at the block where it's destroyed. The comment states "exclude it from processing in subsequent handlers within the same block," which aligns with this logic.However, please verify the intended behavior:
- When a datasource is destroyed during block N processing (e.g., in an early handler), should handlers that run LATER in the same block still process that datasource?
- The current implementation will skip it for all handlers after destruction in the same block, which appears intentional but should be confirmed.
If the datasource should remain active for handlers executed BEFORE the destruction call within the same block, the current approach is correct. If it should be excluded entirely from block N when destroyed at block N, this is also correct. Please confirm this is the desired behavior and consider adding a test case that validates destruction timing within a single block.
stwiname
left a comment
stwiname
left a comment
There was a problem hiding this comment.
I still don't see a test that addresses this case. #2904 (comment)
| // Skip datasources that have been destroyed at or before this block | ||
| // When a datasource is destroyed, its endBlock is set to the current blockHeight | ||
| // We want to exclude it from processing in subsequent handlers within the same block | ||
| const endBlock = | ||
| 'endBlock' in ds ? (ds as { endBlock?: number }).endBlock : undefined; | ||
| if (endBlock !== undefined && endBlock <= blockHeight) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This is a bad place to do this, the provided dataSources should already have the filtered dataSources to be relevant.
Having this here makes it really hard to find as it's separate from the rest of the filtering
There was a problem hiding this comment.
Now all filtering happens centrally in filterDataSources method in filterDataSources method and the VM callback calls it to reassign the filtered datasources.
…se for filtering that check it is correct
Gmin2
force-pushed
the
dynamic-datasource
branch
from
2d35591 to
5cf5af1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/node/src/indexer/indexer.manager.ts (1)
184-202: UnusedblockHeightparameter and misleading comment.The
blockHeightparameter is passed toindexContentbut never used within the returned function. The comment on line 197 states datasources are "removed from the array and the loop exits early" when destroyed, but the implementation shows a standard iteration without any early-exit logic or array modification handling.Based on the relevant code snippet from
packages/node-core/src/indexer/indexer.manager.ts, filtering of destroyed datasources happens infilterDataSourcesbefore datasources reach this manager. If that's the intended design:
- Remove the unused
blockHeightparameter fromindexContent- Update the comment to accurately reflect the behavior
private indexContent( kind: SubstrateHandlerKind, - blockHeight: number, ): ( content: | SubstrateBlock | SubstrateExtrinsic | SubstrateEvent | LightSubstrateEvent, dataSources: SubstrateProjectDs[], getVM: (d: SubstrateProjectDs) => Promise<IndexerSandbox>, ) => Promise<void> { return async (content, dataSources, getVM) => { - // When a datasource is destroyed, its removed from the array and the loop exits early for (let i = 0; i < dataSources.length; i++) { const ds = dataSources[i]; await this.indexData(kind, content, ds, getVM); } }; }packages/node-core/src/indexer/dynamic-ds.service.spec.ts (1)
459-494: Consider adding integration test for actual filtering path.This test simulates the
filterDataSourceslogic locally rather than exercising the actual production code path inindexer.manager.ts. While this validates the expected contract, an integration test using the realfilterDataSourcesmethod would provide stronger guarantees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/node-core/src/indexer/dynamic-ds.service.spec.ts(6 hunks)packages/node-core/src/indexer/dynamic-ds.service.ts(4 hunks)packages/node-core/src/indexer/indexer.manager.ts(3 hunks)packages/node/src/indexer/indexer.manager.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/node-core/src/indexer/indexer.manager.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript throughout with strict configuration in the monorepo
Use Ethers.js for Ethereum integration
Files:
packages/node/src/indexer/indexer.manager.tspackages/node-core/src/indexer/dynamic-ds.service.tspackages/node-core/src/indexer/dynamic-ds.service.spec.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Run ESLint with TypeScript support, Prettier, and various plugins across all TypeScript files using configured husky pre-commit hooks and lint-staged
Files:
packages/node/src/indexer/indexer.manager.tspackages/node-core/src/indexer/dynamic-ds.service.tspackages/node-core/src/indexer/dynamic-ds.service.spec.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use custom Jest configuration with module name mapping for workspace packages, enforce UTC timezone for tests, and handle Polkadot packages in transform ignore patterns
Files:
packages/node-core/src/indexer/dynamic-ds.service.spec.ts
🧬 Code graph analysis (2)
packages/node-core/src/indexer/dynamic-ds.service.ts (2)
packages/types-core/src/interfaces.ts (1)
DynamicDatasourceInfo(10-24)packages/node-core/logger.js (1)
logger(5-5)
packages/node-core/src/indexer/dynamic-ds.service.spec.ts (2)
packages/node-core/src/indexer/dynamic-ds.service.ts (1)
DatasourceParams(18-23)packages/node-core/src/indexer/indexer.manager.ts (1)
filterDataSources(148-168)
🔇 Additional comments (12)
packages/node/src/indexer/indexer.manager.ts (1)
100-101: Block height extraction is correct but may be unused.The extraction logic is correct. However, if the
blockHeightparameter is removed fromindexContentas suggested above, this variable and the comment become unnecessary.packages/node-core/src/indexer/dynamic-ds.service.spec.ts (6)
18-20: LGTM!The test helper correctly exposes
getTemplatewith the new optionalendBlockparameter, maintaining consistency with the production API changes.
23-28: LGTM!Test fixtures are well-structured with multiple templates supporting comprehensive cross-template destruction testing.
Also applies to: 41-49
74-135: LGTM!Thorough test coverage for destruction lifecycle including error cases and recovery scenarios. The tests correctly validate endBlock propagation to both
_datasourceParamsand_datasourcesinternal arrays.
150-168: LGTM!The reset test correctly verifies that destroyed datasources retain their
endBlockafter aresetDynamicDatasourcecall.
263-333: LGTM!Comprehensive coverage for
getDynamicDatasourcesByTemplateincluding global index tracking, destroyed datasource exclusion, and initialization checks.
495-527: Test pattern doesn't match actual indexer implementation.This test demonstrates array reassignment during iteration, but the actual
indexContentloop inpackages/node/src/indexer/indexer.manager.tsdoesn't implement this re-filtering pattern—it just iterates normally. Either update the indexer manager to match this pattern or adjust test expectations.packages/node-core/src/indexer/dynamic-ds.service.ts (5)
5-5: LGTM!Import correctly added to support the new
DynamicDatasourceInforeturn type.
18-32: LGTM!Interface properly extended with
endBlockfield and new method signatures. Theindexparameter is now required as suggested in past review comments.
98-122: LGTM!Clean implementation that correctly filters active datasources and provides global indices for destruction operations. The documentation clearly explains the
indexfield usage.
124-132: LGTM!Clean implementation using optional chaining as suggested in past review comments.
210-222: LGTM!
getTemplateandgetDatasourcecorrectly propagateendBlockthrough the template construction chain. The implementation aligns with the past review comment noting thatBaseDataSourceincludesendBlock.
| // Update the datasource params | ||
| const updatedParams = {...dsParam, endBlock: currentBlockHeight}; | ||
| this._datasourceParams[index] = updatedParams; | ||
|
|
||
| // Update the datasource object if it exists | ||
| // Note: _datasources and _datasourceParams arrays should always be in sync. | ||
| // If the index is valid for params, it must also be valid for datasources. | ||
| const datasource = this._datasources[index]; | ||
| if (!datasource) { | ||
| throw new Error(`Datasources array out of sync with params at index ${index}`); | ||
| } | ||
| // Set endBlock on the datasource object | ||
| datasource.endBlock = currentBlockHeight; | ||
|
|
||
| await this.metadata.set(METADATA_KEY, this._datasourceParams, tx); |
There was a problem hiding this comment.
In-memory state updated before persistence could cause inconsistency.
If metadata.set fails, the in-memory state (_datasourceParams[index] and _datasources[index].endBlock) is already modified, leaving the service in an inconsistent state. Compare with createDynamicDatasource which persists to metadata before updating in-memory state.
Consider reordering to persist first:
- // Update the datasource params
- const updatedParams = {...dsParam, endBlock: currentBlockHeight};
- this._datasourceParams[index] = updatedParams;
-
- // Update the datasource object if it exists
- const datasource = this._datasources[index];
- if (!datasource) {
- throw new Error(`Datasources array out of sync with params at index ${index}`);
- }
- // Set endBlock on the datasource object
- datasource.endBlock = currentBlockHeight;
-
- await this.metadata.set(METADATA_KEY, this._datasourceParams, tx);
+ // Prepare updated params
+ const updatedParams = {...dsParam, endBlock: currentBlockHeight};
+
+ // Create a copy for persistence
+ const paramsForPersistence = [...this._datasourceParams];
+ paramsForPersistence[index] = updatedParams;
+
+ // Persist first, then update in-memory on success
+ await this.metadata.set(METADATA_KEY, paramsForPersistence, tx);
+
+ // Update in-memory state only after successful persistence
+ this._datasourceParams[index] = updatedParams;
+
+ const datasource = this._datasources[index];
+ if (!datasource) {
+ throw new Error(`Datasources array out of sync with params at index ${index}`);
+ }
+ datasource.endBlock = currentBlockHeight;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
packages/node-core/src/indexer/dynamic-ds.service.ts around lines 165 to 179:
the code mutates in-memory state (_datasourceParams[index] and
_datasources[index].endBlock) before calling metadata.set, which can leave the
service inconsistent if persistence fails; instead, build the updated params
object/array but do NOT assign it into _datasourceParams or modify
datasource.endBlock until metadata.set(METADATA_KEY, updatedParamsArray, tx)
completes successfully; after metadata.set resolves, then assign
this._datasourceParams[index] = updatedParams and set
this._datasources[index].endBlock = currentBlockHeight (and keep throwing the
same error if datasource is missing before doing any in-memory mutation).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/node-core/src/indexer/dynamic-ds.service.spec.ts(6 hunks)packages/node-core/src/indexer/indexer.manager.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/node-core/src/indexer/indexer.manager.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript throughout with strict configuration in the monorepo
Use Ethers.js for Ethereum integration
Files:
packages/node-core/src/indexer/dynamic-ds.service.spec.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use custom Jest configuration with module name mapping for workspace packages, enforce UTC timezone for tests, and handle Polkadot packages in transform ignore patterns
Files:
packages/node-core/src/indexer/dynamic-ds.service.spec.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Run ESLint with TypeScript support, Prettier, and various plugins across all TypeScript files using configured husky pre-commit hooks and lint-staged
Files:
packages/node-core/src/indexer/dynamic-ds.service.spec.ts
🧬 Code graph analysis (1)
packages/node-core/src/indexer/dynamic-ds.service.spec.ts (2)
packages/node-core/src/indexer/dynamic-ds.service.ts (1)
DatasourceParams(18-23)packages/node-core/src/indexer/indexer.manager.ts (1)
filterDataSources(148-168)
🔇 Additional comments (7)
packages/node-core/src/indexer/dynamic-ds.service.spec.ts (7)
18-20: LGTM! Test infrastructure properly extended for endBlock feature.The
getTemplatesignature update and multi-template test setup align well with the PR objectives for dynamic datasource destruction with endBlock support.Also applies to: 27-27, 44-44
150-203: LGTM! Reset and loading tests properly validate endBlock persistence.These tests correctly verify that:
- Destroyed datasources with endBlock are preserved during resets
- endBlock is correctly loaded from metadata and reflected in datasource objects
205-237: LGTM! Metadata and template tests validate correct behavior.The tests properly verify:
- Metadata updates when destroying datasources
- Templates can be created with endBlock parameter
- Template immutability is preserved
239-260: LGTM! Multi-template and initialization tests cover important edge cases.These tests validate:
- Index-based destruction works correctly when multiple datasources share a template name
- Proper error handling when attempting operations on uninitialized service
262-332: LGTM! Comprehensive test coverage for getDynamicDatasourcesByTemplate.This test suite thoroughly validates:
- Retrieval of active datasources filtered by template name
- Exclusion of destroyed datasources
- Global index tracking
- Args propagation
- Error handling for uninitialized service
458-526: LGTM! Excellent integration tests for block processing scenarios.These tests thoroughly validate:
- Filtering datasources by endBlock during block processing (mimicking the actual
filterDataSourceslogic from indexer.manager.ts)- In-place datasource removal during block processing
- Traditional for-loop pattern with array reassignment after destruction
The inline filter function at lines 467-472 correctly mirrors the actual filtering logic, ensuring the destruction feature integrates properly with existing block processing.
438-456: Verify getDatasourceParamByIndex method exists.Line 446 calls
service.getDatasourceParamByIndex(1), but this method is not visible in the test file or the TestDynamicDsService class definition. Ensure this method exists in the DynamicDsService base class.
| it('allows creating new datasource after destroying existing one', async () => { | ||
| const meta = mockMetadata([testParam1]); | ||
| await service.init(meta); | ||
|
|
||
| expect((service as any)._datasourceParams).toEqual([testParam1]); | ||
|
|
||
| // Destroy by index | ||
| await service.destroyDynamicDatasource('Test', 50, 0); | ||
|
|
||
| const paramsAfterDestroy = (service as any)._datasourceParams; | ||
| expect(paramsAfterDestroy[0]).toEqual({...testParam1, endBlock: 50}); | ||
|
|
||
| const newParam = {templateName: 'Test', startBlock: 60}; | ||
| await service.createDynamicDatasource(newParam); | ||
|
|
||
| const finalParams: DatasourceParams[] = (service as any)._datasourceParams; | ||
| const destroyedCount = finalParams.filter((p: DatasourceParams) => p.endBlock !== undefined).length; | ||
| const activeCount = finalParams.filter((p: DatasourceParams) => p.endBlock === undefined).length; | ||
|
|
||
| expect(destroyedCount).toBeGreaterThanOrEqual(1); | ||
| expect(activeCount).toBeGreaterThanOrEqual(1); | ||
|
|
||
| const destroyedParam = finalParams.find((p: DatasourceParams) => p.startBlock === 1 && p.endBlock === 50); | ||
| expect(destroyedParam).toBeDefined(); | ||
|
|
||
| const newParamFound = finalParams.find((p: DatasourceParams) => p.startBlock === 60 && !p.endBlock); | ||
| expect(newParamFound).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strengthen count assertions for precision.
Lines 127-128 use toBeGreaterThanOrEqual(1), but after destroying one datasource and creating one new datasource, the counts should be exact: 1 destroyed and 1 active. Weak assertions may miss bugs where extra datasources are inadvertently created or destroyed.
Apply this diff:
- expect(destroyedCount).toBeGreaterThanOrEqual(1);
- expect(activeCount).toBeGreaterThanOrEqual(1);
+ expect(destroyedCount).toBe(1);
+ expect(activeCount).toBe(1);🤖 Prompt for AI Agents
In packages/node-core/src/indexer/dynamic-ds.service.spec.ts around lines 108 to
135, the test uses weak assertions (toBeGreaterThanOrEqual(1)) for
destroyedCount and activeCount which should be exact after destroying one
datasource and creating one new one; change those two assertions to
expect(destroyedCount).toBe(1) and expect(activeCount).toBe(1) so the test
verifies exactly one destroyed and one active datasource, preventing unnoticed
extra creations or destructions.
Added two test case in 5cf5af1 to check the filtering logic or am I missing something really obivious ones |
2 participants
Description
Implemented dynamic datasource removing themseleves and adding them at end blocks
Fixes #2099
Type of change
Please delete options that are not relevant.
Checklist
Summary by CodeRabbit
New Features
Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.