chore(l1): add default gas limit to eth_call RPC method
#4755
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds default and maximum gas limits to the eth_call RPC method to improve resource safety and align with Ethereum client standards.
- Introduced
DEFAULT_ETH_CALL_GAS_LIMIT(50M) andMAX_ETH_CALL_GAS_LIMIT(100M) constants - Modified
CallRequest::handleto apply default gas limit when none is specified and cap custom gas limits - Enhanced transaction preparation to ensure proper gas limit enforcement before simulation
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let mut transaction = self.transaction.clone(); | ||
| let gas_limit = match transaction.gas { | ||
| Some(gas) => { | ||
| let gas_u64 = u64::try_from(gas).unwrap_or(u64::MAX); |
There was a problem hiding this comment.
Using unwrap_or(u64::MAX) could result in unexpected behavior when the gas value exceeds u64 range. The fallback to u64::MAX bypasses the intended capping logic since std::cmp::min(u64::MAX, MAX_ETH_CALL_GAS_LIMIT) would always return MAX_ETH_CALL_GAS_LIMIT. Consider using unwrap_or(MAX_ETH_CALL_GAS_LIMIT) instead to maintain consistent capping behavior.
| let gas_u64 = u64::try_from(gas).unwrap_or(u64::MAX); | |
| let gas_u64 = u64::try_from(gas).unwrap_or(MAX_ETH_CALL_GAS_LIMIT); |
Copilot uses AI. Check for mistakes.
| // Prepare transaction with gas limit | ||
| let mut transaction = self.transaction.clone(); | ||
| let gas_limit = match transaction.gas { | ||
| Some(gas) => u64::try_from(gas).unwrap_or(u64::MAX), |
There was a problem hiding this comment.
Why not use DEFAULT_ETH_CALL_GAS_LIMIT as the default value for the unwrap?
You could simplify the match with just the unwrap_or
There was a problem hiding this comment.
Totally agree, changing right away
Peponks9
force-pushed
the
fix-eth-call-gas-limit
branch
from
7178c95 to
3d504d9
Compare
Peponks9
force-pushed
the
fix-eth-call-gas-limit
branch
from
08bb041 to
21fd4aa
Compare
| // Prepare transaction with gas limit | ||
| let mut transaction = self.transaction.clone(); | ||
| #[allow(clippy::useless_conversion)] | ||
| let gas_limit = transaction.gas.map_or(DEFAULT_ETH_CALL_GAS_LIMIT, |gas| { |
There was a problem hiding this comment.
let gas_limit = transaction
.gas
.and_then(|gas| u64::try_from(gas).ok())
.unwrap_or(DEFAULT_ETH_CALL_GAS_LIMIT);
is simpler
The
eth_callRPC method lacked a server-side gas limit. This change ensures resource safety and aligns with Ethereum client standards.Description
DEFAULT_ETH_CALL_GAS_LIMITandMAX_ETH_CALL_GAS_LIMITCallRequest::handleto apply the default gas limit if none is specified, and cap custom gas limits.Closes #4366