Refactor: System Call and L1 Message Handling in REVM #18
Conversation
📝 WalkthroughWalkthroughThe changes refactor fee validation and ERC20 token-fee handling in the EVM handler. Fee validation logic is restructured to remove cfg.disable_fee_charge branching for system transactions while preserving L1 message-specific handling. Manual EVM call paths for balance lookup are replaced with system call routes. Cold state marking is added after token transfers executed via EVM. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Fix all issues with AI agents
In `@crates/revm/src/handler.rs`:
- Around line 128-133: The comment above the reimbursement check is stale: it
says "For L1 message transactions & system transactions, no reimbursement is
needed" but the code only checks tx.is_l1_msg() in evm.ctx().all_mut(); update
the comment to reflect the actual behavior (only L1 message transactions are
skipped) or, if you intended to skip system transactions too, add the
corresponding condition. Locate the block using evm.ctx().all_mut() and
tx.is_l1_msg() and either change the comment to "For L1 message transactions, no
reimbursement is needed" or modify the conditional to include the system
transaction predicate you want to enforce.
| let (_, tx, _, _, _, _) = evm.ctx().all_mut(); | ||
|
|
||
| // For L1 message transactions & system transactions, no reimbursement is needed | ||
| if tx.is_l1_msg() || cfg.disable_fee_charge { | ||
| if tx.is_l1_msg() { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Align the reimbursement comment with the new behavior.
Line 130 says system transactions skip reimbursement, but the code only skips for L1 messages. Please update the comment to avoid confusion.
📝 Suggested fix
- // For L1 message transactions & system transactions, no reimbursement is needed
+ // For L1 message transactions, no reimbursement is needed📝 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.
| let (_, tx, _, _, _, _) = evm.ctx().all_mut(); | |
| // For L1 message transactions & system transactions, no reimbursement is needed | |
| if tx.is_l1_msg() || cfg.disable_fee_charge { | |
| if tx.is_l1_msg() { | |
| return Ok(()); | |
| } | |
| let (_, tx, _, _, _, _) = evm.ctx().all_mut(); | |
| // For L1 message transactions, no reimbursement is needed | |
| if tx.is_l1_msg() { | |
| return Ok(()); | |
| } |
🤖 Prompt for AI Agents
In `@crates/revm/src/handler.rs` around lines 128 - 133, The comment above the
reimbursement check is stale: it says "For L1 message transactions & system
transactions, no reimbursement is needed" but the code only checks
tx.is_l1_msg() in evm.ctx().all_mut(); update the comment to reflect the actual
behavior (only L1 message transactions are skipped) or, if you intended to skip
system transactions too, add the corresponding condition. Locate the block using
evm.ctx().all_mut() and tx.is_l1_msg() and either change the comment to "For L1
message transactions, no reimbursement is needed" or modify the conditional to
include the system transaction predicate you want to enforce.
This pull request refactors the handling of system calls and L1 messages within the REVM for improved clarity and efficiency.
The key changes include:
Removal of
disable_fee_chargeflag: Thedisable_fee_chargeflag, which was previously used to bypass fee validation and rewards for system transactions, has been removed. The logic now relies on more explicit checks, such astx.is_l1_msg(), to identify L1 messages and apply the appropriate rules.Improved
balanceOfcall: ThebalanceOffunction in thetoken_feemodule has been updated to use thesystem_call_onemethod instead oftransact_one. This provides a more direct and efficient way to execute the system call.State "cold" optimization: An optimization has been added to mark the state as "cold" after pre-execution state changes. This prevents warm account access in the main transaction execution, improving performance.
Summary by CodeRabbit
Release Notes
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.