fix: improve error message when persistTransaction times out. Increase default timeout to 130 seconds using Exponential Backoff #1316
Conversation
* Update error message to recommend polling getConfirmedTransaction
instead of getTransactionsByChannelId()
* Previously, 500 meant retry after 1, 2, 4, 8... seconds. * Now, 1000 means retry after 1, 2, 4, 8... seconds.
MantisClone
changed the title
|
Warning Rate limit exceeded@MantisClone has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HttpDataAccess
participant Node
User->>HttpDataAccess: Call persistTransaction()
HttpDataAccess->>Node: Fetch transaction confirmation
Node-->>HttpDataAccess: Return error event
HttpDataAccess-->>User: Emit error message with guidance
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 (
|
MantisClone
changed the title
MantisClone
changed the title
MantisClone
changed the title
| maxExponentialBackoffDelay: this.httpConfig.getConfirmationMaxExponentialBackoffDelay, | ||
| }, | ||
| ); | ||
| const confirmedData = await this.getConfirmedTransaction(transactionData); |
There was a problem hiding this comment.
I moved this logic into a new getConfirmedTransaction() function
github-actions
bot
requested review from
alexandre-abrioux,
kevindavee,
sstefdev and
yomarion
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (2)
packages/request-client.js/test/http-data-access.test.ts (1)
Tests do not verify compatibility with the Invoicing Template timeout dialog
The current tests in
packages/request-client.js/test/http-data-access.test.tsdo not cover scenarios ensuring that the new error message does not interfere with the Invoicing Template's timeout dialog. Please add appropriate tests to verify this compatibility.🔗 Analysis chain
Line range hint
1-39: Address TODO: Verify compatibility with Invoicing Template timeout dialogThe PR objectives mention a TODO item to ensure that the new error message doesn't interfere with the timeout dialog in the Invoicing Template. This test file doesn't address that concern directly.
To verify this, we need to check the Invoicing Template code. Let's run a script to find relevant files:
Please review the output of this script to identify the relevant files and code sections that need to be updated to ensure compatibility with the new error message.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find files related to the Invoicing Template and timeout dialog # Search for files that might contain the Invoicing Template code echo "Searching for Invoicing Template files:" fd -e ts -e tsx -e js -e jsx invoice # Search for files that might contain timeout dialog code echo "\nSearching for timeout dialog files:" fd -e ts -e tsx -e js -e jsx timeout # Search for usage of persistTransaction in these files echo "\nSearching for persistTransaction usage in relevant files:" rg "persistTransaction" $(fd -e ts -e tsx -e js -e jsx "invoice|timeout")Length of output: 24606
README.md (1)
Line range hint
92-95: Excellent addition! Consider emphasizing the deprecation.The new "V1 packages" section provides valuable information about the versioning of the project. It clearly states that this repository hosts V2 packages and directs users to the archived V1 packages.
To make the deprecation of V1 packages more noticeable, consider using bold text for the deprecation notice:
## V1 packages This repository hosts the packages for the second version of Request. **The v1 packages are deprecated** and can be found on [requestNetwork-v1-archive](https://github.com/RequestNetwork/requestNetwork-v1-archive).🧰 Tools
🪛 LanguageTool
[style] ~85-~85: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e monorepo.bash yarn run testTest a specific package. ```bash yarn works...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- README.md (1 hunks)
- packages/request-client.js/src/http-data-access.ts (2 hunks)
- packages/request-client.js/test/http-data-access.test.ts (1 hunks)
- packages/utils/src/retry.ts (1 hunks)
- packages/utils/test/retry.test.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/request-client.js/test/http-data-access.test.ts (1)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:25-25 Timestamp: 2024-07-17T15:06:14.563Z Learning: The `persistTransaction()` function in the `no-persist-http-data-access.ts` file is tested in the `in-memory-request.test.ts` file, which is separate from the typical `index.test.ts` file to avoid test interference.
🔇 Additional comments (3)
packages/utils/src/retry.ts (1)
64-64: Approve: Improved intuitive configuration for exponential backoff delayThe modification to the exponential backoff delay calculation enhances the usability of the
retryfunction. By halving theexponentialBackoffDelaybefore applying the exponential factor, the function now provides a more intuitive mapping between the input value and the resulting retry intervals.For example, with this change, setting
exponentialBackoffDelayto 1000 will result in retry intervals of approximately 0.5s, 1s, 2s, 4s, which aligns better with user expectations. This improvement makes it easier for developers to configure the desired retry behavior without needing to account for an unintuitive doubling effect.README.md (2)
91-91: Formatting improvements look good.The addition of a blank line before the "License" section and the removal of an unnecessary line break after the "Test" section enhance the overall readability and consistency of the README.
Line range hint
1-95: Overall, the changes to the README are well-implemented and valuable.The additions and modifications in this file align well with the PR objectives of improving clarity regarding package versions and enhancing the developer experience. The new "V1 packages" section, the additional test instructions, and the minor formatting improvements all contribute to a more informative and readable README.
🧰 Tools
🪛 LanguageTool
[style] ~85-~85: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e monorepo.bash yarn run testTest a specific package. ```bash yarn works...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
README.md (1)
Line range hint
92-95: LGTM! Consider adding a version compatibility note.The addition of the "V1 packages" section is valuable for users, clearly stating that this repository hosts V2 packages and providing information about the deprecated V1 packages. This aligns well with the goal of improving clarity in the documentation.
To further enhance this section, consider adding a brief note about version compatibility or migration, if applicable. For example:
## V1 packages This repository hosts the packages for the second version of Request. The v1 packages are deprecated and can be found on [requestNetwork-v1-archive](https://github.com/RequestNetwork/requestNetwork-v1-archive). Note: V2 packages are not backwards compatible with V1. If you're migrating from V1 to V2, please refer to our migration guide [link to migration guide if available].This additional information would help users understand the implications of the version change and guide them in case they need to migrate their projects.
🧰 Tools
🪛 LanguageTool
[style] ~85-~85: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e monorepo.bash yarn run testTest a specific package. ```bash yarn works...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- README.md (1 hunks)
- packages/request-client.js/test/http-data-access.test.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/request-client.js/test/http-data-access.test.ts (1)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:25-25 Timestamp: 2024-07-17T15:06:14.563Z Learning: The `persistTransaction()` function in the `no-persist-http-data-access.ts` file is tested in the `in-memory-request.test.ts` file, which is separate from the typical `index.test.ts` file to avoid test interference.
🔇 Additional comments (1)
README.md (1)
Line range hint
1-95: Overall, great improvements to the README!The changes made to the README.md file are well-aligned with the PR objectives of improving documentation and clarity. The additions provide valuable information for users, especially regarding package testing and version differences.
Here's a summary of the improvements:
- Added clear instructions for testing specific packages.
- Included a new section about V1 packages, helping users understand the repository structure and version differences.
These changes will significantly enhance the developer experience and reduce potential confusion for users of the Request Network project.
🧰 Tools
🪛 LanguageTool
[style] ~85-~85: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e monorepo.bash yarn run testTest a specific package. ```bash yarn works...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range comments (3)
README.md (1)
Line range hint
91-95: LGTM! Consider moving the V1 packages section.The addition of the V1 packages section is valuable, providing clear information about the deprecation of v1 packages and where to find them. This aligns well with the PR objectives.
Consider moving this section higher up in the README, perhaps after the "Published Packages" section. This would make it more visible to users who might be looking for v1 packages.
🧰 Tools
🪛 LanguageTool
[style] ~85-~85: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e monorepo.bash yarn run testTest a specific package by replacing `@reque...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
packages/request-client.js/src/http-data-access.ts (2)
Line range hint
110-122: Approve changes with a minor suggestion for the error messageThe refactoring of the
persistTransactionmethod to use the newgetConfirmedTransactionmethod is a good improvement. The updated error message provides valuable guidance to app builders.However, the error message is quite long and might benefit from some formatting for better readability.
Consider breaking down the error message into multiple lines for better readability:
error = new Error( `The Request Network SDK timed-out while polling the Request Node to confirm that the Request was persisted successfully. It is likely that the persisted Request will be confirmed eventually. App Builders are discouraged from calling persistTransaction() a second time, which would create a duplicate Request. Instead, App Builders are recommended to catch this error and continue polling the Request Node using getConfirmedTransaction() or by calling the /getConfirmedTransaction endpoint. To avoid timeouts in the future, try adjusting the httpConfig values when instantiating the RequestNetwork object: - getConfirmationDeferDelay: ${this.httpConfig.getConfirmationDeferDelay}ms - getConfirmationMaxRetry: ${this.httpConfig.getConfirmationMaxRetry} - getConfirmationRetryDelay: ${this.httpConfig.getConfirmationRetryDelay}ms - getConfirmationExponentialBackoffDelay: ${this.httpConfig.getConfirmationExponentialBackoffDelay}ms - getConfirmationMaxExponentialBackoffDelay: ${this.httpConfig.getConfirmationMaxExponentialBackoffDelay}ms` );
Line range hint
226-246: Approve changes with a suggestion for code readabilityThe addition of exponential backoff parameters to the
fetchAndRetrymethod aligns well with the PR objectives. The default values are correctly set using the class'shttpConfig.To improve code readability, consider using object destructuring for the
retryConfigparameter:protected async fetchAndRetry<T = unknown>( path: string, params: Record<string, unknown>, { maxRetries = this.httpConfig.httpRequestMaxRetry, retryDelay = this.httpConfig.httpRequestRetryDelay, exponentialBackoffDelay = this.httpConfig.httpRequestExponentialBackoffDelay, maxExponentialBackoffDelay = this.httpConfig.httpRequestMaxExponentialBackoffDelay, }: { maxRetries?: number; retryDelay?: number; exponentialBackoffDelay?: number; maxExponentialBackoffDelay?: number; } = {}, ): Promise<T> { return await retry( async () => await this.fetch<T>('GET', path, params), { maxRetries, retryDelay, exponentialBackoffDelay, maxExponentialBackoffDelay } )(); }This approach eliminates the need for multiple reassignments and makes the default values more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- README.md (1 hunks)
- packages/request-client.js/src/http-data-access.ts (2 hunks)
- packages/request-client.js/test/http-data-access.test.ts (1 hunks)
- packages/utils/test/retry.test.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/request-client.js/test/http-data-access.test.ts (1)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:25-25 Timestamp: 2024-07-17T15:06:14.563Z Learning: The `persistTransaction()` function in the `no-persist-http-data-access.ts` file is tested in the `in-memory-request.test.ts` file, which is separate from the typical `index.test.ts` file to avoid test interference.
🔇 Additional comments (3)
README.md (1)
86-90: LGTM! Clear instructions for package-specific testing.The added instructions for testing specific packages are clear and helpful. The example using
@requestnetwork/request-client.jsis appropriate and aligns with the project structure. This addition improves the developer experience by providing explicit guidance on how to test individual packages.packages/request-client.js/src/http-data-access.ts (1)
Line range hint
1-284: Summary of changesThe modifications to the
HttpDataAccessclass align well with the PR objectives:
- The new
getConfirmedTransactionmethod effectively refactors the logic for retrieving confirmed transactions.- The
persistTransactionmethod has been updated to use the newgetConfirmedTransactionmethod and provide more detailed error messages.- Exponential backoff parameters have been added to the
fetchAndRetrymethod, making it easier to use and configure.These changes improve the overall functionality and usability of the class. The suggestions provided in the review comments are minor and focus on enhancing code readability and error handling.
packages/utils/test/retry.test.ts (1)
141-216: Implementation of the exponential backoff test is accurateThe test correctly simulates the exponential backoff delays and validates the expected call times and elapsed times. The use of fake timers and time advancements is appropriately handled.
…tely if the error event is never emitted.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (2)
packages/request-client.js/test/http-data-access.test.ts (1)
Line range hint
25-30: Consider extracting httpConfig for better readabilityWhile the test setup is good, we could improve its readability by extracting the
httpConfigobject. This would make it easier to understand and modify the configuration in future tests.Consider refactoring the
HttpDataAccessinstantiation as follows:const httpConfig = { getConfirmationDeferDelay: 0, getConfirmationMaxRetry: 0, }; const httpDataAccess = new HttpDataAccess({ httpConfig });This change would make the configuration more explicit and easier to maintain.
packages/request-client.js/src/http-data-access.ts (1)
Line range hint
118-126: Simplify and clarify the error message for better readabilityThe current error message is quite lengthy and may be overwhelming for users. Consider simplifying it to make it more concise and easier to understand. Focus on the essential information and actionable steps.
Apply this diff to simplify the error message:
error = new Error( - `The Request Network SDK timed-out while polling the Request Node to confirm that the Request was persisted successfully. It is likely that the persisted Request will be confirmed eventually. App Builders are discouraged from calling persistTransaction() a second time, which would create a duplicate Request. Instead, App Builders are recommended to catch this error and continue polling the Request Node using getConfirmedTransaction() or by calling the /getConfirmedTransaction endpoint. To avoid timeouts in the future, try adjusting the httpConfig values when instantiating the RequestNetwork object. The current httpConfig settings are: getConfirmationDeferDelay: ${this.httpConfig.getConfirmationDeferDelay}ms, getConfirmationMaxRetry: ${this.httpConfig.getConfirmationMaxRetry}, getConfirmationRetryDelay: ${this.httpConfig.getConfirmationRetryDelay}ms, getConfirmationExponentialBackoffDelay: ${this.httpConfig.getConfirmationExponentialBackoffDelay}ms, getConfirmationMaxExponentialBackoffDelay: ${this.httpConfig.getConfirmationMaxExponentialBackoffDelay}ms`, + `Timeout while confirming the transaction. It is likely that the transaction will be confirmed eventually. Please use getConfirmedTransaction() to continue polling for confirmation or adjust the httpConfig settings to avoid future timeouts.`, );This revision conveys the necessary information more succinctly and directs users to the recommended actions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- packages/request-client.js/src/http-config-defaults.ts (1 hunks)
- packages/request-client.js/src/http-data-access.ts (2 hunks)
- packages/request-client.js/test/http-data-access.test.ts (1 hunks)
- packages/utils/test/retry.test.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/request-client.js/test/http-data-access.test.ts (1)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:25-25 Timestamp: 2024-07-17T15:06:14.563Z Learning: The `persistTransaction()` function in the `no-persist-http-data-access.ts` file is tested in the `in-memory-request.test.ts` file, which is separate from the typical `index.test.ts` file to avoid test interference.
🔇 Additional comments (5)
packages/request-client.js/src/http-config-defaults.ts (2)
11-14: LGTM! Configuration changes align with the new exponential backoff strategy.The modifications to the configuration values are consistent with the exponential backoff strategy described in the comment:
getConfirmationMaxRetry: 7: Reduces the maximum number of retries from 30 to 7, which is appropriate for the new strategy.getConfirmationRetryDelay: 0: Sets the initial delay to 0, as delays are now handled by the exponential backoff.getConfirmationExponentialBackoffDelay: 1000: Introduces a 1-second starting delay for the exponential backoff.getConfirmationMaxExponentialBackoffDelay: 64000: Increases the maximum delay to 64 seconds, allowing for longer waits between retries.These changes should improve the efficiency of the retry mechanism while potentially reducing unnecessary network traffic.
Let's verify that these are the only changes made to the configuration object:
#!/bin/bash # Description: Verify that no other changes were made to the configuration object # Test: Check for any other changes in the configuration object rg --type typescript -C 10 'const config: ClientTypes.IHttpDataAccessConfig = {' packages/request-client.js/src/http-config-defaults.ts # Expected output: Only the changes we've reviewed should be present in the diff
9-10: LGTM! Clear and informative comment.The added comment provides a comprehensive explanation of the new exponential backoff strategy. It clearly outlines the starting delay, doubling mechanism, maximum delay, number of retries, and total timeout calculation.
Let's verify the accuracy of the comment by checking the configuration values:
packages/request-client.js/test/http-data-access.test.ts (1)
32-43: Excellent refactoring of thepersistTransaction()test case!The changes in this test case are well-aligned with the PR objectives and address previous review comments. Here are the key improvements:
- The use of async/await simplifies the test structure and improves readability.
- The implementation of
Promise.race()elegantly handles both the error event and a potential timeout, making the test more robust.- The expanded error message now provides detailed guidance for app builders, which aligns perfectly with the PR objective of enhancing user experience when dealing with transaction timeouts.
- The addition of a 5-second timeout prevents the test from hanging indefinitely if the error event is never emitted, addressing a suggestion from a past review.
- The test now directly uses the
persistTransaction()method, which provides a more realistic scenario.These changes significantly improve the test's reliability and its alignment with the intended behavior of the
persistTransaction()method.packages/request-client.js/src/http-data-access.ts (2)
110-116: Efficient refactoring by utilizinggetConfirmedTransactionRefactoring the code to use the new
getConfirmedTransactionmethod simplifies the logic inpersistTransactionand promotes code reuse.
127-149: Add error handling togetConfirmedTransactionmethodIncluding explicit error handling within the
getConfirmedTransactionmethod can provide more context if thefetchAndRetrycall fails. This will help users understand the reason for the failure and take appropriate action.
| @@ -61,7 +61,7 @@ const retry = <TParams extends unknown[], TReturn>( | |||
| setTimeout( | |||
| resolve, | |||
| retryDelay + | |||
| Math.min(maxExponentialBackoffDelay, exponentialBackoffDelay * 2 ** retry), | |||
| Math.min(maxExponentialBackoffDelay, (exponentialBackoffDelay / 2) * 2 ** retry), | |||
There was a problem hiding this comment.
This change adjusts the retry logic to make exponentialBackoffDelay more intuitive to configure
- Now, 1000 means retry after 1, 2, 4, 8... seconds.
- Previously, 1000 meant retry after 2, 4, 8, 16... seconds.
MantisClone
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- packages/request-client.js/src/http-data-access.ts (2 hunks)
- packages/request-client.js/test/http-data-access.test.ts (1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/request-client.js/src/http-data-access.ts (1)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1316 File: packages/request-client.js/src/http-data-access.ts:127-149 Timestamp: 2024-10-02T05:01:57.381Z Learning: In the `getConfirmedTransaction` method, explicit error handling for the `fetchAndRetry` call is not necessary.packages/request-client.js/test/http-data-access.test.ts (1)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:25-25 Timestamp: 2024-07-17T15:06:14.563Z Learning: The `persistTransaction()` function in the `no-persist-http-data-access.ts` file is tested in the `in-memory-request.test.ts` file, which is separate from the typical `index.test.ts` file to avoid test interference.
🔇 Additional comments (1)
packages/request-client.js/src/http-data-access.ts (1)
127-149: NewgetConfirmedTransactionmethod is well-implementedThe addition of the
getConfirmedTransactionmethod effectively encapsulates the logic for fetching confirmed transactions. It correctly normalizes the transaction hash and utilizesfetchAndRetrywith appropriate configuration parameters.
| // when found, emit the event 'confirmed' | ||
| result.emit('confirmed', confirmedData); | ||
| }) | ||
| .catch((e) => { | ||
| let error: Error = e; | ||
| if (e.status === 404) { | ||
| if (e && 'status' in e && e.status === 404) { |
There was a problem hiding this comment.
This was a coderabbit suggestion.
Description of the changes
getConfirmedTransaction()and adjust theirhttpConfigsettings so timeouts are less likely).getConfirmedTransactionlogic to a new public function in the request-client.js package.exponentialBackoffDelaymore intuitive to configureTODO
Summary by CodeRabbit
Summary by CodeRabbit
New Features
getConfirmedTransaction, for retrieving confirmed transactions.Bug Fixes
Documentation
Tests
persistTransactionto reflect new error handling.retryfunction to align with the new exponential backoff strategy.