feat: create many concurrent requests #14
Conversation
WalkthroughThe changes introduce a new script, Changes
Suggested reviewers
✨ Finishing Touches
🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
README.md (1)
44-44: Documentation update is minimal but sufficient.The new command is documented alongside existing commands, which is good. Consider enhancing this with a brief description of what the command does and any parameters it accepts.
src/createManyConcurrentRequests.js (5)
15-18: Make configuration values externally configurable.Currently, TOTAL_REQUESTS and CONCURRENCY_LIMIT are hard-coded. Consider making these configurable via environment variables or command-line arguments to improve flexibility.
- const TOTAL_REQUESTS = 100; // Total number of requests to create - const CONCURRENCY_LIMIT = 100; // Number of requests to create concurrently + const TOTAL_REQUESTS = parseInt(process.env.TOTAL_REQUESTS || '100', 10); // Total number of requests to create + const CONCURRENCY_LIMIT = parseInt(process.env.CONCURRENCY_LIMIT || '100', 10); // Number of requests to create concurrently
23-26: Consider validating all required environment variables.Currently, only PAYEE_PRIVATE_KEY is validated. Consider checking other potentially required variables like JSON_RPC_PROVIDER_URL that might be needed for the Request Network client.
103-138: Make request parameters configurable.The currency, network, and other request parameters are hard-coded. Consider making these configurable to improve reusability for different testing scenarios.
You could extract key parameters to variables at the top of the file or read them from environment variables:
+ // Request configuration from env vars or defaults + const CURRENCY_TYPE = process.env.CURRENCY_TYPE || Types.RequestLogic.CURRENCY.ERC20; + const CURRENCY_VALUE = process.env.CURRENCY_VALUE || "0x370DE27fdb7D1Ff1e1BaA7D11c5820a324Cf623C"; + const CURRENCY_NETWORK = process.env.CURRENCY_NETWORK || "sepolia"; + const EXPECTED_AMOUNT = process.env.EXPECTED_AMOUNT || "1000000000000000000"; // Later in the requestCreateParameters currency: { - type: Types.RequestLogic.CURRENCY.ERC20, - value: "0x370DE27fdb7D1Ff1e1BaA7D11c5820a324Cf623C", - network: "sepolia", + type: CURRENCY_TYPE, + value: CURRENCY_VALUE, + network: CURRENCY_NETWORK, }, - expectedAmount: "1000000000000000000", + expectedAmount: EXPECTED_AMOUNT,
145-150: Improved error handling.The error handling is good, with simplified error output to prevent console spam during concurrent operations. Consider adding more specific error handling for common failure scenarios to help with debugging.
1-184: Overall excellent implementation with room for configurability improvements.The script is well-structured with proper error handling, progress tracking, and abort handling. The main improvement opportunity is to make the various hard-coded parameters configurable to increase flexibility and reusability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
README.md(1 hunks)package.json(1 hunks)src/createManyConcurrentRequests.js(1 hunks)
🔇 Additional comments (6)
package.json (3)
14-15: Is the "hinkal-deposit" script related to this PR?I notice both a "hinkal-deposit" script and the new "create-many" script were added. Since the PR description only mentions creating concurrent requests, could you clarify if the "hinkal-deposit" script is part of this PR's scope or should be addressed separately?
15-15: The create-many script integration looks good.The script name follows the existing naming pattern and correctly points to the implementation file.
23-23: Dependencies added appropriately.The cli-progress and p-limit libraries are appropriate choices for this feature. The version constraints using caret notation (^) match the project's versioning style.
Also applies to: 26-26
src/createManyConcurrentRequests.js (3)
34-37: Good implementation of abort handling.The script properly handles SIGINT (Ctrl+C) to gracefully abort the operation, which is essential for long-running tasks.
39-54: Excellent progress tracking implementation.The multi-progress bar with successful, failed, and in-progress counters provides clear visibility into the operation status.
170-182: Comprehensive execution summary.The summary at the end provides clear statistics about the operation, which is valuable for understanding the results.
| const requestClient = new RequestNetwork({ | ||
| nodeConnectionConfig: { | ||
| baseURL: "http://localhost:3000/", | ||
| }, | ||
| signatureProvider: epkSignatureProvider, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make node URL configurable.
The node connection URL is hard-coded to "http://localhost:3000/". Consider making this configurable via an environment variable to support different environments.
const requestClient = new RequestNetwork({
nodeConnectionConfig: {
- baseURL: "http://localhost:3000/",
+ baseURL: process.env.REQUEST_NODE_URL || "http://localhost:3000/",
},
signatureProvider: epkSignatureProvider,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const requestClient = new RequestNetwork({ | |
| nodeConnectionConfig: { | |
| baseURL: "http://localhost:3000/", | |
| }, | |
| signatureProvider: epkSignatureProvider, | |
| }); | |
| const requestClient = new RequestNetwork({ | |
| nodeConnectionConfig: { | |
| baseURL: process.env.REQUEST_NODE_URL || "http://localhost:3000/", | |
| }, | |
| signatureProvider: epkSignatureProvider, | |
| }); |
Problem
I needed a way to simulate many concurrent users persisting requests at the same time.
Solution
This PR adds the
createManyConcurrentRequests.jsscript which usesp-limitto control the concurrency of an array of Promises. Each promise creates one request.Summary by CodeRabbit