refactor: resolve code reviews

risechain · Sep 9, 2024 · ed8fdff · ed8fdff
1 parent dda9aa1
commit ed8fdff
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 46 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -4,7 +4,13 @@ version = "0.1.0"
 edition = "2021"
 
 [features]
-optimism = ["revm/optimism"]
+default = ["optimism"]
+optimism = [
+    "revm/optimism",
+    "dep:op-alloy-consensus",
+    "dep:op-alloy-network",
+    "dep:op-alloy-rpc-types",
+]
 
 [dependencies]
 # Put this behind a feature flag if there are use cases & users
@@ -21,9 +27,6 @@ alloy-trie = "0.5.1"
 bitflags = "2.6.0"
 bitvec = "1.0.1"
 dashmap = "6.0.1"
-op-alloy-consensus = "0.2.7"
-op-alloy-network = "0.2.7"
-op-alloy-rpc-types = "0.2.7"
 serde = "1.0.209"
 smallvec = "1.13.2"
 
@@ -41,6 +44,11 @@ alloy-transport-http = "0.3.1"
 reqwest = "0.12.7"
 tokio = { version = "1.40.0", features = ["rt-multi-thread"] }
 
+# OP dependencies
+op-alloy-consensus = { version = "0.2.7", optional = true }
+op-alloy-network = { version = "0.2.7", optional = true }
+op-alloy-rpc-types = { version = "0.2.7", optional = true }
+
 [dev-dependencies]
 bincode = "1.3.3"
 criterion = "0.5.1"

diff --git a/src/chain.rs b/src/chain.rs
@@ -84,6 +84,6 @@ mod ethereum;
 pub use ethereum::PevmEthereum;
 
 #[cfg(feature = "optimism")]
-pub(crate) mod optimism;
+mod optimism;
 #[cfg(feature = "optimism")]
 pub use optimism::PevmOptimism;
