fix: pagination returns less than the page size when decryption fails #1547
Conversation
- Simplified `getChannelsByTopic` and `getChannelsByMultipleTopics` methods by removing pagination parameters (page and pageSize) across various data access layers. - Updated related methods and interfaces to reflect this change, ensuring consistency in data retrieval. - Adjusted tests and API handlers to accommodate the new method signatures. This refactor aims to streamline data access and improve code maintainability.
WalkthroughThis pull request introduces a significant simplification of pagination-related methods across multiple packages in the Request Network project. The changes primarily involve removing Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ 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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
packages/transaction-manager/src/transaction-manager.ts (3)
230-326: Consider adding comprehensive unit tests for the new pagination logic.The
parseMultipleChannelsmethod now incorporates complex pagination logic, including sorting and filtering channels. To ensure correctness and reliability, especially with various edge cases (e.g., empty channels, large datasets, undefinedpageorpageSize), add unit tests that cover these scenarios.
328-332: Enhance type safety by specifying concrete types instead ofany.The use of
any[]inisValidChannelreduces type safety and may lead to runtime errors. Replaceany[]with specific types to improve code reliability and maintainability.Apply this diff to improve type annotations:
-private isValidChannel(cleaned: { transactions: any[]; ignoredTransactions: any[] }): boolean { +private isValidChannel(cleaned: { transactions: TransactionTypes.ITransaction[]; ignoredTransactions: TransactionTypes.ITransaction[] }): boolean {
359-363: Avoid unnecessary type assertions to improve code clarity.The double casting using
as unknown asmay hide potential type issues and makes the code less readable. Ensure thatcleaned.transactionsandcleaned.ignoredTransactionsare correctly typed to eliminate the need for such assertions.Apply this diff to remove unnecessary casts:
-result.transactions[channelId] = - cleaned.transactions as unknown as TransactionTypes.ITransaction[]; -result.ignoredTransactions[channelId] = - cleaned.ignoredTransactions as unknown as TransactionTypes.ITransaction[]; +result.transactions[channelId] = cleaned.transactions; +result.ignoredTransactions[channelId] = cleaned.ignoredTransactions;If
cleaneddoes not have the correct types, consider updating its type definition accordingly.packages/data-access/src/in-memory-indexer.ts (1)
Line range hint
58-71: Consider implementing batch processing for large datasets.While using Set for deduplication is efficient, processing all transactions at once could consume significant memory for large datasets.
Consider implementing batch processing:
async getTransactionsByTopics(topics: string[]): Promise<StorageTypes.IGetTransactionsResponse> { const channelIdsSet = new Set(topics.flatMap((topic) => this.#topicToChannelsIndex.get(topic))); const channelIds = Array.from(channelIdsSet); + // Process locations in batches + const BATCH_SIZE = 100; const locations = channelIds .map((channel) => this.#channelToLocationsIndex.get(channel)) .flat(); - const transactions = await this.parseDocuments(locations); + const transactions = []; + for (let i = 0; i < locations.length; i += BATCH_SIZE) { + const batch = locations.slice(i, i + BATCH_SIZE); + const batchTransactions = await this.parseDocuments(batch); + transactions.push(...batchTransactions); + } return { blockNumber: 0, transactions, }; }packages/data-access/src/data-read.ts (1)
73-88: Consider memory usage when handling large transaction sets.The new implementation loads all transactions into memory before filtering. While this simplifies the logic, it could impact performance with large datasets.
Consider implementing streaming or chunked processing if memory usage becomes a concern with large transaction sets.
packages/request-client.js/src/api/request-network.ts (1)
398-407: Consider extracting common pagination logic.The pagination handling logic is duplicated across multiple methods. Consider extracting it into a utility function.
private wrapWithPaginationMeta<T>( data: T[], meta: RequestLogicTypes.IReturnMeta, options?: { page?: number; pageSize?: number } ): T[] | { meta: RequestLogicTypes.IReturnGetRequestsByTopic['meta']; requests: T[] } { if (options?.page && options?.pageSize) { return { requests: data, meta }; } return data; }Usage:
- if (options?.page && options?.pageSize) { - return { - requests, - meta: requestsAndMeta.meta, - }; - } else { - return requests; - } + return this.wrapWithPaginationMeta(requests, requestsAndMeta.meta, options);Also applies to: 473-481
packages/request-logic/src/request-logic.ts (2)
629-629: LGTM! Consider adding type safety for pagination.The addition of pagination metadata from transactionManagerMeta aligns with moving pagination logic to a higher level.
Consider adding type safety by explicitly typing the pagination property:
- pagination: transactionManagerMeta.pagination, + pagination: transactionManagerMeta.pagination as { hasMore: boolean; nextPage: number },
651-655: LGTM! Consider handling undefined pagination.The spread operator usage correctly merges pagination metadata while preserving existing data.
Consider handling cases where pagination might be undefined:
- finalResult.meta.pagination = { - ...finalResult.meta.pagination, - ...requestAndMeta.pagination, - }; + if (requestAndMeta.pagination) { + finalResult.meta.pagination = { + ...finalResult.meta.pagination, + ...requestAndMeta.pagination, + }; + }packages/integration-test/test/node-client.test.ts (1)
276-276: LGTM! Consider using type inference.The explicit type annotations improve type safety and clarity.
Consider leveraging TypeScript's type inference by using
as constand letting the compiler infer the type:- let requests: Request[] = (await requestNetwork.fromTopic(topicsRequest1and2[0])) as Request[]; + let requests = await requestNetwork.fromTopic(topicsRequest1and2[0]) as const;Also applies to: 289-291, 344-346
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/data-access/src/combined-data-access.ts(1 hunks)packages/data-access/src/data-read.ts(2 hunks)packages/data-access/src/in-memory-indexer.ts(1 hunks)packages/integration-test/test/node-client.test.ts(3 hunks)packages/request-client.js/src/api/request-network.ts(6 hunks)packages/request-client.js/src/http-data-access.ts(2 hunks)packages/request-logic/src/request-logic.ts(2 hunks)packages/request-node/src/request/getChannelsByTopic.ts(1 hunks)packages/thegraph-data-access/src/queries.ts(2 hunks)packages/thegraph-data-access/src/subgraph-client.ts(1 hunks)packages/transaction-manager/src/transaction-manager.ts(4 hunks)packages/types/src/data-access-types.ts(0 hunks)packages/types/src/request-logic-types.ts(1 hunks)packages/types/src/storage-types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/types/src/data-access-types.ts
🧰 Additional context used
📓 Learnings (1)
packages/data-access/src/data-read.ts (1)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1508
File: packages/data-access/src/data-read.ts:115-134
Timestamp: 2024-12-04T17:31:54.981Z
Learning: In `packages/data-access/src/data-read.ts`, within the `getChannelsByMultipleTopics` method, including transactions outside the `updatedBetween` range when retrieving all transactions for the identified channels is intentional and expected behavior, as specified in the code comments.
🔇 Additional comments (10)
packages/transaction-manager/src/transaction-manager.ts (2)
199-201: Pagination logic moved toparseMultipleChannelsmethod.The removal of
pageandpageSizefrom thedataAccess.getChannelsByTopiccall and passing them toparseMultipleChannelsaligns with the new pagination handling strategy. Ensure that this change does not affect the expected behavior and thatparseMultipleChannelscorrectly handles pagination.
373-375: Verify the correctness oftotalValidChannelscalculation.The formula used in
calculateTotalValidChannelsmay lead to inaccurate totals ifprocessedCountdoes not properly reflect the number of channels processed. Ensure that this calculation accurately represents the total number of valid channels, especially when paginating results.Consider adding tests or using the following script to check the calculation logic:
✅ Verification successful
The
totalValidChannelscalculation is mathematically sound and efficient.The implementation correctly handles both paginated and non-paginated scenarios:
- Without pagination: Returns exact count of valid channels
- With pagination: Uses statistical extrapolation based on the success rate of processed channels, which is an efficient approach for large datasets where processing all channels would be expensive
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify totalValidChannels calculation logic. # Expected: The total number of valid channels should be consistent with the actual number of valid channels. # Command to check the calculation (this is a placeholder and may require actual implementation context). echo "Please review the calculation of totalValidChannels in calculateTotalValidChannels method."Length of output: 190
Script:
#!/bin/bash # Find the complete implementation of the method echo "=== Method Implementation ===" ast-grep --pattern 'calculateTotalValidChannels($_) { $$$ }' echo -e "\n=== Test Files ===" fd -e test.ts -e spec.ts -e test.js -e spec.js --exec grep -l "calculateTotalValidChannels" echo -e "\n=== Method Usage ===" rg "calculateTotalValidChannels" -A 2Length of output: 916
Script:
#!/bin/bash # Get the complete file content echo "=== Complete File Content ===" cat packages/transaction-manager/src/transaction-manager.ts echo -e "\n=== Method Implementation with Context ===" ast-grep --pattern 'private calculateTotalValidChannels( $_, $_, $_ ) { $$$ }'Length of output: 14220
packages/request-node/src/request/getChannelsByTopic.ts (1)
18-26: Removal of pagination parameters from request handling is appropriate.The elimination of
pageandpageSizefrom query parameters simplifies the handler and aligns with the updated data access methods. Ensure that client applications are updated accordingly to reflect this change.packages/data-access/src/combined-data-access.ts (1)
35-35: Updated method signatures reflect the new pagination approach.The removal of
pageandpageSizefromgetChannelsByTopicandgetChannelsByMultipleTopicsaligns with the shift of pagination logic to higher levels of the application. This simplifies the data access layer and focuses on retrieving all relevant data.Also applies to: 41-41
packages/thegraph-data-access/src/queries.ts (2)
110-119: LGTM! Efficient implementation of the count query.The count query is well-implemented as it only retrieves the necessary
idfield, making it efficient for large datasets.
80-89: Consider adding a safety limit to prevent excessive data retrieval.While removing pagination from the GraphQL query aligns with the PR objectives, fetching all transactions without limits could lead to performance issues if a channel contains many transactions.
Consider either:
- Adding a maximum limit in the GraphQL query
- Implementing cursor-based pagination instead of offset pagination
query GetTransactionsByTopics($topics: [String!]!) { ${metaQueryBody} channels( where: { topics_contains: $topics } ){ transactions( orderBy: blockTimestamp, - orderDirection: asc + orderDirection: asc, + first: 1000 # Add a reasonable maximum limit ) { ...TransactionsBody } } }packages/data-access/src/data-read.ts (2)
51-53: LGTM! Method signature simplified.The removal of pagination parameters aligns with the PR objective to handle pagination at a higher level.
90-97: Verify transaction ordering within channels.The grouping logic maintains the chronological order of transactions within each channel due to the prior sorting step. However, ensure this ordering is required by consumers of this API.
✅ Verification successful
Transaction ordering is correctly implemented and required.
The chronological ordering of transactions is explicitly maintained by sorting by
blockTimestampbefore grouping, and this order is required by the request logic for correct state processing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for code that relies on transaction ordering within channels rg -A 3 "transactionsByChannel|transactions: \{" --type tsLength of output: 9623
Script:
#!/bin/bash # Search for sorting operations in data-read.ts rg -B 10 "transactionsByChannel" packages/data-access/src/data-read.tsLength of output: 771
Script:
#!/bin/bash # Search for allTransactions definition and sorting operations rg -B 5 "allTransactions" packages/data-access/src/data-read.tsLength of output: 658
packages/types/src/storage-types.ts (1)
63-63: LGTM! Interface simplified.The removal of pagination parameters from
getTransactionsByTopicsis consistent with moving pagination handling to a higher level.packages/request-client.js/src/api/request-network.ts (1)
297-299: LGTM! Return type enhancement supports pagination metadata.The union type allows for backward compatibility while supporting pagination metadata when needed.
Also applies to: 322-324, 352-354, 426-428
github-actions
bot
requested review from
alexandre-abrioux,
kevindavee,
leoslr and
sstefdev
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
packages/request-client.js/test/api/request-network.test.ts (2)
Line range hint
230-235: Remove unused 'page' and 'pageSize' parameters from mock 'getChannelsByMultipleTopics'The
getChannelsByMultipleTopicsmethod no longer acceptspageandpageSizeparameters. Update the mock implementation to reflect the updated method signature.Apply this diff to update the method signature:
async getChannelsByMultipleTopics( topics: [string], updatedBetween?: DataAccessTypes.ITimestampBoundaries, - page?: number, - pageSize?: number, ): Promise<any> {
Line range hint
301-306: Remove unused 'page' and 'pageSize' parameters from mock 'getChannelsByMultipleTopics'Update the mock implementation of
getChannelsByMultipleTopicsto remove thepageandpageSizeparameters, reflecting the updated method signature.Apply this diff to update the method signature:
async getChannelsByMultipleTopics(): Promise<any> { return { meta: { [TestData.actionRequestId]: [], [TestData.actionRequestIdSecondRequest]: [], - transactionsStorageLocation: {}, },
🧹 Nitpick comments (12)
packages/transaction-manager/test/index.test.ts (8)
49-53: Specify a precise type for 'pagination' instead of setting it to undefinedSetting
paginationtoundefinedwithout a clear type reduces clarity and type safety. Consider specifying the correct type forpaginationand providing meaningful initial values.Apply this diff to define the type and initialize
paginationproperly:meta: { - pagination: undefined, + pagination: {}, storageMeta: {}, transactionsStorageLocation: { [channelId]: ['fakeDataId1', 'fakeDataId2'] }, },
1137-1143: Update assertions to include 'pagination' in metadataSince
paginationwas added to themetaobject, update the assertions in the test to include and verify thepaginationproperty.Apply this diff to update the assertions:
expect(ret.meta).toEqual({ dataAccessMeta: fakeMetaDataAccessGetChannelsReturn.meta, ignoredTransactions: { '01a98f126de3fab2b5130af5161998bf6e59b2c380deafeff938ff3f798281bf23': [null, null], }, + pagination: undefined, });
1156-1157: Remove unnecessary 'pagination' property or provide a proper typeThe
paginationproperty is set toundefinedin the test metadata. If pagination is not used in this context, consider removing it; otherwise, provide it with a proper value and type.Apply this diff:
meta: { - pagination: undefined, + // pagination: { page: 1, pageSize: 10 }, storageMeta: {}, transactionsStorageLocation: { [channelId]: ['fakeDataId1'], },
1201-1213: Update test expectations to include 'pagination' in metadataAdjust the test assertions to include the
paginationproperty in themetaobject since it's now part of the returned data.Apply this diff to the assertions:
expect(ret.meta).toEqual({ dataAccessMeta: { + pagination: undefined, storageMeta: {}, transactionsStorageLocation: { [channelId]: ['fakeDataId1'], }, }, ignoredTransactions: { [channelId]: [null], }, });
1353-1376: Include 'pagination' in test metadata and assertionsUpdate the test metadata and corresponding assertions to include the
paginationproperty, ensuring consistency with the updated data structures.Adjust the metadata as follows:
expect(ret.meta).toEqual({ dataAccessMeta: { + pagination: undefined, storageMeta: {}, transactionsStorageLocation: { [channelId]: ['fakeDataId1', 'fakeDataId2'], }, }, ignoredTransactions: { [channelId]: [ { reason: 'No decryption or cipher provider given', transaction: { state: TransactionTypes.TransactionState.PENDING, timestamp: 1, transaction: encryptedTx, }, }, null, ], }, });
1414-1434: Adjust test expectations to account for 'pagination' in metadataUpdate the assertions in the test to include the
paginationproperty within thedataAccessMetaobject.Apply this diff to the assertions:
expect(ret.meta).toEqual({ dataAccessMeta: { + pagination: undefined, storageMeta: {}, transactionsStorageLocation: { [channelId]: ['fakeDataId1', 'fakeDataId1', 'fakeDataId2'], }, }, ignoredTransactions: { [channelId]: [ { reason: 'as first transaction, the hash of the transaction do not match the channelId', transaction: txWrongHash, }, null, null, ], }, });
1500-1517: Include 'pagination' in test metadata and verify its correctnessEnsure that the
paginationproperty is included in themetaobject and that the test assertions verify its presence and correctness.Update the test metadata and assertions:
expect(ret.meta).toEqual({ dataAccessMeta: { + pagination: undefined, storageMeta: {}, transactionsStorageLocation: { [channelId]: ['fakeDataId1'], [channelId2]: ['fakeDataId2'], }, }, ignoredTransactions: { [channelId]: [null], [channelId2]: [null], }, });
1530-1535: Update assertions to reflect added 'pagination' in metadataSince
paginationis now part of themetaobject, update the test assertions to include and verify it.Modify the assertions:
expect(ret.meta).toEqual({ dataAccessMeta: fakeMetaDataAccessGetChannelsReturn.meta, ignoredTransactions: { '01a98f126de3fab2b5130af5161998bf6e59b2c380deafeff938ff3f798281bf23': [null, null], }, + pagination: undefined, });packages/transaction-manager/src/transaction-manager.ts (1)
365-369: Improve channel validation logic.The validation could be more robust by checking for additional conditions.
private isValidChannel(cleaned: { transactions: any[]; ignoredTransactions: any[] }): boolean { - return ( - cleaned.transactions && cleaned.transactions.length > 0 && cleaned.transactions[0] !== null - ); + return Boolean( + cleaned.transactions?.length > 0 && + cleaned.transactions[0] !== null && + cleaned.transactions[0] !== undefined + ); }packages/request-client.js/test/index.test.ts (3)
861-864: LGTM! Consider adding error handling test cases.The response handling for
fromTopicis well-structured and handles both array and object responses consistently.Consider adding test cases for error scenarios:
it('handles error responses from fromTopic', async () => { // Test case for network errors // Test case for invalid response format // Test case for empty response });
902-909: LGTM! Consider adding pagination test cases.The response handling for
fromMultipleTopicsfollows the same consistent pattern and properly handles the response structure.Consider adding test cases for pagination scenarios:
it('handles pagination in fromMultipleTopics response', async () => { // Test case for response with next page // Test case for response with previous page // Test case for response with both next and previous pages });
935-938: LGTM! Consider adding validation test cases.The response handling for
fromIdentitymaintains consistency with the other methods.Consider adding test cases for identity validation:
it('validates identity in fromIdentity response', async () => { // Test case for invalid identity format // Test case for non-existent identity // Test case for unauthorized identity });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/data-access/src/data-read.ts(1 hunks)packages/request-client.js/src/http-data-read.ts(0 hunks)packages/request-client.js/test/api/request-network.test.ts(8 hunks)packages/request-client.js/test/index.test.ts(3 hunks)packages/transaction-manager/src/transaction-manager.ts(4 hunks)packages/transaction-manager/test/index.test.ts(13 hunks)packages/types/src/data-access-types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/request-client.js/src/http-data-read.ts
🧰 Additional context used
📓 Learnings (1)
packages/data-access/src/data-read.ts (1)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1508
File: packages/data-access/src/data-read.ts:115-134
Timestamp: 2024-12-04T17:31:54.981Z
Learning: In `packages/data-access/src/data-read.ts`, within the `getChannelsByMultipleTopics` method, including transactions outside the `updatedBetween` range when retrieving all transactions for the identified channels is intentional and expected behavior, as specified in the code comments.
🔇 Additional comments (9)
packages/request-client.js/test/api/request-network.test.ts (1)
175-178:⚠️ Potential issueRemove unused 'page' and 'pageSize' parameters from mock 'getChannelsByTopic'
Ensure the
getChannelsByTopicmock implementation does not include obsolete parameterspageandpageSize.Apply this diff:
async getChannelsByTopic(): Promise<any> { return { meta: { [TestData.actionRequestId]: [], [TestData.actionRequestIdSecondRequest]: [], - transactionsStorageLocation: {}, },Likely invalid or redundant comment.
packages/transaction-manager/test/index.test.ts (3)
970-974: Ensure consistent handling of encrypted transactions in testsThe test case mixes encrypted and clear transactions, which might not accurately reflect expected behavior. Verify that the test correctly handles cases where clear transactions follow encrypted ones and adjust the expectations accordingly.
Review the test logic and ensure it aligns with the expected behavior when dealing with encrypted channels.
1261-1290: Clarify handling of ignored transactions in metadataIn the absence of a decryption provider, the transactions are ignored, and the reason is specified. Ensure that the
ignoredTransactionsandmetaobjects accurately reflect this and includepaginationif applicable.Review the test output for correctness and completeness, ensuring consistent inclusion of
paginationand clear reasons for ignored transactions.
1546-1568: Ensure 'pagination' is correctly initialized in test mock dataIn the
fakeMetaDataAccessGetChannelsReturnobject, thepaginationproperty is provided with values. Ensure this is intentional and consistent throughout the tests.Review the test data to confirm that
paginationis correctly used and reflects realistic pagination metadata.packages/data-access/src/data-read.ts (2)
53-53: LGTM! Simplified method implementation.The implementation correctly delegates to
getChannelsByMultipleTopicswith a single topic, following the DRY principle.
60-60: LGTM! Simplified method signature.The removal of pagination parameters aligns with the PR objectives to move pagination logic to a higher level.
packages/transaction-manager/src/transaction-manager.ts (3)
199-201: LGTM! Simplified method implementation.The implementation correctly moves pagination handling to
parseMultipleChannels.
217-219: LGTM! Simplified method implementation.The implementation correctly moves pagination handling to
parseMultipleChannels.
297-312: LGTM! Well-structured pagination implementation.The pagination logic is clear and handles edge cases correctly.
MantisClone
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/transaction-manager/src/transaction-manager.ts (2)
365-369: Ensure all transactions are valid when determining if a channel is validThe
isValidChannelmethod currently checks only the first transaction for null. It's safer to verify that all transactions are not null to accurately determine the validity of a channel.Apply this diff to check all transactions:
- return ( - cleaned.transactions && cleaned.transactions.length > 0 && cleaned.transactions[0] !== null - ); + return ( + cleaned.transactions && + cleaned.transactions.length > 0 && + cleaned.transactions.every((tx) => tx !== null) + );
265-295: Optimize channel processing to avoid redundant decryptionCurrently,
decryptAndCleanChannelis called twice for the same channels: once to check validity and again during processing. This can be inefficient, especially with a large number of channels.Consider storing the result of the initial decryption to reuse later, reducing unnecessary computations.
packages/thegraph-data-access/src/queries.ts (2)
80-80: Potential performance impact due to removal of paginationBy removing the
$firstand$skippagination parameters, theGetTransactionsByTopicsquery might retrieve a large number of transactions in a single request, which could affect performance and exceed query limits.Consider reintroducing pagination to manage the volume of data returned and to optimize query performance.
110-119: Optimize transaction count retrievalThe
GetTransactionsByTopicsCountquery fetches all transaction IDs to determine the count, which can be inefficient for topics with many transactions.If supported, modify the query to directly return the count without fetching all transaction IDs. This can improve performance by reducing the amount of data transferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/data-access/src/combined-data-access.ts(1 hunks)packages/data-access/src/data-read.ts(1 hunks)packages/data-access/src/in-memory-indexer.ts(1 hunks)packages/integration-test/test/node-client.test.ts(3 hunks)packages/request-client.js/src/api/request-network.ts(6 hunks)packages/request-client.js/src/http-data-access.ts(2 hunks)packages/request-client.js/src/http-data-read.ts(0 hunks)packages/request-client.js/test/api/request-network.test.ts(8 hunks)packages/request-client.js/test/index.test.ts(3 hunks)packages/request-logic/src/request-logic.ts(2 hunks)packages/request-node/src/request/getChannelsByTopic.ts(1 hunks)packages/thegraph-data-access/src/queries.ts(2 hunks)packages/thegraph-data-access/src/subgraph-client.ts(1 hunks)packages/transaction-manager/src/transaction-manager.ts(4 hunks)packages/transaction-manager/test/index.test.ts(13 hunks)packages/types/src/data-access-types.ts(1 hunks)packages/types/src/request-logic-types.ts(1 hunks)packages/types/src/storage-types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/request-client.js/src/http-data-read.ts
🧰 Additional context used
📓 Learnings (4)
packages/request-client.js/test/api/request-network.test.ts (1)
Learnt from: rodrigopavezi
PR: RequestNetwork/requestNetwork#1547
File: packages/request-client.js/test/api/request-network.test.ts:267-273
Timestamp: 2025-01-21T01:54:03.941Z
Learning: In the RequestNetwork class, pagination functionality is supported in methods like fromMultipleIdentities, fromTopic, etc., with options for page and pageSize. The pagination was moved to a higher level but not removed.
packages/data-access/src/data-read.ts (1)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1508
File: packages/data-access/src/data-read.ts:115-134
Timestamp: 2024-12-04T17:31:54.981Z
Learning: In `packages/data-access/src/data-read.ts`, within the `getChannelsByMultipleTopics` method, including transactions outside the `updatedBetween` range when retrieving all transactions for the identified channels is intentional and expected behavior, as specified in the code comments.
packages/types/src/data-access-types.ts (1)
Learnt from: rodrigopavezi
PR: RequestNetwork/requestNetwork#1547
File: packages/types/src/data-access-types.ts:95-95
Timestamp: 2025-01-21T01:49:12.846Z
Learning: In the RequestNetwork codebase, the `pagination` property in `IReturnGetChannelsByTopic` interface is intentionally typed as `any` to maintain flexibility. A more specific type can be considered in future improvements.
packages/request-client.js/src/api/request-network.ts (1)
Learnt from: rodrigopavezi
PR: RequestNetwork/requestNetwork#1547
File: packages/request-client.js/test/api/request-network.test.ts:147-150
Timestamp: 2025-01-21T01:54:29.040Z
Learning: In the RequestNetwork class, methods like `fromIdentity`, `fromTopic`, `fromMultipleIdentities`, and `fromMultipleTopics` support pagination options and can return either an array of Request objects or an object containing both requests and pagination metadata.
🔇 Additional comments (25)
packages/transaction-manager/src/transaction-manager.ts (1)
291-294: Consider improving error handling and loggingThe error handling in the catch block could be more informative and utilize a proper logging mechanism instead of
console.warn. This can help in better monitoring and debugging of issues in production environments.packages/data-access/src/combined-data-access.ts (1)
35-35: Simplification of method interfaces looks goodThe removal of pagination parameters from
getChannelsByTopicandgetChannelsByMultipleTopicssimplifies the method signatures and aligns with the updated pagination handling.Also applies to: 41-41
packages/data-access/src/in-memory-indexer.ts (1)
Line range hint
58-71: LGTM! Simplified transaction retrieval aligns with architectural changes.The removal of pagination parameters and simplified implementation aligns with moving pagination logic to a higher level. This change helps ensure correct handling of decrypted requests in paginated results.
Let's verify that pagination is now handled at a higher level:
✅ Verification successful
Verified: Pagination is correctly implemented in TransactionManager
The removal of pagination from the in-memory-indexer is appropriate as pagination is properly handled in the TransactionManager's
parseMultipleChannelsmethod, which:
- Processes and validates all transactions first
- Applies pagination after decryption and validation
- Handles both paginated and non-paginated cases efficiently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that pagination is handled at the transaction manager level # Expected: Find pagination logic in transaction manager rg -A 5 "parseMultipleChannels.*page.*pageSize" packages/transaction-manager/src/Length of output: 1043
Script:
#!/bin/bash # Description: Examine the implementation of parseMultipleChannels ast-grep --pattern 'parseMultipleChannels($$$) { $$$ }'Length of output: 59
Script:
#!/bin/bash # Description: Find parseMultipleChannels implementation using ripgrep rg -A 15 "private parseMultipleChannels" packages/transaction-manager/src/Length of output: 76
Script:
#!/bin/bash # Description: Find parseMultipleChannels implementation with broader search rg -A 20 "parseMultipleChannels" packages/transaction-manager/src/transaction-manager.tsLength of output: 2055
Script:
#!/bin/bash # Description: Get the remaining implementation of parseMultipleChannels rg -B 5 -A 30 "filteredTransactions," packages/transaction-manager/src/transaction-manager.tsLength of output: 2499
packages/types/src/data-access-types.ts (1)
95-95: Flexible pagination type maintained.The
anytype for pagination is intentionally kept flexible, as confirmed by previous discussions.packages/request-client.js/src/http-data-access.ts (1)
114-114: Pagination removal aligns with architectural changes.Removing pagination parameters from HTTP data access is consistent with handling pagination at the transaction manager level.
Also applies to: 127-127
packages/data-access/src/data-read.ts (2)
53-53: LGTM! Clean and efficient implementation.The simplified implementation correctly delegates to
getChannelsByMultipleTopics, maintaining functionality while reducing code duplication.
Line range hint
60-102: LGTM! Robust transaction handling with proper filtering.The implementation:
- Correctly retrieves and combines storage and pending transactions
- Properly filters channels based on updatedBetween boundaries
- Efficiently constructs the response structure
packages/types/src/storage-types.ts (1)
63-63: Breaking change: Method signature updated in IIndexer interface.The removal of pagination parameters from
getTransactionsByTopicsis a breaking change. While this aligns with the PR objective to fix pagination issues, it requires updates in all implementations of theIIndexerinterface.Let's verify the impact:
packages/types/src/request-logic-types.ts (1)
101-101: LGTM: Addition of pagination metadata.The addition of the
paginationproperty toIReturnMetaaligns with the codebase's design choice of usinganytype for flexibility.packages/request-client.js/src/api/request-network.ts (2)
297-299: LGTM: Updated return types to support pagination metadata.The return type changes consistently handle both paginated and non-paginated responses across all methods.
Also applies to: 322-324, 352-354, 426-428
400-407: LGTM: Consistent pagination handling logic.The conditional logic ensures that pagination metadata is only included when pagination parameters are provided, maintaining backward compatibility.
Also applies to: 474-481
packages/request-logic/src/request-logic.ts (1)
629-629: LGTM: Proper handling of pagination metadata.The code correctly extracts and merges pagination metadata from multiple requests, ensuring that pagination information is preserved throughout the request processing chain.
Also applies to: 652-655
packages/request-client.js/test/api/request-network.test.ts (7)
145-149: LGTM! Pagination options are correctly maintained.The test case correctly verifies that pagination options (
pageandpageSize) are still supported in thefromIdentitymethod, which aligns with the retrieved learning that pagination was moved to a higher level but not removed.
202-203: LGTM! Result handling is consistent with the new response structure.The test case correctly handles the response from
fromTopicby checking if the result is an array or an object containing requests, which aligns with the changes to support consistent pagination handling at a higher level.
265-273: LGTM! Pagination options are correctly maintained.The test case correctly verifies that pagination options (
pageandpageSize) are still supported in thefromMultipleIdentitiesmethod, which aligns with the retrieved learning that pagination was moved to a higher level but not removed.
328-329: LGTM! Result handling is consistent with the new response structure.The test case correctly handles the response from
fromMultipleTopicsby checking if the result is an array or an object containing requests, which aligns with the changes to support consistent pagination handling at a higher level.
Line range hint
861-864: LGTM! Result handling is consistent for encrypted requests.The test case correctly handles the response from
fromTopicfor encrypted requests by checking if the result is an array or an object containing requests, which ensures consistent pagination handling even with encryption.
Line range hint
902-909: LGTM! Result handling is consistent for encrypted requests with multiple topics.The test case correctly handles the response from
fromMultipleTopicsfor encrypted requests by checking if the result is an array or an object containing requests, and verifies the data integrity of both requests.
Line range hint
935-938: LGTM! Result handling is consistent for encrypted requests with identity.The test case correctly handles the response from
fromIdentityfor encrypted requests by checking if the result is an array or an object containing requests, which ensures consistent pagination handling even with encryption.packages/request-client.js/test/index.test.ts (3)
861-864: LGTM! Integration test verifies result handling for encrypted requests.The integration test correctly verifies that the response from
fromTopicfor encrypted requests is handled consistently, checking if the result is an array or an object containing requests, and validates the decrypted data.
902-909: LGTM! Integration test verifies result handling for encrypted requests with multiple topics.The integration test correctly verifies that the response from
fromMultipleTopicsfor encrypted requests is handled consistently, checking if the result is an array or an object containing requests, and validates the decrypted data for both requests.
935-938: LGTM! Integration test verifies result handling for encrypted requests with identity.The integration test correctly verifies that the response from
fromIdentityfor encrypted requests is handled consistently, checking if the result is an array or an object containing requests, and validates the decrypted data.packages/integration-test/test/node-client.test.ts (1)
276-277: LGTM! Type assertions improve type safety.The added type assertions for the
requestsvariable ensure proper typing of the return value fromfromTopiccalls, making the code more type-safe.Also applies to: 289-291
packages/transaction-manager/test/index.test.ts (1)
49-53: LGTM! Test expectations updated to match new metadata structure.The test cases have been properly updated to:
- Include
paginationandstorageMetafields in metadata- Verify correct handling of decryption failures
- Ensure proper error messages in ignored transactions
Also applies to: 1137-1143, 1156-1157, 1201-1213, 1226-1227, 1261-1290, 1293-1376, 1414-1434, 1447-1448, 1500-1517, 1530-1535, 1546-1546, 1552-1568, 1581-1585
packages/thegraph-data-access/src/subgraph-client.ts (1)
70-74: Verify error handling for GraphQL query failures.Since this change is part of fixing decryption failures, ensure that GraphQL query failures are properly propagated to help diagnose issues.
#!/bin/bash # Description: Check error handling patterns in GraphQL queries # Test: Search for error handling patterns in GraphQL queries echo "Checking error handling in GraphQL queries..." rg -A 5 "graphql\.request" # Test: Search for error types and handling echo "Checking error types..." ast-grep --pattern 'catch (e) { $$$ }'
There was a problem hiding this comment.
Approved 👍
@rodrigopavezi and I briefly discussed adding more tests. I leave it to @rodrigopavezi to determine how much additional testing to add, if any.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/transaction-manager/src/transaction-manager.ts (3)
235-296: Consider optimizing channel processing and error handling.
- The channel processing could be optimized by using array methods instead of multiple iterations
- Error handling uses console.warn which may not be appropriate for production
Consider these improvements:
- channelsWithTimestamps = Object.entries(resultGetTx.result.transactions).map( - ([channelId, transactions]) => { - const filteredTransactions = transactions.filter((tx) => { - const timestamp = tx.timestamp || 0; - if (updatedBetween?.from && timestamp < updatedBetween.from) return false; - if (updatedBetween?.to && timestamp > updatedBetween.to) return false; - return true; - }); - return { - channelId, - latestTimestamp: Math.max(...filteredTransactions.map((tx) => tx.timestamp || 0), 0), - hasValidTransactions: filteredTransactions.length > 0, - filteredTransactions, - }; - }, - ); + channelsWithTimestamps = Object.entries(resultGetTx.result.transactions) + .map(([channelId, transactions]) => { + const filteredTransactions = transactions.filter(tx => { + const timestamp = tx.timestamp || 0; + return (!updatedBetween?.from || timestamp >= updatedBetween.from) && + (!updatedBetween?.to || timestamp <= updatedBetween.to); + }); + const latestTimestamp = filteredTransactions.reduce((max, tx) => + Math.max(max, tx.timestamp || 0), 0); + return { + channelId, + latestTimestamp, + hasValidTransactions: filteredTransactions.length > 0, + filteredTransactions, + }; + });Also consider replacing console.warn with a proper logging system:
- console.warn(`Failed to decrypt channel ${channelId}:`, error); + logger.warn(`Failed to decrypt channel ${channelId}: ${error instanceof Error ? error.message : String(error)}`);
298-345: Consider adding constants for pagination logic.The pagination implementation is solid, but readability could be improved.
Consider these improvements:
+ const PAGINATION_META_FIELDS = ['storageMeta', 'transactionsStorageLocation'] as const; + if (page !== undefined && pageSize !== undefined) { const startIndex = (page - 1) * pageSize; const endIndex = Math.min(startIndex + pageSize, validChannels.length); channelsToProcess = validChannels.slice(startIndex, endIndex); paginationMeta = { total: validChannels.length, page, pageSize, hasMore: endIndex < validChannels.length, }; } // Create the final meta object const successfulChannelIds = Object.keys(result.transactions); const paginatedMeta = { ...resultGetTx.meta, - storageMeta: Object.keys(resultGetTx.meta.storageMeta || {}) - .filter((key) => successfulChannelIds.includes(key)) - .reduce((acc, key) => { - acc[key] = resultGetTx.meta.storageMeta?.[key] || []; - return acc; - }, {} as Record<string, StorageTypes.IEntryMetadata[]>), - transactionsStorageLocation: Object.keys(resultGetTx.meta.transactionsStorageLocation) - .filter((key) => successfulChannelIds.includes(key)) - .reduce((acc, key) => { - acc[key] = resultGetTx.meta.transactionsStorageLocation[key]; - return acc; - }, {} as Record<string, string[]>), + ...PAGINATION_META_FIELDS.reduce((acc, field) => ({ + ...acc, + [field]: Object.entries(resultGetTx.meta[field] || {}) + .filter(([key]) => successfulChannelIds.includes(key)) + .reduce((fieldAcc, [key, value]) => ({ + ...fieldAcc, + [key]: value, + }), {}), + }), {}), pagination: paginationMeta, };
358-362: Add type safety to channel validation.The current implementation could benefit from stronger typing.
Consider this improvement:
- private isValidChannel(cleaned: { transactions: any[]; ignoredTransactions: any[] }): boolean { + private isValidChannel(cleaned: { + transactions: (TransactionTypes.ITimestampedTransaction | null)[]; + ignoredTransactions: (TransactionTypes.IIgnoredTransaction | null)[]; + }): boolean { return ( cleaned.transactions && cleaned.transactions.length > 0 && cleaned.transactions[0] !== null ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/integration-test/test/node-client.test.ts(3 hunks)packages/request-client.js/src/api/request-network.ts(7 hunks)packages/request-logic/src/request-logic.ts(2 hunks)packages/thegraph-data-access/src/queries.ts(1 hunks)packages/transaction-manager/src/transaction-manager.ts(4 hunks)packages/transaction-manager/test/index.test.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/request-logic/src/request-logic.ts
- packages/thegraph-data-access/src/queries.ts
- packages/integration-test/test/node-client.test.ts
- packages/request-client.js/src/api/request-network.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (5)
packages/transaction-manager/src/transaction-manager.ts (4)
7-11: LGTM! Clean import additions for pagination support.The new imports for StorageTypes and validatePaginationParams are appropriate for the pagination implementation.
200-202: LGTM! Proper validation and pagination handling.The changes correctly:
- Validate pagination parameters before processing
- Move pagination handling to a higher level while keeping the data access layer focused on data retrieval
218-220: LGTM! Consistent implementation with getChannelsByTopic.The changes maintain a consistent approach to pagination handling across both channel retrieval methods.
231-233: LGTM! Clear method signature update.The added parameters properly support the pagination and timestamp filtering functionality.
packages/transaction-manager/test/index.test.ts (1)
1593-1854: LGTM! Comprehensive test coverage for pagination.The test suite thoroughly covers:
- Pagination functionality with various page sizes
- Edge cases (empty results, boundary conditions)
- Error handling for invalid parameters
- Timestamp boundaries with pagination
- Parameter validation
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 (3)
packages/transaction-manager/src/transaction-manager.ts(4 hunks)packages/transaction-manager/test/index.test.ts(13 hunks)packages/utils/src/utils.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (10)
packages/utils/src/utils.ts (2)
172-183: LGTM! Robust validation for pagination parameters.The validation logic correctly ensures that either both pagination parameters are provided or neither is provided, preventing partial pagination scenarios.
185-191: LGTM! Clear error messages for invalid pagination values.The error messages clearly indicate the validation failures and include the actual values that caused the failure.
packages/transaction-manager/src/transaction-manager.ts (6)
200-202: LGTM! Proper validation and parameter passing.The method correctly:
- Validates pagination parameters
- Passes timestamp boundaries to data access
- Forwards pagination handling to parseMultipleChannels
218-220: LGTM! Consistent pagination handling.The method mirrors the pagination handling approach of getChannelsByTopic, maintaining consistency across the API.
235-249: LGTM! Robust timestamp filtering and channel processing.The implementation correctly:
- Filters transactions based on timestamp boundaries
- Tracks the latest timestamp for each channel
- Maintains valid transaction state
253-257: LGTM! Proper channel ordering and filtering.The implementation:
- Filters out channels without valid transactions
- Sorts channels by latest timestamp
- Maintains channel order for pagination
277-277: Consider using a proper logging system.The console warning might not be appropriate for production code. Consider using a proper logging system.
357-361: LGTM! Clear validation of channel validity.The isValidChannel method properly checks for:
- Existence of transactions array
- Non-empty transactions
- Valid first transaction
packages/transaction-manager/test/index.test.ts (2)
1591-1677: LGTM! Comprehensive pagination tests.The tests thoroughly verify:
- Basic pagination with multiple pages
- Correct page size handling
- Proper metadata in response
- hasMore flag accuracy
1866-1937: LGTM! Thorough validation error testing.The tests comprehensively cover all validation error cases:
- Invalid page numbers
- Invalid page sizes
- Partial pagination parameters
- Both single and multiple topic scenarios
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/transaction-manager/test/index.test.ts (1)
1741-1801: Consider extracting common test setup code.The timestamp boundary test setup contains duplicated channel data structure that appears in multiple tests. Consider extracting this into a shared test fixture or helper function to improve maintainability.
Example refactor:
function createTestChannels() { return { [channelId]: [ { state: TransactionTypes.TransactionState.PENDING, timestamp: 2000, transaction: { data }, }, { state: TransactionTypes.TransactionState.PENDING, timestamp: 2500, transaction: { data }, }, ], [channelId2]: [ { state: TransactionTypes.TransactionState.PENDING, timestamp: 3000, transaction: { data: data2 }, }, ], }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/transaction-manager/test/index.test.ts(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (3)
packages/transaction-manager/test/index.test.ts (3)
49-53: Well-structured metadata initialization!The addition of
paginationandstorageMetafields to the metadata structure aligns well with the pagination improvements mentioned in the PR objectives.
1137-1142: Consistent pagination metadata handling!The test expectations have been properly updated to validate the new pagination metadata structure across different scenarios.
Also applies to: 1156-1157
1591-1874: Excellent pagination test coverage!The new test suite thoroughly covers pagination functionality including:
- Basic pagination with page and pageSize
- Empty result handling
- Parameter validation
- Timestamp boundary handling
- Error cases for invalid inputs
This comprehensive coverage will help ensure the reliability of the pagination implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/transaction-manager/src/transaction-manager.ts (1)
246-250: Simplify timestamp filtering logic.The nested ternary operations make the code harder to read. Consider extracting these checks into a separate function for better readability.
- const hasTransactionsAfterFrom = - !updatedBetween?.from || timestamps.some((t) => t >= updatedBetween.from!); - const hasTransactionsBeforeTo = - !updatedBetween?.to || timestamps.some((t) => t <= updatedBetween.to!); + const hasTransactionsInRange = (timestamps: number[], bounds: TransactionTypes.ITimestampBoundaries) => { + const afterFrom = !bounds.from || timestamps.some(t => t >= bounds.from); + const beforeTo = !bounds.to || timestamps.some(t => t <= bounds.to); + return afterFrom && beforeTo; + }; + hasValidTransactions = hasTransactionsInRange(timestamps, updatedBetween);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/transaction-manager/src/transaction-manager.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (3)
packages/transaction-manager/src/transaction-manager.ts (3)
200-202: LGTM! Consistent validation of pagination parameters.The validation is correctly placed before data access operations and consistently implemented across both topic methods.
Also applies to: 218-220
327-336: LGTM! Well-structured pagination metadata.The pagination metadata is well-structured and provides all necessary information for clients to handle paginated results effectively.
200-202: Verify impact on API consumers.The changes to pagination handling could affect API consumers who rely on the previous behavior where invalid channels were included in non-paginated results.
Also applies to: 218-220
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/transaction-manager/src/transaction-manager.ts (1)
368-375: Enhance channel validation checks.While the current validation is functional, consider adding more comprehensive checks:
private isValidChannel(cleaned: { transactions: (TransactionTypes.ITimestampedTransaction | null)[]; ignoredTransactions: (TransactionTypes.IIgnoredTransaction | null)[]; }): boolean { return ( - cleaned.transactions && cleaned.transactions.length > 0 && cleaned.transactions[0] !== null + cleaned && + Array.isArray(cleaned.transactions) && + cleaned.transactions.length > 0 && + cleaned.transactions[0] !== null && + Array.isArray(cleaned.ignoredTransactions) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/transaction-manager/src/transaction-manager.ts(4 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/transaction-manager/src/transaction-manager.ts (1)
Learnt from: rodrigopavezi
PR: RequestNetwork/requestNetwork#1547
File: packages/transaction-manager/src/transaction-manager.ts:301-302
Timestamp: 2025-01-22T10:28:23.757Z
Learning: In the RequestNetwork codebase, when filtering channels in pagination logic, validation (channel.isValid) is only applied when pagination is requested (page is defined). This allows seeing all channels (valid or invalid) when not using pagination, while ensuring only valid channels are included in paginated results.
🔇 Additional comments (4)
packages/transaction-manager/src/transaction-manager.ts (4)
7-7: LGTM! Import changes are well-organized.The new imports for
StorageTypesandvalidatePaginationParamsare correctly placed and support the pagination-related changes.Also applies to: 11-11
200-202: LGTM! Pagination validation is properly implemented.The changes correctly:
- Validate pagination parameters before processing
- Move pagination handling to a higher level in parseMultipleChannels
- Simplify the dataAccess calls by removing pagination parameters
This aligns well with the PR objective of fixing pagination issues.
Also applies to: 218-220
288-288: Note about error logging.The console.warn for decryption errors was previously discussed and intentionally kept as is.
235-364: LGTM! Robust pagination implementation.The parseMultipleChannels implementation:
- Correctly handles failed decryption cases
- Maintains channel order by latest timestamp
- Provides comprehensive pagination metadata
- Properly filters channels based on timestamp boundaries
This effectively addresses the PR's objective of fixing pagination when decryption fails.
Problem
First pagination implementation has performance issues and it will not work for request which cannot be decrypted. For instance when requesting 10 encrypted requests there might be requests that cannot be decrypted so it would return less than 10. That would cause confusion to the consumer.
Changes
Move the pagination logic to upper level so it will return the right number of valid requests.
Fixes #1382
Summary by CodeRabbit
Release Notes
Refactor
Types
Performance
New Features