feat: Refactor alt-fee mechanism and update EVM configuration #14
Conversation
📝 WalkthroughWalkthroughThis change set introduces runtime hardfork configuration capability, disables specific Ethereum opcodes (SELFDESTRUCT, BLOBHASH, BLOBBASEFEE) for Morph execution, reduces system call gas limits, and restructures fee handling to support L1 messages, system transactions, and ERC20-based fee payments with improved L1 cost calculations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/revm/src/token_fee.rs (1)
271-288: Thedisable_fee_chargeflag is not restored after the EVM call.Setting
evm.cfg.disable_fee_charge = truemodifies the EVM's configuration, but it's not restored to its original value aftertransact_onecompletes. If the same EVM instance is reused after this function call, subsequent transactions will unexpectedly have fee charging disabled.🐛 Proposed fix
// Execute using transact_one + let original_disable_fee_charge = evm.cfg.disable_fee_charge; evm.cfg.disable_fee_charge = true; // Disable fee charge for system call - match evm.transact_one(morph_tx) { + let result = match evm.transact_one(morph_tx) { Ok(result) => { if result.is_success() { // Parse the returned balance (32 bytes) if let Some(output) = result.output() && output.len() >= 32 { return Ok(U256::from_be_slice(&output[..32])); } } Ok(U256::ZERO) } Err(_) => { // On error, return zero (matches original behavior) Ok(U256::ZERO) } - } + }; + evm.cfg.disable_fee_charge = original_disable_fee_charge; + result }
🧹 Nitpick comments (5)
crates/chainspec/src/spec.rs (1)
169-175: Theset_hardforkmethod only supports timestamp-based activation.This method unconditionally uses
ForkCondition::Timestamp(time), but some Morph hardforks (Bernoulli and Curie) are block-based according to lines 142-147. If this method is intended to support all hardforks, consider adding a variant that accepts block-based conditions or documenting this limitation.♻️ Suggested improvement
impl MorphChainSpec { - pub fn set_hardfork(&mut self, hardfork: MorphHardfork, time: u64) { + /// Set a timestamp-based hardfork activation. + /// + /// Note: This only supports timestamp-based hardforks (Morph203, Viridian, Emerald, MPTFork). + /// For block-based hardforks (Bernoulli, Curie), use `set_hardfork_at_block`. + pub fn set_hardfork_at_timestamp(&mut self, hardfork: MorphHardfork, time: u64) { self.inner .hardforks .insert(hardfork, ForkCondition::Timestamp(time)); } + + /// Set a block-based hardfork activation. + pub fn set_hardfork_at_block(&mut self, hardfork: MorphHardfork, block: u64) { + self.inner + .hardforks + .insert(hardfork, ForkCondition::Block(block)); + } }crates/revm/src/tx.rs (1)
145-175: Silent failure in RLP extraction could mask malformed transactions.Both
extract_fee_token_id_from_rlpandextract_fee_limit_from_rlpreturn default values (0 /U256::default()) when decoding fails. For anALT_FEE_TXwith malformed RLP, this causesfee_token_idto become 0, which later triggers aTokenIdZeroNotSupportederror inhandler.rs. This masks the actual issue (malformed RLP) with a misleading error.Consider logging a warning or returning an
Option/Resultto surface decoding failures.♻️ Suggested improvement
fn extract_fee_token_id_from_rlp(rlp_bytes: &Bytes) -> u16 { if rlp_bytes.is_empty() { return 0; } // Skip the type byte (0x7F) and decode the AltFeeTx let payload = &rlp_bytes[1..]; - TxAltFee::decode(&mut &payload[..]) - .map(|tx| tx.fee_token_id) - .unwrap_or(0) + match TxAltFee::decode(&mut &payload[..]) { + Ok(tx) => tx.fee_token_id, + Err(e) => { + tracing::warn!(error = ?e, "Failed to decode AltFeeTx for fee_token_id extraction"); + 0 + } + } }crates/revm/src/evm.rs (1)
45-51: Consider using named constants for disabled opcodes.The opcode bytes are hardcoded as hex values. Using named constants would improve readability and maintainability.
♻️ Suggested improvement
+// Disabled opcodes in Morph EVM +const OPCODE_SELFDESTRUCT: u8 = 0xff; +const OPCODE_BLOBHASH: u8 = 0x49; +const OPCODE_BLOBBASEFEE: u8 = 0x4a; + impl<DB: Database, I> MorphEvm<DB, I> { pub fn new(ctx: MorphContext<DB>, inspector: I) -> Self { let spec = ctx.cfg.spec; let precompiles = MorphPrecompiles::new_with_spec(spec); let mut instructions = EthInstructions::new_mainnet(); - // SELFDESTRUCT is disabled in Morph - instructions.insert_instruction(0xff, Instruction::unknown()); - // BLOBHASH is disabled in Morph - instructions.insert_instruction(0x49, Instruction::unknown()); - // BLOBBASEFEE is disabled in Morph - instructions.insert_instruction(0x4a, Instruction::unknown()); + // Disable unsupported opcodes in Morph L2 + instructions.insert_instruction(OPCODE_SELFDESTRUCT, Instruction::unknown()); + instructions.insert_instruction(OPCODE_BLOBHASH, Instruction::unknown()); + instructions.insert_instruction(OPCODE_BLOBBASEFEE, Instruction::unknown());crates/revm/src/handler.rs (2)
363-372: Minor inconsistency: Usingtoken_fee_info.callerinstead of localcallervariable.Line 368 passes
token_fee_info.callerwhile the slot-based transfer at line 357-361 uses the localcallervariable. While they should be equivalent (sincetoken_fee_infowas fetched withcallerat line 339), using the local variable consistently would be clearer.♻️ Suggested fix
} else { // Transfer with evm call. transfer_erc20_with_evm( evm, beneficiary, - token_fee_info.caller, + caller, token_fee_info.token_address, token_amount_required, )?; }
473-482: Same inconsistency:token_fee_info.callervscaller_addrin EVM transfer.The slot-based transfer (line 454) uses
caller_addr, but the EVM-based transfer (line 477) usestoken_fee_info.caller. For consistency and clarity, both should usecaller_addr.♻️ Suggested fix
} else { // Transfer with evm call. transfer_erc20_with_evm( evm, - token_fee_info.caller, + caller_addr, beneficiary, token_fee_info.token_address, token_amount_required, )?; }
This PR introduces several significant changes to the Morph EVM and chain specification.
Key Changes:
Use
system_callfor Alt-Fee Gas: The mechanism for paying transaction fees using alternative tokens (alt-fee) has been fundamentally refactored. It now utilizes asystem_callto execute the ERC20 token transfer for the fee payment. This isolates the fee payment logic from the main transaction's execution context, leading to a more robust and secure implementation.Enable Dynamic Hardfork Configuration: The
MorphChainSpecnow includes aset_hardforkfunction. This allows for the dynamic configuration of hardfork activation times, which is essential for testing and managing network upgrades more flexibly.Disable Opcodes: To enhance security and align with the specific requirements of the Morph network, the following opcodes have been disabled:
SELFDESTRUCTBLOBHASHBLOBBASEFEEOther Changes:
These changes improve the security, flexibility, and robustness of the Morph chain.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.