adding gas limit #4
Conversation
|
Warning Rate limit exceeded@jlafiandra6 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 24 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 changes in this pull request primarily enhance transaction fee validation in the Changes
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 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: 2
🧹 Outside diff range and nitpick comments (1)
src/server.ts (1)
Line range hint
20-86: Consider additional architectural improvementsWhile the basic gas limit functionality is implemented, consider these improvements:
- Add unit tests for the gas limit validation logic
- Implement monitoring/metrics to track when transactions hit limits
- Add startup configuration validation to ensure all required environment variables are properly set
- Consider making the limit configurable per transaction type (regular vs blob transactions)
Would you like help implementing any of these improvements? I can help with:
- Writing unit tests for the gas limit validation
- Adding configuration validation at startup
- Implementing basic metrics collection
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/server.ts (3 hunks)
🧰 Additional context used
🪛 Biome
src/server.ts
[error] 40-40: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (1)
src/server.ts (1)
60-86:⚠️ Potential issueImprove gas calculation safety and efficiency
The current implementation has several concerns:
- No protection against BigInt overflow in calculations
- Redundant variable initializations
- Gas cost calculation might be incorrect (adding instead of using max)
- Missing function documentation
Consider this safer implementation:
+/** + * Validates if the transaction's total gas costs (including blob gas) are within limits + * @param transactionArgs The transaction parameters to validate + * @returns true if gas costs are within limits, false otherwise + */ async function feesTooHigh(transactionArgs: TransactionArgs) { - let maxFeePerGas = BigInt(0); - let maxPriorityFeePerGas = BigInt(0); - let maxFeePerBlobGas = BigInt(0); - if (transactionArgs.maxFeePerGas ){ - maxFeePerGas = BigInt(transactionArgs.maxFeePerGas); - } - if (transactionArgs.maxPriorityFeePerGas) { - maxPriorityFeePerGas = BigInt(transactionArgs.maxPriorityFeePerGas); - } - if (transactionArgs.maxFeePerBlobGas) { - maxFeePerBlobGas = BigInt(transactionArgs.maxFeePerBlobGas); - } + try { + const maxFeePerGas = BigInt(transactionArgs.maxFeePerGas || 0); + const maxPriorityFeePerGas = BigInt(transactionArgs.maxPriorityFeePerGas || 0); + const gas = BigInt(transactionArgs.gas); - var gasCost = BigInt(transactionArgs.gas) * (maxFeePerGas + maxPriorityFeePerGas); - if (gasCost > TX_LIMIT) { - return false; - } + // Use max of the two fees instead of adding them + const effectiveGasPrice = maxFeePerGas > maxPriorityFeePerGas ? maxFeePerGas : maxPriorityFeePerGas; + const gasCost = gas * effectiveGasPrice; + + if (gasCost > TX_LIMIT) { + return false; + } - if (transactionArgs.blobVersionedHashes && transactionArgs.blobVersionedHashes.length > 0) { - var blobGasCost = BigInt(transactionArgs.gas) * maxFeePerBlobGas; - if (blobGasCost > TX_LIMIT) { - return false; + // Check blob gas costs if applicable + if (transactionArgs.blobVersionedHashes?.length > 0) { + const maxFeePerBlobGas = BigInt(transactionArgs.maxFeePerBlobGas || 0); + const blobGasCost = gas * maxFeePerBlobGas; + + if (blobGasCost > TX_LIMIT) { + return false; + } } + + return true; + } catch (error) { + console.error('Error calculating gas costs:', error); + return false; } - return true; }Let's verify the gas calculation approach:
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/server.ts (3 hunks)
🧰 Additional context used
🪛 Biome
src/server.ts
[error] 44-44: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/server.ts (3 hunks)
🔇 Additional comments (2)
src/server.ts (2)
40-48:⚠️ Potential issueFix switch case scoping and improve error handling
Previous review comments about switch case scoping and error response format are still valid. Additionally:
- Error message includes raw BigInt values which may cause JSON serialization issues
- Missing error logging as suggested in past reviews
Apply these fixes:
- - if (TX_BLOBPRICE_LIMIT > 0 || TX_GASPRICE_LIMIT > 0) { - let areFeesTooHigh = await feesTooHigh(result.data); - if (areFeesTooHigh) { - reply.code(400).send({error: `Fees too high TX_GAS_LIMIT|TX_BLOBPRICE_LIMIT [${TX_GASPRICE_LIMIT} |${TX_BLOBPRICE_LIMIT}] reached`}); - return; - } - } + { + const areFeesTooHigh = await feesTooHigh(result.data); + if (areFeesTooHigh) { + console.error('Transaction rejected due to excessive fees'); + reply.code(400).send({ + error: 'Invalid request', + message: 'Transaction fees exceed the configured limits' + }); + return; + } + }
64-94: 🛠️ Refactor suggestionImprove gas cost validation implementation
Previous review comments about improving the implementation are still valid. The current implementation needs:
- Input validation
- More efficient variable initialization
- Gas limit validation
- Consistent use of const
Consider this improved implementation:
async function feesTooHigh(transactionArgs: TransactionArgs) { - let maxFeePerGas = BigInt(0); - let maxPriorityFeePerGas = BigInt(0); - let maxFeePerBlobGas = BigInt(0); - - - - if (transactionArgs.maxFeePerGas ){ - maxFeePerGas = BigInt(transactionArgs.maxFeePerGas); - } - if (transactionArgs.maxPriorityFeePerGas) { - maxPriorityFeePerGas = BigInt(transactionArgs.maxPriorityFeePerGas); - } - if (transactionArgs.maxFeePerBlobGas) { - maxFeePerBlobGas = BigInt(transactionArgs.maxFeePerBlobGas); - } + // Skip validation if both limits are disabled + if (TX_GASPRICE_LIMIT <= 0 && TX_BLOBPRICE_LIMIT <= 0) { + return false; + } + + if (!transactionArgs.gas) { + throw new Error('Gas limit is required'); + } + + const maxFeePerGas = BigInt(transactionArgs.maxFeePerGas ?? 0); + const maxPriorityFeePerGas = BigInt(transactionArgs.maxPriorityFeePerGas ?? 0); + const maxFeePerBlobGas = BigInt(transactionArgs.maxFeePerBlobGas ?? 0); + const gasLimit = BigInt(transactionArgs.gas); - var gasPrice = (maxFeePerGas + maxPriorityFeePerGas); - if (gasPrice > TX_GASPRICE_LIMIT) { - console.error('Tx fees too high: %d > %d', gasPrice, TX_GASPRICE_LIMIT); - return true; - } + // Only check gas price if limit is enabled + if (TX_GASPRICE_LIMIT > 0) { + const gasPrice = maxFeePerGas + maxPriorityFeePerGas; + if (gasPrice > TX_GASPRICE_LIMIT) { + console.error('Transaction gas price too high: %d > %d', gasPrice, TX_GASPRICE_LIMIT); + return true; + } + } - if (transactionArgs.blobVersionedHashes && transactionArgs.blobVersionedHashes.length > 0) { - if (maxFeePerBlobGas > TX_BLOBPRICE_LIMIT) { - console.error('Blob fees too high: %d > %d', maxFeePerBlobGas, TX_BLOBPRICE_LIMIT ); - return true; + // Only check blob gas price if limit is enabled and blobs are present + if (TX_BLOBPRICE_LIMIT > 0 && transactionArgs.blobVersionedHashes?.length > 0) { + if (maxFeePerBlobGas > TX_BLOBPRICE_LIMIT) { + console.error('Blob gas price too high: %d > %d', maxFeePerBlobGas, TX_BLOBPRICE_LIMIT); + return true; } } return false; }
jlafiandra6
force-pushed
the
jlafi/gas_limit
branch
from
59c5b1c
to
a6cd114
Compare
Summary by CodeRabbit