Move bundle validation to primitives folder

crates/op-rbuilder/src/primitives/bundle.rs

@@ -1,5 +1,6 @@
 use alloy_primitives::{Bytes, B256};
 use alloy_rpc_types_eth::erc4337::TransactionConditional;
+use reth_rpc_eth_types::EthApiError;
 use serde::{Deserialize, Serialize};
 
 pub const MAX_BLOCK_RANGE_BLOCKS: u64 = 10;
@@ -29,15 +30,89 @@ pub struct Bundle {
     pub block_number_min: Option<u64>,
 }
 
+impl From<BundleConditionalError> for EthApiError {
+    fn from(err: BundleConditionalError) -> Self {
+        EthApiError::InvalidParams(err.to_string())
+    }
+}
+
+#[derive(Debug, thiserror::Error)]
+pub enum BundleConditionalError {
+    #[error("block_number_min ({min}) is greater than block_number_max ({max})")]
+    MinGreaterThanMax { min: u64, max: u64 },
+    #[error("block_number_max ({max}) is a past block (current: {current})")]
+    MaxBlockInPast { max: u64, current: u64 },
+    #[error(
+        "block_number_max ({max}) is too high (current: {current}, max allowed: {max_allowed})"
+    )]
+    MaxBlockTooHigh {
+        max: u64,
+        current: u64,
+        max_allowed: u64,
+    },
+    #[error(
+        "block_number_min ({min}) is too high with default max range (max allowed: {max_allowed})"
+    )]
+    MinTooHighForDefaultRange { min: u64, max_allowed: u64 },
+}
+
 impl Bundle {
-    pub fn conditional(&self) -> TransactionConditional {
-        TransactionConditional {
-            block_number_min: self.block_number_min,
-            block_number_max: self.block_number_max,
+    pub fn conditional(
+        &self,
+        last_block_number: u64,
+    ) -> Result<TransactionConditional, BundleConditionalError> {
+        let mut block_number_max = self.block_number_max;
+        let block_number_min = self.block_number_min;
+
+        // Validate block number ranges
+        if let Some(max) = block_number_max {
+            // Check if min > max
+            if let Some(min) = block_number_min {
+                if min > max {
+                    return Err(BundleConditionalError::MinGreaterThanMax { min, max });
+                }
+            }
+
+            // The max block cannot be a past block
+            if max <= last_block_number {
+                return Err(BundleConditionalError::MaxBlockInPast {
+                    max,
+                    current: last_block_number,
+                });
+            }
+
+            // Validate that it is not greater than the max_block_range
+            let max_allowed = last_block_number + MAX_BLOCK_RANGE_BLOCKS;
+            if max > max_allowed {
+                return Err(BundleConditionalError::MaxBlockTooHigh {
+                    max,
+                    current: last_block_number,
+                    max_allowed,
+                });
+            }
+        } else {
+            // If no upper bound is set, use the maximum block range
+            let default_max = last_block_number + MAX_BLOCK_RANGE_BLOCKS;
+            block_number_max = Some(default_max);
+
+            // Ensure that the new max is not smaller than the min
+            if let Some(min) = block_number_min {
+                if min > default_max {
+                    return Err(BundleConditionalError::MinTooHighForDefaultRange {
+                        min,
+                        max_allowed: default_max,
+                    });
+                }
+            }
+        }
+
+        Ok(TransactionConditional {
+            block_number_min,
+            block_number_max,
             known_accounts: Default::default(),
             timestamp_max: None,
             timestamp_min: None,
-        }
+        })
     }
 }
 
@@ -46,3 +121,176 @@ pub struct BundleResult {
     #[serde(rename = "bundleHash")]
     pub bundle_hash: B256,
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_bundle_conditional_no_bounds() {
+        let bundle = Bundle {
+            transactions: vec![],
+            reverting_hashes: None,
+            block_number_max: None,
+            block_number_min: None,
+        };
+
+        let last_block = 1000;
+        let result = bundle.conditional(last_block).unwrap();
+
+        assert_eq!(result.block_number_min, None);
+        assert_eq!(
+            result.block_number_max,
+            Some(last_block + MAX_BLOCK_RANGE_BLOCKS)
+        );
+    }
+
+    #[test]
+    fn test_bundle_conditional_with_valid_bounds() {
+        let bundle = Bundle {
+            transactions: vec![],
+            reverting_hashes: None,
+            block_number_max: Some(1005),
+            block_number_min: Some(1002),
+        };
+
+        let last_block = 1000;
+        let result = bundle.conditional(last_block).unwrap();
+
+        assert_eq!(result.block_number_min, Some(1002));
+        assert_eq!(result.block_number_max, Some(1005));
+    }
+
+    #[test]
+    fn test_bundle_conditional_min_greater_than_max() {
+        let bundle = Bundle {
+            transactions: vec![],
+            reverting_hashes: None,
+            block_number_max: Some(1005),
+            block_number_min: Some(1010),
+        };
+
+        let last_block = 1000;
+        let result = bundle.conditional(last_block);
+
+        assert!(matches!(
+            result,
+            Err(BundleConditionalError::MinGreaterThanMax {
+                min: 1010,
+                max: 1005
+            })
+        ));
+    }
+
+    #[test]
+    fn test_bundle_conditional_max_in_past() {
+        let bundle = Bundle {
+            transactions: vec![],
+            reverting_hashes: None,
+            block_number_max: Some(999),
+            block_number_min: None,
+        };
+
+        let last_block = 1000;
+        let result = bundle.conditional(last_block);
+
+        assert!(matches!(
+            result,
+            Err(BundleConditionalError::MaxBlockInPast {
+                max: 999,
+                current: 1000
+            })
+        ));
+    }
+
+    #[test]
+    fn test_bundle_conditional_max_too_high() {
+        let bundle = Bundle {
+            transactions: vec![],
+            reverting_hashes: None,
+            block_number_max: Some(1020),
+            block_number_min: None,
+        };
+
+        let last_block = 1000;
+        let result = bundle.conditional(last_block);
+
+        assert!(matches!(
+            result,
+            Err(BundleConditionalError::MaxBlockTooHigh {
+                max: 1020,
+                current: 1000,
+                max_allowed: 1010
+            })
+        ));
+    }
+
+    #[test]
+    fn test_bundle_conditional_min_too_high_for_default_range() {
+        let bundle = Bundle {
+            transactions: vec![],
+            reverting_hashes: None,
+            block_number_max: None,
+            block_number_min: Some(1015),
+        };
+
+        let last_block = 1000;
+        let result = bundle.conditional(last_block);
+
+        assert!(matches!(
+            result,
+            Err(BundleConditionalError::MinTooHighForDefaultRange {
+                min: 1015,
+                max_allowed: 1010
+            })
+        ));
+    }
+
+    #[test]
+    fn test_bundle_conditional_with_only_min() {
+        let bundle = Bundle {
+            transactions: vec![],
+            reverting_hashes: None,
+            block_number_max: None,
+            block_number_min: Some(1005),
+        };
+
+        let last_block = 1000;
+        let result = bundle.conditional(last_block).unwrap();
+
+        assert_eq!(result.block_number_min, Some(1005));
+        assert_eq!(result.block_number_max, Some(1010)); // last_block + MAX_BLOCK_RANGE_BLOCKS
+    }
+
+    #[test]
+    fn test_bundle_conditional_with_only_max() {
+        let bundle = Bundle {
+            transactions: vec![],
+            reverting_hashes: None,
+            block_number_max: Some(1008),
+            block_number_min: None,
+        };
+
+        let last_block = 1000;
+        let result = bundle.conditional(last_block).unwrap();
+
+        assert_eq!(result.block_number_min, None);
+        assert_eq!(result.block_number_max, Some(1008));
+    }
+
+    #[test]
+    fn test_bundle_conditional_min_lower_than_last_block() {
+        let bundle = Bundle {
+            transactions: vec![],
+            reverting_hashes: None,
+            block_number_max: None,
+            block_number_min: Some(999),
+        };
+
+        let last_block = 1000;
+        let result = bundle.conditional(last_block).unwrap();
+
+        assert_eq!(result.block_number_min, Some(999));
+        assert_eq!(result.block_number_max, Some(1010));
+    }
+}

crates/op-rbuilder/src/revert_protection.rs

@@ -1,5 +1,5 @@
 use crate::{
-    primitives::bundle::{Bundle, BundleResult, MAX_BLOCK_RANGE_BLOCKS},
+    primitives::bundle::{Bundle, BundleResult},
     tx::{FBPooledTransaction, MaybeRevertingTransaction},
 };
 use alloy_json_rpc::RpcObject;
@@ -82,7 +82,7 @@ where
     Pool: TransactionPool<Transaction = FBPooledTransaction> + Clone + 'static,
     Provider: StateProviderFactory + Send + Sync + Clone + 'static,
 {
-    async fn send_bundle(&self, mut bundle: Bundle) -> RpcResult<BundleResult> {
+    async fn send_bundle(&self, bundle: Bundle) -> RpcResult<BundleResult> {
         let last_block_number = self
             .provider
             .best_block_number()
@@ -105,48 +105,16 @@ where
             }
         };
 
-        if let Some(block_number_max) = bundle.block_number_max {
-            if let Some(block_number_min) = bundle.block_number_min {
-                if block_number_min > block_number_max {
-                    return Err(EthApiError::InvalidParams(
-                        "block_number_min is greater than block_number_max".into(),
-                    )
-                    .into());
-                }
-            }
-
-            // The max block cannot be a past block
-            if block_number_max <= last_block_number {
-                return Err(
-                    EthApiError::InvalidParams("block_number_max is a past block".into()).into(),
-                );
-            }
-
-            // Validate that it is not greater than the max_block_range
-            if block_number_max > last_block_number + MAX_BLOCK_RANGE_BLOCKS {
-                return Err(
-                    EthApiError::InvalidParams("block_number_max is too high".into()).into(),
-                );
-            }
-        } else {
-            // If no upper bound is set, use the maximum block range
-            bundle.block_number_max = Some(last_block_number + MAX_BLOCK_RANGE_BLOCKS);
-            // Ensure that the new max is not smaller than the min
-            if let Some(block_number_min) = bundle.block_number_min {
-                if block_number_min > bundle.block_number_max.unwrap() {
-                    return Err(
-                        EthApiError::InvalidParams("block_number_min is too high".into()).into(),
-                    );
-                }
-            }
-        }
+        let conditional = bundle
+            .conditional(last_block_number)
+            .map_err(EthApiError::from)?;
 
         let recovered = recover_raw_transaction(&bundle_transaction)?;
         let mut pool_transaction: FBPooledTransaction =
             OpPooledTransaction::from_pooled(recovered).into();
 
         pool_transaction.set_reverted_hashes(bundle.reverting_hashes.clone().unwrap_or_default());
-        pool_transaction.set_conditional(bundle.conditional());
+        pool_transaction.set_conditional(conditional);
 
         let hash = self
             .pool