-
Notifications
You must be signed in to change notification settings - Fork 117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for pagination at controller level. #1420
Add support for pagination at controller level. #1420
Conversation
WalkthroughThe changes across various files in the indexer services primarily introduce and enhance pagination features in API endpoints. New test cases ensure functionality, while controllers have been updated to handle pagination parameters. Additionally, the API documentation and response types have been adjusted to reflect these updates, ensuring a consistent and scalable user experience when interacting with the system's data retrieval features. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Out of diff range and nitpick comments (20)
indexer/services/comlink/public/api-documentation.md (20)
Line range hint
14-14
: Consider adding a more detailed description for theGetAddress
endpoint to enhance clarity.
Line range hint
188-188
: Add a space after the colon for better readability in the parameters table of theGetSubaccount
endpoint.
Line range hint
313-313
: Add a space after the colon in the parameters table of theGetParentSubaccount
endpoint for consistency.
Line range hint
448-448
: Insert a space after the colon in the parameters table of theGetAssetPositions
endpoint to improve the formatting.
Line range hint
525-525
: Insert a space after the colon in the parameters table of theGetAssetPositionsForParentSubaccount
endpoint for uniformity across the document.
Line range hint
603-603
: Add a space after the colon in the parameters table of theGetCandles
endpoint to enhance the readability of the table.
Line range hint
611-611
: Ensure there is a space between the number and the unit in the enumerated values for theresolution
parameter to adhere to standard formatting practices.
Line range hint
769-769
: Add a space after the colon in the parameters table of theGetFills
endpoint to maintain consistency in the document's formatting.
Line range hint
871-871
: Insert a space after the colon in the parameters table of theGetFillsForParentSubaccount
endpoint to keep the formatting consistent throughout the document.
Line range hint
1032-1032
: Add a space after the colon in the parameters table of theGetTradingRewards
endpoint for better readability.
Line range hint
1107-1107
: Insert a space after the colon in the parameters table of theGetHistoricalFunding
endpoint to improve the clarity of the table.
Line range hint
1186-1186
: Add a space after the colon in the parameters table of theGetHistoricalPnl
endpoint to enhance the document's uniformity.
Line range hint
1271-1271
: Insert a space after the colon in the parameters table of theGetAggregations
endpoint for consistent formatting across the document.
Line range hint
1629-1629
: Add a space after the colon in the parameters table of theListOrdersForParentSubaccount
endpoint to maintain a uniform appearance in the documentation.
Line range hint
1906-1906
: Ensure there is a space after the colon in the parameters table of theListPerpetualMarkets
endpoint to keep the formatting consistent.
Line range hint
2023-2023
: Insert a space after the colon in the parameters table of theListPositions
endpoint for better readability and consistency.
Line range hint
2123-2123
: Add a space after the colon in the parameters table of theListPositionsForParentSubaccount
endpoint to enhance the clarity of the table.
2362-2362
: Ensure there is a space after the colon in the parameters table of theGetTrades
endpoint to maintain uniformity in the document's formatting.
Line range hint
2447-2447
: Insert a space after the colon in the parameters table of theGetTransfers
endpoint for consistent formatting across the document.
Line range hint
2541-2541
: Add a space after the colon in the parameters table of theGetTransfersForParentSubaccount
endpoint to enhance the document's readability.
@@ -77,6 +88,7 @@ class FillsController extends Controller { | |||
? createdBeforeOrAtHeight.toString() | |||
: undefined, | |||
createdBeforeOrAt, | |||
page, | |||
}, | |||
[QueryableField.LIMIT], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to specify a default ordering. Can you add
{ orderBy: [[FillColumns.eventId, Ordering.ASC]], },
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -141,6 +162,7 @@ class FillsController extends Controller { | |||
? createdBeforeOrAtHeight.toString() | |||
: undefined, | |||
createdBeforeOrAt, | |||
page, | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Can you add
{ orderBy: [[FillColumns.eventId, Ordering.ASC]], },
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -62,6 +68,7 @@ class TradesController extends Controller { | |||
? createdBeforeOrAtHeight.toString() | |||
: undefined, | |||
createdBeforeOrAt, | |||
page, | |||
}, | |||
[QueryableField.LIQUIDITY, QueryableField.CLOB_PAIR_ID, QueryableField.LIMIT], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to specify a default ordering. Can you add
{ orderBy: [[FillColumns.eventId, Ordering.ASC]], },
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -77,6 +81,7 @@ class TransfersController extends Controller { | |||
? createdBeforeOrAtHeight.toString() | |||
: undefined, | |||
createdBeforeOrAt, | |||
page, | |||
}, | |||
[QueryableField.LIMIT], | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the ordering to be by eventId desc here? To make transfer ordering deterministic if they occurred at the same height.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -165,6 +179,7 @@ class TransfersController extends Controller { | |||
? createdBeforeOrAtHeight.toString() | |||
: undefined, | |||
createdBeforeOrAt, | |||
page, | |||
}, | |||
[QueryableField.LIMIT], | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the ordering to be by eventId desc here? To make transfer ordering deterministic if they occurred at the same height.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- indexer/services/comlink/src/controllers/api/v4/fills-controller.ts (16 hunks)
- indexer/services/comlink/src/controllers/api/v4/trades-controller.ts (8 hunks)
- indexer/services/comlink/src/controllers/api/v4/transfers-controller.ts (14 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 (12)
indexer/services/comlink/src/controllers/api/v4/trades-controller.ts (4)
28-28
: Import statement forCheckPaginationSchema
looks good.
50-50
: Addition ofpage
query parameter ingetTrades
method is appropriate.
Line range hint
59-76
: The changes to thefindAll
method call to include pagination parameters and ordering are correct and necessary for pagination support.
83-85
: Inclusion of pagination fields (pageSize
,totalResults
,offset
) in the response object is appropriate.indexer/services/comlink/src/controllers/api/v4/transfers-controller.ts (4)
33-33
: Import statement forCheckPaginationSchema
looks good.
65-65
: Addition ofpage
query parameter ingetTransfers
method is appropriate.
Line range hint
71-94
: The changes to thefindAllToOrFromSubaccountId
method call to include pagination parameters and ordering are correct and necessary for pagination support.
142-144
: Inclusion of pagination fields (pageSize
,totalResults
,offset
) in the response object is appropriate.indexer/services/comlink/src/controllers/api/v4/fills-controller.ts (4)
37-37
: Import statement forCheckPaginationSchema
looks good.
65-65
: Addition ofpage
query parameter ingetFills
method is appropriate.
Line range hint
79-96
: The changes to thefindAll
method call to include pagination parameters and ordering are correct and necessary for pagination support.
116-118
: Inclusion of pagination fields (pageSize
,totalResults
,offset
) in the response object is appropriate.
https://github.com/Mergifyio backport release/indexer/v5.x |
✅ Backports have been created
|
* feat: ✨ add pagination for getFills * feat: ✨ add pagination support for getTrades * feat: ✨ add pagination support for getTransfers * test(comlink): ✅ add trades controller pagination tests * test(comlink): ✅ add transfers controller pagination tests * test(comlink): ✅ add fills controller pagination tests * feat: ✨ add pagination check schema * test(comlink): ✅ update pagination tests * feat: ✨ add default orderBy for pagination (cherry picked from commit 1aa9e26)
* feat: ✨ add pagination for getFills * feat: ✨ add pagination support for getTrades * feat: ✨ add pagination support for getTransfers * test(comlink): ✅ add trades controller pagination tests * test(comlink): ✅ add transfers controller pagination tests * test(comlink): ✅ add fills controller pagination tests * feat: ✨ add pagination check schema * test(comlink): ✅ update pagination tests * feat: ✨ add default orderBy for pagination (cherry picked from commit 1aa9e26) Co-authored-by: Davide Segullo <davidesegullo@gmail.com>
Changelist
This pr is a continuation of PR #1244.
In particular, it modifies the controllers of fills, trades, and transfers to support paging in REST services.
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
state-breaking
label.indexer-postgres-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.Summary by CodeRabbit
New Features
page
,pageSize
,totalResults
,offset
) to API endpoints for fills, trades, and transfers.Bug Fixes
createdAtHeight
and correctly paginated.Documentation
Tests