Add config var to exclude specific stateful order ids from being processed. #2513
Conversation
WalkthroughThe pull request introduces modifications across several test files, a configuration file, and various modules related to event processing and validation. Key changes include the addition of a new configuration variable Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
indexer/services/ender/src/config.ts (1)
27-33: LGTM: New configuration variable added correctly.The new
SKIP_STATEFUL_ORDER_UUIDSconfiguration variable is well-implemented and documented. It provides a flexible way to skip processing specific stateful order events, which aligns with the PR objectives.Consider adding a brief example of the expected format in the comment, e.g.:
// Example: SKIP_STATEFUL_ORDER_UUIDS="uuid1,uuid2,uuid3"This would further clarify the expected input format for users.
indexer/services/ender/src/validators/validator.ts (1)
62-69: LGTM! Minor suggestion for JSDoc comment.The implementation of
shouldExcludeEvent()looks good and aligns well with the PR objective. It provides a clean way for subclasses to implement custom exclusion logic.Consider fixing the typo in the JSDoc comment:
- * Allows aribtrary logic to exclude events from being processed. + * Allows arbitrary logic to exclude events from being processed.indexer/services/ender/src/scripts/handlers/dydx_block_processor_ordered_handlers.sql (1)
72-73: LGTM! Consider enhancing skipped event handling.The addition of the "skipped_event" case aligns well with the PR objective of introducing a way to skip processing specific stateful order events. This change enhances the event processing logic by explicitly handling skipped events without raising exceptions or returning NULL.
Consider the following improvements:
- Instead of an empty JSON object, you might want to include minimal information about the skipped event, such as its ID or type. This could be helpful for debugging or auditing purposes.
- Consider adding logging for skipped events. This could be valuable for monitoring and troubleshooting.
Example implementation:
WHEN '"skipped_event"'::jsonb THEN rval[i] = jsonb_build_object( 'skipped', true, 'event_id', event_->'id', 'event_type', event_->'type' ); -- Optionally add logging hereThese suggestions aim to improve the traceability and debuggability of skipped events while maintaining the core functionality of your implementation.
indexer/services/ender/src/scripts/handlers/dydx_block_processor_unordered_handlers.sql (1)
70-71: LGTM! Consider adding a comment for clarity.The addition of the "skipped_event" case is a good implementation that aligns with the PR objective. It allows for handling skipped events without disrupting the existing logic.
For improved clarity, consider adding a brief comment explaining the purpose of this case:
WHEN '"skipped_event"'::jsonb THEN -- Handle skipped events by returning an empty object rval[i] = jsonb_build_object();indexer/services/ender/__tests__/handlers/stateful-order/stateful-order-removal-handler.test.ts (1)
161-190: LGTM: Comprehensive test case for skipping order removal events.The new test case effectively verifies the behavior when an order removal event is skipped. It covers both transaction and block events, ensuring thorough testing of the feature.
A minor suggestion for improvement:
Consider adding an assertion to verify that the
onMessagefunction was called successfully. This would ensure that the event was processed without errors, even though it was skipped. You can do this by spying on theonMessagefunction:const onMessageSpy = jest.spyOn(require('../../../src/lib/on-message'), 'onMessage'); await onMessage(kafkaMessage); expect(onMessageSpy).toHaveBeenCalledTimes(1);This addition would provide an extra layer of confidence that the event was processed correctly, even when skipped.
indexer/services/ender/__tests__/handlers/stateful-order/conditional-order-triggered-handler.test.ts (2)
44-45: LGTM: Addition of prevSkippedOrderUUIDs variable.The
prevSkippedOrderUUIDsvariable is correctly added to preserve the original configuration state. This is a good practice to avoid side effects between tests.Consider using
constinstead ofletforprevSkippedOrderUUIDsas it's not reassigned:-let prevSkippedOrderUUIDs: string = config.SKIP_STATEFUL_ORDER_UUIDS; +const prevSkippedOrderUUIDs: string = config.SKIP_STATEFUL_ORDER_UUIDS;
171-205: LGTM: New test case for skipping order trigger events.The new test case effectively verifies the functionality of skipping order trigger events based on the
SKIP_STATEFUL_ORDER_UUIDSconfiguration. It covers both transaction and block events, and correctly asserts that the order is created but not triggered when its UUID is in the skip list.For improved clarity, consider adding an assertion to explicitly check that the order was not triggered:
expect(order).toEqual(expect.objectContaining({ - status: OrderStatus.OPEN, + status: OrderStatus.UNTRIGGERED, updatedAt: defaultDateTime.toISO(), updatedAtHeight: defaultHeight.toString(), })); + expect(producerSendMock).not.toHaveBeenCalled();This change would make it more evident that the order remained untriggered and no message was sent to Vulcan.
indexer/services/ender/__tests__/handlers/stateful-order/conditional-order-placement-handler.test.ts (1)
234-250: LGTM: New test case for skipping orders added.The new test case effectively verifies the behavior when an order is skipped based on the
SKIP_STATEFUL_ORDER_UUIDSconfiguration. It covers both transaction and block events, which is thorough. The test aligns well with the PR objective of introducing a configuration variable to skip specific order UUIDs.A minor suggestion for improvement:
Consider adding an assertion to verify that
producerSendMockwas not called, to ensure that no Kafka message was produced for the skipped order. This would provide an additional layer of verification. For example:expect(producerSendMock).not.toHaveBeenCalled();indexer/services/ender/__tests__/handlers/stateful-order/stateful-order-placement-handler.test.ts (1)
258-278: LGTM: New test case for skipping orders added.The new test case for skipping orders is well-structured and comprehensive. It covers multiple scenarios using
it.each, which is an efficient approach. The test correctly sets up theSKIP_STATEFUL_ORDER_UUIDSconfiguration and verifies that skipped orders are not added to the database.Consider adding a brief comment explaining the purpose of this test case for improved readability. For example:
// Test case to verify that orders are skipped when their UUID is in SKIP_STATEFUL_ORDER_UUIDS it.each([ // ... (existing test cases) ])('successfully skips order with %s', async ( // ... (rest of the test case) ));indexer/services/ender/src/validators/stateful-order-validator.ts (1)
Missing Unit Tests for Exclusion Feature
The test suite for
StatefulOrderValidatordoes not include tests for the new exclusion feature related toSKIP_STATEFUL_ORDER_UUIDS. To ensure the feature is properly validated, please add unit tests covering the following scenarios:
- Empty
SKIP_STATEFUL_ORDER_UUIDS: Verify behavior when no UUIDs are specified to be skipped.- Valid UUIDs to Skip: Ensure that orders with UUIDs listed in
SKIP_STATEFUL_ORDER_UUIDSare correctly excluded from processing.- UUIDs Not to Skip: Confirm that orders with UUIDs not listed in
SKIP_STATEFUL_ORDER_UUIDSare processed as expected.- Edge Cases for
getOrderUUId: Test all event types and potential error scenarios to ensuregetOrderUUIdhandles them correctly.Implementing these tests will validate the new exclusion functionality and prevent potential issues related to order processing.
🔗 Analysis chain
Line range hint
1-273: Overall implementation looks good, don't forget unit tests.The changes successfully implement the new feature to exclude specific order UUIDs from processing, aligning with the PR objectives. The implementation is correct but could benefit from the optimizations and error handling improvements suggested in the previous comments.
Remember to implement unit tests for this new functionality, as mentioned in the PR objectives. These tests should cover:
- Behavior when
SKIP_STATEFUL_ORDER_UUIDSis empty- Behavior when
SKIP_STATEFUL_ORDER_UUIDScontains UUIDs that should be skipped- Behavior when
SKIP_STATEFUL_ORDER_UUIDScontains UUIDs that should not be skipped- Edge cases for
getOrderUUIdmethod, including all event types and potential error scenariosTo ensure the new functionality is properly tested, please run the following command:
This will help verify that appropriate test files exist for the
StatefulOrderValidatorclass.
The test file
indexer/services/ender/__tests__/validators/stateful-order-validator.test.tsexists. To ensure that comprehensive unit tests are implemented for the new exclusion feature, let's review the contents of this test file.
It appears that the current test suite for
StatefulOrderValidatordoes not explicitly cover the new exclusion feature related toSKIP_STATEFUL_ORDER_UUIDS. To ensure comprehensive testing, let's verify if there are any test cases addressing this functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files related to StatefulOrderValidator fd -e ts -e js --full-path '.*test.*' | xargs grep -l "StatefulOrderValidator"Length of output: 155
Script:
#!/bin/bash # Output the contents of the test file cat indexer/services/ender/__tests__/validators/stateful-order-validator.test.tsLength of output: 11660
Script:
#!/bin/bash # Search for occurrences of 'SKIP_STATEFUL_ORDER_UUIDS' and 'shouldExcludeEvent' in the test file grep -E 'SKIP_STATEFUL_ORDER_UUIDS|shouldExcludeEvent' indexer/services/ender/__tests__/validators/stateful-order-validator.test.tsLength of output: 133
indexer/services/ender/src/lib/block-processor.ts (1)
225-239: Consider adding unit tests for the event exclusion logicTo ensure the new event skipping functionality works as intended, please consider adding unit tests that cover the scenario where
validator.shouldExcludeEvent()returnstrue. This will enhance the robustness of the code by verifying that events are correctly excluded from processing and that the system responds appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- indexer/services/ender/tests/handlers/stateful-order/conditional-order-placement-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/stateful-order/conditional-order-triggered-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-placement-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-removal-handler.test.ts (3 hunks)
- indexer/services/ender/src/config.ts (2 hunks)
- indexer/services/ender/src/lib/block-processor.ts (2 hunks)
- indexer/services/ender/src/lib/types.ts (1 hunks)
- indexer/services/ender/src/scripts/handlers/dydx_block_processor_ordered_handlers.sql (1 hunks)
- indexer/services/ender/src/scripts/handlers/dydx_block_processor_unordered_handlers.sql (1 hunks)
- indexer/services/ender/src/validators/stateful-order-validator.ts (3 hunks)
- indexer/services/ender/src/validators/validator.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (20)
indexer/services/ender/src/config.ts (1)
9-9: LGTM: Import statement updated correctly.The addition of
parseStringto the import statement is consistent with its usage in the new configuration variable and is correctly placed with other related imports.indexer/services/ender/src/validators/validator.ts (1)
62-69: Verify usage in subclasses and event processing logic.The addition of this method is a good extension point. To ensure it's being used correctly:
- Check if any subclasses are overriding this method to implement the skipping logic for specific order UUIDs.
- Verify that the event processing logic is calling this method before processing events.
Let's check for implementations and usage:
✅ Verification successful
Verification Successful:
shouldExcludeEventis correctly implemented and utilized.
- The method is properly overridden in
stateful-order-validator.tsto implement specific exclusion logic.- Event processing logic in
block-processor.tscorrectly callsshouldExcludeEventbefore processing events.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for implementations and usage of shouldExcludeEvent # Test 1: Search for subclasses overriding shouldExcludeEvent echo "Searching for subclasses overriding shouldExcludeEvent:" ast-grep --lang typescript --pattern 'class $_ extends Validator { $$$ shouldExcludeEvent() { $$$ } $$$ }' # Test 2: Search for usage of shouldExcludeEvent in event processing logic echo "Searching for usage of shouldExcludeEvent in event processing:" rg --type typescript 'shouldExcludeEvent\(\)' -C 5Length of output: 464
Script:
#!/bin/bash # Description: Re-verify implementations and usage of shouldExcludeEvent with corrected commands # Test 1: Search for subclasses overriding shouldExcludeEvent echo "Searching for subclasses overriding shouldExcludeEvent:" ast-grep --lang typescript --pattern 'class $_ extends Validator { $$$ shouldExcludeEvent() { $$$ } $$$ }' # Test 2: Search for usage of shouldExcludeEvent in event processing logic using correct file type echo "Searching for usage of shouldExcludeEvent in event processing:" rg --type ts 'shouldExcludeEvent\(\)' -C 5 # Test 3: Search for function calls to shouldExcludeEvent using ast-grep echo "Searching for function calls to shouldExcludeEvent in event processing:" ast-grep --lang typescript --pattern '$_.$_$_$$$$$.shouldExcludeEvent($_)'Length of output: 3271
indexer/services/ender/src/scripts/handlers/dydx_block_processor_unordered_handlers.sql (1)
70-71: Verify integration with event skipping logic.The addition of the "skipped_event" case is well-integrated and doesn't affect the processing of other event types. This change enhances the function's flexibility to handle skipped events as intended by the PR.
To ensure proper integration, please run the following script to verify that the "skipped_event" subtype is being generated correctly in the relevant parts of the codebase:
✅ Verification successful
"skipped_event" subtype is correctly integrated across handlers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the generation and handling of "skipped_event" subtype # Test: Search for the generation of "skipped_event" subtype rg --type go -e 'subtype.*"skipped_event"' -g '!*test*' # Test: Search for any additional handling of "skipped_event" subtype rg --type go -e '"skipped_event"' -g '!*test*'Length of output: 106
Script:
#!/bin/bash # Description: Verify the generation and handling of "skipped_event" subtype in SQL files # Test: Search for the generation of "skipped_event" subtype in SQL files rg --type sql -e 'subtype.*"skipped_event"' # Test: Search for any additional handling of "skipped_event" subtype in SQL files rg --type sql -e '"skipped_event"'Length of output: 350
indexer/services/ender/__tests__/handlers/stateful-order/stateful-order-removal-handler.test.ts (3)
40-40: LGTM: Import statement for config module.The addition of the import statement for the
configmodule is appropriate and necessary for accessing theSKIP_STATEFUL_ORDER_UUIDSconfiguration in the test suite.
43-44: LGTM: Proper test isolation forSKIP_STATEFUL_ORDER_UUIDS.The addition of
prevSkippedOrderUUIDsand the modification to theafterEachblock ensure proper test isolation by preserving and restoring the original value ofSKIP_STATEFUL_ORDER_UUIDS. This prevents tests from interfering with each other through modifications to the global configuration.Also applies to: 58-58
Line range hint
1-190: Overall assessment: Well-implemented feature with good test coverage.The changes in this file effectively implement and test the new feature for skipping specific order removal events. The code maintains good test isolation, and the new test case provides coverage for an important scenario. The implementation aligns well with the PR objectives of introducing a configuration variable to exclude specific stateful order IDs from being processed.
indexer/services/ender/__tests__/handlers/stateful-order/conditional-order-triggered-handler.test.ts (3)
41-41: LGTM: Import statement for config module.The import statement for the
configmodule is correctly added and placed appropriately at the end of the import statements.
59-59: LGTM: Config reset in afterEach block.The addition of
config.SKIP_STATEFUL_ORDER_UUIDS = prevSkippedOrderUUIDs;in theafterEachblock is correct and necessary. It ensures that the configuration is restored after each test, maintaining test isolation and preventing side effects between tests.
Line range hint
1-205: Overall assessment: Changes look good with minor suggestions.The changes effectively implement and test the new functionality for skipping specific order trigger events. The additions include:
- Importing the
configmodule- Preserving and resetting the
SKIP_STATEFUL_ORDER_UUIDSconfiguration- Adding a new test case for skipping order trigger events
These changes enhance the test coverage and ensure proper handling of skipped events. The code is well-structured and follows good testing practices. Consider the minor suggestions for further improvements in clarity and consistency.
indexer/services/ender/src/lib/types.ts (1)
64-65: Approve the addition ofSKIPPED_EVENT_SUBTYPEconstant with suggestions.The addition of the
SKIPPED_EVENT_SUBTYPEconstant is approved as it aligns with the PR objective of introducing a way to exclude specific stateful order ids from being processed. The constant name and value are clear and self-explanatory.However, I have a couple of suggestions and questions:
Consider adding this constant to the
DydxIndexerSubtypesenum for consistency with other event subtypes. This would make it easier for developers to find all event subtypes in one place.Could you please clarify why this constant is not part of the
DydxIndexerSubtypesenum? Is there a specific reason for keeping it separate?It would be helpful to add a brief comment explaining the purpose and usage of this constant, especially since it's not part of the enum.
To ensure this constant is used correctly throughout the codebase, please run the following script:
This will help us understand how and where the new constant is being used in the project.
✅ Verification successful
Approve the addition of the
SKIPPED_EVENT_SUBTYPEconstant with suggestions.The addition of the
SKIPPED_EVENT_SUBTYPEconstant is approved as it aligns with the PR objective of introducing a way to exclude specific stateful order IDs from being processed. The constant name and value are clear and self-explanatory.However, I have a couple of suggestions and questions:
Consider Adding to
DydxIndexerSubtypesEnum: For consistency with other event subtypes, adding this constant to theDydxIndexerSubtypesenum would make it easier for developers to locate all event subtypes in one place.Clarification on Separation: Could you please clarify why this constant is not included in the
DydxIndexerSubtypesenum? Is there a specific reason for keeping it separate?Add Documentation: It would be helpful to add a brief comment explaining the purpose and usage of this constant, especially since it's not part of the enum.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of SKIPPED_EVENT_SUBTYPE constant # Search for imports of the constant echo "Imports of SKIPPED_EVENT_SUBTYPE:" rg --type typescript "import.*SKIPPED_EVENT_SUBTYPE.*from.*types" -g '!**/types.ts' # Search for usage of the constant echo "\nUsage of SKIPPED_EVENT_SUBTYPE:" rg --type typescript "SKIPPED_EVENT_SUBTYPE" -g '!**/types.ts'Length of output: 372
Script:
#!/bin/bash # Description: Check usage of SKIPPED_EVENT_SUBTYPE constant # Search for imports of the constant echo "Imports of SKIPPED_EVENT_SUBTYPE:" rg "import.*SKIPPED_EVENT_SUBTYPE.*from.*types" --glob '!**/types.ts' --glob '*.ts' --glob '*.tsx' # Search for usage of the constant echo "\nUsage of SKIPPED_EVENT_SUBTYPE:" rg "SKIPPED_EVENT_SUBTYPE" --glob '!**/types.ts' --glob '*.ts' --glob '*.tsx'Length of output: 671
indexer/services/ender/__tests__/handlers/stateful-order/conditional-order-placement-handler.test.ts (4)
47-47: LGTM: Import statement for config added.The addition of the
configimport is necessary for accessing theSKIP_STATEFUL_ORDER_UUIDSconfiguration. It's correctly placed at the end of the import statements, following the common practice for local imports.
50-51: LGTM: Added variable to preserve original configuration state.The addition of
prevSkippedOrderUUIDsis a good practice. It stores the original value ofconfig.SKIP_STATEFUL_ORDER_UUIDS, allowing the tests to restore the configuration to its initial state after each test run. This prevents potential side effects between test cases.
65-65: LGTM: Configuration reset added to afterEach block.The addition of
config.SKIP_STATEFUL_ORDER_UUIDS = prevSkippedOrderUUIDs;in theafterEachblock is crucial. It ensures that the configuration is reset to its original state after each test, maintaining test isolation and preventing potential interference between test cases. This is a best practice in test suite design.
Line range hint
1-250: Overall assessment: Changes are well-implemented and align with PR objectives.The modifications to this test file effectively introduce and test the new
SKIP_STATEFUL_ORDER_UUIDSconfiguration. The changes maintain test isolation, improve test coverage, and follow best practices for test suite design. The new test case for skipping orders directly addresses the PR's main objective.A minor suggestion was made to enhance the new test case, but overall, the implementation is solid and ready for merging after addressing the small feedback provided.
indexer/services/ender/__tests__/handlers/stateful-order/stateful-order-placement-handler.test.ts (4)
47-47: LGTM: Import statement for config added.The import statement for the local
configmodule is correctly added and placed appropriately at the end of the import list.
50-51: LGTM: Constant for preserving original configuration added.The
prevSkippedOrderUUIDsconstant is correctly added to store the original value ofconfig.SKIP_STATEFUL_ORDER_UUIDS. This practice ensures that the configuration state can be restored after each test, maintaining test isolation.
65-65: LGTM: Configuration reset added in afterEach block.The addition of
config.SKIP_STATEFUL_ORDER_UUIDS = prevSkippedOrderUUIDs;in theafterEachblock is crucial for maintaining test isolation. It ensures that any changes made to this configuration during a test do not affect subsequent tests.
Line range hint
1-278: Summary: Changes align well with PR objectives and enhance test coverage.The modifications to this test file successfully implement the new feature for skipping specific stateful order IDs during processing. The changes include:
- Adding necessary imports and configuration variables.
- Implementing configuration preservation and reset mechanisms.
- Adding a new test case to verify the order skipping functionality.
These changes align perfectly with the PR objectives of introducing a configuration variable to exclude specific stateful order IDs from being processed. The new test case ensures that the feature works as intended, which addresses the test plan mentioned in the PR summary.
The implementation maintains good testing practices by ensuring test isolation and covering multiple scenarios. Overall, these changes contribute to a more robust and comprehensive test suite for the
StatefulOrderPlacementHandler.indexer/services/ender/src/validators/stateful-order-validator.ts (1)
17-17: LGTM: Import added for new functionality.The addition of the
configimport is appropriate for the new functionality that uses configuration variables.indexer/services/ender/src/lib/block-processor.ts (1)
225-239: Ensure proper handling of events with 'SKIPPED_EVENT_SUBTYPE' downstreamBy setting the subtype of skipped events to
SKIPPED_EVENT_SUBTYPE, please verify that all parts of the system that process event subtypes can handle this new subtype appropriately. This includes ensuring that no unintended side effects are introduced and that the system behaves correctly when encountering skipped events.Run the following script to identify how
SKIPPED_EVENT_SUBTYPEis handled throughout the codebase:
Changelist
Add a way to specify order uuid of stateful order events to skip.
Test Plan
Unit tests.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Documentation