From 9050dc893cc2217fe94b2957bea6eef10d394e16 Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Tue, 23 Jul 2024 16:46:47 +0200 Subject: [PATCH 1/7] TxReq into Eip4844 without sidecar --- .../rpc-types-eth/src/transaction/request.rs | 52 ++++++++++++++++++- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/crates/rpc-types-eth/src/transaction/request.rs b/crates/rpc-types-eth/src/transaction/request.rs index 36c85addd3c..ef64c73c0a9 100644 --- a/crates/rpc-types-eth/src/transaction/request.rs +++ b/crates/rpc-types-eth/src/transaction/request.rs @@ -247,7 +247,53 @@ impl TransactionRequest { /// /// If required fields are missing. Use `complete_4844` to check if the /// request can be built. - fn build_4844(mut self) -> TxEip4844WithSidecar { + fn build_4844(self) -> TxEip4844Variant { + if self.sidecar.is_none() { + self.build_4844_without_sidecar().into() + } else { + self.build_4844_with_sidecar().into() + } + } + + /// Build an EIP-4844 transaction with sidecar. + /// + /// # Panics + /// + /// If required fields are missing. Use `complete_4844` to check if the + /// request can be built. + fn build_4844_without_sidecar(self) -> TxEip4844 { + let checked_to = self.to.expect("checked in complete_4844."); + let to_address = match checked_to { + TxKind::Create => panic!("the field `to` can only be of type TxKind::Call(Account). Please change it accordingly."), + TxKind::Call(to) => to, + }; + + TxEip4844 { + chain_id: self.chain_id.unwrap_or(1), + nonce: self.nonce.expect("checked in complete_4844"), + gas_limit: self.gas.expect("checked in complete_4844"), + max_fee_per_gas: self.max_fee_per_gas.expect("checked in complete_4844"), + max_priority_fee_per_gas: self + .max_priority_fee_per_gas + .expect("checked in complete_4844"), + to: to_address, + value: self.value.unwrap_or_default(), + access_list: self.access_list.unwrap_or_default(), + blob_versioned_hashes: self + .blob_versioned_hashes + .expect("populated at top of block"), + max_fee_per_blob_gas: self.max_fee_per_blob_gas.expect("checked in complete_4844"), + input: self.input.into_input().unwrap_or_default(), + } + } + + /// Build an EIP-4844 transaction with sidecar. + /// + /// # Panics + /// + /// If required fields are missing. Use `complete_4844` to check if the + /// request can be built. + fn build_4844_with_sidecar(mut self) -> TxEip4844WithSidecar { self.populate_blob_hashes(); let checked_to = self.to.expect("checked in complete_4844."); @@ -390,8 +436,10 @@ impl TransactionRequest { missing.push("to"); } - if self.sidecar.is_none() { + // Either `sidecar` or `blob_versioned_hashes` must be set. + if self.sidecar.is_none() && self.blob_versioned_hashes.is_none() { missing.push("sidecar"); + missing.push("blob_versioned_hashes"); } if self.max_fee_per_blob_gas.is_none() { From 1d4878e27be8ee311258c1d174cfb20f2cf9d2fd Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Tue, 23 Jul 2024 16:47:15 +0200 Subject: [PATCH 2/7] Formatting --- .../rpc-types-eth/src/transaction/request.rs | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/crates/rpc-types-eth/src/transaction/request.rs b/crates/rpc-types-eth/src/transaction/request.rs index ef64c73c0a9..4895e1c04f8 100644 --- a/crates/rpc-types-eth/src/transaction/request.rs +++ b/crates/rpc-types-eth/src/transaction/request.rs @@ -269,22 +269,20 @@ impl TransactionRequest { }; TxEip4844 { - chain_id: self.chain_id.unwrap_or(1), - nonce: self.nonce.expect("checked in complete_4844"), - gas_limit: self.gas.expect("checked in complete_4844"), - max_fee_per_gas: self.max_fee_per_gas.expect("checked in complete_4844"), - max_priority_fee_per_gas: self - .max_priority_fee_per_gas - .expect("checked in complete_4844"), - to: to_address, - value: self.value.unwrap_or_default(), - access_list: self.access_list.unwrap_or_default(), - blob_versioned_hashes: self - .blob_versioned_hashes - .expect("populated at top of block"), - max_fee_per_blob_gas: self.max_fee_per_blob_gas.expect("checked in complete_4844"), - input: self.input.into_input().unwrap_or_default(), - } + chain_id: self.chain_id.unwrap_or(1), + nonce: self.nonce.expect("checked in complete_4844"), + gas_limit: self.gas.expect("checked in complete_4844"), + max_fee_per_gas: self.max_fee_per_gas.expect("checked in complete_4844"), + max_priority_fee_per_gas: self + .max_priority_fee_per_gas + .expect("checked in complete_4844"), + to: to_address, + value: self.value.unwrap_or_default(), + access_list: self.access_list.unwrap_or_default(), + blob_versioned_hashes: self.blob_versioned_hashes.expect("populated at top of block"), + max_fee_per_blob_gas: self.max_fee_per_blob_gas.expect("checked in complete_4844"), + input: self.input.into_input().unwrap_or_default(), + } } /// Build an EIP-4844 transaction with sidecar. From 9d7b570466c2be3eefd275584f5a697fabc21eef Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Tue, 23 Jul 2024 16:51:56 +0200 Subject: [PATCH 3/7] Minor comments --- crates/rpc-types-eth/src/transaction/request.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/rpc-types-eth/src/transaction/request.rs b/crates/rpc-types-eth/src/transaction/request.rs index 4895e1c04f8..fe2866aaf83 100644 --- a/crates/rpc-types-eth/src/transaction/request.rs +++ b/crates/rpc-types-eth/src/transaction/request.rs @@ -255,7 +255,7 @@ impl TransactionRequest { } } - /// Build an EIP-4844 transaction with sidecar. + /// Build an EIP-4844 transaction without sidecar. /// /// # Panics /// @@ -279,7 +279,7 @@ impl TransactionRequest { to: to_address, value: self.value.unwrap_or_default(), access_list: self.access_list.unwrap_or_default(), - blob_versioned_hashes: self.blob_versioned_hashes.expect("populated at top of block"), + blob_versioned_hashes: self.blob_versioned_hashes.expect("checked in complete_4844"), max_fee_per_blob_gas: self.max_fee_per_blob_gas.expect("checked in complete_4844"), input: self.input.into_input().unwrap_or_default(), } From b8ab4ee52a72f9190876807b9b237a2b1b18960a Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Thu, 25 Jul 2024 14:57:00 +0200 Subject: [PATCH 4/7] Address review comments --- crates/rpc-types-eth/src/transaction/request.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/rpc-types-eth/src/transaction/request.rs b/crates/rpc-types-eth/src/transaction/request.rs index fe2866aaf83..20be9866101 100644 --- a/crates/rpc-types-eth/src/transaction/request.rs +++ b/crates/rpc-types-eth/src/transaction/request.rs @@ -241,17 +241,17 @@ impl TransactionRequest { } } - /// Build an EIP-4844 transaction. + /// Build an EIP-4844 transaction variant - either with or without sidecar. /// /// # Panics /// /// If required fields are missing. Use `complete_4844` to check if the /// request can be built. - fn build_4844(self) -> TxEip4844Variant { + fn build_4844_variant(self) -> TxEip4844Variant { if self.sidecar.is_none() { self.build_4844_without_sidecar().into() } else { - self.build_4844_with_sidecar().into() + self.build_4844().into() } } @@ -291,7 +291,7 @@ impl TransactionRequest { /// /// If required fields are missing. Use `complete_4844` to check if the /// request can be built. - fn build_4844_with_sidecar(mut self) -> TxEip4844WithSidecar { + fn build_4844(mut self) -> TxEip4844WithSidecar { self.populate_blob_hashes(); let checked_to = self.to.expect("checked in complete_4844."); @@ -518,7 +518,7 @@ impl TransactionRequest { TxType::Legacy => self.build_legacy().into(), TxType::Eip2930 => self.build_2930().into(), TxType::Eip1559 => self.build_1559().into(), - TxType::Eip4844 => self.build_4844().into(), + TxType::Eip4844 => self.build_4844_variant().into(), }) } } From 901e3fbe4d2bbec9cc314e384c1f6e6f26545a9c Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Fri, 26 Jul 2024 14:53:12 +0200 Subject: [PATCH 5/7] Address review comments --- crates/rpc-types-eth/Cargo.toml | 1 + .../rpc-types-eth/src/transaction/request.rs | 345 +++++++++++++----- 2 files changed, 259 insertions(+), 87 deletions(-) diff --git a/crates/rpc-types-eth/Cargo.toml b/crates/rpc-types-eth/Cargo.toml index e1c998b73e0..d68b20477a3 100644 --- a/crates/rpc-types-eth/Cargo.toml +++ b/crates/rpc-types-eth/Cargo.toml @@ -51,6 +51,7 @@ alloy-eips = { workspace = true, features = ["arbitrary", "k256"] } arbitrary = { workspace = true, features = ["derive"] } rand.workspace = true similar-asserts.workspace = true +assert_matches.workspace = true [features] arbitrary = [ diff --git a/crates/rpc-types-eth/src/transaction/request.rs b/crates/rpc-types-eth/src/transaction/request.rs index 20be9866101..c8523d623a8 100644 --- a/crates/rpc-types-eth/src/transaction/request.rs +++ b/crates/rpc-types-eth/src/transaction/request.rs @@ -178,148 +178,125 @@ impl TransactionRequest { /// Build a legacy transaction. /// - /// # Panics - /// - /// If required fields are missing. Use `complete_legacy` to check if the - /// request can be built. - fn build_legacy(self) -> TxLegacy { - let checked_to = self.to.expect("checked in complete_legacy."); + /// Returns an error if required fields are missing. + /// Use `complete_legacy` to check if the request can be built. + fn build_legacy(self) -> Result { + let checked_to = self.to.ok_or("Missing 'to' field for legacy transaction.")?; - TxLegacy { + Ok(TxLegacy { chain_id: self.chain_id, - nonce: self.nonce.expect("checked in complete_legacy"), - gas_price: self.gas_price.expect("checked in complete_legacy"), - gas_limit: self.gas.expect("checked in complete_legacy"), + nonce: self.nonce.ok_or("Missing 'nonce' field for legacy transaction.")?, + gas_price: self.gas_price.ok_or("Missing 'gas_price' for legacy transaction.")?, + gas_limit: self.gas.ok_or("Missing 'gas_limit' for legacy transaction.")?, to: checked_to, value: self.value.unwrap_or_default(), input: self.input.into_input().unwrap_or_default(), - } + }) } /// Build an EIP-1559 transaction. /// - /// # Panics - /// - /// If required fields are missing. Use `complete_1559` to check if the + /// Returns ane error if required fields are missing. Use `complete_1559` to check if the /// request can be built. - fn build_1559(self) -> TxEip1559 { - let checked_to = self.to.expect("checked in complete_1559."); + fn build_1559(self) -> Result { + let checked_to = self.to.ok_or("Missing 'to' field for Eip1559 transaction.")?; - TxEip1559 { + Ok(TxEip1559 { chain_id: self.chain_id.unwrap_or(1), - nonce: self.nonce.expect("checked in invalid_common_fields"), + nonce: self.nonce.ok_or("Missing 'nonce' field for Eip1559 transaction.")?, max_priority_fee_per_gas: self .max_priority_fee_per_gas - .expect("checked in invalid_1559_fields"), - max_fee_per_gas: self.max_fee_per_gas.expect("checked in invalid_1559_fields"), - gas_limit: self.gas.expect("checked in invalid_common_fields"), + .ok_or("Missing 'max_priority_fee_per_gas' field for Eip1559 transaction.")?, + max_fee_per_gas: self + .max_fee_per_gas + .ok_or("Missing 'max_fee_per_gas' field for Eip1559 transaction.")?, + gas_limit: self.gas.ok_or("Missing 'gas_limit' field for Eip1559 transaction.")?, to: checked_to, value: self.value.unwrap_or_default(), input: self.input.into_input().unwrap_or_default(), access_list: self.access_list.unwrap_or_default(), - } + }) } /// Build an EIP-2930 transaction. /// - /// # Panics - /// - /// If required fields are missing. Use `complete_2930` to check if the + /// Returns an error if required fields are missing. Use `complete_2930` to check if the /// request can be built. - fn build_2930(self) -> TxEip2930 { - let checked_to = self.to.expect("checked in complete_2930."); + fn build_2930(self) -> Result { + let checked_to = self.to.ok_or("Missing 'to' field for Eip2930 transaction.")?; - TxEip2930 { + Ok(TxEip2930 { chain_id: self.chain_id.unwrap_or(1), - nonce: self.nonce.expect("checked in complete_2930"), - gas_price: self.gas_price.expect("checked in complete_2930"), - gas_limit: self.gas.expect("checked in complete_2930"), + nonce: self.nonce.ok_or("Missing 'nonce' field for Eip2930 transaction.")?, + gas_price: self + .gas_price + .ok_or("Missing 'gas_price' field for Eip2930 transaction.")?, + gas_limit: self.gas.ok_or("Missing 'gas_limit' field for Eip2930 transaction.")?, to: checked_to, value: self.value.unwrap_or_default(), input: self.input.into_input().unwrap_or_default(), access_list: self.access_list.unwrap_or_default(), - } + }) } /// Build an EIP-4844 transaction variant - either with or without sidecar. /// - /// # Panics - /// - /// If required fields are missing. Use `complete_4844` to check if the + /// Returns an error if required fields are missing. Use `complete_4844` to check if the /// request can be built. - fn build_4844_variant(self) -> TxEip4844Variant { + fn build_4844_variant(self) -> Result { if self.sidecar.is_none() { - self.build_4844_without_sidecar().into() + self.build_4844_without_sidecar().map(Into::into) } else { - self.build_4844().into() + self.build_4844_with_sidecar().map(Into::into) } } /// Build an EIP-4844 transaction without sidecar. /// - /// # Panics - /// - /// If required fields are missing. Use `complete_4844` to check if the + /// Returns an error if required fields are missing. Use `complete_4844` to check if the /// request can be built. - fn build_4844_without_sidecar(self) -> TxEip4844 { - let checked_to = self.to.expect("checked in complete_4844."); + fn build_4844_without_sidecar(self) -> Result { + let checked_to = self.to.ok_or("Missing 'to' field for Eip4844 transaction.")?; + let to_address = match checked_to { - TxKind::Create => panic!("the field `to` can only be of type TxKind::Call(Account). Please change it accordingly."), + TxKind::Create => return Err("The field `to` can only be of type TxKind::Call(Account). Please change it accordingly."), TxKind::Call(to) => to, }; - TxEip4844 { + Ok(TxEip4844 { chain_id: self.chain_id.unwrap_or(1), - nonce: self.nonce.expect("checked in complete_4844"), - gas_limit: self.gas.expect("checked in complete_4844"), - max_fee_per_gas: self.max_fee_per_gas.expect("checked in complete_4844"), + nonce: self.nonce.ok_or("Missing 'nonce' field for Eip4844 transaction.")?, + gas_limit: self.gas.ok_or("Missing 'gas_limit' field for Eip4844 transaction.")?, + max_fee_per_gas: self + .max_fee_per_gas + .ok_or("Missing 'max_fee_per_gas' field for Eip4844 transaction.")?, max_priority_fee_per_gas: self .max_priority_fee_per_gas - .expect("checked in complete_4844"), + .ok_or("Missing 'max_priority_fee_per_gas' field for Eip4844 transaction.")?, to: to_address, value: self.value.unwrap_or_default(), access_list: self.access_list.unwrap_or_default(), - blob_versioned_hashes: self.blob_versioned_hashes.expect("checked in complete_4844"), - max_fee_per_blob_gas: self.max_fee_per_blob_gas.expect("checked in complete_4844"), + blob_versioned_hashes: self + .blob_versioned_hashes + .ok_or("Missing 'blob_versioned_hashes' field for Eip4844 transaction.")?, + max_fee_per_blob_gas: self + .max_fee_per_blob_gas + .ok_or("Missing 'max_fee_per_blob_gas' field for Eip4844 transaction.")?, input: self.input.into_input().unwrap_or_default(), - } + }) } /// Build an EIP-4844 transaction with sidecar. /// - /// # Panics - /// - /// If required fields are missing. Use `complete_4844` to check if the + /// Returns an error if required fields are missing. Use `complete_4844` to check if the /// request can be built. - fn build_4844(mut self) -> TxEip4844WithSidecar { + fn build_4844_with_sidecar(mut self) -> Result { self.populate_blob_hashes(); - let checked_to = self.to.expect("checked in complete_4844."); - let to_address = match checked_to { - TxKind::Create => panic!("the field `to` can only be of type TxKind::Call(Account). Please change it accordingly."), - TxKind::Call(to) => to, - }; + let sidecar = + self.sidecar.clone().ok_or("Missing 'sidecar' field for Eip4844 transaction.")?; - TxEip4844WithSidecar { - sidecar: self.sidecar.expect("checked in complete_4844"), - tx: TxEip4844 { - chain_id: self.chain_id.unwrap_or(1), - nonce: self.nonce.expect("checked in complete_4844"), - gas_limit: self.gas.expect("checked in complete_4844"), - max_fee_per_gas: self.max_fee_per_gas.expect("checked in complete_4844"), - max_priority_fee_per_gas: self - .max_priority_fee_per_gas - .expect("checked in complete_4844"), - to: to_address, - value: self.value.unwrap_or_default(), - access_list: self.access_list.unwrap_or_default(), - blob_versioned_hashes: self - .blob_versioned_hashes - .expect("populated at top of block"), - max_fee_per_blob_gas: self.max_fee_per_blob_gas.expect("checked in complete_4844"), - input: self.input.into_input().unwrap_or_default(), - }, - } + Ok(TxEip4844WithSidecar { sidecar, tx: self.build_4844_without_sidecar()? }) } fn check_reqd_fields(&self) -> Vec<&'static str> { @@ -426,6 +403,8 @@ impl TransactionRequest { /// Check if all necessary keys are present to build a 4844 transaction, /// returning a list of keys that are missing. + /// + /// **NOTE:** `sidecar` must be present, even if `blob_versioned_hashes` is set. pub fn complete_4844(&self) -> Result<(), Vec<&'static str>> { let mut missing = self.check_reqd_fields(); self.check_1559_fields(&mut missing); @@ -435,9 +414,8 @@ impl TransactionRequest { } // Either `sidecar` or `blob_versioned_hashes` must be set. - if self.sidecar.is_none() && self.blob_versioned_hashes.is_none() { + if self.sidecar.is_none() { missing.push("sidecar"); - missing.push("blob_versioned_hashes"); } if self.max_fee_per_blob_gas.is_none() { @@ -506,7 +484,15 @@ impl TransactionRequest { Some(pref) } + // TODO: change the return type of `build_typed_tx` to `Result`? + // That way we eliminate all panics from the code. + // + // Question for the reviewer :) + /// Build an [`TypedTransaction`] + /// + /// In case `Ok(...)` is returned, the `TypedTransaction` is guaranteed to be _complete_, e.g. + /// sendable to the network. pub fn build_typed_tx(self) -> Result { let tx_type = self.buildable_type(); @@ -515,14 +501,28 @@ impl TransactionRequest { } Ok(match tx_type.expect("checked") { - TxType::Legacy => self.build_legacy().into(), - TxType::Eip2930 => self.build_2930().into(), - TxType::Eip1559 => self.build_1559().into(), - TxType::Eip4844 => self.build_4844_variant().into(), + TxType::Legacy => self.build_legacy().expect("checked)").into(), + TxType::Eip2930 => self.build_2930().expect("checked)").into(), + TxType::Eip1559 => self.build_1559().expect("checked)").into(), + // `sidecar` is a hard requirement since this must be a _sendable_ transaction. + TxType::Eip4844 => self.build_4844_with_sidecar().expect("checked)").into(), }) } } +impl TryFrom for TypedTransaction { + type Error = &'static str; + + fn try_from(tx: TransactionRequest) -> Result { + match tx.preferred_type() { + TxType::Legacy => tx.build_legacy().map(Into::into), + TxType::Eip2930 => tx.build_2930().map(Into::into), + TxType::Eip1559 => tx.build_1559().map(Into::into), + TxType::Eip4844 => tx.build_4844_variant().map(Into::into), + } + } +} + /// Helper type that supports both `data` and `input` fields that map to transaction input data. /// /// This is done for compatibility reasons where older implementations used `data` instead of the @@ -830,6 +830,7 @@ mod tests { use super::*; use alloy_primitives::b256; use alloy_serde::WithOtherFields; + use assert_matches::assert_matches; // #[test] @@ -946,4 +947,174 @@ mod tests { assert!(req.sidecar.is_none()); // Sidecar won't be deserialized. } + + #[test] + fn try_from_tx_args_into_typed_tx() { + // Legacy + { + // Positive case + let legacy_gas_limit = 123456; + let legacy_request: TransactionRequest = TransactionRequest { + to: Some(TxKind::Call(Address::repeat_byte(0xDE))), + gas_price: Some(1234), + nonce: Some(57), + gas: Some(legacy_gas_limit), + ..Default::default() + }; + + let maybe_legacy_tx: Result = legacy_request.try_into(); + assert_matches!(maybe_legacy_tx, Ok(TypedTransaction::Legacy(TxLegacy { gas_limit, .. })) if gas_limit == legacy_gas_limit); + + // Negative case + let legacy_request_missing_gas: TransactionRequest = TransactionRequest { + to: Some(TxKind::Call(Address::repeat_byte(0xDE))), + gas_price: Some(1234), + nonce: Some(57), + ..Default::default() + }; + let maybe_legacy_tx: Result = + legacy_request_missing_gas.try_into(); + assert_matches!(maybe_legacy_tx, Err(..)); + } + + // EIP-2930 + { + // Positive case + let access_list = AccessList(vec![alloy_eips::eip2930::AccessListItem { + address: Address::repeat_byte(0x01), + storage_keys: vec![B256::repeat_byte(0x02), B256::repeat_byte(0x04)], + }]); + let eip2930_request: TransactionRequest = TransactionRequest { + to: Some(TxKind::Call(Address::repeat_byte(0xDE))), + gas_price: Some(1234), + nonce: Some(57), + gas: Some(123456), + access_list: Some(access_list.clone()), + ..Default::default() + }; + + let maybe_eip2930_tx: Result = eip2930_request.try_into(); + assert_matches!(maybe_eip2930_tx, Ok(TypedTransaction::Eip2930(TxEip2930 { access_list, .. })) if access_list == access_list); + + // Negative case + let eip2930_request_missing_nonce: TransactionRequest = TransactionRequest { + to: Some(TxKind::Call(Address::repeat_byte(0xDE))), + gas_price: Some(1234), + gas: Some(123456), + access_list: Some(access_list), + ..Default::default() + }; + + let maybe_eip2930_tx: Result = + eip2930_request_missing_nonce.try_into(); + assert_matches!(maybe_eip2930_tx, Err(..)); + } + + // EIP-1559 + { + // Positive case + let max_prio_fee = 987; + let eip1559_request: TransactionRequest = TransactionRequest { + to: Some(TxKind::Call(Address::repeat_byte(0xDE))), + max_fee_per_gas: Some(1234), + max_priority_fee_per_gas: Some(max_prio_fee), + nonce: Some(57), + gas: Some(123456), + ..Default::default() + }; + + let maybe_eip1559_tx: Result = eip1559_request.try_into(); + assert_matches!(maybe_eip1559_tx, Ok(TypedTransaction::Eip1559(TxEip1559 { max_priority_fee_per_gas, .. })) if max_priority_fee_per_gas == max_prio_fee); + + // Negative case + let eip1559_request_missing_max_fee: TransactionRequest = TransactionRequest { + to: Some(TxKind::Call(Address::repeat_byte(0xDE))), + max_priority_fee_per_gas: Some(max_prio_fee), + nonce: Some(57), + gas: Some(123456), + ..Default::default() + }; + + let maybe_eip1559_tx: Result = + eip1559_request_missing_max_fee.try_into(); + assert_matches!(maybe_eip1559_tx, Err(..)); + } + + // EIP-4844 without sidecar + { + // Positive case + let max_fee_per_blob_gas = 13579; + let eip4844_request: TransactionRequest = TransactionRequest { + to: Some(TxKind::Call(Address::repeat_byte(0xDE))), + max_fee_per_gas: Some(1234), + max_priority_fee_per_gas: Some(678), + nonce: Some(57), + gas: Some(123456), + max_fee_per_blob_gas: Some(max_fee_per_blob_gas), + blob_versioned_hashes: Some(vec![B256::repeat_byte(0xAB)]), + ..Default::default() + }; + + let maybe_eip4844_tx: Result = eip4844_request.try_into(); + assert_matches!(maybe_eip4844_tx, Ok(TypedTransaction::Eip4844(TxEip4844Variant::TxEip4844(TxEip4844 { max_fee_per_blob_gas, .. }))) if max_fee_per_blob_gas == max_fee_per_blob_gas); + + // Negative case + let eip4844_request_incorrect_to: TransactionRequest = TransactionRequest { + to: Some(TxKind::Create), + max_fee_per_gas: Some(1234), + max_priority_fee_per_gas: Some(678), + nonce: Some(57), + gas: Some(123456), + max_fee_per_blob_gas: Some(max_fee_per_blob_gas), + blob_versioned_hashes: Some(vec![B256::repeat_byte(0xAB)]), + ..Default::default() + }; + + let maybe_eip4844_tx: Result = + eip4844_request_incorrect_to.try_into(); + assert_matches!(maybe_eip4844_tx, Err(..)); + } + + // EIP-4844 with sidecar + { + use alloy_eips::eip4844::{Blob, BlobTransactionSidecar}; + + // Positive case + let sidecar = + BlobTransactionSidecar::new(vec![Blob::repeat_byte(0xFA)], Vec::new(), Vec::new()); + let eip4844_request: TransactionRequest = TransactionRequest { + to: Some(TxKind::Call(Address::repeat_byte(0xDE))), + max_fee_per_gas: Some(1234), + max_priority_fee_per_gas: Some(678), + nonce: Some(57), + gas: Some(123456), + max_fee_per_blob_gas: Some(13579), + blob_versioned_hashes: Some(vec![B256::repeat_byte(0xAB)]), + sidecar: Some(sidecar.clone()), + ..Default::default() + }; + + let maybe_eip4844_tx: Result = eip4844_request.try_into(); + assert_matches!(maybe_eip4844_tx, + Ok(TypedTransaction::Eip4844(TxEip4844Variant::TxEip4844WithSidecar(TxEip4844WithSidecar { + sidecar, .. }))) if sidecar == sidecar); + + // Negative case + let eip4844_request_incorrect_to: TransactionRequest = TransactionRequest { + to: Some(TxKind::Create), + max_fee_per_gas: Some(1234), + max_priority_fee_per_gas: Some(678), + nonce: Some(57), + gas: Some(123456), + max_fee_per_blob_gas: Some(13579), + blob_versioned_hashes: Some(vec![B256::repeat_byte(0xAB)]), + sidecar: Some(sidecar), + ..Default::default() + }; + + let maybe_eip4844_tx: Result = + eip4844_request_incorrect_to.try_into(); + assert_matches!(maybe_eip4844_tx, Err(..)); + } + } } From 68f0932f09669a311f05ebde152cd1c12cfb31b5 Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Fri, 26 Jul 2024 14:55:53 +0200 Subject: [PATCH 6/7] Remove comment --- crates/rpc-types-eth/src/transaction/request.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/rpc-types-eth/src/transaction/request.rs b/crates/rpc-types-eth/src/transaction/request.rs index c8523d623a8..e0d007a85c3 100644 --- a/crates/rpc-types-eth/src/transaction/request.rs +++ b/crates/rpc-types-eth/src/transaction/request.rs @@ -413,7 +413,6 @@ impl TransactionRequest { missing.push("to"); } - // Either `sidecar` or `blob_versioned_hashes` must be set. if self.sidecar.is_none() { missing.push("sidecar"); } From 048740fc03e8251f2ae0255dd0f39f918340b873 Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Wed, 7 Aug 2024 07:40:14 +0200 Subject: [PATCH 7/7] Review comments --- .../rpc-types-eth/src/transaction/request.rs | 72 ++++++++++++------- 1 file changed, 46 insertions(+), 26 deletions(-) diff --git a/crates/rpc-types-eth/src/transaction/request.rs b/crates/rpc-types-eth/src/transaction/request.rs index e0d007a85c3..713102be0ae 100644 --- a/crates/rpc-types-eth/src/transaction/request.rs +++ b/crates/rpc-types-eth/src/transaction/request.rs @@ -483,11 +483,6 @@ impl TransactionRequest { Some(pref) } - // TODO: change the return type of `build_typed_tx` to `Result`? - // That way we eliminate all panics from the code. - // - // Question for the reviewer :) - /// Build an [`TypedTransaction`] /// /// In case `Ok(...)` is returned, the `TypedTransaction` is guaranteed to be _complete_, e.g. @@ -507,18 +502,30 @@ impl TransactionRequest { TxType::Eip4844 => self.build_4844_with_sidecar().expect("checked)").into(), }) } -} -impl TryFrom for TypedTransaction { - type Error = &'static str; - - fn try_from(tx: TransactionRequest) -> Result { - match tx.preferred_type() { - TxType::Legacy => tx.build_legacy().map(Into::into), - TxType::Eip2930 => tx.build_2930().map(Into::into), - TxType::Eip1559 => tx.build_1559().map(Into::into), - TxType::Eip4844 => tx.build_4844_variant().map(Into::into), + /// Build an [`TypedTransaction`]. + /// + /// In case `Ok(...)` is returned, the `TypedTransaction` does not guarantee to be _complete_, + /// e.g. sendable to the network. + /// + /// E.g. a particular case is when the transaction is of type `Eip4844` and the `sidecar` is not + /// set, in this case the transaction is not _complete_. It can still be used to calculate the + /// signature of the transaction though. + /// + /// In case the requirement is to build a _complete_ transaction, use `build_typed_tx` instead. + pub fn build_consensus_tx(self) -> Result { + match self.preferred_type() { + TxType::Legacy => self.clone().build_legacy().map(Into::into), + TxType::Eip2930 => self.clone().build_2930().map(Into::into), + TxType::Eip1559 => self.clone().build_1559().map(Into::into), + TxType::Eip4844 => self.clone().build_4844_variant().map(Into::into), } + .map_err(|msg| self.into_tx_err(msg)) + } + + /// Converts the transaction request into a `BuildTransactionErr` with the given message. + fn into_tx_err(self, message: &'static str) -> BuildTransactionErr { + BuildTransactionErr { tx: self, error: message.to_string() } } } @@ -824,6 +831,15 @@ impl From for TransactionRequest { #[non_exhaustive] pub struct TransactionInputError; +/// Error thrown when a transaction request cannot be built into a transaction. +#[derive(Debug)] +pub struct BuildTransactionErr { + /// Transaction request that failed to build into a transaction. + pub tx: TransactionRequest, + /// Error message. + pub error: String, +} + #[cfg(test)] mod tests { use super::*; @@ -948,7 +964,7 @@ mod tests { } #[test] - fn try_from_tx_args_into_typed_tx() { + fn build_consensus_tx_works() { // Legacy { // Positive case @@ -961,7 +977,7 @@ mod tests { ..Default::default() }; - let maybe_legacy_tx: Result = legacy_request.try_into(); + let maybe_legacy_tx: Result = legacy_request.build_consensus_tx(); assert_matches!(maybe_legacy_tx, Ok(TypedTransaction::Legacy(TxLegacy { gas_limit, .. })) if gas_limit == legacy_gas_limit); // Negative case @@ -972,7 +988,7 @@ mod tests { ..Default::default() }; let maybe_legacy_tx: Result = - legacy_request_missing_gas.try_into(); + legacy_request_missing_gas.build_consensus_tx(); assert_matches!(maybe_legacy_tx, Err(..)); } @@ -992,7 +1008,8 @@ mod tests { ..Default::default() }; - let maybe_eip2930_tx: Result = eip2930_request.try_into(); + let maybe_eip2930_tx: Result = + eip2930_request.build_consensus_tx(); assert_matches!(maybe_eip2930_tx, Ok(TypedTransaction::Eip2930(TxEip2930 { access_list, .. })) if access_list == access_list); // Negative case @@ -1005,7 +1022,7 @@ mod tests { }; let maybe_eip2930_tx: Result = - eip2930_request_missing_nonce.try_into(); + eip2930_request_missing_nonce.build_consensus_tx(); assert_matches!(maybe_eip2930_tx, Err(..)); } @@ -1022,7 +1039,8 @@ mod tests { ..Default::default() }; - let maybe_eip1559_tx: Result = eip1559_request.try_into(); + let maybe_eip1559_tx: Result = + eip1559_request.build_consensus_tx(); assert_matches!(maybe_eip1559_tx, Ok(TypedTransaction::Eip1559(TxEip1559 { max_priority_fee_per_gas, .. })) if max_priority_fee_per_gas == max_prio_fee); // Negative case @@ -1035,7 +1053,7 @@ mod tests { }; let maybe_eip1559_tx: Result = - eip1559_request_missing_max_fee.try_into(); + eip1559_request_missing_max_fee.build_consensus_tx(); assert_matches!(maybe_eip1559_tx, Err(..)); } @@ -1054,7 +1072,8 @@ mod tests { ..Default::default() }; - let maybe_eip4844_tx: Result = eip4844_request.try_into(); + let maybe_eip4844_tx: Result = + eip4844_request.build_consensus_tx(); assert_matches!(maybe_eip4844_tx, Ok(TypedTransaction::Eip4844(TxEip4844Variant::TxEip4844(TxEip4844 { max_fee_per_blob_gas, .. }))) if max_fee_per_blob_gas == max_fee_per_blob_gas); // Negative case @@ -1070,7 +1089,7 @@ mod tests { }; let maybe_eip4844_tx: Result = - eip4844_request_incorrect_to.try_into(); + eip4844_request_incorrect_to.build_consensus_tx(); assert_matches!(maybe_eip4844_tx, Err(..)); } @@ -1093,7 +1112,8 @@ mod tests { ..Default::default() }; - let maybe_eip4844_tx: Result = eip4844_request.try_into(); + let maybe_eip4844_tx: Result = + eip4844_request.build_consensus_tx(); assert_matches!(maybe_eip4844_tx, Ok(TypedTransaction::Eip4844(TxEip4844Variant::TxEip4844WithSidecar(TxEip4844WithSidecar { sidecar, .. }))) if sidecar == sidecar); @@ -1112,7 +1132,7 @@ mod tests { }; let maybe_eip4844_tx: Result = - eip4844_request_incorrect_to.try_into(); + eip4844_request_incorrect_to.build_consensus_tx(); assert_matches!(maybe_eip4844_tx, Err(..)); } }