[BUG2-156] Only return recent best effort canceled orders. #2716
Conversation
WalkthroughThis pull request introduces two new filtering parameters— Changes
Sequence Diagram(s)Order Retrieval FlowsequenceDiagram
participant Client
participant API
participant OrdersController
participant OrderStore
Client->>API: GET /orders (with filtering params)
API->>OrdersController: Forward request
OrdersController->>OrderStore: Call findAll(goodTilBlockAfter, goodTilBlockTimeAfter)
OrderStore-->>OrdersController: Return filtered orders
OrdersController-->>API: Send orders response
API-->>Client: Deliver response
Subscription Data Retrieval FlowsequenceDiagram
participant Subscriber
participant SubscriptionService
participant Axios
Subscriber->>SubscriptionService: Subscribe for order updates
SubscriptionService->>Axios: Request orders (with block height filtering)
Axios-->>SubscriptionService: Return order data and BEST_EFFORT_CANCELED orders
SubscriptionService-->>Subscriber: Send combined subscription response
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 (
|
vincentwschau
changed the title
![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
🔭 Outside diff range comments (1)
indexer/packages/postgres/src/types/query-types.ts (1)
75-75: Define a more specific type forinitialMessage.Using
Objectas a type is too broad and provides little type safety. Since the value is['a', 'b'], consider using a more specific type.Apply this diff to improve type safety:
-const initialMessage: Object = ['a', 'b']; +const initialMessage: string[] = ['a', 'b'];
🧹 Nitpick comments (3)
indexer/services/socks/src/lib/subscription.ts (1)
557-559: Consider making the block range configurable.The hardcoded value of 20 blocks in the URL query parameter could be moved to a configuration variable for better maintainability and flexibility.
+const RECENT_BLOCKS_RANGE = 20; // Move to config const blockHeight: string = await blockHeightRefresher.getLatestBlockHeight(); const numBlockHeight: number = parseInt(blockHeight, 10); // Use the config variable -url: `${COMLINK_URL}/v4/orders?address=${address}&subaccountNumber=${subaccountNumber}&status=BEST_EFFORT_CANCELED&goodTilBlockAfter=${numBlockHeight - 20}`, +url: `${COMLINK_URL}/v4/orders?address=${address}&subaccountNumber=${subaccountNumber}&status=BEST_EFFORT_CANCELED&goodTilBlockAfter=${numBlockHeight - RECENT_BLOCKS_RANGE}`,Also applies to: 588-596
indexer/services/comlink/public/api-documentation.md (2)
2000-2008: New Query Parameters for ListOrders EndpointThe addition of the parameters
goodTilBlockAfterandgoodTilBlockTimeAfter(along with their “BeforeOrAt” counterparts) appears to be implemented correctly. However, the current description column lists "none" for these parameters. It would improve usability to add brief descriptive texts that explain these parameters (e.g. “Filter orders with a goodTilBlock greater than the provided block height” and “Filter orders with a goodTilBlockTime after the provided ISO timestamp”). This will help client developers understand the filtering behavior—especially in light of the PR’s goal to only return recent best effort canceled orders.
2190-2208: New Query Parameters for ListOrdersForParentSubaccount EndpointSimilarly, for the
GET /orders/parentSubaccountNumberendpoint, the new query parametersgoodTilBlockAfterandgoodTilBlockTimeAfterhave been added. Please consider updating their description fields to clarify their function. Consistency is key, so the documentation for this endpoint should mirror the explanations provided for the standard ListOrders endpoint. This consistency will reduce client confusion and ensure that filtering for recent best effort canceled orders is clearly communicated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
indexer/packages/postgres/__tests__/stores/order-table.test.ts(1 hunks)indexer/packages/postgres/src/stores/order-table.ts(3 hunks)indexer/packages/postgres/src/types/query-types.ts(2 hunks)indexer/services/comlink/__tests__/controllers/api/v4/orders-controller.test.ts(5 hunks)indexer/services/comlink/public/api-documentation.md(2 hunks)indexer/services/comlink/public/swagger.json(4 hunks)indexer/services/comlink/src/controllers/api/v4/orders-controller.ts(15 hunks)indexer/services/comlink/src/types.ts(2 hunks)indexer/services/socks/__tests__/lib/subscriptions.test.ts(1 hunks)indexer/services/socks/src/lib/subscription.ts(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
indexer/services/socks/__tests__/lib/subscriptions.test.ts
[error] 75-75: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: test / run_command
- GitHub Check: check-build-auxo
- GitHub Check: lint
- GitHub Check: run_command
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (10)
indexer/packages/postgres/src/stores/order-table.ts (1)
171-179: LGTM! The filtering logic is well-implemented.The implementation of
goodTilBlockAfterandgoodTilBlockTimeAfterfilters follows the established patterns, correctly handles null values, and uses appropriate comparison operators.Also applies to: 191-199
indexer/packages/postgres/src/types/query-types.ts (1)
61-61: LGTM! The enum values are well-defined.The new enum values follow the established naming convention and pattern.
Also applies to: 63-63
indexer/services/socks/__tests__/lib/subscriptions.test.ts (1)
61-62: LGTM! The URL patterns are correctly updated.The URL patterns are appropriately split to handle BEST_EFFORT_CANCELED orders separately with the new goodTilBlockAfter parameter.
Also applies to: 70-71
indexer/packages/postgres/__tests__/stores/order-table.test.ts (1)
257-278: LGTM! The test cases provide good coverage.The new test cases thoroughly verify the filtering logic for
goodTilBlockAfterandgoodTilBlockTimeAfter, including proper handling of null values.indexer/services/comlink/src/types.ts (1)
532-534: LGTM! The new filter parameters are properly typed.The addition of optional properties
goodTilBlockAfterandgoodTilBlockTimeAfterto both interfaces is consistent and well-structured.Also applies to: 544-546
indexer/services/comlink/src/controllers/api/v4/orders-controller.ts (1)
87-89: LGTM! The new filter parameters are properly integrated.The changes consistently:
- Update function signatures with new parameters
- Add parameters to query config
- Include proper validation schema for new parameters
Also applies to: 106-108, 131-133, 216-218, 232-234, 249-251, 269-271, 368-374, 380-384
indexer/services/socks/src/lib/subscription.ts (1)
599-604: LGTM! The order merging logic is correct.The parsing and concatenation of orders from multiple responses is implemented correctly.
indexer/services/comlink/__tests__/controllers/api/v4/orders-controller.test.ts (1)
448-511: LGTM! The test coverage is comprehensive.The new test cases thoroughly validate:
- Block-based filtering with
goodTilBlockAfter- Time-based filtering with
goodTilBlockTimeAfter- Edge cases and proper error handling
indexer/services/comlink/public/swagger.json (2)
2753-2777: New Query Parameters for Orders FilteringThe new parameters
goodTilBlockAfter(a number with double format) andgoodTilBlockTimeAfter(an ISO timestamp via theIsoStringschema) have been correctly added in the/ordersendpoint. They are defined as optional, which is appropriate for filtering best effort canceled orders based on block height and time. This change aligns with the PR objective of returning only recent best effort canceled orders. Please ensure that the backend query logic is updated correspondingly to use these filters.
2879-2903: New Query Parameters for Parent Subaccount OrdersThe additions of
goodTilBlockAfterandgoodTilBlockTimeAfterin the/orders/parentSubaccountNumberendpoint mirror those in the/ordersendpoint. The definitions correctly use the double format for block height and refer to theIsoStringschema for the timestamp. This consistency is beneficial for API consumers and supports the intended filtering behavior. Verify that the corresponding service logic and database queries properly incorporate these new parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
indexer/services/comlink/__tests__/controllers/api/v4/orders-controller.test.ts (1)
203-1052: Add test case for 20-block limit on best effort canceled orders.While the test suite comprehensively covers the new filtering parameters, consider adding a specific test case that validates the requirement to return only best effort canceled orders from the last 20 blocks. This would ensure the core objective of the PR is thoroughly tested.
Example test case structure:
it('Successfully returns only best effort canceled orders from last 20 blocks', async () => { // Setup orders at different block heights const currentBlock = 1000; const orders = [ // Order within 20 blocks { ...testConstants.defaultOrder, goodTilBlock: currentBlock - 10 }, // Order outside 20 blocks { ...testConstants.defaultOrder, goodTilBlock: currentBlock - 30 }, ]; await Promise.all(orders.map(order => OrderTable.create(order))); const response = await sendRequest({ type: RequestMethod.GET, path: `/v4/orders?${getQueryString({ ...defaultQueryParams, status: APIOrderStatusEnum.BEST_EFFORT_CANCELED, goodTilBlockAfter: currentBlock - 20, })}`, }); expect(response.body).toHaveLength(1); expect(response.body[0].goodTilBlock).toBe(currentBlock - 10); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
indexer/services/comlink/__tests__/controllers/api/v4/orders-controller.test.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: test / run_command
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: check-build-auxo
- GitHub Check: lint
- GitHub Check: run_command
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
indexer/services/comlink/__tests__/controllers/api/v4/orders-controller.test.ts (3)
412-425: LGTM! Parameter renaming is consistent and well-tested.The test cases for
goodTilBlockBeforeOrAtparameter are comprehensive and cover both regular and isolated market scenarios.Also applies to: 427-445
447-480: LGTM! New test cases forgoodTilBlockAfterare thorough.The test cases effectively validate the new
goodTilBlockAfterparameter for both regular and isolated market scenarios, ensuring proper filtering of orders based on block height.
497-508: LGTM! Test cases forgoodTilBlockTimeAfterare well-implemented.The test cases validate the new
goodTilBlockTimeAfterparameter using ISO timestamp format, ensuring proper filtering of orders based on block time.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
indexer/services/socks/__tests__/lib/message-forwarder.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
indexer/services/socks/__tests__/lib/message-forwarder.test.ts
[error] 177-177: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
[error] 178-178: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: test / run_command
- GitHub Check: check-build-auxo
- GitHub Check: lint
- GitHub Check: run_command
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
indexer/services/socks/__tests__/lib/message-forwarder.test.ts (1)
177-181: Verify test coverage for the new order fetching behavior.The test data structure has been modified from an object to an array, which suggests changes in the API response format. Let's verify that the test cases adequately cover the new order fetching behavior.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for related test files and their coverage of order fetching scenarios fd "test.*\.(ts|js)$" | xargs rg -l "orders.*concat|orders.*filter" # Search for order-related test assertions rg "expect.*orders" --type tsLength of output: 3067
Order Fetching Behavior Test Coverage Confirmed
The tests adequately assert the new behavior where orders are now represented as an array. Key findings include:
- The test in
indexer/services/socks/__tests__/lib/message-forwarder.test.tsexplicitly uses array concatenation (via.concat) to construct the orders property.- Additional tests (e.g., in e2e tests and in
indexer/packages/postgres/__tests__/stores/order-table.test.ts) validate the structure by asserting on array length and content using methods likeexpect(...).toHaveLengthand object comparisons.- Overall, the modifications in the API response (object to array) are reflected in the test assertions across multiple test files, ensuring that the new order fetching behavior is properly covered.
🧰 Tools
🪛 Biome (1.9.4)
[error] 177-177: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
[error] 178-178: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
| const mockAxiosResponse: Object[] = ['a', 'b']; | ||
| const subaccountInitialMessage: Object = { | ||
| ...mockAxiosResponse, | ||
| orders: mockAxiosResponse, | ||
| orders: mockAxiosResponse.concat(mockAxiosResponse), | ||
| blockHeight: '2', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Object type with a more specific type definition.
The current use of Object type is too broad and can hide potential type-related bugs. Consider using a more specific type that matches the actual data structure.
-const mockAxiosResponse: Object[] = ['a', 'b'];
-const subaccountInitialMessage: Object = {
+type OrderData = string; // Update this type to match the actual order data structure
+const mockAxiosResponse: OrderData[] = ['a', 'b'];
+const subaccountInitialMessage: {
+ orders: OrderData[];
+ blockHeight: string;
+} = {
...mockAxiosResponse,
orders: mockAxiosResponse.concat(mockAxiosResponse),
blockHeight: '2',
};Additionally, consider adding a comment to document the test data structure and its relationship to the actual API response format.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mockAxiosResponse: Object[] = ['a', 'b']; | |
| const subaccountInitialMessage: Object = { | |
| ...mockAxiosResponse, | |
| orders: mockAxiosResponse, | |
| orders: mockAxiosResponse.concat(mockAxiosResponse), | |
| blockHeight: '2', | |
| type OrderData = string; // Update this type to match the actual order data structure | |
| const mockAxiosResponse: OrderData[] = ['a', 'b']; | |
| const subaccountInitialMessage: { | |
| orders: OrderData[]; | |
| blockHeight: string; | |
| } = { | |
| ...mockAxiosResponse, | |
| orders: mockAxiosResponse.concat(mockAxiosResponse), | |
| blockHeight: '2', | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 177-177: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
[error] 178-178: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
|
@Mergifyio backport release/indexer/v8.x |
✅ Backports have been created
|
(cherry picked from commit d425749)
Changelist
Only return best effort canceled orders within the last 20 blocks when fetching orders for subaccount channel subscriptions.
Update order store to allow querying for orders with goodTilBlock after a given block height, and update comlink to take in the same parameter.
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
Tests
Documentation