Emergency patch to handle invalid TimeInForce. #942
Conversation
|
Warning Rate Limit Exceeded@dydxwill has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 21 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughAn emergency patch has been applied across several components of a system handling order processing to address an issue with unexpected TimeInForce (TIF) values. Instead of throwing errors, the system now defaults to using 'Good-Til-Cancelled' (GTT) when encountering unrecognized TIFs. This change affects test cases, the protocol translations logic, and a SQL script, suggesting a coordinated update to ensure the system remains functional while dealing with unanticipated TIF inputs. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- indexer/packages/postgres/tests/lib/protocol-translations.test.ts (2 hunks)
- indexer/packages/postgres/src/lib/protocol-translations.ts (1 hunks)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (1 hunks)
- indexer/services/ender/src/scripts/helpers/dydx_from_protocol_time_in_force.sql (1 hunks)
Additional comments: 3
indexer/services/ender/src/scripts/helpers/dydx_from_protocol_time_in_force.sql (1)
- 17-19: The change to default to 'GTT' for unrecognized
TimeInForcevalues could have implications for system behavior. Ensure that downstream systems are capable of handling these default values without issues. It's also important to consider logging such occurrences for monitoring any unexpected values that might indicate other issues in the system.Verification successful
Based on the search results, the 'GTT' value is consistently handled within the
dydx_from_protocol_time_in_force.sqlscript for various cases, including the default case for unrecognizedTimeInForcevalues. This suggests that the system is prepared to handle 'GTT' as a default value, and the change made in the PR is consistent with the script's logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the default 'GTT' value for unrecognized TIFs is handled correctly downstream. # This script checks for any handling of 'GTT' values in the codebase. rg --type sql "GTT" -- "indexer/services/ender/src/scripts/"Length of output: 553
indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (2)
- 1454-1676: The test case added for handling invalid
TimeInForcevalues seems to correctly simulate the scenario where theOrderHandlerreceives orders with invalidTimeInForcevalues. The test asserts that the system treats these invalid values asTimeInForce.GTT(Good-Til-Time), which aligns with the emergency patch described in the PR objectives. This is a critical update to ensure the system's robustness in the face of unexpectedTimeInForcevalues.However, it's important to ensure that this behavior is consistent across all parts of the system that deal with
TimeInForce. If there are other modules or services that also processTimeInForcevalues, they should be updated and tested accordingly to handle invalid values in the same manner.
- 1451-1686: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-1453]
The rest of the test file contains various test cases for the
OrderHandlermodule. These tests cover a range of scenarios, including creating fills and orders, updating existing orders, and handling different types of orders such as limit, post-only, and immediate-or-cancel (IOC).It's important to ensure that these tests are comprehensive and cover all the edge cases. Additionally, the tests should be clear and maintainable, with proper descriptions and assertions that accurately reflect the expected behavior of the system.
One potential area of improvement is to ensure that the test descriptions are clear and descriptive. For example, the test case descriptions should clearly state the expected outcome of the test, such as what the status of an order should be after processing, especially in cases where orders are partially filled or canceled.
| // Test case for emergency patch | ||
| ['INVALID', (100 as IndexerOrder_TimeInForce), TimeInForce.GTT], |
There was a problem hiding this comment.
The addition of the test case for the emergency patch is good practice to ensure the new behavior is as expected. However, it's important to ensure that the commented-out test case does not leave a gap in the test coverage. If the behavior change is permanent, consider removing the commented-out test case to avoid confusion.
| // Commented out for emergency patch | ||
| /* it('throws error if unrecognized protocolTIF given', () => { | ||
| expect( | ||
| () => { | ||
| protocolOrderTIFToTIF(100 as IndexerOrder_TimeInForce); | ||
| }, | ||
| ).toThrow(new Error('Unexpected TimeInForce from protocol: 100')); | ||
| }); | ||
| });*/ |
There was a problem hiding this comment.
Commenting out the test case that expects an exception for an unrecognized protocolTIF is appropriate given the emergency patch. However, ensure that this is a temporary measure and that a plan is in place to either reinstate or permanently remove this test case based on the long-term decision regarding the handling of unrecognized protocolTIF values.
| return TimeInForce.GTT; | ||
| // Removed for emergency patch | ||
| // throw new Error(`Unexpected TimeInForce from protocol: ${protocolOrderTIF}`); |
There was a problem hiding this comment.
Based on the context provided from the order-translations.ts file, the error logging present does not pertain to the handling of 'GTT' values. There is no explicit logging for cases where an unrecognized TimeInForce defaults to 'GTT'. This suggests that while the system may handle 'GTT' as a default value, there may not be monitoring in place specifically for these fallback cases. It would be advisable to consider adding such logging to aid in monitoring and debugging.
Analysis chain
The modification to return TimeInForce.GTT for unrecognized protocolOrderTIF values, instead of throwing an error, aligns with the changes in the SQL script. Ensure that this change is reflected in all parts of the system that rely on this function and that proper logging is in place for such cases.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the default 'GTT' value for unrecognized TIFs is handled correctly in all parts of the system.
# This script checks for any handling of 'GTT' values in the codebase.
rg --type ts "GTT" -- "indexer/packages/postgres/src/lib/"
Length of output: 900
Script:
#!/bin/bash
# Check for logging related to the default 'GTT' value being used in the system.
rg --type ts "logger.*GTT" -- "indexer/packages/postgres/src/lib/"
Length of output: 66
Script:
#!/bin/bash
# Check for general logging mechanisms that might capture the use of the default 'GTT' value.
rg --type ts "logger" -- "indexer/packages/postgres/src/lib/"
Length of output: 393
Script:
#!/bin/bash
# Examine the context of the logger calls in order-translations.ts to determine if they are related to 'GTT' handling.
rg --type ts -C 5 "logger.error" -- "indexer/packages/postgres/src/lib/order-translations.ts"
Length of output: 1197
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (1 hunks)
Additional comments: 5
indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (5)
- 1454-1675: The test case added for handling invalid
TimeInForcevalues is critical and should ensure the system's stability. It's good to see parameterized tests being used to cover different scenarios. However, there are a few points to consider:
- Ensure that the test case covers all possible invalid
TimeInForcevalues that the system might encounter.- Verify that the test case includes assertions for all relevant aspects of the system's state after processing the orders.
- Check if there is any cleanup required after the test runs to prevent state leakage into other tests.
- Confirm that the logging and error handling in the test case are appropriate and provide enough information for debugging if the test fails.
- 1485-1485: Casting a number to an enum type (
IndexerOrder_TimeInForce) is a clever way to simulate invalidTimeInForcevalues. However, ensure that the range of invalid values tested reflects possible real-world scenarios that the system might encounter.// Invalid TIF timeInForce: (4 as IndexerOrder_TimeInForce),
- 1577-1578: The test asserts that an invalid
TimeInForcein the order is treated asTimeInForce.GTT. This behavior should be explicitly documented in the system's handling of orders to avoid confusion and ensure that it aligns with the business logic.// Invalid TIF in order is treated as GTT timeInForce: TimeInForce.GTT,
1676-1676: It's important to include negative test cases, such as when
fillOrderEventfails validation. This ensures that the system behaves correctly in the face of invalid input. The use ofexpectfunctions to assert the absence of certain states, like candles, is a good practice.1562-1565: Ensure that the fixed-point notation used for prices in the test cases matches the precision expected by the system. Using fixed-point notation helps avoid floating-point precision issues, which is crucial in financial systems.
const makerPrice: string = '0.00000000000001'; const takerPrice: string = '0.0000000000000015';
dydxwill
changed the title
* Emergency patch to handle invalid TimeInForce. (#907) --------- Co-authored-by: vincentwschau <[email protected]>
* Emergency patch to handle invalid TimeInForce. (#907) --------- Co-authored-by: vincentwschau <[email protected]>
* Emergency patch to handle invalid TimeInForce. (#907) --------- Co-authored-by: vincentwschau <[email protected]>
Changelist
#907
Test Plan
Tested on internal mainnet indexer.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.