diff --git a/src/chain/ethereum.rs b/src/chain/ethereum.rs
@@ -195,7 +195,7 @@ impl PevmChain for PevmEthereum {
         txs: &BlockTransactions<Self::Transaction>,
         tx_results: &[PevmTxExecutionResult],
     ) -> B256 {
-        // 1. Create an iterator of ReceiptEnvelope
+        // 1. Create an iterator of [ReceiptEnvelope]
         let tx_type_iter = txs
             .txns()
             .map(|tx| TxType::try_from(tx.transaction_type.unwrap_or_default()).unwrap());

diff --git a/src/chain/optimism.rs b/src/chain/optimism.rs
@@ -1,5 +1,4 @@
 //! Optimism
-#![allow(missing_docs)]
 use std::collections::{BTreeMap, HashMap};
 
 use alloy_chains::NamedChain;
@@ -8,7 +7,6 @@ use alloy_primitives::{Bytes, B256, U256};
 use alloy_rpc_types::{BlockTransactions, Header};
 use op_alloy_consensus::{OpDepositReceipt, OpReceiptEnvelope, OpTxEnvelope, OpTxType, TxDeposit};
 use op_alloy_network::eip2718::Encodable2718;
-use op_alloy_rpc_types::Transaction;
 use revm::{
     primitives::{BlockEnv, OptimismFields, SpecId, TxEnv},
     Handler,
@@ -25,6 +23,7 @@ pub struct PevmOptimism {
 }
 
 impl PevmOptimism {
+    /// Optimism Mainnet
     pub fn mainnet() -> Self {
         PevmOptimism {
             id: NamedChain::Optimism.into(),
@@ -48,10 +47,11 @@ pub enum OptimismTransactionParsingError {
     MissingSourceHash,
     OverflowedGasLimit,
     SerdeError(String),
-    UnexpectedType(u8),
 }
 
-fn get_optimism_gas_price(tx: &Transaction) -> Result<U256, OptimismTransactionParsingError> {
+fn get_optimism_gas_price(
+    tx: &op_alloy_rpc_types::Transaction,
+) -> Result<U256, OptimismTransactionParsingError> {
     let tx_type_raw = tx.inner.transaction_type.unwrap_or_default();
     let Ok(tx_type) = OpTxType::try_from(tx_type_raw) else {
         return Err(OptimismTransactionParsingError::InvalidType(tx_type_raw));
@@ -73,52 +73,51 @@ fn get_optimism_gas_price(tx: &Transaction) -> Result<U256, OptimismTransactionP
 }
 
 /// Convert [Transaction] to [OptimismFields]
+/// https://github.com/paradigmxyz/reth/blob/fc4c037e60b623b81b296fe9242fa905ff36b89a/crates/primitives/src/transaction/compat.rs#L99
 pub(crate) fn get_optimism_fields(
-    tx: &Transaction,
+    tx: &op_alloy_rpc_types::Transaction,
 ) -> Result<OptimismFields, OptimismTransactionParsingError> {
-    let envelope_buf = {
-        let tx_type = tx.inner.transaction_type.unwrap_or_default();
-        let op_tx_type = OpTxType::try_from(tx_type)
-            .map_err(|_err| OptimismTransactionParsingError::UnexpectedType(tx_type))?;
-        let inner = tx.inner.clone();
-        let tx_envelope = match op_tx_type {
-            OpTxType::Legacy => Signed::<TxLegacy>::try_from(inner).map(OpTxEnvelope::from),
-            OpTxType::Eip2930 => Signed::<TxEip2930>::try_from(inner).map(OpTxEnvelope::from),
-            OpTxType::Eip1559 => Signed::<TxEip1559>::try_from(inner).map(OpTxEnvelope::from),
-            OpTxType::Eip4844 => Signed::<TxEip4844>::try_from(inner).map(OpTxEnvelope::from),
-            OpTxType::Deposit => {
-                let tx_deposit = TxDeposit {
-                    source_hash: tx
-                        .source_hash
-                        .ok_or(OptimismTransactionParsingError::MissingSourceHash)?,
-                    from: tx.inner.from,
-                    to: tx.inner.to.into(),
-                    mint: tx.mint,
-                    value: tx.inner.value,
-                    gas_limit: tx.inner.gas,
-                    is_system_transaction: tx.is_system_tx.unwrap_or_default(),
-                    input: tx.inner.input.clone(),
-                };
-                Ok(OpTxEnvelope::from(tx_deposit))
-            }
+    let tx_type = tx.inner.transaction_type.unwrap_or_default();
+    let op_tx_type = OpTxType::try_from(tx_type)
+        .map_err(|_err| OptimismTransactionParsingError::InvalidType(tx_type))?;
+
+    let inner = tx.inner.clone();
+    let tx_envelope = match op_tx_type {
+        OpTxType::Legacy => Signed::<TxLegacy>::try_from(inner).map(OpTxEnvelope::from),
+        OpTxType::Eip2930 => Signed::<TxEip2930>::try_from(inner).map(OpTxEnvelope::from),
+        OpTxType::Eip1559 => Signed::<TxEip1559>::try_from(inner).map(OpTxEnvelope::from),
+        OpTxType::Eip4844 => Signed::<TxEip4844>::try_from(inner).map(OpTxEnvelope::from),
+        OpTxType::Deposit => {
+            let tx_deposit = TxDeposit {
+                source_hash: tx
+                    .source_hash
+                    .ok_or(OptimismTransactionParsingError::MissingSourceHash)?,
+                from: tx.inner.from,
+                to: tx.inner.to.into(),
+                mint: tx.mint,
+                value: tx.inner.value,
+                gas_limit: tx.inner.gas,
+                is_system_transaction: tx.is_system_tx.unwrap_or_default(),
+                input: tx.inner.input.clone(),
+            };
+            Ok(OpTxEnvelope::from(tx_deposit))
         }
-        .map_err(|err| OptimismTransactionParsingError::ConversionError(err.to_string()))?;
+    }
+    .map_err(|err| OptimismTransactionParsingError::ConversionError(err.to_string()))?;
 
-        let mut envelope_buf = Vec::<u8>::new();
-        tx_envelope.encode_2718(&mut envelope_buf);
-        Bytes::from(envelope_buf)
-    };
+    let mut envelope_buf = Vec::<u8>::new();
+    tx_envelope.encode_2718(&mut envelope_buf);
 
     Ok(OptimismFields {
         source_hash: tx.source_hash,
         mint: tx.mint,
         is_system_transaction: tx.is_system_tx,
-        enveloped_tx: Some(envelope_buf),
+        enveloped_tx: Some(Bytes::from(envelope_buf)),
     })
 }
 
 impl PevmChain for PevmOptimism {
-    type Transaction = Transaction;
+    type Transaction = op_alloy_rpc_types::Transaction;
     type BlockSpecError = OptimismBlockSpecError;
     type TransactionParsingError = OptimismTransactionParsingError;
 
@@ -127,13 +126,18 @@ impl PevmChain for PevmOptimism {
     }
 
     fn build_tx_from_alloy_tx(&self, tx: alloy_rpc_types::Transaction) -> Self::Transaction {
-        Transaction {
+        Self::Transaction {
             inner: tx,
-            ..Transaction::default()
+            mint: None,
+            source_hash: None,
+            is_system_tx: None,
+            deposit_receipt_version: None,
         }
     }
 
     fn get_block_spec(&self, header: &Header) -> Result<SpecId, Self::BlockSpecError> {
+        // TODO: The implementation below is only true for Optimism Mainnet.
+        // When supporting other networks (e.g. Optimism Sepolia), remember to adjust the code here.
         if header.timestamp >= 1720627201 {
             Ok(SpecId::FJORD)
         } else if header.timestamp >= 1710374401 {
@@ -167,6 +171,8 @@ impl PevmChain for PevmOptimism {
                     .or_insert_with(|| Vec::with_capacity(txs.len()))
                     .push(index);
             } else {
+                // TODO: Benchmark to check whether adding these estimated
+                // locations helps or harms the performance.
                 estimated_locations
                     .entry(l1_fee_recipient_location_hash)
                     .or_insert_with(|| Vec::with_capacity(1))
@@ -215,7 +221,7 @@ impl PevmChain for PevmOptimism {
         txs: &BlockTransactions<Self::Transaction>,
         tx_results: &[PevmTxExecutionResult],
     ) -> B256 {
-        // 1. Create an iterator of ReceiptEnvelope
+        // 1. Create an iterator of [ReceiptEnvelope]
         let receipt_envelope_iter =
             Iterator::zip(txs.txns(), tx_results.iter()).map(|(tx, tx_result)| {
                 let receipt = tx_result.receipt.clone();

diff --git a/src/vm.rs b/src/vm.rs
@@ -713,6 +713,8 @@ impl<'a, S: Storage, C: PevmChain> Vm<'a, S, C> {
                 if is_deposit {
                     SmallVec::new()
                 } else {
+                    // TODO: Better error handling
+                    // https://github.com/bluealloy/revm/blob/16e1ecb9a71544d9f205a51a22d81e2658202fde/crates/revm/src/optimism/handler_register.rs#L267
                     let Some(enveloped_tx) = &tx.optimism.enveloped_tx else {
                         panic!("[OPTIMISM] Failed to load enveloped transaction.");
                     };