Conversation
📝 WalkthroughWalkthroughAdds new workspace crates ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MorphEthApi
participant TokenFeeInfo
participant Database
participant EVM
Client->>MorphEthApi: eth_call / eth_estimateGas
MorphEthApi->>MorphEthApi: build EVM env, compute L1 fee
alt fee_token_id present
MorphEthApi->>TokenFeeInfo: fetch(db, token_id)
TokenFeeInfo->>Database: load_registry_entry / read storage
Database-->>TokenFeeInfo: registry data
MorphEthApi->>Database: read token balance (mapping_slot_for) or call EVM
Database-->>MorphEthApi: balance (or EVM->Database->EVM path)
MorphEthApi->>MorphEthApi: eth_to_token_amount(L1_fee), compute token allowance
else pay with ETH
MorphEthApi->>MorphEthApi: compute ETH-based allowance from balance
end
MorphEthApi->>EVM: simulate call with computed gas_limit
EVM->>Database: state reads
Database-->>EVM: state
EVM-->>MorphEthApi: simulation result
MorphEthApi-->>Client: return result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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
🧪 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 |
panos-xyz
requested review from
anylots and
chengwenxi
and removed request for
anylots
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/payload/types/src/attributes.rs (1)
17-23:⚠️ Potential issue | 🟠 MajorGate
#[serde(flatten)]to match other serde attributes.The unconditional
#[serde(flatten)]on line 22 will cause compile errors when the serde feature is disabled. Gate it with#[cfg_attr(feature = "serde", ...)]to match the serde derives on lines 18-19 and the attribute on lines 29-32.✅ Suggested fix
- #[serde(flatten)] + #[cfg_attr(feature = "serde", serde(flatten))]crates/payload/types/src/executable_l2_data.rs (1)
126-190:⚠️ Potential issue | 🟡 MinorSerde tests need feature gating.
The serde roundtrip tests (
test_serde_roundtrip,test_serde_without_base_fee,test_serde_camel_case) useserde_jsonserialization but are not gated behind#[cfg(feature = "serde")]. These tests will fail to compile when theserdefeature is disabled since theSerialize/Deserializederives won't be present.🔧 Proposed fix
+ #[cfg(feature = "serde")] #[test] fn test_serde_roundtrip() { // ... } + #[cfg(feature = "serde")] #[test] fn test_serde_without_base_fee() { // ... } + #[cfg(feature = "serde")] #[test] fn test_serde_camel_case() { // ... }
🤖 Fix all issues with AI agents
In `@crates/payload/types/src/safe_l2_data.rs`:
- Around line 15-50: The serde-based tests test_serde_roundtrip and
test_serde_without_optional_fields must be compiled only when the "serde"
feature is enabled; add #[cfg(feature = "serde")] (or wrap them in a
#[cfg(feature = "serde")] mod) above each test to gate them, and ensure any uses
of serde_json in those tests are only present under that cfg so builds with
--no-default-features compile cleanly.
In `@crates/rpc/Cargo.toml`:
- Around line 12-17: The morph-primitives dependency in Cargo.toml currently
enables features ["serde","reth-codec"] but should follow the repository
migration to serde-bincode-compat; update the morph-primitives entry (the line
starting with morph-primitives = { workspace = true, features = [...] }) to use
features = ["serde-bincode-compat"] instead of ["serde","reth-codec"] so the RPC
crate matches other crates and removes the unused reth-codec feature.
🧹 Nitpick comments (5)
crates/revm/src/token_fee.rs (2)
209-225: Clarify expected key encoding formapping_slot.A short doc tweak reduces the chance of callers passing non-ABI-encoded keys.
✏️ Suggested doc clarification
-/// For `mapping(keyType => valueType)` at slot `base_slot`, -/// the value for `key` is at `keccak256(key ++ base_slot)`. +/// For `mapping(keyType => valueType)` at slot `base_slot`, +/// `key` should be ABI-encoded to 32 bytes for static types; then the value is at +/// `keccak256(key ++ base_slot)`.
369-393: Add a rounding-up test case to lock in token fee behavior.Current coverage focuses on exact division and zero ratio; a remainder case would protect the round-up behavior.
Based on learnings: When reviewing token fee logic, rounding up via calculate_token_amount is intentional (geth-consistent) and should be protected with regression tests.✅ Suggested test addition
@@ let token_amount = info.eth_to_token_amount(eth_amount); assert_eq!(token_amount, U256::from(500_000_000_000_000_000u128)); // 0.5 tokens + + let info_round = TokenFeeInfo { + price_ratio: U256::from(3_000_000_000_000_000_000u128), // 3 ETH per token + scale: U256::from(1_000_000_000_000_000_000u128), + ..Default::default() + }; + let token_amount_round = info_round.eth_to_token_amount(eth_amount); + assert_eq!(token_amount_round, U256::from(333_333_333_333_333_334u128));crates/primitives/src/lib.rs (1)
33-34: Clarify the purpose of the unused import.The
as _pattern indicates this import is for side effects (likely trait implementations). Consider adding a brief comment explaining why this import is needed under this feature flag.📝 Suggested documentation
#[cfg(feature = "serde-bincode-compat")] +// Required for trait implementations from reth_ethereum_primitives use reth_ethereum_primitives as _;crates/payload/types/src/executable_l2_data.rs (1)
64-65: Consider usingBloomtype instead ofBytesforlogs_bloom.The
logs_bloomfield is typed asBytes, but logs bloom filters have a fixed 256-byte size. Usingalloy_primitives::Bloomwould provide compile-time size guarantees and better type safety. The test at line 139 usesBytes::from(vec![0; 256]), suggesting the fixed size is expected.♻️ Suggested change
-use alloy_primitives::{Address, B256, Bytes}; +use alloy_primitives::{Address, B256, Bloom, Bytes}; // ... /// Bloom filter for logs. - pub logs_bloom: Bytes, + pub logs_bloom: Bloom,crates/primitives/src/receipt/envelope.rs (1)
40-67: Consider accepting owned logs to avoid unconditional cloning.
from_partsalways clones logs. Accepting ownedLogitems lets callers pass already-owned logs without extra cloning (borrowed callers can still use.iter().cloned()).♻️ Proposed tweak
- pub fn from_parts<'a>( - status: bool, - cumulative_gas_used: u64, - logs: impl IntoIterator<Item = &'a Log>, - tx_type: MorphTxType, - ) -> Self { - let logs = logs.into_iter().cloned().collect::<Vec<_>>(); + pub fn from_parts( + status: bool, + cumulative_gas_used: u64, + logs: impl IntoIterator<Item = Log>, + tx_type: MorphTxType, + ) -> Self { + let logs = logs.into_iter().collect::<Vec<_>>();
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/rpc/src/eth/transaction.rs`:
- Around line 241-259: The encoding branch in encode_tx_for_l1_fee currently
treats any presence of tx_env.fee_limit as a signal to build a Morph
transaction; update the condition to only check fee_token_id (i.e., use
tx_env.fee_token_id.unwrap_or_default() > 0) so Morph encoding via
build_morph_tx_from_env(...) and encode_2718(...) happens only when fee_token_id
> 0; otherwise call inner.build_typed_simulate_transaction() and encode_2718 on
that envelope as before. Ensure you remove the "|| tx_env.fee_limit.is_some()"
part and keep error mapping to EthApiError::InvalidParams unchanged.
🧹 Nitpick comments (2)
crates/rpc/Cargo.toml (1)
46-50: Reorganize misplaced dependencies.
tokioandrevmare categorized under "Serialization" but are not serialization libraries. Consider moving them to a more appropriate section (e.g., "Async runtime" for tokio, and grouping revm with the other EVM-related dependencies).♻️ Suggested reorganization
# Serialization serde.workspace = true derive_more.workspace = true -tokio.workspace = true -revm.workspace = true + +# Async runtime +tokio.workspace = true + +# EVM +revm.workspace = truecrates/rpc/src/eth/transaction.rs (1)
117-152: Consider avoiding redundant clone.The
innervariable is cloned on line 130 fortry_into_tx_env, but it's also passed by value toencode_tx_for_l1_feeon line 147. This works but involves two separate uses ofinner. If the order were restructured, one clone could potentially be avoided.♻️ Suggested optimization
fn try_into_tx_env<Spec>( self, evm_env: &EvmEnv<Spec, MorphBlockEnv>, ) -> Result<MorphTxEnv, Self::Err> { let fee_token_id = self.fee_token_id; let fee_limit = self.fee_limit; let inner = self.inner; let access_list = inner.access_list.clone().unwrap_or_default(); - let inner_tx_env = inner - .clone() - .try_into_tx_env(evm_env) - .map_err(EthApiError::from)?; + let inner_tx_env = inner.try_into_tx_env(evm_env).map_err(EthApiError::from)?; let mut tx_env = MorphTxEnv::new(inner_tx_env); tx_env.fee_token_id = match fee_token_id { Some(id) => Some( u16::try_from(id.to::<u64>()) .map_err(|_| EthApiError::InvalidParams("invalid token".to_string()))?, ), None => None, }; tx_env.fee_limit = fee_limit; if tx_env.fee_token_id.unwrap_or_default() > 0 { tx_env.inner.tx_type = morph_primitives::MORPH_TX_TYPE_ID; } - let rlp_bytes = encode_tx_for_l1_fee(&tx_env, access_list, evm_env, inner)?; + let rlp_bytes = encode_tx_for_l1_fee(&tx_env, access_list, evm_env)?; tx_env.rlp_bytes = Some(rlp_bytes); Ok(tx_env) }This would require
encode_tx_for_l1_feeto reconstruct the request fromtx_envfields, or accept only what it needs. Given the current structure, the clone may be acceptable for clarity.
2 participants
Summary by CodeRabbit
New Features
Refactor
Breaking Changes