Use requests builder #2
Conversation
|
|
AlexeyKrasnoperov
requested review from
elmattic,
sudo-shashank,
LesnyRumcajs,
a team and
hanabi1224
and removed request for
a team,
elmattic and
sudo-shashank
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis set of changes refactors how JSON-RPC method definitions are managed and used for benchmarking. Static JavaScript method definitions were removed and replaced by a generated JSON file created via a new build script. Test scripts were updated to load method data dynamically from this JSON. Documentation, environment variables, and configuration files were updated accordingly, including renaming environment variables and adding instructions for generating method definitions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant build_requests.js
participant filecoin-requests-builder
participant methods/requests.json
User->>build_requests.js: Run build-requests script
build_requests.js->>filecoin-requests-builder: fetchRpcContext(RPC_URL)
filecoin-requests-builder-->>build_requests.js: RPC context
build_requests.js->>filecoin-requests-builder: buildRequests(context)
filecoin-requests-builder-->>build_requests.js: Requests object
build_requests.js->>methods/requests.json: Write requests as JSON
sequenceDiagram
participant K6 Test Script
participant methods/requests.json
participant RPC Endpoint
K6 Test Script->>methods/requests.json: Load method definitions
loop For each method
K6 Test Script->>RPC Endpoint: Send request (method, params)
RPC Endpoint-->>K6 Test Script: Response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🧹 Nitpick comments (2)
tests/all.js (2)
7-7: Remove unnecessary await for synchronous JSON parsing.The
open()function is synchronous in k6, soawaitis unnecessary here.-const requests = await JSON.parse(open('../methods/requests.json')); +const requests = JSON.parse(open('../methods/requests.json'));
11-12: Add error handling for malformed JSON or missing file.Consider adding error handling for cases where the JSON file is missing or malformed.
- for (const [method, { params }] of Object.entries(requests)) { - const response = sendRpcRequest(url, method, params); + try { + for (const [method, { params }] of Object.entries(requests)) { + const response = sendRpcRequest(url, method, params); + assertSuccess(response); + } + } catch (error) { + console.error('Error processing requests:', error.message); + throw error; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
.gitignore(1 hunks)Makefile(1 hunks)README.md(2 hunks)methods/index.js(0 hunks)package.json(1 hunks)scripts/build_requests.js(1 hunks)tests/all.js(1 hunks)tests/gas_price.js(0 hunks)tests/single_method.js(1 hunks)tests/top5.js(0 hunks)utils/rpc.js(2 hunks)
🧬 Code Graph Analysis (4)
tests/all.js (4)
scripts/build_requests.js (2)
url(4-4)requests(7-7)tests/single_method.js (4)
url(4-4)requests(8-8)method(5-5)response(16-16)utils/benchmark_params.js (2)
regularBenchmarkParams(2-8)regularBenchmarkParams(2-8)utils/rpc.js (2)
response(16-19)sendRpcRequest(4-26)
scripts/build_requests.js (2)
tests/single_method.js (2)
url(4-4)requests(8-8)tests/all.js (2)
url(4-4)requests(7-7)
tests/single_method.js (3)
scripts/build_requests.js (2)
url(4-4)requests(7-7)tests/all.js (5)
url(4-4)options(6-6)options(6-6)requests(7-7)response(12-12)utils/rpc.js (3)
response(16-19)sendRpcRequest(4-26)assertSuccess(28-36)
utils/rpc.js (3)
scripts/build_requests.js (1)
url(4-4)tests/single_method.js (3)
url(4-4)method(5-5)response(16-16)tests/all.js (2)
url(4-4)response(12-12)
🪛 LanguageTool
README.md
[style] ~17-~17: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...em in the methods/requests.json file. Feel free to adjust parameters manually. ## Local b...
(FEEL_FREE_TO_STYLE_ME)
💤 Files with no reviewable changes (3)
- tests/top5.js
- tests/gas_price.js
- methods/index.js
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/all.js (4)
scripts/build_requests.js (2)
url(4-4)requests(7-7)tests/single_method.js (4)
url(4-4)requests(8-8)method(5-5)response(16-16)utils/benchmark_params.js (2)
regularBenchmarkParams(2-8)regularBenchmarkParams(2-8)utils/rpc.js (2)
response(16-19)sendRpcRequest(4-26)
scripts/build_requests.js (2)
tests/single_method.js (2)
url(4-4)requests(8-8)tests/all.js (2)
url(4-4)requests(7-7)
tests/single_method.js (3)
scripts/build_requests.js (2)
url(4-4)requests(7-7)tests/all.js (5)
url(4-4)options(6-6)options(6-6)requests(7-7)response(12-12)utils/rpc.js (3)
response(16-19)sendRpcRequest(4-26)assertSuccess(28-36)
utils/rpc.js (3)
scripts/build_requests.js (1)
url(4-4)tests/single_method.js (3)
url(4-4)method(5-5)response(16-16)tests/all.js (2)
url(4-4)response(12-12)
🪛 LanguageTool
README.md
[style] ~17-~17: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...em in the methods/requests.json file. Feel free to adjust parameters manually. ## Local b...
(FEEL_FREE_TO_STYLE_ME)
🔇 Additional comments (13)
.gitignore (1)
1-11: LGTM! Proper exclusion patterns for Yarn and generated files.The refined Yarn exclusions follow best practices by preserving necessary Yarn subdirectories while excluding cache and PnP files. Adding
methods/requests.jsonto gitignore is correct since this file is generated by the build script.package.json (2)
11-11: LGTM! ES module configuration enables import syntax.Adding
"type": "module"correctly enables ES module syntax used in the new build script.
8-8: LGTM! New build script aligns with the refactoring.The
build-requestsscript properly integrates the new dynamic request generation workflow.Makefile (1)
2-2: LGTM! Consistent environment variable naming.Renaming
K6_TEST_URLtoK6_RPC_URLacross all targets improves clarity and aligns with the standardization across the codebase.Also applies to: 5-5, 8-8, 11-11
tests/all.js (1)
4-4: LGTM! Consistent environment variable usage.The environment variable change to
K6_RPC_URLaligns with the standardization across the codebase.utils/rpc.js (3)
4-8: LGTM! Clean refactor of function signature.The change from accepting a method object to separate
methodandparamsarguments is a good improvement that:
- Makes the function signature more intuitive and explicit
- Aligns well with the new dynamic JSON-based approach for loading method definitions
- Eliminates the need for method objects with
nameandparamsproperties
18-18: Tagging correctly updated for new signature.The HTTP request tagging now uses the method string directly, which is consistent with the updated function signature.
22-22: Debug logging properly updated.The debug logging now uses the method string directly, maintaining the same debugging functionality with the new signature.
README.md (2)
9-19: Excellent documentation of the new workflow.The new "Methods Definitions" section clearly explains:
- How to generate method definitions using the requests builder
- The purpose and location of the generated
methods/requests.jsonfile- That users can manually adjust parameters for customization
This provides users with the essential information needed to work with the new dynamic approach.
24-34: Well-executed environment variable standardization.The updates consistently replace
K6_TEST_URLwith the more descriptiveK6_RPC_URLthroughout all examples, and properly document the newK6_METHODvariable for single-method benchmarking. This improves clarity and supports the new flexible testing approach.Also applies to: 41-41, 50-54
tests/single_method.js (3)
1-2: Proper imports for the test functionality.The imports correctly bring in the RPC utilities and benchmark parameters needed for the single-method testing.
4-8: Well-structured configuration with appropriate defaults.The configuration properly:
- Uses the standardized
K6_RPC_URLenvironment variable- Defaults to
eth_gasPricewhich is ideal for single-method testing- Loads the generated
methods/requests.jsonfile using the correct async pattern- Exports the benchmark options for k6
11-18: Robust implementation of single-method benchmarking.The test function includes:
- Proper validation that the requested method exists in the loaded definitions
- Clear error messaging for unsupported methods
- Correct usage of the updated
sendRpcRequestfunction with separate method and params arguments- Appropriate success assertion
This provides a flexible foundation for benchmarking any supported RPC method.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
package-lock.json
Outdated
| @@ -0,0 +1,1134 @@ | |||
| { | |||
There was a problem hiding this comment.
Should we remove this in favor of yarn.lock
yarn.lock
Outdated
| languageName: node | ||
| linkType: hard | ||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | ||
| # yarn lockfile v1 |
There was a problem hiding this comment.
How about using yarn@v3 instead of classic v1
There was a problem hiding this comment.
Hm. Not sure how it happened, but I regenerated it with v4.
tests/all.js
Outdated
|
|
||
| export let options = regularBenchmarkParams; | ||
| const requests = JSON.parse(open('../methods/requests.json')); |
There was a problem hiding this comment.
Better to print instructions on how to generate requests.json when it does not exist since it's not included by default.
tests/single_method.js
Outdated
| const method = __ENV.K6_METHOD || "eth_gasPrice"; | ||
|
|
||
| export let options = regularBenchmarkParams; | ||
| const requests = JSON.parse(open('../methods/requests.json')); |
|
|
||
| all-forest-local: | ||
| k6 run -e K6_TEST_URL=http://localhost:2345/rpc/v1 tests/all.js | ||
| k6 run -e K6_RPC_URL=http://localhost:2345/rpc/v1 tests/all.js | ||
|
|
||
| all-lotus-local: |
There was a problem hiding this comment.
How expensive is build-requests? If it's cheap, should we always depend on it
There was a problem hiding this comment.
It takes around 5-10 seconds, but I'd prefer to run it manually as I don't want params to change frequently and potentially affect the benchmarks results. I plan to reuse requests.json file to benchmark different node implementations with the same params.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Makefile (2)
2-11: Avoid duplication by extracting common variables
The four K6 targets are almost identical except for the RPC URL and execution mode. Repeating the full command string makes future edits error-prone (e.g., renaming the test file, tweaking flags). Consider introducing variables such asRPC_FOREST_URL,RPC_LOTUS_URLandK6_FLAGS(cloud vs. local), then composing the targets with those.
This keeps the Makefile DRY and reduces the chance of mismatched options across targets.Example patch outline:
+# --- globals -------------------------------------------------- +RPC_FOREST_URL ?= http://localhost:2345/rpc/v1 +RPC_LOTUS_URL ?= http://localhost:1234/rpc/v1 +TEST_FILE ?= tests/all.js + +# Helper for local vs. cloud +define run_k6 + k6 $(1) -e K6_RPC_URL=$(2) $(TEST_FILE) +endef + all-forest-cloud: - k6 cloud run -e K6_RPC_URL=http://localhost:2345/rpc/v1 --local-execution tests/all.js + $(call run_k6,cloud run --local-execution,$(RPC_FOREST_URL)) ...
16-16: Missing conventional PHONY meta-targets
Static analysis flags the absence of the customaryall,clean, andtestphony targets. If your project intentionally omits them, ignore this warning. Otherwise, adding minimal placeholders (even if they only print help) will silence tooling and improve developer ergonomics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
Makefile(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: AlexeyKrasnoperov
PR: ChainSafe/filecoin-benchmark-suite#2
File: package.json:19-19
Timestamp: 2025-07-24T14:03:44.246Z
Learning: The npm package chainsafe/filecoin-requests-builder exists and is publicly available on the npm registry at https://www.npmjs.com/package/chainsafe/filecoin-requests-builder
Learnt from: AlexeyKrasnoperov
PR: ChainSafe/filecoin-benchmark-suite#2
File: package.json:19-19
Timestamp: 2025-07-24T14:03:44.246Z
Learning: The npm package chainsafe/filecoin-requests-builder version 0.1.0 exists and is available on the official npm registry at https://registry.npmjs.org/chainsafe/filecoin-requests-builder/-/filecoin-requests-builder-0.1.0.tgz
📚 Learning: the npm package @chainsafe/filecoin-requests-builder exists and is publicly available on the npm reg...
Learnt from: AlexeyKrasnoperov
PR: ChainSafe/filecoin-benchmark-suite#2
File: package.json:19-19
Timestamp: 2025-07-24T14:03:44.246Z
Learning: The npm package chainsafe/filecoin-requests-builder exists and is publicly available on the npm registry at https://www.npmjs.com/package/chainsafe/filecoin-requests-builder
Applied to files:
Makefile
📚 Learning: the npm package @chainsafe/filecoin-requests-builder version 0.1.0 exists and is available on the of...
Learnt from: AlexeyKrasnoperov
PR: ChainSafe/filecoin-benchmark-suite#2
File: package.json:19-19
Timestamp: 2025-07-24T14:03:44.246Z
Learning: The npm package chainsafe/filecoin-requests-builder version 0.1.0 exists and is available on the official npm registry at https://registry.npmjs.org/chainsafe/filecoin-requests-builder/-/filecoin-requests-builder-0.1.0.tgz
Applied to files:
Makefile
🪛 checkmake (0.2.2)
Makefile
[warning] 16-16: Missing required phony target "all"
(minphony)
[warning] 16-16: Missing required phony target "clean"
(minphony)
[warning] 16-16: Missing required phony target "test"
(minphony)
🔇 Additional comments (1)
Makefile (1)
13-16: Confirm whether other targets should depend onbuild-requests
build-requestscorrectly wrapsyarn build-requests, but none of the K6 targets invoke it. If tests depend on an up-to-datemethods/requests.json, consider making the “all-*” targets depend onbuild-requests, or at least documenting that order in the README.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
utils/loadRequests.js (2)
3-4: Consider adding JSON structure validation.The file reading and parsing logic is correct, but consider validating that the parsed JSON contains the expected structure (e.g., has request definitions) to provide clearer error messages if the file format is invalid.
try { - return JSON.parse(open(resolvedPath)); + const requests = JSON.parse(open(resolvedPath)); + if (!Array.isArray(requests) && typeof requests !== 'object') { + throw new Error('Invalid requests format: expected object or array'); + } + return requests; } catch (err) {
15-24: Reduce code duplication in error messages.The error handling logic is sound, but there's significant duplication between the missing file and general error messages. Consider extracting the common regeneration instructions to improve maintainability.
+ const regenerationInstructions = ` +To ${err.message.includes('no such file') ? 'generate' : 're-generate'} this file, run: + K6_RPC_URL=http://localhost:2345/rpc/v1 yarn build-requests + +Replace 'http://localhost:2345/rpc/v1' with your actual RPC URL if different. +`; + if (err instanceof Error && err.message.includes('no such file or directory')) { console.error(` ❌ Missing file: ${resolvedPath} - -To generate this file, run: - K6_RPC_URL=http://localhost:2345/rpc/v1 yarn build-requests - -Replace 'http://localhost:2345/rpc/v1' with your actual RPC URL if different. +${regenerationInstructions} `); } else { console.error(` ❌ Error reading ${resolvedPath}: ${err.message} - -To re-generate this file, run: - K6_RPC_URL=http://localhost:2345/rpc/v1 yarn build-requests - -Replace 'http://localhost:2345/rpc/v1' with your actual RPC URL if different. +${regenerationInstructions} `); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/all.js(1 hunks)tests/single_method.js(1 hunks)utils/loadRequests.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/single_method.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/all.js
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: AlexeyKrasnoperov
PR: ChainSafe/filecoin-benchmark-suite#2
File: package.json:19-19
Timestamp: 2025-07-24T14:03:44.246Z
Learning: The npm package chainsafe/filecoin-requests-builder exists and is publicly available on the npm registry at https://www.npmjs.com/package/chainsafe/filecoin-requests-builder
Learnt from: AlexeyKrasnoperov
PR: ChainSafe/filecoin-benchmark-suite#2
File: package.json:19-19
Timestamp: 2025-07-24T14:03:44.246Z
Learning: The npm package chainsafe/filecoin-requests-builder version 0.1.0 exists and is available on the official npm registry at https://registry.npmjs.org/chainsafe/filecoin-requests-builder/-/filecoin-requests-builder-0.1.0.tgz
📚 Learning: the npm package @chainsafe/filecoin-requests-builder exists and is publicly available on the npm reg...
Learnt from: AlexeyKrasnoperov
PR: ChainSafe/filecoin-benchmark-suite#2
File: package.json:19-19
Timestamp: 2025-07-24T14:03:44.246Z
Learning: The npm package chainsafe/filecoin-requests-builder exists and is publicly available on the npm registry at https://www.npmjs.com/package/chainsafe/filecoin-requests-builder
Applied to files:
utils/loadRequests.js
📚 Learning: the npm package @chainsafe/filecoin-requests-builder version 0.1.0 exists and is available on the of...
Learnt from: AlexeyKrasnoperov
PR: ChainSafe/filecoin-benchmark-suite#2
File: package.json:19-19
Timestamp: 2025-07-24T14:03:44.246Z
Learning: The npm package chainsafe/filecoin-requests-builder version 0.1.0 exists and is available on the official npm registry at https://registry.npmjs.org/chainsafe/filecoin-requests-builder/-/filecoin-requests-builder-0.1.0.tgz
Applied to files:
utils/loadRequests.js
🔇 Additional comments (4)
utils/loadRequests.js (4)
1-1: Function signature looks good!The export, naming, and default parameter are well-structured and align with the PR's objective of loading dynamically generated request definitions.
2-2: Path resolution is correctly implemented.Using
import.meta.resolveis the appropriate way to handle relative path resolution in ES modules and k6 runtime.
6-14: Excellent user experience with helpful error messages.The missing file detection and error message provide clear, actionable instructions for users. The formatting with emojis and step-by-step guidance enhances usability.
Note: The error detection relies on string matching which could be platform-dependent, but this is likely the most practical approach available in this context.
26-27: Proper error handling approach.Rethrowing the original error after logging user-friendly messages is the correct approach, maintaining both good UX and proper error propagation for calling code.
There was a problem hiding this comment.
LGTM.
Once nit: the .gitignore file can be generated by https://www.toptal.com/developers/gitignore/api/osx,node,yarn,visualstudiocode
filecoin-requests-builder is used to generate method definitions, k6 just goes through them.
Single methodandallbenchmarks supported.First of all, a user is required to generate requests definitions, they will be stored to the methods/requests.json file, which then could be manually adjusted if needed and used to reproduce the same scenario on another server
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
.gitignorefor Yarn and build artifacts.Refactor
Revert