Add support for pagination at table level. #1244
Conversation
updated pagination tests to also evaluate total pages.
WalkthroughThe recent updates across the indexer system focus on introducing and enhancing pagination features. Functions like Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (3)
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: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
indexer/services/comlink/public/swagger.jsonis excluded by:!**/*.json
Files selected for processing (13)
- indexer/packages/postgres/tests/stores/fill-table.test.ts (8 hunks)
- indexer/packages/postgres/tests/stores/transfer-table.test.ts (4 hunks)
- indexer/packages/postgres/src/stores/fill-table.ts (3 hunks)
- indexer/packages/postgres/src/stores/transfer-table.ts (3 hunks)
- indexer/packages/postgres/src/types/index.ts (1 hunks)
- indexer/packages/postgres/src/types/pagination-types.ts (1 hunks)
- indexer/packages/postgres/src/types/query-types.ts (2 hunks)
- indexer/services/comlink/public/api-documentation.md (11 hunks)
- indexer/services/comlink/src/controllers/api/v4/fills-controller.ts (5 hunks)
- indexer/services/comlink/src/controllers/api/v4/trades-controller.ts (4 hunks)
- indexer/services/comlink/src/controllers/api/v4/transfers-controller.ts (4 hunks)
- indexer/services/comlink/src/types.ts (4 hunks)
- indexer/services/ender/tests/handlers/transfer-handler.test.ts (3 hunks)
Additional comments: 44
indexer/packages/postgres/src/types/pagination-types.ts (1)
- 1-6: The
PaginationFromDatabase<T>interface is well-defined and aligns with the requirements for implementing pagination. The use of generics (<T>) enhances flexibility, allowing this structure to be used across various types of database queries.indexer/packages/postgres/src/types/index.ts (1)
- 28-28: Correctly exporting
pagination-typesensures that the pagination functionality is accessible throughout the application. This is a necessary step for implementing pagination across various components.indexer/services/comlink/src/controllers/api/v4/trades-controller.ts (3)
- 48-48: The addition of the
pagequery parameter enables pagination support for thegetTradesmethod, aligning with the PR's objectives to enhance data retrieval operations.- 57-62: The destructuring of the response from
FillTable.findAllto include pagination metadata (pageSize,offset,total) is correctly implemented, facilitating the return of paginated data to the client.- 80-82: Including pagination metadata (
pageSize,totalResults,offset) in the response object ensures consistency in how pagination information is conveyed to clients, enhancing the API's usability.indexer/services/comlink/src/controllers/api/v4/transfers-controller.ts (3)
- 52-52: Adding the
pagequery parameter to theTransfersControllerenables pagination, which is a key enhancement for handling large datasets efficiently.- 57-59: The response from
TransferTable.findAllToOrFromSubaccountIdis correctly destructured to include pagination metadata, which is essential for returning paginated data to the client.- 125-127: Including pagination metadata in the response object is a good practice, ensuring that clients receive consistent and clear pagination information.
indexer/services/comlink/src/controllers/api/v4/fills-controller.ts (3)
- 58-58: The addition of the
pagequery parameter enables pagination support for thegetFillsmethod, aligning with the PR's objectives to enhance data retrieval operations.- 72-77: The destructuring of the response from
FillTable.findAllto include pagination metadata (pageSize,offset,total) is correctly implemented, facilitating the return of paginated data to the client.- 108-110: Including pagination metadata (
pageSize,totalResults,offset) in the response object ensures consistency in how pagination information is conveyed to clients, enhancing the API's usability.indexer/packages/postgres/src/types/query-types.ts (2)
- 13-13: Adding the
PAGEfield to theQueryableFieldenum is a necessary step for supporting pagination in database queries, aligning with the PR's objectives.- 92-92: Updating the
QueryConfiginterface to include aPAGEproperty enables the configuration of pagination parameters in database queries, which is essential for implementing pagination.indexer/services/comlink/src/types.ts (4)
- 44-48: The introduction of the
PaginationResponseinterface is a well-considered addition, providing a standardized way to include pagination metadata in various response structures.- 130-130: Extending the
FillResponseinterface to include pagination metadata is a necessary enhancement for supporting pagination in the API's response structures.- 153-153: Including pagination metadata in the
TransferResponseinterface aligns with the objectives of enhancing data retrieval operations through pagination.- 194-194: The extension of the
TradeResponseinterface to include pagination metadata ensures consistency in how pagination information is conveyed to clients across different endpoints.indexer/packages/postgres/src/stores/transfer-table.ts (3)
- 24-24: The import of
PaginationFromDatabaseis correctly added to support the new pagination functionality. Good job on maintaining clean and organized imports.- 293-332: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [198-325]
The changes from lines 198 to 325 implement the pagination logic in
findAllToOrFromSubaccountId. The logic correctly handles thepageparameter, ensuring it's at least 1, and calculates the offset accordingly. The return structure includes pagination metadata, which is a good practice for pagination support.However, there's a potential issue with handling cases where
limitis undefined. The current implementation assumeslimitis always provided whenpageis specified. It's important to either enforcelimitas a required parameter when pagination is used or provide a default value forlimitto ensure the function behaves predictably even iflimitis not explicitly provided by the caller.Additionally, consider removing the comment about removing sorting in line 311, as the code does not actually remove sorting but clears the order for the count query, which is a different concern. Clarifying this comment will improve code readability and maintainability.
- // We need to remove the sorting as it is not necessary in this case. + // Clearing the order for the count query as sorting is not necessary for counting entries.Consider adding a default value for
limitor enforcing it as a required parameter whenpageis specified to ensure predictable behavior.
- 293-332: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-327]
Overall, the changes in
transfer-table.tseffectively implement pagination support in thefindAllToOrFromSubaccountIdfunction. The addition of thePaginationFromDatabaseimport is correctly used, and the pagination logic is well-implemented, with a minor suggestion for improvement regarding the handling of thelimitparameter.The rest of the file, including utility functions and unchanged parts, remains consistent with the pagination changes and maintains good coding practices. Great job on enhancing the scalability and user-friendliness of the application with these changes.
indexer/packages/postgres/src/stores/fill-table.ts (5)
- 28-28: The addition of
PaginationFromDatabaseto the import list indicates that pagination support is being integrated into the database queries, which is a positive change for handling large datasets efficiently.- 53-53: Adding a
pageparameter to thefindAllfunction's configuration object is a crucial step in implementing pagination. This allows users to specify which page of results they wish to retrieve.- 57-57: The return type of the
findAllfunction has been updated toPromise<PaginationFromDatabase<FillFromDatabase>>, reflecting the addition of pagination support. This change ensures that the function's return type accurately represents the paginated response structure.- 161-161: The condition to apply a limit without pagination (when
pageis undefined) is preserved, ensuring backward compatibility for calls tofindAllthat do not use pagination.- 168-190: The implementation of pagination logic within the
findAllfunction is well-structured. It correctly calculates theoffsetbased on thepageandlimitparameters and retrieves the total count of records to include in the paginated response. However, there's a potential performance concern with the.clone().clearOrder().count()operation, as it might lead to inefficient queries depending on the underlying database's optimization capabilities.Consider evaluating the performance impact of the count query, especially for large datasets. If performance issues arise, you might explore alternative approaches, such as caching the total count or using more efficient counting strategies tailored to the specific database engine in use.
indexer/packages/postgres/__tests__/stores/transfer-table.test.ts (3)
- 118-118: The modification to destructure the
resultsfrom the response object in the test case is a direct consequence of the pagination feature implementation in the corresponding store function. This change ensures that the tests accurately reflect the new return structure of the function being tested.- 145-145: Similar to the previous comment, destructuring the
resultsfrom the response object in this test case aligns with the updated return structure of the store function due to pagination support. This consistency is crucial for maintaining the validity of the tests.- 158-207: The addition of test cases to verify pagination functionality is a critical step in ensuring the robustness of the new feature. These tests comprehensively cover different pagination scenarios, including fetching specific pages, handling limits, and validating the total count of results. This thorough testing approach helps guarantee that pagination behaves as expected under various conditions.
indexer/packages/postgres/__tests__/stores/fill-table.test.ts (8)
- 74-74: The restructuring of the assignment of results from
FillTable.findAll()calls by destructuring the response object is a necessary adjustment to accommodate the new pagination feature in thefindAllfunction. This change ensures that the tests are aligned with the updated function signature and return type.- 94-94: Similar to the previous comment, this adjustment in the test case is in response to the pagination support added to the
findAllfunction. Ensuring that tests reflect the actual usage and return structure of the function is crucial for maintaining test accuracy and relevance.- 106-157: The addition of new test cases to verify pagination functionality is an excellent practice. These tests cover various pagination scenarios, including fetching different pages, handling page limits, and checking the total count of results. This comprehensive testing ensures that the pagination feature works as intended and enhances the overall test coverage for the
findAllfunction.- 168-168: Adjusting the test case to destructure the
resultsfrom the response object is consistent with the changes made to thefindAllfunction to support pagination. This ensures that the test accurately reflects the function's updated return structure.- 190-190: The restructuring in this test case to destructure the
resultsfrom the response object is necessary due to the pagination feature implementation. This change aligns the test with the updated function signature and return type, maintaining the test's validity.- 211-211: Similar to previous comments, this test case adjustment is in response to the pagination support added to the
findAllfunction. Ensuring that tests accurately reflect the function's usage and return structure is essential for maintaining the integrity of the test suite.- 233-233: The adjustment in this test case to destructure the
resultsfrom the response object is a direct consequence of the pagination feature implementation. This ensures that the test accurately reflects the updated return structure of thefindAllfunction.- 255-255: This test case adjustment, similar to the others, is necessary due to the pagination support added to the
findAllfunction. It ensures that the test is aligned with the function's updated signature and return type, maintaining the test's relevance and accuracy.indexer/services/ender/__tests__/handlers/transfer-handler.test.ts (3)
- 548-548: The destructuring of the result from
TransferTable.findAllToOrFromSubaccountIdintoresultsis a good practice for readability and directly accessing the needed data. However, ensure that the pagination parameters (pageandlimit) are correctly implemented and used in theTransferTable.findAllToOrFromSubaccountIdmethod to support the new pagination functionality. This is crucial for maintaining the efficiency and scalability of data retrieval operations as intended by the PR objectives.- 575-575: Similar to the previous comment, the destructuring of the result from
TransferTable.findAllToOrFromSubaccountIdintoresultsfor the recipient subaccount is well-implemented. Again, verify that the pagination parameters are correctly applied in the method to ensure the pagination functionality works as expected. This is essential for achieving the PR's goal of enhancing data retrieval operations through pagination.- 591-591: The same considerations apply to the destructuring of the result from
TransferTable.findAllToOrFromSubaccountIdintoresultsfor the sender subaccount. It's important to confirm that the pagination logic is correctly integrated into the method, aligning with the PR's objectives to improve data retrieval efficiency through pagination.indexer/services/comlink/public/api-documentation.md (5)
- 691-691: The addition of the
pagequery parameter for pagination support is a welcome improvement. It's crucial to ensure that the documentation clearly explains how this parameter is used, including its default value (if any) and how it interacts with other parameters likelimitto control data fetching.Consider adding a brief explanation or example usage of the
pageparameter to enhance clarity for API consumers.
- 706-708: The introduction of pagination metadata (
pageSize,totalResults, andoffset) in the JSON response is a significant enhancement for API usability. It's important to ensure that these fields are accurately described in the documentation.It would be beneficial to include a description of each pagination-related field in the response schema section to help users understand the structure and purpose of the returned data.
- 807-809: Similar to the previous comment, the addition of pagination metadata in the JSON response for the
GetFillsForParentSubaccountendpoint enhances the API's functionality. Ensuring clear documentation for these fields is essential.Adding descriptions for the
pageSize,totalResults, andoffsetfields in the response schema section will improve the documentation's comprehensiveness and user-friendliness.
- 1984-1986: The inclusion of pagination metadata in the JSON response for the
GetTradesendpoint is consistent with the changes made to other endpoints. This consistency is crucial for a cohesive API experience.As with other endpoints, providing detailed descriptions of the pagination-related fields (
pageSize,totalResults, andoffset) in the response schema section will enhance the documentation's clarity and usefulness.
- 2070-2072: The addition of pagination metadata to the JSON response for the
GetTransfersendpoint follows the pattern established by other endpoints, contributing to a unified API design.Including clear descriptions of the
pageSize,totalResults, andoffsetfields in the response schema section is recommended to ensure users fully understand the pagination mechanism.
| const currentPage = Math.max(1, page); | ||
| const offset = (currentPage - 1) * limit; | ||
|
|
||
| /** | ||
| * We need to remove the sorting as it is not necessary in this case. | ||
| * Also a casting of the ts type is required since the infer of the type | ||
| * obtained from the count is not performed. | ||
| */ | ||
| const count = await baseQuery.clone().clearOrder().count({ count: '*' }).first() as unknown as { count?: string }; |
There was a problem hiding this comment.
nit: Please add types to all declared variables.
There was a problem hiding this comment.
Yeah, we "fixed" in 0d14f11.
Thank you!
| const currentPage = Math.max(1, page); | ||
| const offset = (currentPage - 1) * limit; | ||
|
|
||
| /** | ||
| * We need to remove the sorting as it is not necessary in this case. | ||
| * Also a casting of the ts type is required since the infer of the type | ||
| * obtained from the count is not performed. | ||
| */ | ||
| const count = await baseQuery.clone().clearOrder().count({ count: '*' }).first() as unknown as { count?: string }; |
There was a problem hiding this comment.
What's the sort order in this case then? Is there some default?
There was a problem hiding this comment.
Here we're just counting all the elements. No matter what order they are in, because the COUNT() function returns the number of lines that match a given criterion. For this reason we get the first element as it is the only one available.
There was a problem hiding this comment.
Clients are expecting a reasonable default order per endpoint. That way, random records won't be skipped/repeated in subsequent pages. I think it makes sense to use ascending eventId order for trades/fills/transfers related endpoints, and orderId for the orders endpoint.
There was a problem hiding this comment.
I think there may be a misunderstanding here. The sort order is only removed for a query that asks for a count (a total number of rows), not for the list of items.
Such a query is used to retrieve the total number of items, which we return to the client to say "there are x rows in total". This helps the user to know how many records have not yet been received and for which they could make requests.
Regarding the list of items, as you can see, the order is maintained because the clone() method clones the current query chain, which is useful for reusing partial query snippets in other queries without mutating the original.
I hope this helps to clarify things.
There was a problem hiding this comment.
I understand. I don't think the change is required in this file.
But can you pass in an orderBy param set to eventId ASC in fills-controller, trades-controller, and transfers-controller? And can you add unit tests to those controllers verifying the order? We also want pagination on the orders endpoint too.
There was a problem hiding this comment.
Got it 💪
In such a scenario, I think you might agree that adding the ordering feature is not strictly related to the pagination feature. So it might be interesting (and probably better) to add such a feature, but via a new issue and pull request to keep the context.
WRT the ability to add pagination on the orders endpoint too, that might be a good suggestion.
We'll work on it 👷
There was a problem hiding this comment.
If that's the case, can you remove all the changes to the API in this PR? We don't want to be in a partial state of having pagination exposed but not having the correct order.
There was a problem hiding this comment.
Yeah. We will remove the "controller level" changes and leave only the "table level" changes.
Then we'll move to a new PR related to the same issue (since you want to keep the pagination and sorting related) to add both pagination and ordering at "controller level" and we can merge the two.
Do you agree?
There was a problem hiding this comment.
As agreed, we have implemented the changes at table level.
We're now moving to the controller level with a new PR!
added explicit types for fill and transfer tables.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/packages/postgres/src/stores/fill-table.ts (3 hunks)
- indexer/packages/postgres/src/stores/transfer-table.ts (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/packages/postgres/src/stores/fill-table.ts
- indexer/packages/postgres/src/stores/transfer-table.ts
| */ | ||
| const count: { count?: string } = await baseQuery.clone().clearOrder().count({ count: '*' }).first() as unknown as { count?: string }; | ||
|
|
||
| baseQuery = baseQuery.offset(offset).limit(limit); |
There was a problem hiding this comment.
Question: How does this logic interact with additional items being added that are higher in the sort order than items returned?
E.g.
I want fills created after height X in descending order, page size of 10. I get page 2, which is the 10th-20th most recent fills. 10 new fills get added, and now I get page 3, would that be the same set of fills I got for page 2?
There was a problem hiding this comment.
To address your example: yes, you are right.
We use offset-based pagination to improve UX through parallel data retrieval, recognising the potential for data inconsistencies with new insertions. While this method provides fast access to any data segment, in certain scenarios it can lead to duplicate entries if data is added between two queries.
Conversely, cursor-based pagination offers better data consistency by using a pointer (cursor) to retrieve data sequentially, eliminating duplicates or missed entries due to data movement. However, it lacks the flexibility of parallel retrieval and direct access to specific data segments.
The request for such a feature stems from the need to add pagination to retrieve a user's full trading history at the front-end.
Given the frequency of data updates relative to request receipt in our context, we believe that offset-based pagination strikes the best balance for our current needs, optimising the user experience while considering the trade-off in data consistency. In fact, in this scenario, if a user has a very long history, a sequential approach could actually degrade the user experience, as we would have to wait for each request to continue with the new one.
Anyway, give us more feedback, we are open to adapting our approach as requirements evolve.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (5)
- indexer/packages/postgres/tests/stores/order-table.test.ts (5 hunks)
- indexer/packages/postgres/src/stores/order-table.ts (3 hunks)
- indexer/services/comlink/src/controllers/api/v4/orders-controller.ts (3 hunks)
- indexer/services/roundtable/tests/tasks/cancel-stale-orders.test.ts (2 hunks)
- indexer/services/roundtable/src/tasks/cancel-stale-orders.ts (1 hunks)
Additional Context Used
Path-based Instructions (5)
indexer/services/roundtable/src/tasks/cancel-stale-orders.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/roundtable/__tests__/tasks/cancel-stale-orders.test.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/packages/postgres/src/stores/order-table.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/packages/postgres/__tests__/stores/order-table.test.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/comlink/src/controllers/api/v4/orders-controller.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (13)
indexer/services/roundtable/src/tasks/cancel-stale-orders.ts (1)
25-25: The restructuring of the assignment to use object destructuring aligns with the PR's objective of adding pagination support and correctly handles the paginated response fromOrderTable.findAll().indexer/services/roundtable/__tests__/tasks/cancel-stale-orders.test.ts (2)
86-86: The change in the test file correctly adapts to the new return type ofOrderTable.findAllby using object destructuring to extractresults, ensuring the test logic remains valid.
148-148: The change in the test file correctly adapts to the new return type ofOrderTable.findAllby using object destructuring to extractresults, ensuring the test logic remains valid.indexer/packages/postgres/src/stores/order-table.ts (3)
18-18: AddingPaginationFromDatabaseto imports is necessary for supporting the new pagination functionality in thefindAllmethod.
71-75: The modification of thefindAllfunction's return type toPromise<PaginationFromDatabase<OrderFromDatabase>>and the addition of thepageparameter are essential for implementing pagination support.
190-223: The logic to handle pagination based on thepageandlimitparameters is correctly implemented. UsingMath.maxto ensure the page number is always >= 1 and calculating the offset are best practices for pagination.indexer/packages/postgres/__tests__/stores/order-table.test.ts (5)
52-52: The adaptation to the new return type ofOrderTable.findAllusing object destructuring is correctly implemented in the test, ensuring the test logic remains valid.
70-70: The adaptation to the new return type ofOrderTable.findAllusing object destructuring is correctly implemented in the test, ensuring the test logic remains valid.
82-124: The addition of tests for pagination functionality is crucial for validating the new feature. These tests correctly implement pagination scenarios, ensuring the functionality works as expected.
153-153: The adaptation to the new return type ofOrderTable.findAllusing object destructuring is correctly implemented in the test, ensuring the test logic remains valid.
249-249: The adaptation to the new return type ofOrderTable.findAllusing object destructuring is correctly implemented in the test, ensuring the test logic remains valid.indexer/services/comlink/src/controllers/api/v4/orders-controller.ts (2)
99-102: The use of object destructuring to extractresultsfrom the paginated response ofOrderTable.findAllis correctly implemented, ensuring the controller adapts to the modified method.
144-150: The additional fetching of orders based on Redis order IDs not returned from the initial Postgres query is correctly handled, ensuring the controller accurately merges data from both sources.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- indexer/services/comlink/src/controllers/api/v4/fills-controller.ts (2 hunks)
- indexer/services/comlink/src/controllers/api/v4/trades-controller.ts (1 hunks)
- indexer/services/comlink/src/controllers/api/v4/transfers-controller.ts (2 hunks)
Additional Context Used
Path-based Instructions (3)
indexer/services/comlink/src/controllers/api/v4/trades-controller.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/comlink/src/controllers/api/v4/transfers-controller.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/comlink/src/controllers/api/v4/fills-controller.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (4)
indexer/services/comlink/src/controllers/api/v4/trades-controller.ts (1)
56-56: LGTM! The change correctly implements pagination by destructuring theresultsproperty from theFillTable.findAllcall, aligning with the PR's objectives.indexer/services/comlink/src/controllers/api/v4/transfers-controller.ts (1)
56-56: LGTM! The change correctly implements pagination by destructuring theresultsproperty from theTransferTable.findAllToOrFromSubaccountIdcall, aligning with the PR's objectives.indexer/services/comlink/src/controllers/api/v4/fills-controller.ts (2)
71-71: LGTM! The change correctly implements pagination by destructuring theresultsproperty from theFillTable.findAllcall in thegetFillsmethod, aligning with the PR's objectives.
135-135: LGTM! The change correctly implements pagination by destructuring theresultsproperty from theFillTable.findAllcall in thegetFillsForParentSubaccountmethod, aligning with the PR's objectives.
indexer/services/roundtable/__tests__/tasks/cancel-stale-orders.test.ts
Outdated
Show resolved
Hide resolved
indexer/services/roundtable/__tests__/tasks/cancel-stale-orders.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (4)
- indexer/packages/postgres/tests/stores/order-table.test.ts (6 hunks)
- indexer/services/comlink/src/controllers/api/v4/orders-controller.ts (3 hunks)
- indexer/services/roundtable/tests/tasks/cancel-stale-orders.test.ts (3 hunks)
- indexer/services/roundtable/src/tasks/cancel-stale-orders.ts (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- indexer/packages/postgres/tests/stores/order-table.test.ts
- indexer/services/comlink/src/controllers/api/v4/orders-controller.ts
- indexer/services/roundtable/tests/tasks/cancel-stale-orders.test.ts
- indexer/services/roundtable/src/tasks/cancel-stale-orders.ts
| @@ -49,10 +50,11 @@ export async function findAll( | |||
| createdOnOrAfter, | |||
| clientMetadata, | |||
| fee, | |||
| page, | |||
There was a problem hiding this comment.
Ideally, we'd have a check that if page is set, then orderBy must contain eventId. Can you add a TODO for now?
| @@ -291,11 +293,41 @@ export async function findAllToOrFromSubaccountId( | |||
| } | |||
| } | |||
|
|
|||
| if (limit !== undefined) { | |||
| if (limit !== undefined && page === undefined) { | |||
There was a problem hiding this comment.
Ensure that the limit is only applied when page is not provided to avoid unintended data truncation.
- if (limit !== undefined && page === undefined) {
+ if (limit !== undefined && page == null) {
baseQuery = baseQuery.limit(limit);
}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.
| if (limit !== undefined && page === undefined) { | |
| if (limit !== undefined && page == null) { |
| * We need to remove the sorting as it is not necessary in this case. | ||
| * Also a casting of the ts type is required since the infer of the type | ||
| * obtained from the count is not performed. | ||
| */ | ||
| const count: { count?: string } = await baseQuery.clone().clearOrder().count({ count: '*' }).first() as unknown as { count?: string }; |
There was a problem hiding this comment.
The comment about removing sorting is misleading because the sorting is actually necessary for consistent pagination results. Recommend rephrasing or removing this comment to avoid confusion.
- * We need to remove the sorting as it is not necessary in this case.
+ * Ensure sorting is applied to maintain consistent pagination results.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.
| * We need to remove the sorting as it is not necessary in this case. | |
| * Also a casting of the ts type is required since the infer of the type | |
| * obtained from the count is not performed. | |
| */ | |
| const count: { count?: string } = await baseQuery.clone().clearOrder().count({ count: '*' }).first() as unknown as { count?: string }; | |
| * Ensure sorting is applied to maintain consistent pagination results. | |
| * Also a casting of the ts type is required since the infer of the type | |
| * obtained from the count is not performed. | |
| */ | |
| const count: { count?: string } = await baseQuery.clone().clearOrder().count({ count: '*' }).first() as unknown as { count?: string }; |
| @@ -156,11 +158,41 @@ export async function findAll( | |||
| Ordering.DESC, | |||
| ); | |||
|
|
|||
| if (limit !== undefined) { | |||
| if (limit !== undefined && page === undefined) { | |||
There was a problem hiding this comment.
Ensure that the limit is only applied when page is not provided to avoid unintended data truncation.
- if (limit !== undefined && page === undefined) {
+ if (limit !== undefined && page == null) {
baseQuery = baseQuery.limit(limit);
}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.
| if (limit !== undefined && page === undefined) { | |
| if (limit !== undefined && page == null) { |
| * We need to remove the sorting as it is not necessary in this case. | ||
| * Also a casting of the ts type is required since the infer of the type | ||
| * obtained from the count is not performed. | ||
| */ | ||
| const count: { count?: string } = await baseQuery.clone().clearOrder().count({ count: '*' }).first() as unknown as { count?: string }; |
There was a problem hiding this comment.
The comment about removing sorting is misleading because the sorting is actually necessary for consistent pagination results. Recommend rephrasing or removing this comment to avoid confusion.
- * We need to remove the sorting as it is not necessary in this case.
+ * Ensure sorting is applied to maintain consistent pagination results.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.
| * We need to remove the sorting as it is not necessary in this case. | |
| * Also a casting of the ts type is required since the infer of the type | |
| * obtained from the count is not performed. | |
| */ | |
| const count: { count?: string } = await baseQuery.clone().clearOrder().count({ count: '*' }).first() as unknown as { count?: string }; | |
| * Ensure sorting is applied to maintain consistent pagination results. | |
| * Also a casting of the ts type is required since the infer of the type | |
| * obtained from the count is not performed. | |
| */ | |
| const count: { count?: string } = await baseQuery.clone().clearOrder().count({ count: '*' }).first() as unknown as { count?: string }; |
improve pagination comment thanks to coderabbitai.
vincentwschau
changed the title
vincentwschau
changed the title
|
https://github.com/Mergifyio backport release/indexer/v5.x |
✅ Backports have been created
|
* feat(postgres): ✨ add transfer pagination support * test(postgres): ✅ add transfer pagination test * feat(comlink): ✨ add pagination for getTransfers * test(ender): ✅ update transfer handler test * feat(postgres): ✨ add fill pagination support * test(postgres): ✅ add fill pagination test * feat(comlink): ✨ add pagination for getTrades and getFills * test: ✅ update pagination tests updated pagination tests to also evaluate total pages. * feat(postgres): 🏷️ add explicit types added explicit types for fill and transfer tables. * feat(postgres): ✨ add order pagination support * test(postgres): ✅ add order pagination test * feat: ♻️ update order findAll usage * revert: ⏪ revert add pagination for getTransfers * revert: ⏪ revert add pagination for getTrades and getFills * feat: 🏷️ add explicit ts types * docs: 💡 add pagination todo comment * docs(postgres): 💡 improve pagination comment improve pagination comment thanks to coderabbitai. --------- Co-authored-by: Davide Segullo <[email protected]> (cherry picked from commit e12ebf2)
* feat(postgres): ✨ add transfer pagination support * test(postgres): ✅ add transfer pagination test * feat(comlink): ✨ add pagination for getTransfers * test(ender): ✅ update transfer handler test * feat(postgres): ✨ add fill pagination support * test(postgres): ✅ add fill pagination test * feat(comlink): ✨ add pagination for getTrades and getFills * test: ✅ update pagination tests updated pagination tests to also evaluate total pages. * feat(postgres): 🏷️ add explicit types added explicit types for fill and transfer tables. * feat(postgres): ✨ add order pagination support * test(postgres): ✅ add order pagination test * feat: ♻️ update order findAll usage * revert: ⏪ revert add pagination for getTransfers * revert: ⏪ revert add pagination for getTrades and getFills * feat: 🏷️ add explicit ts types * docs: 💡 add pagination todo comment * docs(postgres): 💡 improve pagination comment improve pagination comment thanks to coderabbitai. --------- Co-authored-by: Davide Segullo <[email protected]> (cherry picked from commit e12ebf2) Co-authored-by: Giorgio Nocera <[email protected]>
Changelist
This PR aims to solve #1239 issue.
Specifically, it modifies the low-level functions used by the API provided by the indexer to add the pagination feature.
Test Plan
Tests are delivered with the features.
We have not tested the environment with a full live (development) environment, but have performed unit tests.
We encourage you to perform full (integration) testing before merging.
Author/Reviewer Checklist
If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the
state-breakinglabel.No breaking changes.
If the PR has breaking postgres changes to the indexer add the
indexer-postgres-breakinglabel.No breaking changes.
If this PR isn't state-breaking but has changes that modify behavior in
PrepareProposalorProcessProposal, manually add the labelproposal-breaking.No changes in
PrepareProposalORProcessProposal.If this PR is one of many that implement a specific feature, manually label them all
feature:[feature-name].Probably should add feature:[support-pagination-in-transfers]
If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label
backport/[branch-name].Manually add any of the following labels:
refactor,chore,bug.Summary by CodeRabbit
New Features
Refactor
Tests