fix(revm): make L1 fee fallback encoding tx-type exact#131
Merged
Conversation
The simulation-path fallback encoder (eth_call/eth_estimateGas) inferred the envelope type from field shapes, mis-encoding explicit EIP-2930 txs with empty access lists as legacy and dropping EIP-7702 authorization lists entirely. The env's tx_type is always populated by both callers (alloy-evm's try_into_tx_env and the statetest schema), so dispatch on it directly and add the missing EIP-7702 envelope. go-ethereum's current EstimateL1DataFeeForMessage still sizes untyped txs as dynamic-fee envelopes whenever a base fee exists; that quirk is mirrored only in the morph-statetest harness to keep state-root parity with geth-generated fixtures, and can be dropped once geth derives the envelope from the actual tx type. Closes #128
📝 WalkthroughWalkthroughThis PR makes L1-fee fallback encoding respect explicit tx types (EIP-2930, EIP-1559, EIP-7702), preserves type-specific fields in rebuilt envelopes, delegates access-list setting to upstream tx env, and forces Eip1559 for untyped state-test txs when a base fee exists. ChangesL1 Fee Type Preservation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
crates/revm/src/tx.rs (1)
727-759: 💤 Low valueConsider adding test coverage for recovered authorization path.
The test covers
Either::Left(signed)authorizations which are cloned directly. For completeness, consider adding a test case withEither::Right(RecoveredAuthorization)to verify the re-signing with placeholder signature path (lines 223-225) produces valid output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/revm/src/tx.rs` around lines 727 - 759, Add a test that mirrors encode_for_l1_fee_preserves_eip7702_authorization_list but uses Either::Right(RecoveredAuthorization{...}) instead of Either::Left(signed) to exercise the recovered-then-re-signed path; construct a MorphTxEnv with TransactionType::Eip7702 and authorization_list containing Either::Right(a RecoveredAuthorization for the same chain_id/address/nonce), call tx.encode_for_l1_fee(chain_id), decode with MorphTxEnvelope::decode_2718 and assert that decoded.tx().authorization_list contains the expected signed authorization (i.e., the RecoveredAuthorization was re-signed/converted to the same shape as a signed authorization). Ensure you reference the RecoveredAuthorization type and the encode_for_l1_fee / MorphTxEnvelope::decode_2718 flow when adding the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/revm/src/tx.rs`:
- Around line 727-759: Add a test that mirrors
encode_for_l1_fee_preserves_eip7702_authorization_list but uses
Either::Right(RecoveredAuthorization{...}) instead of Either::Left(signed) to
exercise the recovered-then-re-signed path; construct a MorphTxEnv with
TransactionType::Eip7702 and authorization_list containing Either::Right(a
RecoveredAuthorization for the same chain_id/address/nonce), call
tx.encode_for_l1_fee(chain_id), decode with MorphTxEnvelope::decode_2718 and
assert that decoded.tx().authorization_list contains the expected signed
authorization (i.e., the RecoveredAuthorization was re-signed/converted to the
same shape as a signed authorization). Ensure you reference the
RecoveredAuthorization type and the encode_for_l1_fee /
MorphTxEnvelope::decode_2718 flow when adding the new test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be1bcd67-4b49-473a-95f7-391a00b6abb8
📒 Files selected for processing (2)
bin/morph-statetest/src/schema.rscrates/revm/src/tx.rs
Upstream TxEnv::set_access_list upgrades tx_type from Legacy to Eip2930; the MorphTxEnv override only assigned the field, leaving the type byte stale. Now that the type byte is authoritative for fallback L1 fee encoding, delegate to the inner impl to stay consistent with upstream.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/revm/src/tx.rs (1)
729-761: 💤 Low valueConsider adding test coverage for recovered authorization re-signing.
The test verifies the
Either::Left(pre-signed) authorization path, but theEither::Right(recovered) path at lines 223-225 isn't exercised. That branch re-signs recovered authorizations with the placeholder signature.A supplemental test case could populate
authorization_listwith anEither::Right(RecoveredAuthorization)to verify the conversion logic produces valid EIP-2718 output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/revm/src/tx.rs` around lines 729 - 761, The test encode_for_l1_fee_preserves_eip7702_authorization_list covers Either::Left (pre-signed) authorizations but not the Either::Right (recovered) branch that re-signs recovered authorizations (the logic around encode_for_l1_fee / encode_for_l1_fee’s handling of RecoveredAuthorization). Add a new test similar to the existing one that constructs a MorphTxEnv with authorization_list containing Either::Right(RecoveredAuthorization { chain_id, address, nonce, signature_placeholder }) (or the appropriate RecoveredAuthorization constructor), call tx.encode_for_l1_fee(...) with the matching chain id, decode via MorphTxEnvelope::decode_2718 and assert the resulting decoded.tx().authorization_list contains the expected re-signed Authorization (i.e., matches the placeholder-resigned form), thereby exercising the re-signing branch in encode_for_l1_fee.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/revm/src/tx.rs`:
- Around line 729-761: The test
encode_for_l1_fee_preserves_eip7702_authorization_list covers Either::Left
(pre-signed) authorizations but not the Either::Right (recovered) branch that
re-signs recovered authorizations (the logic around encode_for_l1_fee /
encode_for_l1_fee’s handling of RecoveredAuthorization). Add a new test similar
to the existing one that constructs a MorphTxEnv with authorization_list
containing Either::Right(RecoveredAuthorization { chain_id, address, nonce,
signature_placeholder }) (or the appropriate RecoveredAuthorization
constructor), call tx.encode_for_l1_fee(...) with the matching chain id, decode
via MorphTxEnvelope::decode_2718 and assert the resulting
decoded.tx().authorization_list contains the expected re-signed Authorization
(i.e., matches the placeholder-resigned form), thereby exercising the re-signing
branch in encode_for_l1_fee.
chengwenxi
approved these changes
Jun 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MorphTxEnv::encode_for_l1_fee(used byeth_call/eth_estimateGaswhen no signed bytes exist) previously inferred the envelope type from field shapes. This mis-encoded explicit EIP-2930 transactions with empty access lists as legacy, and dropped EIP-7702 authorization lists entirely.tx_typeis always populated by both callers (alloy-evm'stry_into_tx_envviaminimal_tx_type(), and the statetest schema), so the encoder now dispatches onTransactionTypedirectly: EIP-2930 / EIP-1559 / EIP-7702 are re-encoded exactly (includingauthorization_list, re-signed with the fee-sizing placeholder for recovered authorizations); everything else stays legacy.EstimateL1DataFeeForMessagestill sizes untyped txs as dynamic-fee envelopes whenever a base fee exists. That quirk is mirrored only in themorph-statetestharness to keep state-root parity with geth-generated fixtures, and can be dropped once geth derives the envelope from the actual tx type.Test plan
encode_for_l1_fee_dynamic_matches_go_ethereum_bytes, placeholder-signature shape) still pass.cargo nextest run -p morph-revm -p morph-rpc -p morph-statetest— 154 passed.cargo clippy --all-targets -- -D warningsandcargo fmt --all -- --checkclean.Closes #128
Summary by CodeRabbit
Bug Fixes
Tests