Fix json_strucutres for gloas variant and add tests

beacon_node/execution_layer/src/engine_api/json_structures.rs

@@ -487,8 +487,7 @@ pub struct JsonExecutionRequests(pub Vec<String>);
 
 impl<E: EthSpec> From<ExecutionRequests<E>> for JsonExecutionRequests {
     fn from(requests: ExecutionRequests<E>) -> Self {
-        // Each element is a `RequestType`-prefixed, SSZ-encoded request list (EIP-7685).
+        // Each element is a `RequestType`-prefixed, SSZ-encoded request list.
-        // The Gloas variant additionally emits builder deposit/exit requests.
         let result = requests
             .get_execution_requests_list()
             .into_iter()
@@ -498,123 +497,124 @@ impl<E: EthSpec> From<ExecutionRequests<E>> for JsonExecutionRequests {
     }
 }
 
-impl<E: EthSpec> TryFrom<JsonExecutionRequests> for ExecutionRequests<E> {
+/// Parse an EIP-7685 `JsonExecutionRequests` list into its component request lists.
-    type Error = RequestsError;
+///
+/// Returns the deposit, withdrawal, consolidation, builder deposit and builder exit lists.
+/// Builder lists are empty pre-gloas or post-gloas when no builder requests are present.
+#[allow(clippy::type_complexity)]
+fn parse_execution_requests<E: EthSpec>(
+    value: JsonExecutionRequests,
+) -> Result<
+    (
+        DepositRequests<E>,
+        WithdrawalRequests<E>,
+        ConsolidationRequests<E>,
+        BuilderDepositRequests<E>,
+        BuilderExitRequests<E>,
+    ),
+    RequestsError,
+> {
+    let mut deposits = DepositRequests::<E>::default();
+    let mut withdrawals = WithdrawalRequests::<E>::default();
+    let mut consolidations = ConsolidationRequests::<E>::default();
+    let mut builder_deposits = BuilderDepositRequests::<E>::default();
+    let mut builder_exits = BuilderExitRequests::<E>::default();
+    let mut prev_prefix: Option<RequestType> = None;
+    for (i, request) in value.0.into_iter().enumerate() {
+        // hex string
+        let decoded_bytes = hex::decode(request.strip_prefix("0x").unwrap_or(&request))
+            .map_err(RequestsError::InvalidHex)?;
 
-    fn try_from(value: JsonExecutionRequests) -> Result<Self, Self::Error> {
+        // The first byte of each element is the `request_type` and the remaining bytes are the `request_data`.
-        let mut deposits = DepositRequests::<E>::default();
+        // Elements with empty `request_data` **MUST** be excluded from the list.
-        let mut withdrawals = WithdrawalRequests::<E>::default();
+        let Some((prefix_byte, request_bytes)) = decoded_bytes.split_first() else {
-        let mut consolidations = ConsolidationRequests::<E>::default();
+            return Err(RequestsError::EmptyRequest(i));
-        let mut builder_deposits = BuilderDepositRequests::<E>::default();
+        };
-        let mut builder_exits = BuilderExitRequests::<E>::default();
+        if request_bytes.is_empty() {
-        // [New in Gloas:EIP8282] The presence of builder requests determines the variant: the
+            return Err(RequestsError::EmptyRequest(i));
-        // EIP-7685 list is fork-agnostic, so we only know it is Gloas-shaped once a builder
+        }
-        // request type appears.
+        // Elements of the list **MUST** be ordered by `request_type` in ascending order
-        let mut has_builder_requests = false;
+        let current_prefix =
-        let mut prev_prefix: Option<RequestType> = None;
+            RequestType::from_u8(*prefix_byte).ok_or(RequestsError::InvalidPrefix(*prefix_byte))?;
-        for (i, request) in value.0.into_iter().enumerate() {
+        if let Some(prev) = prev_prefix
-            // hex string
+            && prev.to_u8() >= current_prefix.to_u8()
-            let decoded_bytes = hex::decode(request.strip_prefix("0x").unwrap_or(&request))
+        {
-                .map_err(RequestsError::InvalidHex)?;
+            return Err(RequestsError::InvalidOrdering);
+        }
+        prev_prefix = Some(current_prefix);
 
-            // The first byte of each element is the `request_type` and the remaining bytes are the `request_data`.
+        match current_prefix {
-            // Elements with empty `request_data` **MUST** be excluded from the list.
+            RequestType::Deposit => {
-            let Some((prefix_byte, request_bytes)) = decoded_bytes.split_first() else {
+                deposits = DepositRequests::<E>::from_ssz_bytes(request_bytes).map_err(|e| {
-                return Err(RequestsError::EmptyRequest(i));
+                    RequestsError::DecodeError(format!(
-            };
+                        "Failed to decode DepositRequest from EL: {:?}",
-            if request_bytes.is_empty() {
+                        e
-                return Err(RequestsError::EmptyRequest(i));
+                    ))
+                })?;
             }
-            // Elements of the list **MUST** be ordered by `request_type` in ascending order
+            RequestType::Withdrawal => {
-            let current_prefix = RequestType::from_u8(*prefix_byte)
+                withdrawals =
-                .ok_or(RequestsError::InvalidPrefix(*prefix_byte))?;
+                    WithdrawalRequests::<E>::from_ssz_bytes(request_bytes).map_err(|e| {
-            if let Some(prev) = prev_prefix
+                        RequestsError::DecodeError(format!(
-                && prev.to_u8() >= current_prefix.to_u8()
+                            "Failed to decode WithdrawalRequest from EL: {:?}",
-            {
+                            e
-                return Err(RequestsError::InvalidOrdering);
+                        ))
+                    })?;
             }
-            prev_prefix = Some(current_prefix);
+            RequestType::Consolidation => {
-
+                consolidations = ConsolidationRequests::<E>::from_ssz_bytes(request_bytes)
-            match current_prefix {
+                    .map_err(|e| {
-                RequestType::Deposit => {
+                        RequestsError::DecodeError(format!(
-                    deposits =
+                            "Failed to decode ConsolidationRequest from EL: {:?}",
-                        DepositRequests::<E>::from_ssz_bytes(request_bytes).map_err(|e| {
+                            e
-                            RequestsError::DecodeError(format!(
+                        ))
-                                "Failed to decode DepositRequest from EL: {:?}",
+                    })?;
-                                e
+            }
-                            ))
+            RequestType::BuilderDeposit => {
-                        })?;
+                builder_deposits = BuilderDepositRequests::<E>::from_ssz_bytes(request_bytes)
-                }
+                    .map_err(|e| {
-                RequestType::Withdrawal => {
-                    withdrawals =
-                        WithdrawalRequests::<E>::from_ssz_bytes(request_bytes).map_err(|e| {
-                            RequestsError::DecodeError(format!(
-                                "Failed to decode WithdrawalRequest from EL: {:?}",
-                                e
-                            ))
-                        })?;
-                }
-                RequestType::Consolidation => {
-                    consolidations = ConsolidationRequests::<E>::from_ssz_bytes(request_bytes)
-                        .map_err(|e| {
-                            RequestsError::DecodeError(format!(
-                                "Failed to decode ConsolidationRequest from EL: {:?}",
-                                e
-                            ))
-                        })?;
-                }
-                RequestType::BuilderDeposit => {
-                    builder_deposits = BuilderDepositRequests::<E>::from_ssz_bytes(request_bytes)
-                        .map_err(|e| {
                         RequestsError::DecodeError(format!(
                             "Failed to decode BuilderDepositRequest from EL: {:?}",
                             e
                         ))
                     })?;
-                    has_builder_requests = true;
+            }
-                }
+            RequestType::BuilderExit => {
-                RequestType::BuilderExit => {
+                builder_exits =
-                    builder_exits = BuilderExitRequests::<E>::from_ssz_bytes(request_bytes)
+                    BuilderExitRequests::<E>::from_ssz_bytes(request_bytes).map_err(|e| {
-                        .map_err(|e| {
+                        RequestsError::DecodeError(format!(
-                            RequestsError::DecodeError(format!(
+                            "Failed to decode BuilderExitRequest from EL: {:?}",
-                                "Failed to decode BuilderExitRequest from EL: {:?}",
+                            e
-                                e
+                        ))
-                            ))
+                    })?;
-                        })?;
-                    has_builder_requests = true;
-                }
             }
         }
-
-        // Without any builder requests the list is indistinguishable from a pre-Gloas one, so we
-        // produce the Electra-shaped variant. Consumers that require the Gloas variant lift it
-        // (carrying empty builder lists) at their boundary.
-        if has_builder_requests {
-            Ok(ExecutionRequests::Gloas(ExecutionRequestsGloas {
-                deposits,
-                withdrawals,
-                consolidations,
-                builder_deposits,
-                builder_exits,
-            }))
-        } else {
-            Ok(ExecutionRequests::Electra(ExecutionRequestsElectra {
-                deposits,
-                withdrawals,
-                consolidations,
-            }))
-        }
     }
+
+    Ok((
+        deposits,
+        withdrawals,
+        consolidations,
+        builder_deposits,
+        builder_exits,
+    ))
 }
 
 impl<E: EthSpec> TryFrom<JsonExecutionRequests> for ExecutionRequestsElectra<E> {
     type Error = RequestsError;
 
     fn try_from(value: JsonExecutionRequests) -> Result<Self, Self::Error> {
-        match ExecutionRequests::<E>::try_from(value)? {
+        let (deposits, withdrawals, consolidations, builder_deposits, builder_exits) =
-            ExecutionRequests::Electra(requests) => Ok(requests),
+            parse_execution_requests::<E>(value)?;
-            ExecutionRequests::Gloas(_) => Err(RequestsError::VariantMismatch),
+        // Builder requests are not valid pre-Gloas.
+        if !builder_deposits.is_empty() || !builder_exits.is_empty() {
+            return Err(RequestsError::VariantMismatch);
         }
+        Ok(ExecutionRequestsElectra {
+            deposits,
+            withdrawals,
+            consolidations,
+        })
     }
 }
 
@@ -622,10 +622,15 @@ impl<E: EthSpec> TryFrom<JsonExecutionRequests> for ExecutionRequestsGloas<E> {
     type Error = RequestsError;
 
     fn try_from(value: JsonExecutionRequests) -> Result<Self, Self::Error> {
-        match ExecutionRequests::<E>::try_from(value)? {
+        let (deposits, withdrawals, consolidations, builder_deposits, builder_exits) =
-            ExecutionRequests::Gloas(requests) => Ok(requests),
+            parse_execution_requests::<E>(value)?;
-            ExecutionRequests::Electra(_) => Err(RequestsError::VariantMismatch),
+        Ok(ExecutionRequestsGloas {
-        }
+            deposits,
+            withdrawals,
+            consolidations,
+            builder_deposits,
+            builder_exits,
+        })
     }
 }
 
@@ -1224,7 +1229,8 @@ mod tests {
     use bls::{PublicKeyBytes, SignatureBytes};
     use ssz::Encode;
     use types::{
-        ConsolidationRequest, DepositRequest, MainnetEthSpec, RequestType, WithdrawalRequest,
+        BuilderDepositRequest, BuilderExitRequest, ConsolidationRequest, DepositRequest,
+        MainnetEthSpec, RequestType, WithdrawalRequest,
     };
 
     use super::*;
@@ -1269,7 +1275,7 @@ mod tests {
 
         // First check a valid request with all requests
         assert!(
-            ExecutionRequests::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
                 create_request_string(RequestType::Deposit.to_u8(), &deposit_request),
                 create_request_string(RequestType::Withdrawal.to_u8(), &withdrawal_request),
                 create_request_string(RequestType::Consolidation.to_u8(), &consolidation_request),
@@ -1279,21 +1285,21 @@ mod tests {
 
         // Single requests
         assert!(
-            ExecutionRequests::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
                 create_request_string(RequestType::Deposit.to_u8(), &deposit_request),
             ]))
             .is_ok()
         );
 
         assert!(
-            ExecutionRequests::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
                 create_request_string(RequestType::Withdrawal.to_u8(), &withdrawal_request),
             ]))
             .is_ok()
         );
 
         assert!(
-            ExecutionRequests::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
                 create_request_string(RequestType::Consolidation.to_u8(), &consolidation_request),
             ]))
             .is_ok()
@@ -1301,7 +1307,7 @@ mod tests {
 
         // Out of order
         assert!(matches!(
-            ExecutionRequests::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
                 create_request_string(RequestType::Withdrawal.to_u8(), &withdrawal_request),
                 create_request_string(RequestType::Deposit.to_u8(), &deposit_request),
             ]))
@@ -1310,7 +1316,7 @@ mod tests {
         ));
 
         assert!(matches!(
-            ExecutionRequests::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
                 create_request_string(RequestType::Consolidation.to_u8(), &consolidation_request),
                 create_request_string(RequestType::Withdrawal.to_u8(), &withdrawal_request),
             ]))
@@ -1319,7 +1325,7 @@ mod tests {
         ));
 
         assert!(matches!(
-            ExecutionRequests::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
                 create_request_string(RequestType::Consolidation.to_u8(), &consolidation_request),
                 create_request_string(RequestType::Deposit.to_u8(), &deposit_request),
             ]))
@@ -1329,7 +1335,7 @@ mod tests {
 
         // Multiple requests of same type
         assert!(matches!(
-            ExecutionRequests::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
                 create_request_string(RequestType::Deposit.to_u8(), &deposit_request),
                 create_request_string(RequestType::Deposit.to_u8(), &deposit_request),
             ]))
@@ -1339,7 +1345,7 @@ mod tests {
 
         // Invalid prefix
         assert!(matches!(
-            ExecutionRequests::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
                 create_request_string(42, &deposit_request),
             ]))
             .unwrap_err(),
@@ -1348,7 +1354,7 @@ mod tests {
 
         // Prefix followed by no data
         assert!(matches!(
-            ExecutionRequests::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
                 create_request_string(RequestType::Deposit.to_u8(), &deposit_request),
                 create_request_string(
                     RequestType::Consolidation.to_u8(),
@@ -1360,12 +1366,141 @@ mod tests {
         ));
         // Empty request
         assert!(matches!(
-            ExecutionRequests::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
                 create_request_string(RequestType::Deposit.to_u8(), &deposit_request),
                 "0x".to_string()
             ]))
             .unwrap_err(),
             RequestsError::EmptyRequest(1)
         ));
+
+        // Builder requests are not valid  pre-gloas.
+        let builder_deposit_request = BuilderDepositRequest {
+            pubkey: PublicKeyBytes::empty(),
+            withdrawal_credentials: Hash256::random(),
+            amount: 32,
+            signature: SignatureBytes::empty(),
+        };
+        assert!(matches!(
+            ExecutionRequestsElectra::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+                create_request_string(
+                    RequestType::BuilderDeposit.to_u8(),
+                    &builder_deposit_request
+                ),
+            ]))
+            .unwrap_err(),
+            RequestsError::VariantMismatch
+        ));
+    }
+
+    #[test]
+    fn test_gloas_execution_requests() {
+        let deposit_request = DepositRequest {
+            pubkey: PublicKeyBytes::empty(),
+            withdrawal_credentials: Hash256::random(),
+            amount: 32,
+            signature: SignatureBytes::empty(),
+            index: 0,
+        };
+
+        let withdrawal_request = WithdrawalRequest {
+            amount: 32,
+            source_address: Address::random(),
+            validator_pubkey: PublicKeyBytes::empty(),
+        };
+
+        let consolidation_request = ConsolidationRequest {
+            source_address: Address::random(),
+            source_pubkey: PublicKeyBytes::empty(),
+            target_pubkey: PublicKeyBytes::empty(),
+        };
+
+        let builder_deposit_request = BuilderDepositRequest {
+            pubkey: PublicKeyBytes::empty(),
+            withdrawal_credentials: Hash256::random(),
+            amount: 32,
+            signature: SignatureBytes::empty(),
+        };
+
+        let builder_exit_request = BuilderExitRequest {
+            source_address: Address::random(),
+            pubkey: PublicKeyBytes::empty(),
+        };
+
+        // Valid request with all five request types, in ascending prefix order.
+        assert!(
+            ExecutionRequestsGloas::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+                create_request_string(RequestType::Deposit.to_u8(), &deposit_request),
+                create_request_string(RequestType::Withdrawal.to_u8(), &withdrawal_request),
+                create_request_string(RequestType::Consolidation.to_u8(), &consolidation_request),
+                create_request_string(
+                    RequestType::BuilderDeposit.to_u8(),
+                    &builder_deposit_request
+                ),
+                create_request_string(RequestType::BuilderExit.to_u8(), &builder_exit_request),
+            ]))
+            .is_ok()
+        );
+
+        // A builder-less list is a valid Gloas value (builder lists are simply empty).
+        assert!(
+            ExecutionRequestsGloas::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+                create_request_string(RequestType::Deposit.to_u8(), &deposit_request),
+            ]))
+            .is_ok()
+        );
+
+        // Only builder requests.
+        assert!(
+            ExecutionRequestsGloas::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+                create_request_string(
+                    RequestType::BuilderDeposit.to_u8(),
+                    &builder_deposit_request
+                ),
+                create_request_string(RequestType::BuilderExit.to_u8(), &builder_exit_request),
+            ]))
+            .is_ok()
+        );
+
+        // Out of order: builder exit must come after a builder deposit.
+        assert!(matches!(
+            ExecutionRequestsGloas::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+                create_request_string(RequestType::BuilderExit.to_u8(), &builder_exit_request),
+                create_request_string(
+                    RequestType::BuilderDeposit.to_u8(),
+                    &builder_deposit_request
+                ),
+            ]))
+            .unwrap_err(),
+            RequestsError::InvalidOrdering
+        ));
+
+        // Duplicate builder request type.
+        assert!(matches!(
+            ExecutionRequestsGloas::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+                create_request_string(
+                    RequestType::BuilderDeposit.to_u8(),
+                    &builder_deposit_request
+                ),
+                create_request_string(
+                    RequestType::BuilderDeposit.to_u8(),
+                    &builder_deposit_request
+                ),
+            ]))
+            .unwrap_err(),
+            RequestsError::InvalidOrdering
+        ));
+
+        // Empty builder request data.
+        assert!(matches!(
+            ExecutionRequestsGloas::<MainnetEthSpec>::try_from(JsonExecutionRequests(vec![
+                create_request_string(
+                    RequestType::BuilderDeposit.to_u8(),
+                    &Vec::<BuilderDepositRequest>::new()
+                ),
+            ]))
+            .unwrap_err(),
+            RequestsError::EmptyRequest(0)
+        ));
     }
 }