Cherry-pick fix for indexer v3.0.0 #943
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe overall changes include an emergency patch to the time in force (TIF) handling logic, where unexpected TIF values now default to 'GTT' instead of raising an error. A new test case was added for this scenario, and an existing test was commented out. The perpetual markets table in the database was updated to include a 'FINAL_SETTLEMENT' status. Additionally, there were updates to handling trading rewards in the block processor and related tests, including a new subtype and associated validation logic. 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: 4
Configuration used: CodeRabbit UI
Files selected for processing (10)
- indexer/packages/postgres/tests/lib/protocol-translations.test.ts (2 hunks)
- indexer/packages/postgres/tests/stores/perpetual-market-table.test.ts (2 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20240108175501_add_perpetual_markets_status_final_settlement.ts (1 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/tests/handlers/trading-rewards-handler.test.ts (1 hunks)
- indexer/services/ender/src/lib/block-processor.ts (1 hunks)
- indexer/services/ender/src/scripts/helpers/dydx_from_protocol_time_in_force.sql (1 hunks)
- indexer/services/scripts/src/helpers/block-helpers.ts (2 hunks)
- indexer/services/scripts/src/helpers/types.ts (1 hunks)
Additional comments: 15
indexer/packages/postgres/src/db/migrations/migration_files/20240108175501_add_perpetual_markets_status_final_settlement.ts (2)
3-9: The migration script correctly adds the 'FINAL_SETTLEMENT' status to the 'perpetual_markets' table. Ensure that all dependent services are aware of this new status and handle it appropriately.
12-18: The rollback script correctly reverts the changes made by the up script. This is important for maintaining database integrity in case the migration needs to be undone.
indexer/services/scripts/src/helpers/types.ts (1)
- 23-23: The addition of the 'TRADING_REWARD' enum value to 'DydxIndexerSubtypes' is correct and follows the established pattern of the enum.
indexer/services/ender/src/scripts/helpers/dydx_from_protocol_time_in_force.sql (1)
- 17-19: The emergency patch to the 'dydx_from_protocol_time_in_force' function now returns 'GTT' for any unrecognized 'tif' values instead of raising an exception. This change should be communicated to all stakeholders to ensure that the new default behavior is understood and handled correctly in all consuming services.
indexer/services/scripts/src/helpers/block-helpers.ts (1)
- 102-107: The new case for 'TRADING_REWARD' in the 'annotateIndexerTendermintEvent' function is correctly implemented, including the decoding of 'TradingRewardsEventV1'.
indexer/packages/postgres/__tests__/stores/perpetual-market-table.test.ts (2)
1-7: The modifications to the imports are correct, adding necessary imports and removing unused ones.
154-167: The new test case for winding down a perpetual market is well-structured and correctly asserts the expected behavior after updating the market status to 'FINAL_SETTLEMENT'.
indexer/services/ender/__tests__/handlers/trading-rewards-handler.test.ts (1)
- 39-39: The change of 'defaultTransactionIndex' to '-2' is noted. Ensure that this value aligns with the intended test scenarios and that it does not inadvertently affect other tests that rely on this constant.
indexer/packages/postgres/__tests__/lib/protocol-translations.test.ts (1)
- 171-172: The new test case for the emergency patch in 'protocolOrderTIFToTIF' correctly tests the new behavior of returning 'GTT' for an invalid TimeInForce value.
indexer/services/ender/src/lib/block-processor.ts (1)
- 54-54: The addition of
TradingRewardsValidatortoTXN_EVENT_SUBTYPE_VERSION_TO_VALIDATOR_MAPPINGand its removal fromBLOCK_EVENT_SUBTYPE_VERSION_TO_VALIDATOR_MAPPINGshould be verified to ensure that it aligns with the intended event processing logic. Confirm thatTradingRewardsValidatoris now correctly associated with transaction events rather than block events.Verification successful
The shell script output confirms that the
TradingRewardsValidatorclass is defined intrading-rewards-validator.tsand is referenced inblock-processor.ts. However, the output does not explicitly confirm the removal ofTradingRewardsValidatorfromBLOCK_EVENT_SUBTYPE_VERSION_TO_VALIDATOR_MAPPINGor its exclusive association with transaction events.To verify the review comment, we need to check the usage of
TradingRewardsValidatorwithinblock-processor.tsto ensure it's only associated with transaction events and not block events. We should also check if there are any other references toTradingRewardsValidatorin the context of block events.
The output from the
rgcommand shows thatTradingRewardsValidatoris imported inblock-processor.tsand is used in theTXN_EVENT_SUBTYPE_VERSION_TO_VALIDATOR_MAPPINGmapping. The specific line of code provided confirms thatTradingRewardsValidatoris associated with a serialized subtype and version, presumably for transaction events.The
ast-grepcommand did not return any matches, which suggests thatTradingRewardsValidatoris not present in theBLOCK_EVENT_SUBTYPE_VERSION_TO_VALIDATOR_MAPPING. This supports the review comment thatTradingRewardsValidatorhas been removed from block event processing and is now correctly associated with transaction events.Based on the provided script outputs, the review comment is consistent with the changes in the codebase. The
TradingRewardsValidatoris indeed used for transaction events and not for block events inblock-processor.ts.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that TradingRewardsValidator is only used in transaction events and not block events. ast-grep --lang javascript --pattern $'class TradingRewardsValidator { $$$ }' # Ensure no references to TradingRewardsValidator in block event processing. rg "TradingRewardsValidator" --files-with-matchesLength of output: 247
Script:
#!/bin/bash # Check the usage of TradingRewardsValidator in block-processor.ts to confirm it's associated with transaction events. rg "TradingRewardsValidator" indexer/services/ender/src/lib/block-processor.ts # Search for any mapping of TradingRewardsValidator within BLOCK_EVENT_SUBTYPE_VERSION_TO_VALIDATOR_MAPPING. ast-grep --lang javascript --pattern $'const BLOCK_EVENT_SUBTYPE_VERSION_TO_VALIDATOR_MAPPING = { $$$ $_: TradingRewardsValidator, $$$ }'Length of output: 413
indexer/packages/postgres/src/lib/protocol-translations.ts (1)
- 278-280: The change to the
protocolOrderTIFToTIFfunction to returnTimeInForce.GTTfor unexpectedprotocolOrderTIFvalues instead of throwing an error is a significant behavior change. Ensure that this emergency patch is well-documented and that all stakeholders are aware of the new default behavior, as it could have implications on how orders are processed.indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (4)
1566-1566: The comment on line 1566 indicates that an invalid
TimeInForcein the order is treated asGTT. This is a significant behavior change and should be clearly documented in the function that processes the orders, not just in the test. Ensure that the actual code reflects this behavior and that it is intentional and documented.1670-1670: The test case on line 1670 is creating a partial
IndexerOrderwith anundefinedorderId. This is a good test for validation logic, but ensure that the system is expected to handle such cases gracefully and that appropriate error handling is in place in the production code.1602-1602: The comment on line 1602 about the fixed-point notation for
quoteAmountis helpful for understanding the expected format. It's good practice to include such comments when dealing with financial calculations to avoid confusion about precision and representation.1640-1662: The
expectDefaultOrderAndFillSubaccountKafkaMessagesfunction is being called with a set of parameters to validate Kafka messages. It's important to ensure that the Kafka message expectations are correctly aligned with the test cases and that the Kafka producer mock is being verified for the correct calls with the expected message content.
| @@ -1451,12 +1451,223 @@ describe('OrderHandler', () => { | |||
| ]); | |||
| }); | |||
|
|
|||
| // Testing emergency patch | |||
| it('handles invalid TimeInForce (%s)', async () => { | |||
There was a problem hiding this comment.
The test description for handling invalid TimeInForce is missing a parameter in the it function call. It should include a parameter that describes the test case, which is typically passed to it.each for parameterized tests.
- it('handles invalid TimeInForce (%s)', async () => {
+ it('handles invalid TimeInForce', async () => {Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| it('handles invalid TimeInForce (%s)', async () => { | |
| it('handles invalid TimeInForce', async () => { |
| clobPairId: testConstants.defaultPerpetualMarket3.clobPairId, | ||
| orderFlags: ORDER_FLAG_SHORT_TERM.toString(), | ||
| // Invalid TIF | ||
| timeInForce: (4 as IndexerOrder_TimeInForce), |
There was a problem hiding this comment.
The casting of an invalid TimeInForce value directly in the test case is a good way to simulate the behavior when an invalid value is encountered. However, it would be beneficial to add a comment explaining the choice of the specific invalid values (4 and 17) for clarity and maintainability.
+ // Using out-of-range values to simulate invalid TimeInForce
timeInForce: (4 as IndexerOrder_TimeInForce),Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| timeInForce: (4 as IndexerOrder_TimeInForce), | |
| // Using out-of-range values to simulate invalid TimeInForce | |
| timeInForce: (4 as IndexerOrder_TimeInForce), |
| }); | ||
|
|
There was a problem hiding this comment.
The test case starting on line 1663 is missing a description for the it.each function call. This should be added for clarity and to ensure the test runner can properly display which test case is running.
- it.each([
+ it.each([
+ // Descriptions of each test case for better readability and reporting
[
undefined, // no maker order
],Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| }); | |
| // Descriptions of each test case for better readability and reporting | |
| it.each([ | |
| [ | |
| undefined, // no maker order | |
| ], | |
| }); | |
| // 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.
The commented-out test case for an unrecognized TimeInForce value is part of the emergency patch. Ensure that this test case is revisited once the emergency patch is no longer needed, as it is important for catching unexpected values.
dydxwill
changed the title
dydxwill
changed the title
Changelist
cherry-picked fixes:
#940
Test Plan
tested in staging
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.