From 4aa8a2ab128d63cf124bf5d32c1bbfe0e7086add Mon Sep 17 00:00:00 2001
From: Paul Hauner <paul@paulhauner.com>
Date: Fri, 17 Feb 2023 11:58:33 +1100
Subject: [PATCH] Suggestions for Capella `execution_layer` (#3983)

* Restrict Engine::request to FnOnce

* Use `Into::into`

* Impl IntoIterator for VariableList

* Use Instant rather than SystemTime
---
 .../execution_layer/src/engine_api/http.rs       | 16 ++++------------
 .../src/engine_api/json_structures.rs            |  4 ----
 beacon_node/execution_layer/src/engines.rs       |  2 +-
 beacon_node/execution_layer/src/lib.rs           | 11 +++--------
 consensus/ssz_types/src/variable_list.rs         |  9 +++++++++
 5 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs
index 8f88de79a..4416d6a37 100644
--- a/beacon_node/execution_layer/src/engine_api/http.rs
+++ b/beacon_node/execution_layer/src/engine_api/http.rs
@@ -10,7 +10,7 @@ use serde_json::json;
 use std::collections::HashSet;
 use tokio::sync::Mutex;
 
-use std::time::{Duration, SystemTime};
+use std::time::{Duration, Instant};
 use types::EthSpec;
 
 pub use deposit_log::{DepositLog, Log};
@@ -559,14 +559,14 @@ pub mod deposit_methods {
 #[derive(Clone, Debug)]
 pub struct CapabilitiesCacheEntry {
     engine_capabilities: EngineCapabilities,
-    fetch_time: SystemTime,
+    fetch_time: Instant,
 }
 
 impl CapabilitiesCacheEntry {
     pub fn new(engine_capabilities: EngineCapabilities) -> Self {
         Self {
             engine_capabilities,
-            fetch_time: SystemTime::now(),
+            fetch_time: Instant::now(),
         }
     }
 
@@ -575,15 +575,7 @@ impl CapabilitiesCacheEntry {
     }
 
     pub fn age(&self) -> Duration {
-        // duration_since() may fail because measurements taken earlier
-        // are not guaranteed to always be before later measurements
-        // due to anomalies such as the system clock being adjusted
-        // either forwards or backwards
-        //
-        // In such cases, we'll just say the age is zero
-        SystemTime::now()
-            .duration_since(self.fetch_time)
-            .unwrap_or(Duration::ZERO)
+        Instant::now().duration_since(self.fetch_time)
     }
 
     /// returns `true` if the entry's age is >= age_limit
diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs
index ace15ebd8..a6ebc1952 100644
--- a/beacon_node/execution_layer/src/engine_api/json_structures.rs
+++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs
@@ -145,7 +145,6 @@ impl<T: EthSpec> From<ExecutionPayloadCapella<T>> for JsonExecutionPayloadV2<T>
             withdrawals: payload
                 .withdrawals
                 .into_iter()
-                .cloned()
                 .map(Into::into)
                 .collect::<Vec<_>>()
                 .into(),
@@ -173,7 +172,6 @@ impl<T: EthSpec> From<ExecutionPayloadEip4844<T>> for JsonExecutionPayloadV3<T>
             withdrawals: payload
                 .withdrawals
                 .into_iter()
-                .cloned()
                 .map(Into::into)
                 .collect::<Vec<_>>()
                 .into(),
@@ -231,7 +229,6 @@ impl<T: EthSpec> From<JsonExecutionPayloadV2<T>> for ExecutionPayloadCapella<T>
             withdrawals: payload
                 .withdrawals
                 .into_iter()
-                .cloned()
                 .map(Into::into)
                 .collect::<Vec<_>>()
                 .into(),
@@ -259,7 +256,6 @@ impl<T: EthSpec> From<JsonExecutionPayloadV3<T>> for ExecutionPayloadEip4844<T>
             withdrawals: payload
                 .withdrawals
                 .into_iter()
-                .cloned()
                 .map(Into::into)
                 .collect::<Vec<_>>()
                 .into(),
diff --git a/beacon_node/execution_layer/src/engines.rs b/beacon_node/execution_layer/src/engines.rs
index 1ee355e47..ce413cb11 100644
--- a/beacon_node/execution_layer/src/engines.rs
+++ b/beacon_node/execution_layer/src/engines.rs
@@ -341,7 +341,7 @@ impl Engine {
     /// deadlock.
     pub async fn request<'a, F, G, H>(self: &'a Arc<Self>, func: F) -> Result<H, EngineError>
     where
-        F: Fn(&'a Engine) -> G,
+        F: FnOnce(&'a Engine) -> G,
         G: Future<Output = Result<H, EngineApiError>>,
     {
         match func(self).await {
diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs
index 6798d49bc..af5e49155 100644
--- a/beacon_node/execution_layer/src/lib.rs
+++ b/beacon_node/execution_layer/src/lib.rs
@@ -1348,16 +1348,11 @@ impl<T: EthSpec> ExecutionLayer<T> {
             .set_latest_forkchoice_state(forkchoice_state)
             .await;
 
-        let payload_attributes_ref = &payload_attributes;
         let result = self
             .engine()
             .request(|engine| async move {
                 engine
-                    .notify_forkchoice_updated(
-                        forkchoice_state,
-                        payload_attributes_ref.clone(),
-                        self.log(),
-                    )
+                    .notify_forkchoice_updated(forkchoice_state, payload_attributes, self.log())
                     .await
             })
             .await;
@@ -1723,7 +1718,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
                     capella_block
                         .withdrawals
                         .into_iter()
-                        .map(|w| w.into())
+                        .map(Into::into)
                         .collect(),
                 )
                 .map_err(ApiError::DeserializeWithdrawals)?;
@@ -1750,7 +1745,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
                     eip4844_block
                         .withdrawals
                         .into_iter()
-                        .map(|w| w.into())
+                        .map(Into::into)
                         .collect(),
                 )
                 .map_err(ApiError::DeserializeWithdrawals)?;
diff --git a/consensus/ssz_types/src/variable_list.rs b/consensus/ssz_types/src/variable_list.rs
index ef1f113bb..3361f7509 100644
--- a/consensus/ssz_types/src/variable_list.rs
+++ b/consensus/ssz_types/src/variable_list.rs
@@ -176,6 +176,15 @@ impl<'a, T, N: Unsigned> IntoIterator for &'a VariableList<T, N> {
     }
 }
 
+impl<T, N: Unsigned> IntoIterator for VariableList<T, N> {
+    type Item = T;
+    type IntoIter = std::vec::IntoIter<T>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.vec.into_iter()
+    }
+}
+
 impl<T, N: Unsigned> tree_hash::TreeHash for VariableList<T, N>
 where
     T: tree_hash::TreeHash,