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 dynamic datasource destruction and retrieval by template: datasources gain optional endBlock; services, indexer manager, worker host and VM bindings expose destroy/get operations by index/template; tests expanded to cover destruction, endBlock propagation, and in-memory/persisted metadata consistency. 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 DB as Metadata Store
Handler->>VM: destroyDynamicDatasource(templateName, index)
VM->>Manager: destroyDynamicDatasource(templateName, index)
Manager->>Service: destroyDynamicDatasource(templateName, currentBlockHeight, index)
Service->>Service: getDatasourceParamByIndex(index)
alt exists & active
Service->>Service: set param.endBlock = currentBlockHeight
Service->>Service: update in-memory datasource (endBlock)
Service->>DB: persist updated params (metadata)
Service-->>Manager: success
Manager->>Manager: re-filter filteredDataSources (apply endBlock)
Manager-->>VM: ack
VM-->>Handler: continue (datasource no longer active)
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 Potential focus areas:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
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
Tests