refactor: simplify logic across crates#56
Merged
chengwenxi merged 3 commits intomainfrom Mar 25, 2026
Merged
Conversation
Eliminate duplication and reduce boilerplate: - consensus: Add From<MorphConsensusError> for ConsensusError, simplify 12 error sites - primitives/receipt: Extract rlp_header_inner_impl, use as_receipt() delegation - primitives/transaction: Use matches! macro, simplify encode_2718, extract decode_common_fields - revm/token_fee: Reuse compute_mapping_slot_for_address, merge query_erc20_balance - revm/tx: Chain builder calls - revm/handler: Simplify reimburse_caller paths - engine-api: Extract update_block_tag helper - payload: Remove redundant new() wrappers, simplify builder constructors - chainspec: Remove duplicate comment - rpc: Remove duplicate import alias Net reduction: ~70 lines, no functional changes.
📝 WalkthroughWalkthroughThis PR consolidates and refactors code across multiple crates by extracting helper methods, delegating to existing utilities, removing redundant constructors, and simplifying error handling through trait conversions. Changes maintain existing functionality while improving code maintainability and reducing duplication. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
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📝 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.
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)
258-268:⚠️ Potential issue | 🟠 MajorDon't squash
EVMError::Databaseinto a zero balance here.This helper now returns
Ok(U256::ZERO)for everysystem_call_onefailure, which makes theErr(EVMError::Database(_))arm inread_token_balance_with_fallbackdead. A backend read failure then gets misreported as a zero token balance / insufficient-funds condition instead of surfacing the real DB error.Suggested fix
match evm.system_call_one(token, calldata) { Ok(result) if result.is_success() => { if let Some(output) = result.output() && output.len() >= 32 { return Ok(U256::from_be_slice(&output[..32])); } Ok(U256::ZERO) } Ok(_) => Ok(U256::ZERO), - Err(_) => Ok(U256::ZERO), + Err(err @ EVMError::Database(_)) => Err(err), + Err(_) => Ok(U256::ZERO), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/token_fee.rs` around lines 258 - 268, The current match on evm.system_call_one(token, calldata) maps all failures to Ok(U256::ZERO), which hides EVMError::Database and makes read_token_balance_with_fallback's database-error arm unreachable; update the Err(_) arm to inspect the error (capture it as Err(e)) and if it is EVMError::Database return Err(e) (propagate the DB error) otherwise keep returning Ok(U256::ZERO); locate the match around evm.system_call_one in token_fee.rs and change the Err arm accordingly so EVMError::Database is not squashed.
🧹 Nitpick comments (1)
crates/primitives/src/transaction/morph_transaction.rs (1)
400-432: LGTM! Clean DRY refactor.The helper successfully eliminates duplication by extracting the 11 common RLP fields shared by both V0 and V1 decoders. The decoding order matches the encoding order in
encode_fields, and error propagation is correct.💡 Optional: Consider a helper struct for better readability
The 11-element tuple is a bit unwieldy. If this grows further, consider a private helper struct with named fields:
+#[derive(Debug)] +struct CommonFields { + chain_id: ChainId, + nonce: u64, + max_priority_fee_per_gas: u128, + max_fee_per_gas: u128, + gas_limit: u64, + to: TxKind, + value: U256, + input: Bytes, + access_list: AccessList, + fee_token_id: u16, + fee_limit: U256, +} + -fn decode_common_fields(buf: &mut &[u8]) -> alloy_rlp::Result<(...)> { +fn decode_common_fields(buf: &mut &[u8]) -> alloy_rlp::Result<CommonFields> { - Ok(( - Decodable::decode(buf)?, - ... - )) + Ok(CommonFields { + chain_id: Decodable::decode(buf)?, + ... + })However, for a stable 11-field set, the current tuple approach is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/primitives/src/transaction/morph_transaction.rs` around lines 400 - 432, The current decode_common_fields returns an 11-element tuple which is hard to read; create a private helper struct (e.g., CommonTxFields) with named fields for chain_id, nonce, max_priority_fee_per_gas, max_fee_per_gas, gas_limit, to, value, input, access_list, fee_token_id, fee_limit, change decode_common_fields to return that struct instead of a tuple, and update any callers (V0/V1 decoders that unpack decode_common_fields) and the encode_fields implementation to construct/consume CommonTxFields; keep the decoding logic identical but use named fields for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/revm/src/token_fee.rs`:
- Around line 258-268: The current match on evm.system_call_one(token, calldata)
maps all failures to Ok(U256::ZERO), which hides EVMError::Database and makes
read_token_balance_with_fallback's database-error arm unreachable; update the
Err(_) arm to inspect the error (capture it as Err(e)) and if it is
EVMError::Database return Err(e) (propagate the DB error) otherwise keep
returning Ok(U256::ZERO); locate the match around evm.system_call_one in
token_fee.rs and change the Err arm accordingly so EVMError::Database is not
squashed.
---
Nitpick comments:
In `@crates/primitives/src/transaction/morph_transaction.rs`:
- Around line 400-432: The current decode_common_fields returns an 11-element
tuple which is hard to read; create a private helper struct (e.g.,
CommonTxFields) with named fields for chain_id, nonce, max_priority_fee_per_gas,
max_fee_per_gas, gas_limit, to, value, input, access_list, fee_token_id,
fee_limit, change decode_common_fields to return that struct instead of a tuple,
and update any callers (V0/V1 decoders that unpack decode_common_fields) and the
encode_fields implementation to construct/consume CommonTxFields; keep the
decoding logic identical but use named fields for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0fdec55-f943-461b-857f-eb147553f91d
📒 Files selected for processing (20)
crates/chainspec/src/spec.rscrates/consensus/src/error.rscrates/consensus/src/validation.rscrates/engine-api/src/builder.rscrates/evm/src/block/mod.rscrates/node/src/node.rscrates/payload/builder/src/builder.rscrates/payload/types/src/executable_l2_data.rscrates/payload/types/src/safe_l2_data.rscrates/primitives/src/receipt/envelope.rscrates/primitives/src/receipt/mod.rscrates/primitives/src/transaction/envelope.rscrates/primitives/src/transaction/l1_transaction.rscrates/primitives/src/transaction/morph_transaction.rscrates/revm/src/handler.rscrates/revm/src/precompiles.rscrates/revm/src/token_fee.rscrates/revm/src/tx.rscrates/rpc/src/types/transaction.rscrates/txpool/src/validator.rs
💤 Files with no reviewable changes (4)
- crates/chainspec/src/spec.rs
- crates/revm/src/precompiles.rs
- crates/payload/types/src/executable_l2_data.rs
- crates/payload/types/src/safe_l2_data.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/primitives/src/receipt/mod.rs (1)
131-167: Consider applying the sameas_receipt()simplification for consistency.Since
rlp_encoded_fields_lengthandrlp_encode_fieldsnow delegate toas_receipt(), you could apply the same pattern here for consistency:♻️ Optional refactor to use `as_receipt()` for consistency
pub fn rlp_encoded_fields_length_without_bloom(&self) -> usize { - match self { - Self::Legacy(r) - | Self::Eip2930(r) - | Self::Eip1559(r) - | Self::Eip7702(r) - | Self::Morph(r) => { - r.inner.status.length() - + r.inner.cumulative_gas_used.length() - + r.inner.logs.length() - } - Self::L1Msg(r) => r.status.length() + r.cumulative_gas_used.length() + r.logs.length(), - } + let r = self.as_receipt(); + r.status.length() + r.cumulative_gas_used.length() + r.logs.length() } pub fn rlp_encode_fields_without_bloom(&self, out: &mut dyn BufMut) { - match self { - Self::Legacy(r) - | Self::Eip2930(r) - | Self::Eip1559(r) - | Self::Eip7702(r) - | Self::Morph(r) => { - r.inner.status.encode(out); - r.inner.cumulative_gas_used.encode(out); - r.inner.logs.encode(out); - } - Self::L1Msg(r) => { - r.status.encode(out); - r.cumulative_gas_used.encode(out); - r.logs.encode(out); - } - } + let r = self.as_receipt(); + r.status.encode(out); + r.cumulative_gas_used.encode(out); + r.logs.encode(out); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/primitives/src/receipt/mod.rs` around lines 131 - 167, The two methods rlp_encoded_fields_length_without_bloom and rlp_encode_fields_without_bloom duplicate pattern-matching across receipt variants; refactor them to call as_receipt() like rlp_encoded_fields_length and rlp_encode_fields do: replace the match arms in rlp_encoded_fields_length_without_bloom with let r = self.as_receipt(); then compute r.status.length() + r.cumulative_gas_used.length() + r.logs.length(), and in rlp_encode_fields_without_bloom call let r = self.as_receipt(); then r.status.encode(out); r.cumulative_gas_used.encode(out); r.logs.encode(out); ensuring you handle the existing Borrowing/inner access differences by using the unified as_receipt() view.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/primitives/src/receipt/mod.rs`:
- Around line 131-167: The two methods rlp_encoded_fields_length_without_bloom
and rlp_encode_fields_without_bloom duplicate pattern-matching across receipt
variants; refactor them to call as_receipt() like rlp_encoded_fields_length and
rlp_encode_fields do: replace the match arms in
rlp_encoded_fields_length_without_bloom with let r = self.as_receipt(); then
compute r.status.length() + r.cumulative_gas_used.length() + r.logs.length(),
and in rlp_encode_fields_without_bloom call let r = self.as_receipt(); then
r.status.encode(out); r.cumulative_gas_used.encode(out); r.logs.encode(out);
ensuring you handle the existing Borrowing/inner access differences by using the
unified as_receipt() view.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c967feb-8756-42ce-bbeb-bfbdea3c0aba
📒 Files selected for processing (5)
crates/consensus/src/error.rscrates/evm/src/block/mod.rscrates/primitives/src/receipt/mod.rscrates/revm/src/token_fee.rscrates/revm/src/tx.rs
✅ Files skipped from review due to trivial changes (1)
- crates/revm/src/tx.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/evm/src/block/mod.rs
- crates/revm/src/token_fee.rs
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
chengwenxi
approved these changes
Mar 25, 2026
This was referenced Mar 26, 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
Test Plan
Summary by CodeRabbit
Refactor
Chores