[IND-485]: Query for PerpetualPositions by openEventId #777
Conversation
IND-485 Inconsistent perpetual position querying
It seems like two perpetual positions of the same height can be created. We should use |
# Walkthrough
The changes primarily revolve around the introduction of a new constant `defaultTendermintEvent4` and modifications to the sorting order in various functions and SQL queries. The sorting order has been changed from `createdAtHeight` to `openEventId` in the `findAll` function and in several SQL scripts. Additionally, new test cases have been added to verify the correct sorting of `PerpetualPositions` based on `openEventId`.
# Changes
| File Path | Change Summary |
| --- | --- |
| `.../postgres/__tests__/helpers/constants.ts`<br>`.../postgres/__tests__/helpers/mock-generators.ts`<br>`.../postgres/__tests__/stores/perpetual-position-table.test.ts` | Addition of a new constant `defaultTendermintEvent4` of type `TendermintEventCreateObject` and `defaultTendermintEventId4` of type `Buffer`. Corresponding creation and usage in test data and functions. |
| `.../postgres/src/stores/perpetual-position-table.ts`<br>`.../ender/src/scripts/dydx_order_fill_handler_per_order.sql`<br>`.../ender/src/scripts/dydx_update_perpetual_position_aggregate_fields.sql` | Modification of sorting order from `createdAtHeight` to `openEventId` in the `findAll` function and SQL queries. |
| `.../ender/__tests__/handlers/order-fills/liquidation-handler.test.ts`<br>`.../ender/__tests__/handlers/order-fills/order-handler.test.ts` | Changes to event IDs and test constants. |TipsChat with CodeRabbit Bot (
|
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- indexer/packages/postgres/tests/helpers/constants.ts (2 hunks)
- indexer/packages/postgres/tests/helpers/mock-generators.ts (2 hunks)
- indexer/packages/postgres/tests/stores/perpetual-position-table.test.ts (3 hunks)
- indexer/packages/postgres/src/stores/perpetual-position-table.ts (1 hunks)
- indexer/services/ender/tests/handlers/order-fills/liquidation-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (4 hunks)
- indexer/services/ender/src/scripts/dydx_order_fill_handler_per_order.sql (1 hunks)
Files skipped from review due to trivial changes (2)
- indexer/packages/postgres/tests/helpers/constants.ts
- indexer/packages/postgres/tests/helpers/mock-generators.ts
Additional comments: 11
indexer/packages/postgres/src/stores/perpetual-position-table.ts (1)
- 124-129: The ordering of the results from the
findAllfunction has been changed fromcreatedAtHeighttoopenEventId. Ensure that this change does not affect any dependent functions or modules that rely on the previous ordering.indexer/services/ender/src/scripts/dydx_order_fill_handler_per_order.sql (1)
- 167-173: The change in the ORDER BY clause from "createdAtHeight" to "openEventId" may affect the order of the records retrieved from the database. Ensure that this change does not introduce any unintended side effects in the application logic that relies on the order of these records.
- ORDER BY "createdAtHeight" DESC; + ORDER BY "openEventId" DESC;indexer/services/ender/__tests__/handlers/order-fills/liquidation-handler.test.ts (3)
144-145: Ensure that the
openEventIdandlastEventIdvalues are correctly set and that they are consistent with the new ordering logic.237-237: The message 'creates fills and orders (with %s), sends vulcan message for maker order update and updates perpetualPosition' is clear and descriptive. Ensure that it accurately reflects the test case.
300-300: Ensure that the
openEventIdvalue is correctly set and that it is consistent with the new ordering logic.indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (4)
146-147: Ensure that the
openEventIdandlastEventIdare correctly updated in all relevant places in the codebase.314-318: Ensure that the
openEventIdis correctly updated in all relevant places in the codebase.917-920: Ensure that the
openEventIdis correctly updated in all relevant places in the codebase.1129-1140: Ensure that the
openEventIdis correctly updated in all relevant places in the codebase.indexer/packages/postgres/__tests__/stores/perpetual-position-table.test.ts (2)
15-18: The new import
TendermintEventCreateObjectis used in the new test case. Ensure that this import is used correctly and that the type is defined as expected.191-224: This new test case checks if the
PerpetualPositionsare sorted byopenEventId. It creates two positions with differentopenEventIds and checks if the returned positions are sorted correctly. The test case seems to be correct, but ensure that thePerpetualPositionTable.findAllfunction is implemented to sort byopenEventId.+ it('Successfully finds PerpetualPositions sorted by openEventId', async () => { + const earlierPosition: PerpetualPositionCreateObject = { + ...defaultPerpetualPosition, + openEventId: defaultTendermintEventId3, + lastEventId: defaultTendermintEventId3, + }; + const nextTendermintEvent: TendermintEventCreateObject = { + blockHeight: defaultTendermintEvent3.blockHeight, + transactionIndex: defaultTendermintEvent3.transactionIndex, + eventIndex: defaultTendermintEvent3.eventIndex + 1, + }; + const nextTendermintEventId: Buffer = TendermintEventTable.createEventId( + nextTendermintEvent.blockHeight, + nextTendermintEvent.transactionIndex, + nextTendermintEvent.eventIndex, + ); + const laterPosition: PerpetualPositionCreateObject = { + ...defaultPerpetualPosition, + openEventId: nextTendermintEventId, + lastEventId: nextTendermintEventId, + }; + await TendermintEventTable.create(nextTendermintEvent); + await Promise.all([ + await PerpetualPositionTable.create(earlierPosition), + await PerpetualPositionTable.create(laterPosition), + ]); + + const perpetualPositions: PerpetualPositionFromDatabase[] = await + PerpetualPositionTable.findAll({}, [], { readReplica: true }); + + expect(perpetualPositions.length).toEqual(2); + expect(perpetualPositions[0]).toEqual(expect.objectContaining(laterPosition)); + expect(perpetualPositions[1]).toEqual(expect.objectContaining(earlierPosition)); + });
Christopher-Li
force-pushed
the
cl_openEventId
branch
from
910858c
to
a564b30
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- indexer/packages/postgres/tests/helpers/constants.ts (2 hunks)
- indexer/packages/postgres/tests/helpers/mock-generators.ts (2 hunks)
- indexer/packages/postgres/tests/stores/perpetual-position-table.test.ts (3 hunks)
- indexer/packages/postgres/src/stores/perpetual-position-table.ts (1 hunks)
- indexer/services/ender/tests/handlers/order-fills/liquidation-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (4 hunks)
- indexer/services/ender/src/scripts/dydx_update_perpetual_position_aggregate_fields.sql (1 hunks)
Files skipped from review due to trivial changes (2)
- indexer/packages/postgres/tests/helpers/mock-generators.ts
- indexer/services/ender/tests/handlers/order-fills/liquidation-handler.test.ts
Additional comments: 9
indexer/services/ender/src/scripts/dydx_update_perpetual_position_aggregate_fields.sql (1)
- 22-30: The change in ordering from "createdAtHeight" to "openEventId" could potentially alter the result of the query. Ensure that this change is intended and that it does not break any dependent functionality. Also, verify that "openEventId" is always unique and non-null to prevent unexpected results.
indexer/packages/postgres/__tests__/stores/perpetual-position-table.test.ts (2)
15-18: The new import
TendermintEventCreateObjectis used in the new test case for sortingPerpetualPositionsbyopenEventId. This is in line with the changes made in the PR.191-224: This new test case checks if the
PerpetualPositionsare sorted byopenEventId. It creates twoPerpetualPositionswith differentopenEventIds and checks if the returned positions are sorted correctly. This is a good addition to the test suite as it verifies the new functionality introduced in this PR.indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (3)
146-147: Ensure that the new
openEventIdandlastEventIdvalues are correct and that they are being used correctly in the tests.317-318: Ensure that the
openEventIdvalue is correct and that it is being used correctly in the tests.1140-1140: Ensure that the
openEventIdvalue is correct and that it is being used correctly in the tests.indexer/packages/postgres/__tests__/helpers/constants.ts (2)
287-294: The addition of
defaultTendermintEvent4is consistent with the existing pattern of defining test constants. Ensure that the values assigned toblockHeight,transactionIndex, andeventIndexare appropriate for the tests where this constant will be used.310-314: The creation of
defaultTendermintEventId4is consistent with the existing pattern of defining test constants. Ensure that the values passed tocreateEventIdare appropriate for the tests where this constant will be used.indexer/packages/postgres/src/stores/perpetual-position-table.ts (1)
- 124-130: The ordering of the query results has been changed from
createdAtHeighttoopenEventId. Ensure that this change does not affect any dependent logic that relies on the previous ordering.- baseQuery = baseQuery.orderBy( - PerpetualPositionColumns.createdAtHeight, - Ordering.ASC, - ); + baseQuery = baseQuery.orderBy( + PerpetualPositionColumns.subaccountId, + Ordering.ASC, + ).orderBy( + PerpetualPositionColumns.openEventId, + Ordering.DESC, + );
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/src/scripts/dydx_liquidation_fill_handler_per_order.sql (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/ender/src/scripts/dydx_liquidation_fill_handler_per_order.sql
Changelist
Change existing queries to
perpetual_positionsto order byopenEventIdrather thancreatedAtHeightand change the corresponding SQL statement in Ender to also useopentEventIdTest Plan
Unit tests
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.