Add request limits for JSON-RPC batch calls #277
Conversation
WalkthroughThe recent updates introduce batch request and response limits to the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant HTTPServer
participant RPCHandler
Client->>HTTPServer: Send batch RPC request
HTTPServer->>RPCHandler: Forward request with batch limits
RPCHandler-->>HTTPServer: Process and return response
HTTPServer-->>Client: Deliver batch RPC response
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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 and nitpick comments (3)
tests/web3js/eth_non_interactive_test.js (3)
Line range hint
51-51: UseNumber.parseFloatinstead of the globalparseFloat.- assert.equal(parseFloat(flow), conf.fundedAmount) + assert.equal(Number.parseFloat(flow), conf.fundedAmount)Tools
Biome
[error] 7-7: This let declares a variable that is only assigned once.
Line range hint
76-76: Consider usingconstinstead ofletfortxas it is not reassigned.- let tx = await web3.eth.getTransaction(blockTx.hash) + const tx = await web3.eth.getTransaction(blockTx.hash)Tools
Biome
[error] 7-7: This let declares a variable that is only assigned once.
Line range hint
80-80: UseNumber.parseIntinstead of the globalparseInt.- assert.isAbove(parseInt(tx.gas), 1) + assert.isAbove(Number.parseInt(tx.gas), 1)Tools
Biome
[error] 7-7: This let declares a variable that is only assigned once.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- api/server.go (2 hunks)
- tests/web3js/eth_non_interactive_test.js (5 hunks)
Additional context used
Biome
tests/web3js/eth_non_interactive_test.js
[error] 7-7: This let declares a variable that is only assigned once.
[error] 13-13: This let declares a variable that is only assigned once.
[error] 16-16: This let declares a variable that is only assigned once.
[error] 24-24: This let declares a variable that is only assigned once.
[error] 28-28: This let declares a variable that is only assigned once.
[error] 29-29: This let declares a variable that is only assigned once.
[error] 35-35: This let declares a variable that is only assigned once.
[error] 42-42: This let declares a variable that is only assigned once.
[error] 47-47: This let declares a variable that is only assigned once.
[error] 50-50: This let declares a variable that is only assigned once.
[error] 51-51: Use Number.parseFloat instead of the equivalent global.
[error] 53-53: This let declares a variable that is only assigned once.
[error] 58-58: This let declares a variable that is only assigned once.
[error] 63-63: This let declares a variable that is only assigned once.
[error] 68-68: This let declares a variable that is only assigned once.
[error] 73-73: This let declares a variable that is only assigned once.
[error] 76-76: This let declares a variable that is only assigned once.
[error] 80-80: Use Number.parseInt instead of the equivalent global.
[error] 86-86: This let declares a variable that is only assigned once.
[error] 99-99: This let declares a variable that is only assigned once.
Additional comments not posted (3)
tests/web3js/eth_non_interactive_test.js (1)
114-196: The new test case for batch requests is well-implemented and adheres to the new limits set for batch processing.api/server.go (2)
56-58: The new constants for batch processing limits are correctly defined according to the PR objectives.
112-112: The modification in theEnableRPCmethod to set batch limits is correctly implemented.
| @@ -54,22 +54,22 @@ it('get balance', async() => { | |||
| assert.equal(wei, weiAtBlock) | |||
| }) | |||
|
|
|||
| it('get code', async() => { | |||
| it('get code', async () => { | |||
There was a problem hiding this comment.
Consider using const instead of let for code as it is not reassigned.
- let code = await web3.eth.getCode(conf.eoa.address)
+ const code = await web3.eth.getCode(conf.eoa.address)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.
| it('get code', async () => { | |
| it('get code', async () => { | |
| const code = await web3.eth.getCode(conf.eoa.address) |
| @@ -43,7 +43,7 @@ | |||
| assert.isNull(no) | |||
| }) | |||
|
|
|||
| it('get balance', async() => { | |||
| it('get balance', async () => { | |||
There was a problem hiding this comment.
Consider using const instead of let for wei as it is not reassigned.
- let wei = await web3.eth.getBalance(conf.eoa.address)
+ const wei = await web3.eth.getBalance(conf.eoa.address)Committable suggestion was skipped due low confidence.
| let coinbase = await web3.eth.getCoinbase() | ||
| assert.equal(coinbase, conf.serviceEOA) // e2e configured account | ||
| }) | ||
|
|
||
| it('get gas price', async() => { | ||
| it('get gas price', async () => { |
There was a problem hiding this comment.
Consider using const instead of let for gasPrice as it is not reassigned.
- let gasPrice = await web3.eth.getGasPrice()
+ const gasPrice = await web3.eth.getGasPrice()Committable suggestion was skipped due low confidence.
| @@ -3,7 +3,7 @@ | |||
| const conf = require('./config') | |||
| const web3 = conf.web3 | |||
|
|
|||
| it('get chain ID', async() => { | |||
| it('get chain ID', async () => { | |||
There was a problem hiding this comment.
Consider using const instead of let for chainID as it is not reassigned.
- let chainID = await web3.eth.getChainId()
+ const chainID = await web3.eth.getChainId()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.
| it('get chain ID', async () => { | |
| it('get chain ID', async () => { | |
| const chainID = await web3.eth.getChainId() |
| @@ -95,12 +95,12 @@ | |||
| assert.equal(rcp.gasUsed, tx.gas) | |||
| }) | |||
|
|
|||
| it('get mining status', async() => { | |||
| it('get mining status', async () => { | |||
| let mining = await web3.eth.isMining() | |||
There was a problem hiding this comment.
Consider using const instead of let for mining as it is not reassigned.
- let mining = await web3.eth.isMining()
+ const mining = await web3.eth.isMining()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.
| let mining = await web3.eth.isMining() | |
| const mining = await web3.eth.isMining() |
Tools
Biome
[error] 99-99: This let declares a variable that is only assigned once.
| let gasPrice = await web3.eth.getGasPrice() | ||
| assert.equal(gasPrice, 0n) // 0 by default in tests | ||
| }) | ||
|
|
||
| it('get transaction', async() => { | ||
| it('get transaction', async () => { |
There was a problem hiding this comment.
Consider using const instead of let for blockTx as it is not reassigned.
- let blockTx = await web3.eth.getTransactionFromBlock(conf.startBlockHeight, 0)
+ const blockTx = await web3.eth.getTransactionFromBlock(conf.startBlockHeight, 0)Committable suggestion was skipped due low confidence.
| let code = await web3.eth.getCode(conf.eoa.address) | ||
| assert.equal(code, "0x") // empty | ||
| }) | ||
|
|
||
| it('get coinbase', async() => { | ||
| it('get coinbase', async () => { |
There was a problem hiding this comment.
Consider using const instead of let for coinbase as it is not reassigned.
- let coinbase = await web3.eth.getCoinbase()
+ const coinbase = await web3.eth.getCoinbase()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.
| it('get coinbase', async () => { | |
| it('get coinbase', async () => { | |
| const coinbase = await web3.eth.getCoinbase() |
Closes: #211
Description
Add some sane default limits for batch calls:
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Tests